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

[wm4]

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.

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
nested decoder is used only by the decoder implementation itself, and
always uses the default get_buffer callback for the nested codec, so
the returned AVFrame.opaque_ref is returned as NULL. Now it's possible
that the new code will assert if the parent codec has AV_CODEC_CAP_DR1
set. This is because if AV_CODEC_CAP_DR1 is set, the AVFrame is
expected to be allocated by the codec context's ff_get_buffer() (which
would guarantee opaque_ref is set correctly). It looks like avrndec.c
does not do this correctly if is_mjpeg is set. But in fact, this codec
violates the API by declaring DR1 support, but not calling the user
get_buffer2 callback. This could be fixed.

An actually valid point is brought up by Mark Thompson (and not by
you), but this can be fixed easily by wrapping the AVFrame properly
before returning. (Assuming the desired semantics are that the
opaque_ref is returned to the user as-is.)

So, still no problem found. What is the problem?

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