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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171017/0eea1769/attachment.sig>