[FFmpeg-devel] [PATCH 2/6] sink_buffer: make opaque argument optional.

[Stefano Sabatini]

On date Tuesday 2012-06-26 15:53:18 +0200, Nicolas George encoded:
> Le nonidi 9 messidor, an CCXX, Stefano Sabatini a écrit :
> > > +    if (!common_init(ctx))
> > > +        return 0;
> > Why is not propagating the error? 

> The "return 0" propagates success, error is propagated by the fall-through
> case, since currently the only possible failure is ENOMEM. It will require
> some caution if common_init() is expanded, but at this time it is the
> simplest method.

OK but I still would prefer a clean return common_init(...), but I
never remember if we call uninit() automatically in case of failure,
in that case the code could be simplified (you do all cleanup in
uninit anyway).

> > BTW, what's your opinion with regards to the opaque field?
> > 
> > The opaque field was designed to help applications which for a reason
> > or another needs to access a binary struct programmatically.
> > 
> > In case of the buffer and buffersink components they are supposed to
> > be used programmatically, so IMO it makes sense to keep that
> > interface.
> > 
> > The problem I was addressing with the buffersink parameter structure
> > was passing a *list* of formats, and I wanted to avoid all the
> > serialization/deserialization complications to the programmer.
> 
> I think the principle is a good one, but there are two issues that makes it
> doomed to fail.
> 

> First, the lack of uniformity: buffersink is the only filter using the
> opaque argument, some filters use key=value syntax with the option system,
> other a custom parser. We will absolutely need to clean up that.

buffersink is a filter meant to be used by *programs*, and this
explains why it has an opaque parameter, and it would make sense to
extend buffer the same way.

Note that I'm not against supporting also a textual args, this may be
useful for testing buffersink/buffer (e.g. with graph2dot). In general
supporting both interfaces when sensible may be a good idea (but
increases the maintainance effort).

Application-dependant filters also may need to access a global context
or whatever, and the API should allow that in case it is needed,
crippling the API because of a biased opinion (it won't serve, it
isn't "nice") is just silly.

As for what regards the syntax, many filters were ported from mplayer
and we tried to keep the same syntax, also when we started working on
lavfi many parsing facilities were not available (opt/error/eval in
libavcodec, no parsing facilities).

> Second, the API is too verbose: ask the library to allocate the args
> structure, check for error, fill it, use it, free it. The check for error is
> especially annoying.

Sure, this is a concern that I have as well.

> I may have an idea to make things easier: we could use a macro that would
> enable us to write something like that:
> 
>     avfilter_buffersink_create(mandatory_args,
> 			       .formats = (int[]){ AV_SAMPLE_FMT_S16, -1 },
> 			       ...);

This would require a custom API for each filter which needs opaque
data, which seems a worse solution than the previous one (before the
opaque field "cleanup").

> the macro will use the named arguments (through __VA_ARGS__ to create a
> structure initializer.
> 
> The catch with that method is that the size of the structure becomes part of
> the ABI. Here is the trick to solve it: put sizeof(args) as the first field
> of the structure, and let the macro fill it.

Yes I believe some macro magic may do it.
 
> For now, I believe the best course is to let the fork cripple the API as
> they want and import their changes, wait until they lose interest, and then
> rebuild it properly.
-- 
FFmpeg = Fierce and Fascinating Mega Puritan Elected Gem