[FFmpeg-devel] [PATCH] Per-frame metadata

[Nicolas George]

L'octidi 28 prairial, an CCXIX, Stefano Sabatini a écrit :
> > +void *av_realloc_f(void *ptr, size_t nelem, size_t elsize)
> Nit++: nb_elems and elem_size for readability/consistency.

I took the names directly from the prototype of calloc in Single Unix. Are
there functions in our headers with a similar prototype? If so, I'll
immediately adopt their spelling. If not, I'll do it only if you insist.

> Nit: I prefer to avoid references to other functions, so the docs will
> result auto-contained and won't require updates if the referred
> function is changed.

That is a reasonable concern, but I do not think there is much risk for the
API of realloc to change.

On the other hand, when you deal with a maze of twisty little functions, all
alike (malloc, calloc, realloc, reallocf, etc.), knowing immediately what
are their difference is more useful than having almost exactly the same blob
of text for each one.

Would it be ok for you if I write thinks like that:

* This function does the same thing as av_realloc, except: [...]
* More precisely: [copy of the realloc paragraph]

?

> Maybe:
> static inline int av_mult_size(size_t *r, size_t a, size_t b);
> 
> Mult is the verb and thus should be mentioned before (like in
> "multiply size") since we are not adopting hierachi notation here, r

A lot of languages put the verb at the end. I have often found that naming
functions first with what they deal with (here: sizes) and only then with
what they do with it result in a more readable API. I do not have an example
from ffmpeg, but with Gtk+, if find more logical for gtk_label_new to be
near gtk_label_set_text rather than gtk_window_new.

> before a and b is consistent with the usual C notation:
> foo(r, a, b) <=> r = a + b;

True. Changed.

> Dumb question... why not a simple:
> 
> if (b && a > SIZE_MAX / b)
>    return AVERROR(EINVAL);
> ?
> 
> Is avoiding a division worth the added obfuscation?

I did not profile myself, but that is how it is done in glibc (although
their code is even more obfuscated), and I expect they did the benchmarks.
But as I said to Michael on this very point, I do not insist on it.

> nit: Convert

Fixed.

> no need for the final dot (uncomplete sentence)

Fixed all over the place I believe.

> size_in is clearer (since we already have a size_out)

Changed.

> Is this API really needed? Couldn't you always use the heap?

I will not insist on it, but I find the API nice, especially in how it
behaves in cases of failure, and I believe that a few places could benefit
from it. The standard "always use the heap directly" approach is an obvious
invitation to neglect failure checks, as the patch #2 proves clearly IMHO.

> Nit+++: superfluous empty line

Fixed.

> Nit+++: tag_=_NULL

That was a copy-paste. Fixed.

> "tag" is also a bit confusing ("entry" would be better)

Changed.

> Please one patch per thread

Why? This rule was in force for the few months when patchwork was in use,
but I always thought it was a flaw of the tool, not a good practice. The
patches are related, they probably have a lot of common flaws (like the
final periods in the documentation). I find discussing them together much
easier to follow than having to remember in what particular branch of the
thread a particular point was addressed.

What do other think about it?

There are a few open points, and the changes I made at this time are mostly
cosmetic, so I'll wait before sending the updated patches.

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/20110616/c7985435/attachment.asc>