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

[wm4]

Correction: will push as part of cuvid/videotoolbox patches whenever
they're ready.

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

> > 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

Which it is allowed to.

> 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.

There's nothing semantically distinct. The opaque_ref field's semantics
are defined by the owner of the AVFrame, and when ownership is passed
through the components, rewrapping might be needed. Which is what the
patch does.

How often did I repeat this and how often did you not understand this?

> 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.

Anything you suggested is even worse and even more fragile.

> it also violates the API IMO, but thats not so much the point

It does not. I created the fucking API.

> 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.

AVFrame does not dictate a use for this field. The use of the field is
determined by the owner of the AVFrame, which is libavcodec while the
frame is in libavcodec.

How often did I repeat this and how often did you not understand this?

> 
> [...]
> >  
> > > > 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.

The fuck? Unwrapping is equivalent to doing postprocessing required to
make it a valid output frame.

> 
> > 
> > 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.

There's no need for me to justify merging a Libav patch, although I've
wasted more than enough time doing so. If you don't like Libav
patches, maybe you should never have forked.

> 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
> 
> [...]