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

[wm4]

On Wed, 4 Oct 2017 13:37:31 +0200
Tobias Rapp <t.rapp at noa-archive.com> wrote:

> On 04.10.2017 11:34, wm4 wrote:
> > 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.  
> 
> Changing the semantics of a field during its lifetime, even when only 
> done within private code, is at least unexpected behavior.

That's not done. Conceptually the AVFrame with the changed behavior is
a new reference. Internally, AVFrame.opaque_ref will always have the
same semantics, pointing to the FrameDecodeData struct. Only at points
where the AVFrame ref is converted to/from the user struct this is
changed.

> > This is done strictly when returning a valid AVFrame, so there is no
> > room for mistakes.  
> 
> The room for mistake might not increase for external developers but it 
> increases for internal developers (maintenance cost).

Like where? There are only 2 places where the code needs to deal with
it, and these are in shared code, not individual decoders.

There are other cases where this will be needed, like fixing
videotoolbox with frame threading.

> >> 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.  
> 
> What would be the drawback of using a separate field?

That would add a field that is merely needed for an implementation
detail within libavcodec. Would you argue adding one for each case we
encounter? Can user applications request the addition of arbitrary
fields as well, which they might need internally?

opaque_ref is basically a priv pointer, which _always_ has different
meanings depending on the context where it's used. For example, what
does AVCodecContext.priv_data point to? Or AVCodecContext.opaque? It's
even worse with av_log(), which gets a void*, but somehow expects it to
be "something" (a struct where the first member is AVClass* or so). So
don't pretend this commits adds anything confusing if you're fine with
the av_log() shit, or the way how most codecs/filters/(de)muxers are
implemented.

In any case, feel free to make a better suggestion for implementing
this, which doesn't require adding pointless fields to AVFrame. This
is just a Libav merge too - if you care about what gets merged, discuss
it with the original author on the libav-devel mailing list before it
gets committed. I shouldn't have to do this discussion in the author's
place. (In fact I had a similar discussion with the author.)