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

[Andrey Semashev]

On 2019-10-18 10:01, Andrey Semashev wrote:
> 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.

I've posted v3 patch with what I had in mind.