[FFmpeg-devel] [PATCH 2/7] decode: add a method for attaching lavc-internal data to frames

[wm4]

On Fri, 13 Oct 2017 19:41:28 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Oct 06, 2017 at 01:48:14AM +0200, wm4 wrote:
> > On Fri, 6 Oct 2017 00:01:30 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > The opaque_ref wraping is a really bad design. Iam not sure why
> > > people defend it.  
> > 
> > FFmpeg is full of this design. There are plenty of structs with
> > opaque/priv fields that change meaning depending on the context
> > (basically how the struct is used or what uses it). It affects all
> > decoders, encoders, filters, demuxers, muxers, the av_log() call,
> > functions that work with AVClass, AVOption, and probably more.  
> 
> what you write is not true
> 
> each decoder, demuxer, ... CLASS has its own type of private context
> nothing outside code specific to that class messes with it.
> A snow decoder has a snow context.
> If the outside structure is moved around its still a snow decoder with
> a snow private context. No amount of moving the structure around makes
> it invalid.
> 
> OTOH, opaque_ref is defined by the user application.
> There is a single user application in the address space.

You misunderstand how AVFrame works. AVFrame has an owner, and this
owner decides how certain fields are handled. This includes for example
the pts fields, whose meaning entirely depend on an undefined timebase.
opaque_ref is merely a more advanced case of this.

> Before the patch
> all AVFrame opaque_ref have the same type, no amount of moving/passing
> AVFrames around results in an invalid AVFrame.

Wrong. Even a user application could have multiple uses of opaque_ref,
all with their own meaning. You can't interpret this field without
context about where its value came from.

> after the patch this is very different.
> Each AVFrame becomes tied to the code that created it, opaque_ref
> has a type that outside decoders, is defined by the user application
> inside decoders, its a internal structure.
> 
> AVFrames are no longer a universal structure that can simply be passed
> around.
> This is bad design and it is fragile
> 
> private and opaque are also intended to be very different things.
> In FFmpeg private is a internal thing like the internal context of an
> encoder. Opaque is a exteral thing, from the user application or caller

Why can you access the private fields anyway? This is fragile and bad
design.

> You may have misunderstood me but
> I am against AVFrames depending on the code that created them.

Suggest something better.

> This is not just about this implementation (which is in fact buggy too),

Is it.

> Pointing to the implementation issues and bugs was just intended
> to show how fragile this is.
> I do not understand how one can on one hand see all the problems this
> causes yet not see that the API that causes this is what is at fault.
> And it is MUCH easier to fix the API than to fix all the issues that
> context specific AVFrames would cause

Fix what API? Do you even understand for what this is needed?

> add a new field to AVFrame if you want a field with semantics that
> are different.

HAHAHAHAHAHAHAAHAHAHAHA

This is a load of badly designed bullshit. This would add another
AVFrame field for something that is INTERNAL and IMPLEMENTATION DEFINED
to libavcodec. Do you know that we have a field for this, which is
supposed to be used by anything that requires INTERNAL and
IMPLEMENTATION DEFINED values by the AVFrame users? It's called
opaque_ref.

> you said you are against this IIRC. But thats the simple, robust and
> easy maintainable solution

It's not simple, because you STILL can forget to unwrap or wrap the
AVFrame properly. Forgetting to unwrap it would be "harmless" in your
opinion because the user isn't allowed to read it, while with
opaque_ref it would be "dangerous". But the truth is that an
"unwrapped" frame doesn't contain the correct data anyway - post
processing was not run, and returning the frame in this state to the
user is undefined behavior.

Skipping unwrapping is a bug. Unwrapping will restore opaque_ref to the
value it should be.

Please come up with something that is actually better.

Anyway, in my opinion, it would be better to disallow opaque_ref being
set by the user's get_buffer2 callback. That would solve most of the
issues you have with it, but surely you would come up with some crap to
block my work anyway.

> more so if that field is not void* it could provide type checking
> which most people consider a good thing.
> 
> A system simiar to side data for opaque data could be used too, i
> would say thats overkill but some people like side data

Side data has literally nothing to do with this. Side data types can't
be defined by the user anyway. Why do codec/filter/demuxer private
fields have no type checking? Or av_log? Or the whole AVOption API?
Please answer this.

> One entry for the users opaque structure
> One entry for the libavcodec private structure
> One entry for a future libavfilter private structure

One entry for application 1.
...
One entry for application 1000000000.

GREAT DESIGN

> And this way you need no wraping or unwraping, a AVFrame either

YOU FUCKING NEED UNWRAPPING FOR POSTPROCESSING

Both intended uses, cuvid and videotoolbox, require this. Unwrapping
can't be skipped. Skipping unwrapping is a bug.

> has some extra opaque fields or it doesnt. nothing could cast them to
> the wrong type.

Why is such a thing not required for codecs/filters/demuxers? Hell,
your beloved mpegvideo pile of garbage could really use this.

> This is much more robust than putting all these things in opaque_ref
> and spending time and more time and more time writing code wraping
> and unwraping at every interface point and then cursing half the
> interface and likely in the future fighting over every bit of added
> interface as it massivly increases complexity with the wraping

All this code is already written.

And again, you can NOT SKIP UNWRAPPING.

opaque_ref is superior to adding a new AVFrame field for every private
use case.