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

[Michael Niedermayer]

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.

Let me quote the code from one of these patches:

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 437b848248..04f7156154 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
[...]
+        if (frame->opaque_ref) {
+            FrameDecodeData *fdd;
+            AVBufferRef *user_opaque_ref;
+
+            fdd = (FrameDecodeData*)frame->opaque_ref->data;

This violates the API and it is semantically wrong.

about pts, that is off topic, but no amount of moving an AVFrame
around makes a valid AVFrame pts into an invalid AVFrame pts.
AVFrame pts refers to a unspecified timebase either way.
If the code knows that timebase and knows the origin it can interpret
the timestamp.
It may make sense to add more information about the pts into AVFrame
so its more self contained, but this is off topic


> 
> > Before the patch
> > all AVFrame opaque_ref have the same type, no amount of moving/passing
> > AVFrames around results in an invalid AVFrame.
> 

> Wrong. Even a user application could have multiple uses of opaque_ref,
> all with their own meaning. You can't interpret this field without
> context about where its value came from.

No matter if the user app uses 1, 2 or n types in opaque_ref.
Its still at all points the user apps type and thus the same type for
this discussions point of view.
interpreting the user apps opaque_ref is entirely in the user
applications domain. We can never interpret it. The user application
should know how to interpret its own data.


[...]

> > more so if that field is not void* it could provide type checking
> > which most people consider a good thing.
> > 
> > A system simiar to side data for opaque data could be used too, i
> > would say thats overkill but some people like side data
> 
> Side data has literally nothing to do with this. Side data types can't
> be defined by the user anyway.

> Why do codec/filter/demuxer private
> fields have no type checking? Or av_log? Or the whole AVOption API?
> Please answer this.

This thread is the wrong place to discuss this, this is off topic
Of course it would be good if we can check types in more cases ...


[...]

> > And this way you need no wraping or unwraping, a AVFrame either
> 
> YOU FUCKING NEED UNWRAPPING FOR POSTPROCESSING

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


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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/20171014/9cc69aff/attachment.sig>