[FFmpeg-devel] [PATCH] vf_delogo: allow to change the rectangle based on the time

[Nicolas George]

Thanks for the review.

Le nonidi 29 thermidor, an CCXIX, Stefano Sabatini a écrit :
> Add a note regarding # lines.

Done.

> somehow confusing var names (what is alloc_rect? is a single rect, is
> an array of rects, is a number of allocated rects?)

> nit: timed_rects, nb_timed_rects => clear corrispondency

Renamed a few variables to make them clearer.

> I'd prefer avoiding to twiddle with errno, better
> err = AVERROR(ENOMEM);
> goto load_error;

Done.

> don't know if realloc already set the error, but that's pretty opaque
> so better to set it explicitely

Done. Even more so since av_log could clutter errno.

> nit++: s/p/n/ (p is usually used for temporary pointers, n for integer
> numbers)

I changed to n_fields.

> please specify in the docs that band is optional

I believe it was already there.

> More explicit: "invalid time specification, ts %f is <= the ts %f specified in the previous line\n"

Done.

> why AV_TIME_BASE? Why don't you directly store the time as a double?

E remnant from the time when I wanted to use the subtitles parser, which
outputs timestamps in milliseconds. Changed.

> AVERROR(EINVAL)

Fixed.

> why not to use directly delogo_timed_rect?
> same, what about avoiding the n_rect temporary?

That makes longer, less readable lines in the code itself.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vf_delogo-allow-to-change-the-rectangle-based-on-the.patch
Type: text/x-diff
Size: 9068 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110820/1099d690/attachment.bin>
-------------- 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/20110820/1099d690/attachment.asc>