diff mbox series

[FFmpeg-devel] avfilter/src_movie: activate & dr

Message ID CAPYw7P7gSv4zk0iwWrMm7YJ5u+AgPGrdGkp-5QY_S3SXe5rpOw@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] avfilter/src_movie: activate & dr | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished

Commit Message

Paul B Mahol May 18, 2023, 10:22 a.m. UTC
Attached.

Comments

James Almer May 18, 2023, 5:31 p.m. UTC | #1
On 5/18/2023 7:22 AM, Paul B Mahol wrote:
> From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00 2001
> From: Paul B Mahol <onemda@gmail.com>
> Date: Mon, 15 May 2023 21:54:25 +0200
> Subject: [PATCH 26/27] avfilter/src_movie: dr support
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/src_movie.c | 44 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
> index 5937613d13..a2ecc5a625 100644
> --- a/libavfilter/src_movie.c
> +++ b/libavfilter/src_movie.c
> @@ -23,7 +23,6 @@
>   * @file
>   * movie video source
>   *
> - * @todo use direct rendering (no allocation of a new frame)
>   * @todo support a PTS correction mechanism
>   */
>  
> @@ -156,6 +155,43 @@ static AVStream *find_stream(void *log, AVFormatContext *avf, const char *spec)
>      return found;
>  }
>  
> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
> +{
> +    int linesize_align[AV_NUM_DATA_POINTERS];
> +    AVFilterLink *outlink = frame->opaque;
> +    int w, h, ow, oh;
> +    AVFrame *new;
> +
> +    h = oh = frame->height;
> +    w = ow = frame->width;
> +
> +    if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1))
> +        return avcodec_default_get_buffer2(avctx, frame, flags);
> +
> +    switch (avctx->codec_type) {
> +    case AVMEDIA_TYPE_VIDEO:
> +        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
> +        new = ff_default_get_video_buffer(outlink, w, h);
> +        break;
> +    case AVMEDIA_TYPE_AUDIO:
> +        new = ff_default_get_audio_buffer(outlink, frame->nb_samples);

I wonder, are these two functions thread safe?
AVCodecContext.get_buffer2() no longer supports non-thread safe 
callbacks, and these seem to allocate and replace the pool inside 
outlink as required. If called from more than one thread with different 
frame parameters, wouldn't there be a race?

> +        break;
> +    default:
> +        return -1;
> +    }
> +
> +    av_frame_copy_props(new, frame);
> +    av_frame_unref(frame);
> +    av_frame_move_ref(frame, new);
> +    av_frame_free(&new);
> +
> +    frame->opaque = outlink;
> +    frame->width  = ow;
> +    frame->height = oh;
> +
> +    return 0;
> +}
> +
>  static int open_stream(AVFilterContext *ctx, MovieStream *st, int dec_threads)
>  {
>      const AVCodec *codec;
> @@ -171,6 +207,8 @@ static int open_stream(AVFilterContext *ctx, MovieStream *st, int dec_threads)
>      if (!st->codec_ctx)
>          return AVERROR(ENOMEM);
>  
> +    st->codec_ctx->flags |= AV_CODEC_FLAG_COPY_OPAQUE;
> +    st->codec_ctx->get_buffer2 = get_buffer;
>      ret = avcodec_parameters_to_context(st->codec_ctx, st->st->codecpar);
>      if (ret < 0)
>          return ret;
> @@ -479,8 +517,10 @@ static int movie_decode_packet(AVFilterContext *ctx)
>      /* send the packet to its decoder, if any */
>      pkt_out_id = pkt.stream_index > movie->max_stream_index ? -1 :
>                   movie->out_index[pkt.stream_index];
> -    if (pkt_out_id >= 0)
> +    if (pkt_out_id >= 0) {
> +        pkt.opaque = ctx->outputs[pkt_out_id];
>          ret = avcodec_send_packet(movie->st[pkt_out_id].codec_ctx, &pkt);
> +    }
>      av_packet_unref(&pkt);
>  
>      return ret;
> -- 
> 2.39.1
>
Paul B Mahol May 18, 2023, 5:44 p.m. UTC | #2
On Thu, May 18, 2023 at 7:31 PM James Almer <jamrial@gmail.com> wrote:

> On 5/18/2023 7:22 AM, Paul B Mahol wrote:
> > From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00 2001
> > From: Paul B Mahol <onemda@gmail.com>
> > Date: Mon, 15 May 2023 21:54:25 +0200
> > Subject: [PATCH 26/27] avfilter/src_movie: dr support
> >
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavfilter/src_movie.c | 44 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 42 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
> > index 5937613d13..a2ecc5a625 100644
> > --- a/libavfilter/src_movie.c
> > +++ b/libavfilter/src_movie.c
> > @@ -23,7 +23,6 @@
> >   * @file
> >   * movie video source
> >   *
> > - * @todo use direct rendering (no allocation of a new frame)
> >   * @todo support a PTS correction mechanism
> >   */
> >
> > @@ -156,6 +155,43 @@ static AVStream *find_stream(void *log,
> AVFormatContext *avf, const char *spec)
> >      return found;
> >  }
> >
> > +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
> > +{
> > +    int linesize_align[AV_NUM_DATA_POINTERS];
> > +    AVFilterLink *outlink = frame->opaque;
> > +    int w, h, ow, oh;
> > +    AVFrame *new;
> > +
> > +    h = oh = frame->height;
> > +    w = ow = frame->width;
> > +
> > +    if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1))
> > +        return avcodec_default_get_buffer2(avctx, frame, flags);
> > +
> > +    switch (avctx->codec_type) {
> > +    case AVMEDIA_TYPE_VIDEO:
> > +        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
> > +        new = ff_default_get_video_buffer(outlink, w, h);
> > +        break;
> > +    case AVMEDIA_TYPE_AUDIO:
> > +        new = ff_default_get_audio_buffer(outlink, frame->nb_samples);
>
> I wonder, are these two functions thread safe?
> AVCodecContext.get_buffer2() no longer supports non-thread safe
> callbacks, and these seem to allocate and replace the pool inside
> outlink as required. If called from more than one thread with different
> frame parameters, wouldn't there be a race?
>

I encountered no issues in my testing.


>
> > +        break;
> > +    default:
> > +        return -1;
> > +    }
> > +
> > +    av_frame_copy_props(new, frame);
> > +    av_frame_unref(frame);
> > +    av_frame_move_ref(frame, new);
> > +    av_frame_free(&new);
> > +
> > +    frame->opaque = outlink;
> > +    frame->width  = ow;
> > +    frame->height = oh;
> > +
> > +    return 0;
> > +}
> > +
> >  static int open_stream(AVFilterContext *ctx, MovieStream *st, int
> dec_threads)
> >  {
> >      const AVCodec *codec;
> > @@ -171,6 +207,8 @@ static int open_stream(AVFilterContext *ctx,
> MovieStream *st, int dec_threads)
> >      if (!st->codec_ctx)
> >          return AVERROR(ENOMEM);
> >
> > +    st->codec_ctx->flags |= AV_CODEC_FLAG_COPY_OPAQUE;
> > +    st->codec_ctx->get_buffer2 = get_buffer;
> >      ret = avcodec_parameters_to_context(st->codec_ctx,
> st->st->codecpar);
> >      if (ret < 0)
> >          return ret;
> > @@ -479,8 +517,10 @@ static int movie_decode_packet(AVFilterContext *ctx)
> >      /* send the packet to its decoder, if any */
> >      pkt_out_id = pkt.stream_index > movie->max_stream_index ? -1 :
> >                   movie->out_index[pkt.stream_index];
> > -    if (pkt_out_id >= 0)
> > +    if (pkt_out_id >= 0) {
> > +        pkt.opaque = ctx->outputs[pkt_out_id];
> >          ret = avcodec_send_packet(movie->st[pkt_out_id].codec_ctx,
> &pkt);
> > +    }
> >      av_packet_unref(&pkt);
> >
> >      return ret;
> > --
> > 2.39.1
> >
> _______________________________________________
> 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".
>
James Almer May 18, 2023, 6:01 p.m. UTC | #3
On 5/18/2023 2:44 PM, Paul B Mahol wrote:
> On Thu, May 18, 2023 at 7:31 PM James Almer <jamrial@gmail.com> wrote:
> 
>> On 5/18/2023 7:22 AM, Paul B Mahol wrote:
>>>  From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00 2001
>>> From: Paul B Mahol <onemda@gmail.com>
>>> Date: Mon, 15 May 2023 21:54:25 +0200
>>> Subject: [PATCH 26/27] avfilter/src_movie: dr support
>>>
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>>   libavfilter/src_movie.c | 44 +++++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 42 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
>>> index 5937613d13..a2ecc5a625 100644
>>> --- a/libavfilter/src_movie.c
>>> +++ b/libavfilter/src_movie.c
>>> @@ -23,7 +23,6 @@
>>>    * @file
>>>    * movie video source
>>>    *
>>> - * @todo use direct rendering (no allocation of a new frame)
>>>    * @todo support a PTS correction mechanism
>>>    */
>>>
>>> @@ -156,6 +155,43 @@ static AVStream *find_stream(void *log,
>> AVFormatContext *avf, const char *spec)
>>>       return found;
>>>   }
>>>
>>> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
>>> +{
>>> +    int linesize_align[AV_NUM_DATA_POINTERS];
>>> +    AVFilterLink *outlink = frame->opaque;
>>> +    int w, h, ow, oh;
>>> +    AVFrame *new;
>>> +
>>> +    h = oh = frame->height;
>>> +    w = ow = frame->width;
>>> +
>>> +    if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1))
>>> +        return avcodec_default_get_buffer2(avctx, frame, flags);
>>> +
>>> +    switch (avctx->codec_type) {
>>> +    case AVMEDIA_TYPE_VIDEO:
>>> +        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
>>> +        new = ff_default_get_video_buffer(outlink, w, h);
>>> +        break;
>>> +    case AVMEDIA_TYPE_AUDIO:
>>> +        new = ff_default_get_audio_buffer(outlink, frame->nb_samples);
>>
>> I wonder, are these two functions thread safe?
>> AVCodecContext.get_buffer2() no longer supports non-thread safe
>> callbacks, and these seem to allocate and replace the pool inside
>> outlink as required. If called from more than one thread with different
>> frame parameters, wouldn't there be a race?
>>
> 
> I encountered no issues in my testing.

Try a sample that changes dimensions every other frame, like 
vp9-test-vectors/vp90-2-05-resize.ivf, and make sure to set filter 
threads so it's propagated to the src_movie internal decoder, to some 
value like 4 or higher. Run it under tsan.

> 
> 
>>
>>> +        break;
>>> +    default:
>>> +        return -1;
>>> +    }
>>> +
>>> +    av_frame_copy_props(new, frame);
>>> +    av_frame_unref(frame);
>>> +    av_frame_move_ref(frame, new);
>>> +    av_frame_free(&new);
>>> +
>>> +    frame->opaque = outlink;
>>> +    frame->width  = ow;
>>> +    frame->height = oh;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int open_stream(AVFilterContext *ctx, MovieStream *st, int
>> dec_threads)
>>>   {
>>>       const AVCodec *codec;
>>> @@ -171,6 +207,8 @@ static int open_stream(AVFilterContext *ctx,
>> MovieStream *st, int dec_threads)
>>>       if (!st->codec_ctx)
>>>           return AVERROR(ENOMEM);
>>>
>>> +    st->codec_ctx->flags |= AV_CODEC_FLAG_COPY_OPAQUE;
>>> +    st->codec_ctx->get_buffer2 = get_buffer;
>>>       ret = avcodec_parameters_to_context(st->codec_ctx,
>> st->st->codecpar);
>>>       if (ret < 0)
>>>           return ret;
>>> @@ -479,8 +517,10 @@ static int movie_decode_packet(AVFilterContext *ctx)
>>>       /* send the packet to its decoder, if any */
>>>       pkt_out_id = pkt.stream_index > movie->max_stream_index ? -1 :
>>>                    movie->out_index[pkt.stream_index];
>>> -    if (pkt_out_id >= 0)
>>> +    if (pkt_out_id >= 0) {
>>> +        pkt.opaque = ctx->outputs[pkt_out_id];
>>>           ret = avcodec_send_packet(movie->st[pkt_out_id].codec_ctx,
>> &pkt);
>>> +    }
>>>       av_packet_unref(&pkt);
>>>
>>>       return ret;
>>> --
>>> 2.39.1
>>>
>> _______________________________________________
>> 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".
>>
> _______________________________________________
> 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".
Paul B Mahol May 19, 2023, 7:52 a.m. UTC | #4
On Thu, May 18, 2023 at 8:01 PM James Almer <jamrial@gmail.com> wrote:

> On 5/18/2023 2:44 PM, Paul B Mahol wrote:
> > On Thu, May 18, 2023 at 7:31 PM James Almer <jamrial@gmail.com> wrote:
> >
> >> On 5/18/2023 7:22 AM, Paul B Mahol wrote:
> >>>  From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00 2001
> >>> From: Paul B Mahol <onemda@gmail.com>
> >>> Date: Mon, 15 May 2023 21:54:25 +0200
> >>> Subject: [PATCH 26/27] avfilter/src_movie: dr support
> >>>
> >>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >>> ---
> >>>   libavfilter/src_movie.c | 44
> +++++++++++++++++++++++++++++++++++++++--
> >>>   1 file changed, 42 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
> >>> index 5937613d13..a2ecc5a625 100644
> >>> --- a/libavfilter/src_movie.c
> >>> +++ b/libavfilter/src_movie.c
> >>> @@ -23,7 +23,6 @@
> >>>    * @file
> >>>    * movie video source
> >>>    *
> >>> - * @todo use direct rendering (no allocation of a new frame)
> >>>    * @todo support a PTS correction mechanism
> >>>    */
> >>>
> >>> @@ -156,6 +155,43 @@ static AVStream *find_stream(void *log,
> >> AVFormatContext *avf, const char *spec)
> >>>       return found;
> >>>   }
> >>>
> >>> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int
> flags)
> >>> +{
> >>> +    int linesize_align[AV_NUM_DATA_POINTERS];
> >>> +    AVFilterLink *outlink = frame->opaque;
> >>> +    int w, h, ow, oh;
> >>> +    AVFrame *new;
> >>> +
> >>> +    h = oh = frame->height;
> >>> +    w = ow = frame->width;
> >>> +
> >>> +    if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1))
> >>> +        return avcodec_default_get_buffer2(avctx, frame, flags);
> >>> +
> >>> +    switch (avctx->codec_type) {
> >>> +    case AVMEDIA_TYPE_VIDEO:
> >>> +        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
> >>> +        new = ff_default_get_video_buffer(outlink, w, h);
> >>> +        break;
> >>> +    case AVMEDIA_TYPE_AUDIO:
> >>> +        new = ff_default_get_audio_buffer(outlink, frame->nb_samples);
> >>
> >> I wonder, are these two functions thread safe?
> >> AVCodecContext.get_buffer2() no longer supports non-thread safe
> >> callbacks, and these seem to allocate and replace the pool inside
> >> outlink as required. If called from more than one thread with different
> >> frame parameters, wouldn't there be a race?
> >>
> >
> > I encountered no issues in my testing.
>
> Try a sample that changes dimensions every other frame, like
> vp9-test-vectors/vp90-2-05-resize.ivf, and make sure to set filter
> threads so it's propagated to the src_movie internal decoder, to some
> value like 4 or higher. Run it under tsan.
>
>
I tried that, and also own .h264 file hash decoding is always same.
tsan shows warnings for both without and with this filter and frame
threading.


> >
> >
> >>
> >>> +        break;
> >>> +    default:
> >>> +        return -1;
> >>> +    }
> >>> +
> >>> +    av_frame_copy_props(new, frame);
> >>> +    av_frame_unref(frame);
> >>> +    av_frame_move_ref(frame, new);
> >>> +    av_frame_free(&new);
> >>> +
> >>> +    frame->opaque = outlink;
> >>> +    frame->width  = ow;
> >>> +    frame->height = oh;
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>>   static int open_stream(AVFilterContext *ctx, MovieStream *st, int
> >> dec_threads)
> >>>   {
> >>>       const AVCodec *codec;
> >>> @@ -171,6 +207,8 @@ static int open_stream(AVFilterContext *ctx,
> >> MovieStream *st, int dec_threads)
> >>>       if (!st->codec_ctx)
> >>>           return AVERROR(ENOMEM);
> >>>
> >>> +    st->codec_ctx->flags |= AV_CODEC_FLAG_COPY_OPAQUE;
> >>> +    st->codec_ctx->get_buffer2 = get_buffer;
> >>>       ret = avcodec_parameters_to_context(st->codec_ctx,
> >> st->st->codecpar);
> >>>       if (ret < 0)
> >>>           return ret;
> >>> @@ -479,8 +517,10 @@ static int movie_decode_packet(AVFilterContext
> *ctx)
> >>>       /* send the packet to its decoder, if any */
> >>>       pkt_out_id = pkt.stream_index > movie->max_stream_index ? -1 :
> >>>                    movie->out_index[pkt.stream_index];
> >>> -    if (pkt_out_id >= 0)
> >>> +    if (pkt_out_id >= 0) {
> >>> +        pkt.opaque = ctx->outputs[pkt_out_id];
> >>>           ret = avcodec_send_packet(movie->st[pkt_out_id].codec_ctx,
> >> &pkt);
> >>> +    }
> >>>       av_packet_unref(&pkt);
> >>>
> >>>       return ret;
> >>> --
> >>> 2.39.1
> >>>
> >> _______________________________________________
> >> 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".
> >>
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
>
Paul B Mahol May 19, 2023, 8:07 a.m. UTC | #5
On Fri, May 19, 2023 at 9:52 AM Paul B Mahol <onemda@gmail.com> wrote:

>
>
> On Thu, May 18, 2023 at 8:01 PM James Almer <jamrial@gmail.com> wrote:
>
>> On 5/18/2023 2:44 PM, Paul B Mahol wrote:
>> > On Thu, May 18, 2023 at 7:31 PM James Almer <jamrial@gmail.com> wrote:
>> >
>> >> On 5/18/2023 7:22 AM, Paul B Mahol wrote:
>> >>>  From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00
>> 2001
>> >>> From: Paul B Mahol <onemda@gmail.com>
>> >>> Date: Mon, 15 May 2023 21:54:25 +0200
>> >>> Subject: [PATCH 26/27] avfilter/src_movie: dr support
>> >>>
>> >>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> >>> ---
>> >>>   libavfilter/src_movie.c | 44
>> +++++++++++++++++++++++++++++++++++++++--
>> >>>   1 file changed, 42 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
>> >>> index 5937613d13..a2ecc5a625 100644
>> >>> --- a/libavfilter/src_movie.c
>> >>> +++ b/libavfilter/src_movie.c
>> >>> @@ -23,7 +23,6 @@
>> >>>    * @file
>> >>>    * movie video source
>> >>>    *
>> >>> - * @todo use direct rendering (no allocation of a new frame)
>> >>>    * @todo support a PTS correction mechanism
>> >>>    */
>> >>>
>> >>> @@ -156,6 +155,43 @@ static AVStream *find_stream(void *log,
>> >> AVFormatContext *avf, const char *spec)
>> >>>       return found;
>> >>>   }
>> >>>
>> >>> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int
>> flags)
>> >>> +{
>> >>> +    int linesize_align[AV_NUM_DATA_POINTERS];
>> >>> +    AVFilterLink *outlink = frame->opaque;
>> >>> +    int w, h, ow, oh;
>> >>> +    AVFrame *new;
>> >>> +
>> >>> +    h = oh = frame->height;
>> >>> +    w = ow = frame->width;
>> >>> +
>> >>> +    if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1))
>> >>> +        return avcodec_default_get_buffer2(avctx, frame, flags);
>> >>> +
>> >>> +    switch (avctx->codec_type) {
>> >>> +    case AVMEDIA_TYPE_VIDEO:
>> >>> +        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
>> >>> +        new = ff_default_get_video_buffer(outlink, w, h);
>> >>> +        break;
>> >>> +    case AVMEDIA_TYPE_AUDIO:
>> >>> +        new = ff_default_get_audio_buffer(outlink,
>> frame->nb_samples);
>> >>
>> >> I wonder, are these two functions thread safe?
>> >> AVCodecContext.get_buffer2() no longer supports non-thread safe
>> >> callbacks, and these seem to allocate and replace the pool inside
>> >> outlink as required. If called from more than one thread with different
>> >> frame parameters, wouldn't there be a race?
>> >>
>> >
>> > I encountered no issues in my testing.
>>
>> Try a sample that changes dimensions every other frame, like
>> vp9-test-vectors/vp90-2-05-resize.ivf, and make sure to set filter
>> threads so it's propagated to the src_movie internal decoder, to some
>> value like 4 or higher. Run it under tsan.
>>
>>
> I tried that, and also own .h264 file hash decoding is always same.
> tsan shows warnings for both without and with this filter and frame
> threading.
>

Only issue I found is if you use tmix filter after: ffmpeg -f lavfi -i
movie=resolution_change_name.file,tmix
It will produce invalid output with MD5 hash different each time, same as
with threads=1
But exact same issue is without this 2 patches, and present in current
version of FFmpeg.

Nice workaround is just inserting scale filter between movie and tmix.


>
>> >
>> >
>> >>
>> >>> +        break;
>> >>> +    default:
>> >>> +        return -1;
>> >>> +    }
>> >>> +
>> >>> +    av_frame_copy_props(new, frame);
>> >>> +    av_frame_unref(frame);
>> >>> +    av_frame_move_ref(frame, new);
>> >>> +    av_frame_free(&new);
>> >>> +
>> >>> +    frame->opaque = outlink;
>> >>> +    frame->width  = ow;
>> >>> +    frame->height = oh;
>> >>> +
>> >>> +    return 0;
>> >>> +}
>> >>> +
>> >>>   static int open_stream(AVFilterContext *ctx, MovieStream *st, int
>> >> dec_threads)
>> >>>   {
>> >>>       const AVCodec *codec;
>> >>> @@ -171,6 +207,8 @@ static int open_stream(AVFilterContext *ctx,
>> >> MovieStream *st, int dec_threads)
>> >>>       if (!st->codec_ctx)
>> >>>           return AVERROR(ENOMEM);
>> >>>
>> >>> +    st->codec_ctx->flags |= AV_CODEC_FLAG_COPY_OPAQUE;
>> >>> +    st->codec_ctx->get_buffer2 = get_buffer;
>> >>>       ret = avcodec_parameters_to_context(st->codec_ctx,
>> >> st->st->codecpar);
>> >>>       if (ret < 0)
>> >>>           return ret;
>> >>> @@ -479,8 +517,10 @@ static int movie_decode_packet(AVFilterContext
>> *ctx)
>> >>>       /* send the packet to its decoder, if any */
>> >>>       pkt_out_id = pkt.stream_index > movie->max_stream_index ? -1 :
>> >>>                    movie->out_index[pkt.stream_index];
>> >>> -    if (pkt_out_id >= 0)
>> >>> +    if (pkt_out_id >= 0) {
>> >>> +        pkt.opaque = ctx->outputs[pkt_out_id];
>> >>>           ret = avcodec_send_packet(movie->st[pkt_out_id].codec_ctx,
>> >> &pkt);
>> >>> +    }
>> >>>       av_packet_unref(&pkt);
>> >>>
>> >>>       return ret;
>> >>> --
>> >>> 2.39.1
>> >>>
>> >> _______________________________________________
>> >> 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".
>> >>
>> > _______________________________________________
>> > 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".
>> _______________________________________________
>> 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".
>>
>
Anton Khirnov May 22, 2023, 2:48 p.m. UTC | #6
Quoting Paul B Mahol (2023-05-18 12:22:07)
> From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00 2001
> From: Paul B Mahol <onemda@gmail.com>
> Date: Mon, 15 May 2023 21:54:25 +0200
> Subject: [PATCH 26/27] avfilter/src_movie: dr support
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/src_movie.c | 44 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
> index 5937613d13..a2ecc5a625 100644
> --- a/libavfilter/src_movie.c
> +++ b/libavfilter/src_movie.c
> @@ -23,7 +23,6 @@
>   * @file
>   * movie video source
>   *
> - * @todo use direct rendering (no allocation of a new frame)
>   * @todo support a PTS correction mechanism
>   */
>  
> @@ -156,6 +155,43 @@ static AVStream *find_stream(void *log, AVFormatContext *avf, const char *spec)
>      return found;
>  }
>  
> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
> +{
> +    int linesize_align[AV_NUM_DATA_POINTERS];
> +    AVFilterLink *outlink = frame->opaque;

This is not guaranteed to be set. Use AVCodecContext.opaque instead.

> +    int w, h, ow, oh;
> +    AVFrame *new;
> +
> +    h = oh = frame->height;
> +    w = ow = frame->width;
> +
> +    if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1))
> +        return avcodec_default_get_buffer2(avctx, frame, flags);
> +
> +    switch (avctx->codec_type) {
> +    case AVMEDIA_TYPE_VIDEO:
> +        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
> +        new = ff_default_get_video_buffer(outlink, w, h);
> +        break;
> +    case AVMEDIA_TYPE_AUDIO:
> +        new = ff_default_get_audio_buffer(outlink, frame->nb_samples);
> +        break;
> +    default:
> +        return -1;
> +    }
> +
> +    av_frame_copy_props(new, frame);
> +    av_frame_unref(frame);
> +    av_frame_move_ref(frame, new);
> +    av_frame_free(&new);
> +
> +    frame->opaque = outlink;

For what purpose?
James Almer May 22, 2023, 2:55 p.m. UTC | #7
On 5/22/2023 11:48 AM, Anton Khirnov wrote:
> Quoting Paul B Mahol (2023-05-18 12:22:07)
>>  From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00 2001
>> From: Paul B Mahol <onemda@gmail.com>
>> Date: Mon, 15 May 2023 21:54:25 +0200
>> Subject: [PATCH 26/27] avfilter/src_movie: dr support
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>   libavfilter/src_movie.c | 44 +++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
>> index 5937613d13..a2ecc5a625 100644
>> --- a/libavfilter/src_movie.c
>> +++ b/libavfilter/src_movie.c
>> @@ -23,7 +23,6 @@
>>    * @file
>>    * movie video source
>>    *
>> - * @todo use direct rendering (no allocation of a new frame)
>>    * @todo support a PTS correction mechanism
>>    */
>>   
>> @@ -156,6 +155,43 @@ static AVStream *find_stream(void *log, AVFormatContext *avf, const char *spec)
>>       return found;
>>   }
>>   
>> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
>> +{
>> +    int linesize_align[AV_NUM_DATA_POINTERS];
>> +    AVFilterLink *outlink = frame->opaque;
> 
> This is not guaranteed to be set. Use AVCodecContext.opaque instead.

How so? He set the AV_CODEC_FLAG_COPY_OPAQUE flag and the pkt.opaque 
field before calling avcodec_send_packet().

Also, this version is out of date. He pushed a more complete one (that 
could have sent to the list, admittedly).

> 
>> +    int w, h, ow, oh;
>> +    AVFrame *new;
>> +
>> +    h = oh = frame->height;
>> +    w = ow = frame->width;
>> +
>> +    if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1))
>> +        return avcodec_default_get_buffer2(avctx, frame, flags);
>> +
>> +    switch (avctx->codec_type) {
>> +    case AVMEDIA_TYPE_VIDEO:
>> +        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
>> +        new = ff_default_get_video_buffer(outlink, w, h);
>> +        break;
>> +    case AVMEDIA_TYPE_AUDIO:
>> +        new = ff_default_get_audio_buffer(outlink, frame->nb_samples);
>> +        break;
>> +    default:
>> +        return -1;
>> +    }
>> +
>> +    av_frame_copy_props(new, frame);
>> +    av_frame_unref(frame);
>> +    av_frame_move_ref(frame, new);
>> +    av_frame_free(&new);
>> +
>> +    frame->opaque = outlink;
> 
> For what purpose?
>
James Almer May 22, 2023, 3 p.m. UTC | #8
On 5/22/2023 11:55 AM, James Almer wrote:
> On 5/22/2023 11:48 AM, Anton Khirnov wrote:
>> Quoting Paul B Mahol (2023-05-18 12:22:07)
>>>  From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00 2001
>>> From: Paul B Mahol <onemda@gmail.com>
>>> Date: Mon, 15 May 2023 21:54:25 +0200
>>> Subject: [PATCH 26/27] avfilter/src_movie: dr support
>>>
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>>   libavfilter/src_movie.c | 44 +++++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 42 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
>>> index 5937613d13..a2ecc5a625 100644
>>> --- a/libavfilter/src_movie.c
>>> +++ b/libavfilter/src_movie.c
>>> @@ -23,7 +23,6 @@
>>>    * @file
>>>    * movie video source
>>>    *
>>> - * @todo use direct rendering (no allocation of a new frame)
>>>    * @todo support a PTS correction mechanism
>>>    */
>>> @@ -156,6 +155,43 @@ static AVStream *find_stream(void *log, 
>>> AVFormatContext *avf, const char *spec)
>>>       return found;
>>>   }
>>> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
>>> +{
>>> +    int linesize_align[AV_NUM_DATA_POINTERS];
>>> +    AVFilterLink *outlink = frame->opaque;
>>
>> This is not guaranteed to be set. Use AVCodecContext.opaque instead.
> 
> How so? He set the AV_CODEC_FLAG_COPY_OPAQUE flag and the pkt.opaque 
> field before calling avcodec_send_packet().

Oh, i see now reading the doxy for the flag.
diff mbox series

Patch

From af73b69a0be9033fddf222b6e9ac60799de85691 Mon Sep 17 00:00:00 2001
From: Paul B Mahol <onemda@gmail.com>
Date: Mon, 15 May 2023 21:54:25 +0200
Subject: [PATCH 26/27] avfilter/src_movie: dr support

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavfilter/src_movie.c | 44 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
index 5937613d13..a2ecc5a625 100644
--- a/libavfilter/src_movie.c
+++ b/libavfilter/src_movie.c
@@ -23,7 +23,6 @@ 
  * @file
  * movie video source
  *
- * @todo use direct rendering (no allocation of a new frame)
  * @todo support a PTS correction mechanism
  */
 
@@ -156,6 +155,43 @@  static AVStream *find_stream(void *log, AVFormatContext *avf, const char *spec)
     return found;
 }
 
+static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
+{
+    int linesize_align[AV_NUM_DATA_POINTERS];
+    AVFilterLink *outlink = frame->opaque;
+    int w, h, ow, oh;
+    AVFrame *new;
+
+    h = oh = frame->height;
+    w = ow = frame->width;
+
+    if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1))
+        return avcodec_default_get_buffer2(avctx, frame, flags);
+
+    switch (avctx->codec_type) {
+    case AVMEDIA_TYPE_VIDEO:
+        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
+        new = ff_default_get_video_buffer(outlink, w, h);
+        break;
+    case AVMEDIA_TYPE_AUDIO:
+        new = ff_default_get_audio_buffer(outlink, frame->nb_samples);
+        break;
+    default:
+        return -1;
+    }
+
+    av_frame_copy_props(new, frame);
+    av_frame_unref(frame);
+    av_frame_move_ref(frame, new);
+    av_frame_free(&new);
+
+    frame->opaque = outlink;
+    frame->width  = ow;
+    frame->height = oh;
+
+    return 0;
+}
+
 static int open_stream(AVFilterContext *ctx, MovieStream *st, int dec_threads)
 {
     const AVCodec *codec;
@@ -171,6 +207,8 @@  static int open_stream(AVFilterContext *ctx, MovieStream *st, int dec_threads)
     if (!st->codec_ctx)
         return AVERROR(ENOMEM);
 
+    st->codec_ctx->flags |= AV_CODEC_FLAG_COPY_OPAQUE;
+    st->codec_ctx->get_buffer2 = get_buffer;
     ret = avcodec_parameters_to_context(st->codec_ctx, st->st->codecpar);
     if (ret < 0)
         return ret;
@@ -479,8 +517,10 @@  static int movie_decode_packet(AVFilterContext *ctx)
     /* send the packet to its decoder, if any */
     pkt_out_id = pkt.stream_index > movie->max_stream_index ? -1 :
                  movie->out_index[pkt.stream_index];
-    if (pkt_out_id >= 0)
+    if (pkt_out_id >= 0) {
+        pkt.opaque = ctx->outputs[pkt_out_id];
         ret = avcodec_send_packet(movie->st[pkt_out_id].codec_ctx, &pkt);
+    }
     av_packet_unref(&pkt);
 
     return ret;
-- 
2.39.1