On Wed, Jan 22, 2014 at 09:32:22PM +0100, Vittorio Giovara wrote: > Ok let's face this beast.
\o/ > On 16/gen/2014, at 03:55, Diego Biurrun <[email protected]> wrote: > > --- a/libavformat/utils.c > > +++ b/libavformat/utils.c > > @@ -19,35 +19,35 @@ > > > > +#undef NDEBUG > > +#include <assert.h> > > +#include <stdarg.h> > > #include <stdint.h> > > > > -#include "avformat.h" > > -#include "avio_internal.h" > > -#include "internal.h" > > -#include "libavcodec/internal.h" > > -#include "libavcodec/bytestream.h" > > -#include "libavutil/opt.h" > > -#include "libavutil/dict.h" > > -#include "libavutil/internal.h" > > -#include "libavutil/pixdesc.h" > > -#include "metadata.h" > > -#include "id3v2.h" > > +#include "config.h" > > config.h should be e first header to be included. > > > #include "libavutil/avassert.h" > > #include "libavutil/avstring.h" > > #include "libavutil/mathematics.h" > > #include "libavutil/parseutils.h" > > #include "libavutil/time.h" > > -#include "riff.h" > > +#include "libavutil/opt.h" > > +#include "libavutil/dict.h" > > +#include "libavutil/internal.h" > > +#include "libavutil/pixdesc.h" > > +#include "libavcodec/bytestream.h" > > +#include "libavcodec/internal.h" > > #include "audiointerleave.h" > > +#include "avformat.h" > > +#include "avio_internal.h" > > +#include "id3v2.h" > > +#include "internal.h" > > +#include "metadata.h" > > +#include "riff.h" > > #include "url.h" > > -#include <stdarg.h> > > #if CONFIG_NETWORK > > #include "network.h" > > #endif > > I think you should add a new line between header groups. Is that the agreed-upon standard now? > > @@ -72,25 +72,21 @@ const char *avformat_license(void) > > static int append_packet_chunked(AVIOContext *s, AVPacket *pkt, int size) > > { > > int64_t chunk_size = size; > > - int64_t orig_pos = pkt->pos; // av_grow_packet might reset pos > > - int orig_size = pkt->size; > > + int64_t orig_pos = pkt->pos; // av_grow_packet might reset pos > > + int orig_size = pkt->size; > > IMO unnecessary. The assignments are unrelated, that's why I prefer not to group them. > > @@ -236,7 +239,7 @@ static int set_codec_from_probe_data(AVFormatContext > > *s, AVStream *st, AVProbeDa > > > > /** size of probe buffer, for guessing file type from file contents */ > > Shouldn't the comment be on a separate line than **? Doxygen does not require any such thing, no. > > @@ -246,35 +249,35 @@ int av_probe_input_buffer(AVIOContext *pb, > > AVInputFormat **fmt, > > - for(probe_size= PROBE_BUF_MIN; probe_size<=max_probe_size && !*fmt; > > - probe_size = FFMIN(probe_size<<1, FFMAX(max_probe_size, > > probe_size+1))) { > > - int score = probe_size < max_probe_size ? AVPROBE_SCORE_MAX/4 : 0; > > + for (probe_size = PROBE_BUF_MIN; probe_size <= max_probe_size && !*fmt; > > + probe_size = FFMIN(probe_size << 1, > > + FFMAX(max_probe_size, probe_size + 1))) { > > + int score = probe_size < max_probe_size ? AVPROBE_SCORE_MAX / 4 : > > 0; > > > > /* read probe data */ > > So what's the deal with comments? Should they ALL be with capital > letter and full stop? I marked only a few below... If they are complete sentences with noun and verb, then I am in favor of using proper capitalization and punctuation rules. In this file I was inconsistent, yes. I just could not be bothered to adjust all of them. > > @@ -402,7 +415,7 @@ int avformat_open_input(AVFormatContext **ps, const > > char *filename, AVInputForma > > if (s->iformat->priv_class) { > > - *(const AVClass**)s->priv_data = s->iformat->priv_class; > > + *(const AVClass **) s->priv_data = s->iformat->priv_class; > > Extra space after ) That's the way that THE BOOK formats most of its casts. > > @@ -738,49 +757,58 @@ static void compute_pkt_fields(AVFormatContext *s, > > AVStream *st, > > > > - // some mpeg2 in mpeg-ps lack dts (issue171 / input_file.mpg) > > - // we take the conservative approach and discard both > > - // Note, if this is misbehaving for a H.264 file then possibly > > presentation_delayed is not set correctly. > > - if(delay==1 && pkt->dts == pkt->pts && pkt->dts != AV_NOPTS_VALUE && > > presentation_delayed){ > > + /* Some MPEG-2 in MPEG-PS lack dts (issue171 / input_file.mpg). We > > take the > > Maybe "issue #171"? > Also DTS. I would lowercase decode timestamp and presentation timestamp, but uppercase Digital Theater Systems. > > @@ -1337,42 +1369,47 @@ int ff_seek_frame_binary(AVFormatContext *s, int > > stream_index, int64_t target_ts > > > > av_dlog(s, "read_seek: %d %"PRId64"\n", stream_index, target_ts); > > > > - ts_max= > > - ts_min= AV_NOPTS_VALUE; > > - pos_limit= -1; //gcc falsely says it may be uninitialized > > + ts_max = > > + ts_min = AV_NOPTS_VALUE; > > These may go on the same line. I prefer two lines so that it becomes clear that there are two, and not just one, assignments being performed. It's done like that elsewhere. > > @@ -2090,27 +2170,28 @@ static void compute_chapters_end(AVFormatContext *s) > > > > -/* > > - * Is the time base unreliable. > > +/* Is the time base unreliable. > > question mark? Probably. > > @@ -2543,10 +2645,9 @@ void avformat_close_input(AVFormatContext **ps) > > > > - if (s->iformat) { > > + if (s->iformat) > > if (s->iformat->read_close) > > s->iformat->read_close(s); > > - } > > I guess it'd be safe to merge the two checks with a && and reduce indentation. I mostly try to avoid such semi-cosmetic changes, but OK. > > @@ -2627,46 +2728,47 @@ AVProgram *av_new_program(AVFormatContext *ac, int > > id) > > > > -void ff_program_add_stream_index(AVFormatContext *ac, int progid, unsigned > > int idx) > > +void ff_program_add_stream_index(AVFormatContext *ac, int progid, unsigned > > idx) > > don't remove the int type here, the original is more readable. That also happened to shorten the line ;) What is unsigned supposed to be if not an int? Do we have opinions from others? > > @@ -2904,22 +3010,28 @@ static void hex_dump_internal(void *avcl, FILE *f, > > int level, > > -#define PRINT(...) do { if (!f) av_log(avcl, level, __VA_ARGS__); else > > fprintf(f, __VA_ARGS__); } while(0) > > - > > - for(i=0;i<size;i+=16) { > > +#define PRINT(...) \ > > + do { \ > > + if (!f) \ > > + av_log(avcl, level, __VA_ARGS__); \ > > + else \ > > + fprintf(f, __VA_ARGS__); \ > > + } while (0) > > Can you put \ at the 79th column? I generally follow the automatic suggestions of emacs when I press TAB in these cases, unless the \ should be beyond the 72nd column, which my emacs does not move past when I TAB. For compact blocks like this one having the \ so far out looks a tad silly IMO. > > @@ -3354,7 +3486,7 @@ int ff_generate_avci_extradata(AVStream *st) > > > > av_freep(&st->codec->extradata); > > st->codec->extradata_size = 0; > > - st->codec->extradata = av_mallocz(size + FF_INPUT_BUFFER_PADDING_SIZE); > > + st->codec->extradata = av_mallocz(size + > > FF_INPUT_BUFFER_PADDING_SIZE); > > if (!st->codec->extradata) > > return AVERROR(ENOMEM); > > wheee, beast slain! \o/ Diego _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
