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

[Michael Niedermayer]

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.

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

after the patch this is very different.
Each AVFrame becomes tied to the code that created it, opaque_ref
has a type that outside decoders, is defined by the user application
inside decoders, its a internal structure.

AVFrames are no longer a universal structure that can simply be passed
around.
This is bad design and it is fragile

private and opaque are also intended to be very different things.
In FFmpeg private is a internal thing like the internal context of an
encoder. Opaque is a exteral thing, from the user application or caller


You may have misunderstood me but
I am against AVFrames depending on the code that created them.
This is not just about this implementation (which is in fact buggy too),


Pointing to the implementation issues and bugs was just intended
to show how fragile this is.
I do not understand how one can on one hand see all the problems this
causes yet not see that the API that causes this is what is at fault.
And it is MUCH easier to fix the API than to fix all the issues that
context specific AVFrames would cause


> 
> Are you saying that FFmpeg is really bad design? That's funny.
>
> There is nothing unclean about this use of opaque_ref, just that it
> somewhat collides with awful legacy hacks like draw_horiz_band (but
> which could be fixed anyway).
> 

> Also what you said about nested decoders is simply incorrect. The

It is not incorrect in case current implementations of decoders
dont trigger it. 


[...]

> 
> Now I'm very curious to hear what your "cleaner" solution to this
> problem is.

add a new field to AVFrame if you want a field with semantics that
are different.
you said you are against this IIRC. But thats the simple, robust and
easy maintainable solution
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

One entry for the users opaque structure
One entry for the libavcodec private structure
One entry for a future libavfilter private structure

And this way you need no wraping or unwraping, a AVFrame either
has some extra opaque fields or it doesnt. nothing could cast them to
the wrong type.
This is much more robust than putting all these things in opaque_ref
and spending time and more time and more time writing code wraping
and unwraping at every interface point and then cursing half the
interface and likely in the future fighting over every bit of added
interface as it massivly increases complexity with the wraping

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/20171013/9a6de654/attachment.sig>