[FFmpeg-devel] [PATCH 2/6] dvdsubenc: make it usable for transcoding.
nicolas.george at normalesup.org
Thu Aug 9 13:01:56 CEST 2012
Le tridi 23 thermidor, an CCXX, Clément Bœsch a écrit :
> nit: & is known for its low priority, you can safely drop some parenthesis
> here, and eventually remove some spaces to explicit the priority.
This kind of thing has already made me lose a lot of time in the past, I
prefer having the precedences clearly stated.
> > + if (!count[i]) /* avoid useless search */
> > + continue;
> > + color = palette[i];
> the palette always has 256 colors defined?
I do not know, but the test just before ensures only palette entries used in
the image are considered.
> Maybe add another comment on top of the function (sorry I ask for comments
> a lot) describing how hits is sorted; aka if the color gets close to the
> original one and is "important", it will be at the beginning or something.
> A bit like what you said in the description.
> Maybe add "// ignore non-opaque colors" so those magics 1 & 17 make sense
> here and in the following of the function.
Added, but that is not what this test does.
> question: is there some particular reason for using  instead of * here?
Just a hint to show that they are pointers to arrays of statically constant
> I'm still uncomfortable when I see the ( ) like this because they look
> necessary while they aren't.
Spaces too are not necessary. As above, I like the precedence to be
> Please add a comment around this block such as "Define the virtual
> rectangle containing all the other since DVD subs only support one rect"
> or anything you think would be more correct or appropriate.
> Might be nice to av_log debug the select palette.
> av_bprint_finalize(&extradata, (char **)&avctx->extradata);
> if (!avctx->extradata)
> return AVERROR(ENOMEM);
> avctx->extradata_size = extradata.len + 1;
> Sounds better to me (no explicit memcpy, and '\0' at the end of the
The \0 is not supposed to be part of the extradata. I do not mind keeping it
in the mallocated block though. Changed accordingly.
> No more comments from me,
Do you want to comment the other patches in the series?
Or is there anyone else who wants to comment on that?
I will send an updated version (rebased by hand since Git seems to have
trouble handling split files) as soon as FATE finishes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 198 bytes
Desc: Digital signature
More information about the ffmpeg-devel