| Message ID | 20191017201103.3092-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 2019-10-17 23:11, James Almer wrote: > Actually reorder the values. > > Should effectively fix ticket #8300. > > Signed-off-by: James Almer <jamrial@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) { > + AVBufferRef *reordered_opaque = av_buffer_alloc(sizeof(c->reordered_opaque)); > + > + if (!reordered_opaque) { > + dav1d_data_unref(data); > + return AVERROR(ENOMEM); > + } > + > + *reordered_opaque->data = c->reordered_opaque; This slices int64_t to uint8_t. Should memcpy instead. > + res = dav1d_data_wrap_user_data(data, reordered_opaque->data, > + libdav1d_data_free, reordered_opaque); > + if (res < 0) { > + av_buffer_unref(&reordered_opaque); > + dav1d_data_unref(data); > + return res; > + } > + } > } > } > > @@ -260,7 +278,8 @@ static int libdav1d_receive_frame(AVCodecContext *c, AVFrame *frame) > else > frame->format = c->pix_fmt = pix_fmt[p->p.layout][p->seq_hdr->hbd]; > > - frame->reordered_opaque = c->reordered_opaque; > + if (p->m.user_data.data) > + frame->reordered_opaque = *(int64_t *)p->m.user_data.data; > > // match timestamps and packet size > frame->pts = frame->best_effort_timestamp = p->m.timestamp; >
On 2019-10-18 00:43, 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@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) { >> + AVBufferRef *reordered_opaque = >> av_buffer_alloc(sizeof(c->reordered_opaque)); >> + >> + if (!reordered_opaque) { >> + dav1d_data_unref(data); >> + return AVERROR(ENOMEM); >> + } >> + >> + *reordered_opaque->data = c->reordered_opaque; > > This slices int64_t to uint8_t. Should memcpy instead. Also, would it be possible to save at least one allocation by saving a pointer to int64_t or a struct containing int64_t? AVBufferRef seems unnecessary. >> + res = dav1d_data_wrap_user_data(data, >> reordered_opaque->data, >> + libdav1d_data_free, >> reordered_opaque); >> + if (res < 0) { >> + av_buffer_unref(&reordered_opaque); >> + dav1d_data_unref(data); >> + return res; >> + } >> + } >> } >> } >> @@ -260,7 +278,8 @@ static int libdav1d_receive_frame(AVCodecContext >> *c, AVFrame *frame) >> else >> frame->format = c->pix_fmt = >> pix_fmt[p->p.layout][p->seq_hdr->hbd]; >> - frame->reordered_opaque = c->reordered_opaque; >> + if (p->m.user_data.data) >> + frame->reordered_opaque = *(int64_t *)p->m.user_data.data; >> // match timestamps and packet size >> frame->pts = frame->best_effort_timestamp = p->m.timestamp; >> >
On 10/17/2019 6:43 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@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) { >> + AVBufferRef *reordered_opaque = >> av_buffer_alloc(sizeof(c->reordered_opaque)); >> + >> + if (!reordered_opaque) { >> + dav1d_data_unref(data); >> + return AVERROR(ENOMEM); >> + } >> + >> + *reordered_opaque->data = c->reordered_opaque; > > This slices int64_t to uint8_t. Should memcpy instead. Good catch. Changed locally to *(int64_t *)reordered_opaque->data = c->reordered_opaque; > >> + res = dav1d_data_wrap_user_data(data, >> reordered_opaque->data, >> + libdav1d_data_free, >> reordered_opaque); >> + if (res < 0) { >> + av_buffer_unref(&reordered_opaque); >> + dav1d_data_unref(data); >> + return res; >> + } >> + } >> } >> } >> @@ -260,7 +278,8 @@ static int libdav1d_receive_frame(AVCodecContext >> *c, AVFrame *frame) >> else >> frame->format = c->pix_fmt = >> pix_fmt[p->p.layout][p->seq_hdr->hbd]; >> - frame->reordered_opaque = c->reordered_opaque; >> + if (p->m.user_data.data) >> + frame->reordered_opaque = *(int64_t *)p->m.user_data.data; >> // match timestamps and packet size >> frame->pts = frame->best_effort_timestamp = p->m.timestamp; >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 2019-10-17 23:11, James Almer wrote: > Actually reorder the values. > > Should effectively fix ticket #8300. > > Signed-off-by: James Almer <jamrial@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. > + AVBufferRef *reordered_opaque = av_buffer_alloc(sizeof(c->reordered_opaque)); > + > + if (!reordered_opaque) { > + dav1d_data_unref(data); > + return AVERROR(ENOMEM); > + } > + > + *reordered_opaque->data = c->reordered_opaque; > + res = dav1d_data_wrap_user_data(data, reordered_opaque->data, > + libdav1d_data_free, reordered_opaque); > + if (res < 0) { > + av_buffer_unref(&reordered_opaque); > + dav1d_data_unref(data); > + return res; > + } > + } > } > } > > @@ -260,7 +278,8 @@ static int libdav1d_receive_frame(AVCodecContext *c, AVFrame *frame) > else > frame->format = c->pix_fmt = pix_fmt[p->p.layout][p->seq_hdr->hbd]; > > - frame->reordered_opaque = c->reordered_opaque; > + if (p->m.user_data.data) > + frame->reordered_opaque = *(int64_t *)p->m.user_data.data; > > // match timestamps and packet size > frame->pts = frame->best_effort_timestamp = p->m.timestamp; >
On Fri, Oct 18, 2019 at 12:13 AM Andrey Semashev <andrey.semashev@gmail.com> 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@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. > AV_NOPTS_VALUE is the default value of that field in AVCodecContext (see init_context_defaults in avcodec\options.c), so we can easily avoid an allocation while conveying the same value, which will happen a lot since the field isn't used everywhere. I'm not sure if AVFrame also defaults to that value, but even if it doesn't, further down it could just set the field to that if no value is provided. - Hendrik
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@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. 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. > >> + AVBufferRef *reordered_opaque = >> av_buffer_alloc(sizeof(c->reordered_opaque)); >> + >> + if (!reordered_opaque) { >> + dav1d_data_unref(data); >> + return AVERROR(ENOMEM); >> + } >> + >> + *reordered_opaque->data = c->reordered_opaque; >> + res = dav1d_data_wrap_user_data(data, >> reordered_opaque->data, >> + libdav1d_data_free, >> reordered_opaque); >> + if (res < 0) { >> + av_buffer_unref(&reordered_opaque); >> + dav1d_data_unref(data); >> + return res; >> + } >> + } >> } >> } >> @@ -260,7 +278,8 @@ static int libdav1d_receive_frame(AVCodecContext >> *c, AVFrame *frame) >> else >> frame->format = c->pix_fmt = >> pix_fmt[p->p.layout][p->seq_hdr->hbd]; >> - frame->reordered_opaque = c->reordered_opaque; >> + if (p->m.user_data.data) >> + frame->reordered_opaque = *(int64_t *)p->m.user_data.data; >> // match timestamps and packet size >> frame->pts = frame->best_effort_timestamp = p->m.timestamp; >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 10/17/2019 6:57 PM, Andrey Semashev wrote: > On 2019-10-18 00:43, 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@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) { >>> + AVBufferRef *reordered_opaque = >>> av_buffer_alloc(sizeof(c->reordered_opaque)); >>> + >>> + if (!reordered_opaque) { >>> + dav1d_data_unref(data); >>> + return AVERROR(ENOMEM); >>> + } >>> + >>> + *reordered_opaque->data = c->reordered_opaque; >> >> This slices int64_t to uint8_t. Should memcpy instead. > > Also, would it be possible to save at least one allocation by saving a > pointer to int64_t or a struct containing int64_t? AVBufferRef seems > unnecessary. The doxy for the free() function called by libdav1d to free user data states it needs to be thread safe, so i wasn't sure if av_malloc/av_free on const uint8_t *user_data was a good idea, but on second thought i guess it's meant to warn about code handling global state. Will change to a normal alloc/free combination. > >>> + res = dav1d_data_wrap_user_data(data, >>> reordered_opaque->data, >>> + libdav1d_data_free, >>> reordered_opaque); >>> + if (res < 0) { >>> + av_buffer_unref(&reordered_opaque); >>> + dav1d_data_unref(data); >>> + return res; >>> + } >>> + } >>> } >>> } >>> @@ -260,7 +278,8 @@ static int libdav1d_receive_frame(AVCodecContext >>> *c, AVFrame *frame) >>> else >>> frame->format = c->pix_fmt = >>> pix_fmt[p->p.layout][p->seq_hdr->hbd]; >>> - frame->reordered_opaque = c->reordered_opaque; >>> + if (p->m.user_data.data) >>> + frame->reordered_opaque = *(int64_t *)p->m.user_data.data; >>> // match timestamps and packet size >>> frame->pts = frame->best_effort_timestamp = p->m.timestamp; >>> >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 17.10.2019 23:59, James Almer wrote: > On 10/17/2019 6:43 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@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) { >>> + AVBufferRef *reordered_opaque = >>> av_buffer_alloc(sizeof(c->reordered_opaque)); >>> + >>> + if (!reordered_opaque) { >>> + dav1d_data_unref(data); >>> + return AVERROR(ENOMEM); >>> + } >>> + >>> + *reordered_opaque->data = c->reordered_opaque; >> >> This slices int64_t to uint8_t. Should memcpy instead. > > Good catch. Changed locally to > > *(int64_t *)reordered_opaque->data = c->reordered_opaque; Doesn't that violate strict aliasing rules? I'd just use memcpy.
On 2019-10-18 01:27, Hendrik Leppkes wrote: > On Fri, Oct 18, 2019 at 12:13 AM Andrey Semashev > <andrey.semashev@gmail.com> 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@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. >> > > AV_NOPTS_VALUE is the default value of that field in AVCodecContext > (see init_context_defaults in avcodec\options.c), so we can easily > avoid an allocation while conveying the same value, which will happen > a lot since the field isn't used everywhere. > I'm not sure if AVFrame also defaults to that value, but even if it > doesn't, further down it could just set the field to that if no value > is provided. AVFrame::reordered_opaque is zero by default, I looked at get_frame_defaults.
On 10/17/2019 7:27 PM, Hendrik Leppkes wrote: > On Fri, Oct 18, 2019 at 12:13 AM Andrey Semashev > <andrey.semashev@gmail.com> 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@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. >> > > AV_NOPTS_VALUE is the default value of that field in AVCodecContext > (see init_context_defaults in avcodec\options.c), so we can easily > avoid an allocation while conveying the same value, which will happen > a lot since the field isn't used everywhere. > I'm not sure if AVFrame also defaults to that value, but even if it > doesn't, further down it could just set the field to that if no value > is provided. AVFrame sets it to 0 in get_frame_defaults in frame.h I guess we could effectively set it to NOPTS in libdav1d.c if no value is set in avctx->reordered_opaque. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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@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.
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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, Oct 18, 2019 at 1:16 AM James Almer <jamrial@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@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
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@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.
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@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.
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) { + AVBufferRef *reordered_opaque = av_buffer_alloc(sizeof(c->reordered_opaque)); + + if (!reordered_opaque) { + dav1d_data_unref(data); + return AVERROR(ENOMEM); + } + + *reordered_opaque->data = c->reordered_opaque; + res = dav1d_data_wrap_user_data(data, reordered_opaque->data, + libdav1d_data_free, reordered_opaque); + if (res < 0) { + av_buffer_unref(&reordered_opaque); + dav1d_data_unref(data); + return res; + } + } } } @@ -260,7 +278,8 @@ static int libdav1d_receive_frame(AVCodecContext *c, AVFrame *frame) else frame->format = c->pix_fmt = pix_fmt[p->p.layout][p->seq_hdr->hbd]; - frame->reordered_opaque = c->reordered_opaque; + if (p->m.user_data.data) + frame->reordered_opaque = *(int64_t *)p->m.user_data.data; // match timestamps and packet size frame->pts = frame->best_effort_timestamp = p->m.timestamp;
Actually reorder the values. Should effectively fix ticket #8300. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/libdav1d.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)