[FFmpeg-devel] [PATCH] lavfi: add curves filter.

[Nicolas George]

Le quintidi 15 ventôse, an CCXXI, Clement Boesch a écrit :
> I did a s/dot/point/ to change that. Too bad, I liked the short "dot" name
> :-(

point -> pt ?

> Moved locally to init() and removed uninit callback.

You still need to free the options, do you not?

> Honestly, the speed and memory really doesn't matter in this case (init
> only) and scale order (generally just a few dots).

I agree. What bothers me most in this case are the endless checks for memory
allocation failure.

> On the other hand, using a hard limit of 256 will prevent from move the
> code to support 16-bits depth (Paul was asking about rgb48 support). Right
> now, it is flexible enough to switch to 16-bit support easily, and users
> will be able to add 500, 600, ... dots (right, in practice it won't
> happen).

IMHO, even allocating a 1M array (64k * 2 * sizeof(double)) would be
acceptable. Furthermore, you can predict an even better upper boundary:
(strlen(str) + 1) / 4, since each point requires at least four chars in the
string (counting the terminating NUL).

> I fail to see how this will simplify anything; keeping a "last" pointer
> makes the code relatively obvious and simple to me...

See below.

> What about last->next = dot in this scope?

If you use the tail pointer as I suggested, then "last->next = dot" becomes
"*tail = dot" in all cases, so you do not need it anymore.

You still need the last pointer to check the last point, though.

> It looks more logical in the current way...

Adding the first point first is more logical indeed, but your data structure
is already ready for insertions at the end: if you do it first, then you are
sure the list is non-empty when you insert at the beginning, and that makes
the code simpler.

At least you should merge the two code paths for the first extra point.

> Changed.
> 
> Note: originally, I planed to a global "all"/"value" component (so
> NB_COMP=4 and input still has 3 comp), but it can be changed later.

Having NB_COMP everywhere will make it easier to find all places that need
updating.

> because I'm using p[i] two times. I could also have added { } around the
> for and a p++ inside, but that doesn't change much.

You can put the p++ in the increment part of the for statement.

> New patch attached.

I will look at it later.

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/20130305/50113ded/attachment.asc>