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

[Hendrik Leppkes]

On Fri, Oct 18, 2019 at 1:16 AM James Almer <jamrial at gmail.com> 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.
>

The check is fine. The only thing that needs to happen is that the
same value is passed through, and implicitily assuming that its NOPTS
(the AVCodecContext default) when no user data was allocated will
ensure just that.
Make the code further down set it to NOPTS if no userdata is present,
and its absolutely correct.

- Hendrik