[FFmpeg-devel] avfilter/src_movie: dr support

Attached.

> From 6fddad6ea2abf91a7378fb367a439441ef8f32b4 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 2/3] avfilter/src_movie: dr support
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/src_movie.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
> index a7d7c188e6..0629030f91 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
>   */
>  
> @@ -158,6 +157,31 @@ static AVStream *find_stream(void *log, AVFormatContext *avf, const char *spec)
>      return found;
>  }
>  
> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
> +{
> +    AVFrame *new;
> +
> +    if (flags & AV_GET_BUFFER_FLAG_REF)
> +        return avcodec_default_get_buffer2(avctx, frame, flags);

Why? The frames returned by ff_get_{audio,video}_buffer() are refcounted 
and writable.

You should however use avcodec_default_get_buffer2() for non 
AV_CODEC_CAP_DR1 decoders. Also, you need to use 
avcodec_align_dimensions2() to get the correct dimensions.

> +
> +    switch (avctx->codec_type) {
> +    case AVMEDIA_TYPE_VIDEO:
> +        new = ff_get_video_buffer(frame->opaque, frame->width, frame->height);
> +        break;
> +    case AVMEDIA_TYPE_AUDIO:
> +        new = ff_get_audio_buffer(frame->opaque, frame->nb_samples);
> +        break;
> +    default:
> +        return -1;
> +    }
> +
> +    av_frame_unref(frame);

The documentation for get_buffer2() says:

>      * The following fields will be set in the frame before this callback is
>      * called:
>      * - format
>      * - width, height (video only)
>      * - sample_rate, channel_layout, nb_samples (audio only)
>      * Their values may differ from the corresponding values in
>      * AVCodecContext. This callback must use the frame values, not the codec
>      * context values, to calculate the required buffer size.

So you're not really supposed to touch anything else than data, buf, 
extended_data, extended_buf, and linesize, which an unref call will do. 
But as long as the necessary values in the AVFilterLink match those in 
the frame, i guess it should be ok.

> +    av_frame_move_ref(frame, new);
> +    av_frame_free(&new);
> +
> +    return 0;
> +}
> +
>  static int open_stream(AVFilterContext *ctx, MovieStream *st, int dec_threads)
>  {
>      const AVCodec *codec;
> @@ -173,6 +197,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;
> @@ -480,8 +506,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
>

New patch attached.

> From beaaca4147e4d0510393a2dc802fdaee60769f0f 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 2/3] avfilter/src_movie: dr support
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/src_movie.c | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
> index a7d7c188e6..22b8cbbd11 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
>   */
>  
> @@ -158,6 +157,38 @@ static AVStream *find_stream(void *log, AVFormatContext *avf, const char *spec)
>      return found;
>  }
>  
> +static int get_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
> +{
> +    AVFilterLink *outlink = frame->opaque;
> +    int h = frame->height;
> +    int w = frame->width;
> +    int linesize_align[AV_NUM_DATA_POINTERS];
> +    AVFrame *new;
> +
> +    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_move_ref(frame, new);

This may result in leaks as lavc calls ff_decode_frame_props() before 
avctx->get_buffer2(), which may add side data to the frame.
You should do:

av_frame_copy_props(new, frame);
av_frame_unref(frame);
av_frame_move_ref(frame, new);

> +    av_frame_free(&new);
> +
> +    frame->width  = outlink->w;
> +    frame->height = outlink->h;
> +
> +    return 0;
> +}
> +
>  static int open_stream(AVFilterContext *ctx, MovieStream *st, int dec_threads)
>  {
>      const AVCodec *codec;
> @@ -173,6 +204,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;
> @@ -480,8 +513,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
>

Yet another patch.

> From e1dc1a00ac327d450c33586269cb19230f433405 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 2/3] avfilter/src_movie: dr support
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/src_movie.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
> index a7d7c188e6..bc0b8b7ac6 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
>   */
>  
> @@ -158,6 +157,39 @@ 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 h = frame->height;
> +    int w = frame->width;
> +    AVFrame *new;
> +
> +    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, frame->width, frame->height);

You're not using the values from w and h at all. You need to allocate 
the frame buffers with them, since the point of this is padding the 
buffers based on the needs of each decoder. The AVFrame however must 
retain the non altered values.

> +        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;
> +
> +    return 0;
> +}
> +
>  static int open_stream(AVFilterContext *ctx, MovieStream *st, int dec_threads)
>  {
>      const AVCodec *codec;
> @@ -173,6 +205,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;
> @@ -480,8 +514,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
>

Attached.

On 5/16/2023 3:11 PM, Paul B Mahol wrote:
> Attached.

Nothing looks obviously wrong now, so if it's measurably faster and 
doesn't regress, then LGTM.