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

[Nicolas George]

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

Fixed.

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

Added.

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

Added.

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

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

Added.

> Might be nice to av_log debug the select palette.

Added.

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

Regards,

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