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

[James Almer]

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.

> 
> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".