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

[wm4]

On Wed, 4 Oct 2017 11:22:37 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, Oct 04, 2017 at 09:12:29AM +0200, wm4 wrote:
> > On Tue, 3 Oct 2017 21:40:58 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Tue, Oct 03, 2017 at 03:15:13PM +0200, wm4 wrote:  
> > > > From: Anton Khirnov <anton at khirnov.net>
> > > >     
> > >   
> > > > Use the AVFrame.opaque_ref field. The original user's opaque_ref is
> > > > wrapped in the lavc struct and then unwrapped before the frame is
> > > > returned to the caller.    
> > > 
> > > this is a ugly hack
> > > 
> > > one and the same field should not be used to hold both the
> > > users opaque_ref as well as a structure which is itself not a user
> > > opaque_ref  
> > 
> > While the AVFrame is within libavcodec, it's libavcodec's frame, not
> > the user's. Thus your claim doesn't make too much sense. libavcodec
> > fully controls the meaning for its own AVFrames' opaque_ref, but
> > reconstruct the user's value when returning it.  
> 
> i disagree

Well, you're wrong anyway.

> such hacks should not be added, we do have enough hacks already

It's not a hack.

> AVFrames are not really seperated into isolated classes
> There arent "the users AVFrames" vs. "the internal AVFrames"

Oh yes, there is the concept of an "owner" of an AVFrame, and the owner
of the AVFrame decides what opaque_ref means. It's quite literally for
free use by the owner of the reference.

That the code goes through some acrobatics to preserve opaque_ref as
passed in by get_buffer is just a feature.

> its fragile to create and maintain such seperation with interfaces
> that all wrap and unwrap the opaque_ref. Any mistake being a potential
> security issue and or crash

This is done strictly when returning a valid AVFrame, so there is no
room for mistakes.

> Its MUCH more robust and also easier to understand to use a sperate
> field

No, it's not. If you fail to do call post_process() on returned
AVFrames (which is done by the same code which exchanges opaque_ref)
you have bugs that violate the API or crash anyway.

> more so, opaque_ref is used in only 5 lines in the whole codebase,
> so there is not much code to consider when using a different solution

We shouldn't add such special fields, we have enough hacks already. Is
that your only suggestion how to do this? Because it's a bad one.