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

wm4 nfxjfg at googlemail.com
Thu Oct 5 22:22:47 EEST 2017


On Thu, 5 Oct 2017 18:47:01 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, Oct 04, 2017 at 02:04:54PM +0200, wm4 wrote:
> > 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.  
> 
> The semantics are defined by the docs, which state:
> "AVBufferRef for free use by the API user."

libavcodec is the API user. I know the semantics of opaque_ref very
well btw., because I added it.

> And before the patch this is true, all instances of this field are
> controled by the user application and are consistent.

The "user" is the AVFrame user, which includes libavcodec.

> after the patch the AVFrames used by a codec have their opaque_ref
> replaced by a wraped structure relative to what the outside of this
> codec has.
> 
> 
> > 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.  
> 
> just greping for AVFrame in the headers shows callbacks, a direct
> pointer to a AVFrame and the API functions that interface the codec

Do you mean get_buffer? I explained that in-depth.

> Just thinking of a codec that instanciates another codec and how
> exactly the callback which may originate from the user or the outer
> codec would unwrap the potential nested wraping.

That would happen only if you'd nest get_buffer in an incorrect way.
"Nested codecs" are an ugly hack anyway that are likely to break other
things, especially if get_buffer is used.

I see two decoders which recursively open other codecs (weird jpg
variants). Both would not be broken by this patch, because they don't
proxy get_buffer to the underlying codecs. (But what I see is that they
require shitty ugly hacks due to shitty ugly codec locking, which was
added as shitty ugly hack for shitty ugly broken shit. But apparently
accepting that kind of code in FFmpeg is strangely no problem.)

> I really dont think we want this in FFmpeg

Why so obstinate.

> And this is just one example ...

Your one example was invalid, next one please.


More information about the ffmpeg-devel mailing list