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

[Martin Storsjö]

On Mon, 29 Oct 2018, Derek Buitenhuis wrote:

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

Sorry, I think you've misunderstood this patch altogether.

It's not about reviving this field or not, it's all in full use 
already. It was never deprecated with any active plan to remove it, the 
only steps were a few doxygen comments, never any attributes that would 
actually prompt action.

And a few years later someone noticed that these doxygen comments didn't 
match up with reality, and it was decided (with no objections on either 
project) that these really shouldn't be deprecated as it is the only 
actual mechanism we have for doing exactly this.

It's just that the undeprecation commit, ad1ee5fa7, missed one field. And 
the one I'm removing the stray deprecation comment from, is the very 
properly placed one in AVFrame non the less.

// Martin