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

[Stefano Sabatini]

On date Thursday 2011-06-16 17:08:51 +0200, Nicolas George encoded:
> 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.

I prefer internal consistency, nelem and elsize are shizofrenic, UNIX
itself is not particularly renown for being a particularly
self-consistent system.

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

I See your point, and I have no strong position about it, I just note
that when I tried to read the docs I couldn't find out the behavior of
the function without to refer to the referenced functions. Suggestion:
you could put description and difference, yes this is redundant but in
this case it is not necessarily bad.

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

What about inverting them:

* [copy of the realloc paragraph]...
* 
* @note This function does the same thing as av_realloc, except: [...]
 
> > 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 hierachical notation here
> 
> 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.

We have two conventions in FFmpeg:

1) hierarchic notation: av_TAG_VERB

av_image_*
av_samples_*
av_sha_*
av_fifo_*
av_opt_*

this helps auto-completions/grep

2)
av_VERB

e.g.
av_get_cpu_flags()
av_get_sample_fmt_name()
...

this is more consistent with the English language, and is more
readable (in a literal sense: reading the code it will be more similar
to English prose).

The second is used when we don't have enough related functions in the
same category, and I'm not sure we'll have many functions related to
size, but I'm not strong about this (I just keep insisting on doing
the effort to keep the API self-consistent).

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

Is optimization relevant here? If not I tend to prefer plain
readability/maintainability, of course benchmarks are welcome.
 
> > 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.

I'll check again that patch, but my first impression was that the API
looked quite complex, and that using it correctly would require at
least the same effort that adding the missing checks.

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

I think one-patch-per-thread was the (unspoken) rule at least since
three years, but I suppose for RFCs is better to show all the
patchset, for reviewing is better one patch per thread, reviewing 5
big patches at the same time requires much more effort.

> 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.
-- 
FFmpeg = Free & Fancy Meaningful Powerful Elected Gadget