[FFmpeg-devel] [PATCH] avcodec/libdav1d: fix setting AVFrame reordered_opaque

[Andrey Semashev]

On 2019-10-18 02:16, James Almer wrote:
> On 10/17/2019 7:46 PM, Andrey Semashev wrote:
>> On 2019-10-18 01:28, James Almer wrote:
>>> On 10/17/2019 7:13 PM, Andrey Semashev wrote:
>>>> On 2019-10-17 23:11, James Almer wrote:
>>>>> Actually reorder the values.
>>>>>
>>>>> Should effectively fix ticket #8300.
>>>>>
>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>>> ---
>>>>>     libavcodec/libdav1d.c | 21 ++++++++++++++++++++-
>>>>>     1 file changed, 20 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
>>>>> index 8aa248e6cd..87abdb4569 100644
>>>>> --- a/libavcodec/libdav1d.c
>>>>> +++ b/libavcodec/libdav1d.c
>>>>> @@ -191,6 +191,24 @@ static int libdav1d_receive_frame(AVCodecContext
>>>>> *c, AVFrame *frame)
>>>>>                   pkt.buf = NULL;
>>>>>                 av_packet_unref(&pkt);
>>>>> +
>>>>> +            if (c->reordered_opaque != AV_NOPTS_VALUE) {
>>>>
>>>> I'm not sure this comparison is valid. The default value of
>>>> reordered_opaque is 0, and there seems to be no convention whatsoever
>>>> about what this value represents (i.e. its semantics are completely
>>>> user-defined). I think, this check needs to be removed and the code
>>>> below should execute for any reordered_opaque values.
>>>
>>> AVCodecContext->reordered_opaque is by default AV_NOPTS_VALUE, as set in
>>> avcodec_alloc_context3(). This field is normally used for timestamps
>>> (despite not being the only use), and 0 is a valid timestamp, so that
>>> can't be considered a "not set" value.
>>
>> Ok, I see. I was looking at AVFrame initialization, which sets it to 0
>> by default.
>>
>>> And I'd rather not make this code unconditional. It's an allocation per
>>> frame in a zero copy optimized decoder, and i don't want that overhead
>>> when reordered_opaque is rarely going to be used.
>>>
>>> Fwiw, timestamps are properly reordered by libdav1d in this same
>>> function and propagated in the output frame. reordered_opaque is not
>>> really needed for them.
>>
>> FWIW, I'm the reporter of #8300 and our main reason for using
>> reordered_opaque instead of pts is that we don't want to mess with
>> timestamp conversion between our time base and whatever time_base
>> libavcodec might select for a given codec. So, we use reordered_opaque
>> universally, and it just happened to break with libdav1d.
>>
>> Testing for AV_NOPTS_VALUE is ok in our particular case, though I had
>> the impression that ffmpeg is not supposed to interpret
>> reordered_opaque, including not assume it contains a timestamp.
> 
> Unfortunately you're right, and the check is probably not proper use of
> the field, even if valid for pretty much any normal use case for it.
> 
> Will remove it, and simply deal with the malloc overhead.

You could avoid malloc completely on 64-bit systems by passing 
reordered_opaque inside the pointer to user data. It's not pretty, but 
it would get the job done. 32-bit systems would still have to malloc, 
unfortunately.