[FFmpeg-devel] [PATCH 1/2] libavutil: Undeprecate the AVFrame reordered_opaque field

[Derek Buitenhuis]

On 29/10/2018 14:10, Martin Storsjö wrote:
>> I don't understand why this is being used in favour of a proper
>> pointer field? An integer field is just ascting to be misused.
>> Even the doxygen is really sketchy on it.
> 
> It's essentially meant to be used as union { ptr; int64_t } assuming you
> don't have pointers larger than 64 bits.

It's not a union in the API, and I'm pretty sure that it violates the C spec
to use a unions to get an integer out of a pointer, shove it into an int64_t,
and then get it back out, and chnage it back via union. Especially for
32-bit pointers. It encourages terrible code.

I just don't think we should revive this as-is purely for convenience.

>> I also don't understand why this is at the AVCodecContext level
>> and not packet/frame?
> 
> It is on the frame level, but not in the packet struct (probably for
> historical reasons) - instead of in the packet, it's in AVCodecContext.
> For decoding, you set the value in AVCodecContext before feeding packets
> to it, and get the corresponding value reordered into the output AVFrame.
> If things were to be redone from scratch, moving it into AVPacket would
> probably make more sense, but there's not much point in doing that right
> now.

I mean, this is pretty gross, and non-obvious as far as I'm concerned.
Modifying the AVCodecContext on every call is just... eugh.

> At some point, the doxygen got markers saying this mechanism was
> deprecated and one should use the new pkt_pts instead. Before that,
> reordered_opaque was mainly used for getting reordered pts as there
> was no other mechanism for it.
> 
> But even with the proper pkt_pts field, having a generic opaque field that
> travels along with the reordering is useful, which is why the deprecation
> doxygen comments were removed in ad1ee5fa7. But that commit just missed to
> remove one of the doxygen deprecation.

I agree it's very useful, and something we should have, but not that we should
revive/use this partiular field... it's nasty.

- Derek