[FFmpeg-devel] [PATCH 2/6] dvdsubenc: make it usable for transcoding.

Nicolas George 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.

> semi


> 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.

> const?


> 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
explicitly stated.

> 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
> string).

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.


  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120809/8e5a40c7/attachment.asc>

More information about the ffmpeg-devel mailing list