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

[wm4]

I have realized that your veto is actually not valid:
- it's a Libav merge
- it has been for months in the Libav repo and you didn't specifically
  care, nor did you make an attempt to merge the commit in a "fixed" way
- this patch would have been merged normally, and you wouldn't have
  cared at all about it

So unless you intend to make a better, working proposal, I will _not_
allow you to make this "my" problem. I will psuh this in 3 hours. After
that, you're free to to reimplement this in a different way or whatever
as a merge cleanup.

On Tue, 17 Oct 2017 00:58:58 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Mon, Oct 16, 2017 at 09:40:26AM +0200, wm4 wrote:
> > 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.  
> 
> sure, the patch uses the opaque_ref field for its libavcodec internal
> purposes
> 
> Theres where the need for wraping and unwraping comes from.
> These are 2 semantically distinct use cases that do not fit well in a
> single field.
> 
> One can put anything in any (large enough) field of any structure that
> way.
> On all input APIs replace the field by a new structure and put
> the original fields content in that structure.
> On output take the field of the internal struct, free the structure
> and write the original fields value back
> Thats exactly what this patches wraping code does.
> 
> And thats what I called a fragile hack.
> 
> it also violates the API IMO, but thats not so much the point
> 
> The data the user application wants to attach to a AVFrame for the
> user applications extrenal purposes
> and
> The data libavcodec wants to attach for internal (hwaccel) use.
> 
> Are 2 distinct things. You can have none, either one or both theres
> no relation between them.
> 
> 
> [...]
> >  
> > > > 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.  
> 
> We can discuss about cases where unwraping is needed while postprocessing
> is not but i think this would distract from the subject so it would be
> better in a different thread.
> 
> 
> > 
> > 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.  
> 
> If you wish to push this patch against my veto, you have to get the
> vote comittee to override my veto. There is no need for you to accept
> my arguments.
> 
> Iam interrested to hear the argumentation of other developers
> why this code is not a hack, not fragile, not hard to maintain, not a
> security risk and why we should go this path instead of some
> alternative without these issues.
> 
> Maybe iam totally wrong and everyone feels this patch is the way code
> should be written and API designed.
> But it would surprise me alot if there are alot of people supporting
> this kind of design
> 
> Thanks
> 
> [...]