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

[Andrey Semashev]

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.

In any case, I've sent my version of the patch without the check before 
reading the replies. Feel free to ignore it, if you want to keep the check.