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

[wm4]

On Sat, 14 Oct 2017 23:01:41 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Oct 13, 2017 at 09:19:04PM +0200, wm4 wrote:
> > 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.  
> 
> The API docs say about opaque_ref
> "FFmpeg will never check the contents of the buffer ref."
> the unwraping code does exactly that and any code using it internally
> does as well.

It doesn't - not the user's. We use only the field for internal
purposes (as AVFrame users), and we never do anything with the user's
value.

I'm not sure if you get this, so let me repeat this. We never interpret
the user's value, nor do we return our own internal value of this to
the user. Make sure you understand this.

> Let me quote the code from one of these patches:
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 437b848248..04f7156154 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> [...]
> +        if (frame->opaque_ref) {
> +            FrameDecodeData *fdd;
> +            AVBufferRef *user_opaque_ref;
> +
> +            fdd = (FrameDecodeData*)frame->opaque_ref->data;
> 
> This violates the API and it is semantically wrong.


No it does not. Cease your bullshit. It never inspects the user value,
it only wraps it. Stop trying to misrepresent it or inventing semantics
on top of the doxygen (which I write btw.).

> > 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.  
> 
> No matter if the user app uses 1, 2 or n types in opaque_ref.
> Its still at all points the user apps type and thus the same type for
> this discussions point of view.

No. The user app could have multiple uses of it with multiple types, at
different points in the program.

> interpreting the user apps opaque_ref is entirely in the user
> applications domain. We can never interpret it. The user application
> should know how to interpret its own data.

And we never interpret it. Why do you think we do?

Am I talking to a fucking wall? A bot?

> > Both intended uses, cuvid and videotoolbox, require this. Unwrapping
> > can't be skipped. Skipping unwrapping is a bug.  
> 
> IIUC these need a postprocessing function to be called.
> There is no relation between calling a postprocessing function and
> wraping and unwraping the users opaque_ref. You can do either without
> the other.

Yes there is. You claim opaque_ref is "unsafe" because you could forget
unwrapping it. But forgetting unwrapping is equivalent to forgetting
calling the postprocessing, which would be a bug.

Why is one kind of bug so critical that leads you to reject this patch,
while the you barely acknowledge the other bug and don't care?

Unless you have real arguments, I'll push the updated patches in 24h.