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

This was marked as deprecated (but only in the doxygen, not with an
actual deprecation attribute) in 81c623fae05 in 2011, but was
undeprecated in ad1ee5fa7.
---
 libavutil/frame.h   | 1 -
 libavutil/version.h | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

On 25/10/2018 13:58, Martin Storsjö wrote:
> This was marked as deprecated (but only in the doxygen, not with an
> actual deprecation attribute) in 81c623fae05 in 2011, but was
> undeprecated in ad1ee5fa7.
> ---
>   libavutil/frame.h   | 1 -
>   libavutil/version.h | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)

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.

I also don't understand why this is at the AVCodecContext level
and not packet/frame?

- Derek

On Mon, 29 Oct 2018, Derek Buitenhuis wrote:

> On 25/10/2018 13:58, Martin Storsjö wrote:
>> This was marked as deprecated (but only in the doxygen, not with an
>> actual deprecation attribute) in 81c623fae05 in 2011, but was
>> undeprecated in ad1ee5fa7.
>> ---
>>   libavutil/frame.h   | 1 -
>>   libavutil/version.h | 2 +-
>>   2 files changed, 1 insertion(+), 2 deletions(-)
>
> 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.

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

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.

// Martin

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

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

On 29/10/2018 20:10, Martin Storsjö wrote:
> 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.

Well, you're adding new code that uses it... which, in my mind is akin to
encouraging its use. But, yes, you're not technical undeprecating.

I'm kinda tempted to send a patch to actually deprecate it again, and
replace it with something less terrible. You've kinda shone a flashlight
on a particularily terrible (IMO) API, is all. Looking back at the list
where this was undeprecated, no real reasoning was given for keeping it
around in this form, AFAICT, and nobody asked either. It was kept because
it was effort to replace it, I guess.

I won't block the patch set, but will note that I object to continued
existence of such a terrible API, and adding new stuff that uses it.

- Derek