[FFmpeg-devel] [PATCH 1/3] ffmpeg: allocate the output hwaccel AVFrame only once

[Andreas Rheinhardt]

James Almer:
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  fftools/ffmpeg.c    |  3 +++
>  fftools/ffmpeg.h    |  1 +
>  fftools/ffmpeg_hw.c | 17 ++++++-----------
>  3 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index d141f34df9..26030ed25e 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -640,6 +640,7 @@ static void ffmpeg_cleanup(int ret)
>          av_frame_free(&ist->sub2video.frame);
>          av_freep(&ist->filters);
>          av_freep(&ist->hwaccel_device);
> +        av_frame_free(&ist->hwaccel_frame);
>          av_freep(&ist->dts_buffer);
>  
>          avcodec_free_context(&ist->dec_ctx);
> @@ -2412,6 +2413,8 @@ static int decode_video(InputStream *ist, AVPacket *pkt, int *got_output, int64_
>          return AVERROR(ENOMEM);
>      if (!ist->filter_frame && !(ist->filter_frame = av_frame_alloc()))
>          return AVERROR(ENOMEM);
> +    if (!ist->hwaccel_frame && !(ist->hwaccel_frame = av_frame_alloc()))
> +        return AVERROR(ENOMEM);
>      decoded_frame = ist->decoded_frame;
>      if (ist->dts != AV_NOPTS_VALUE)
>          dts = av_rescale_q(ist->dts, AV_TIME_BASE_Q, ist->st->time_base);
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 30225e9ffe..17135cb830 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -382,6 +382,7 @@ typedef struct InputStream {
>      enum AVPixelFormat hwaccel_pix_fmt;
>      enum AVPixelFormat hwaccel_retrieved_pix_fmt;
>      AVBufferRef *hw_frames_ctx;
> +    AVFrame *hwaccel_frame;
>  
>      /* stats */
>      // combined size of all the packets read
> diff --git a/fftools/ffmpeg_hw.c b/fftools/ffmpeg_hw.c
> index 14e702bd92..d5034d4f6a 100644
> --- a/fftools/ffmpeg_hw.c
> +++ b/fftools/ffmpeg_hw.c
> @@ -18,6 +18,7 @@
>  
>  #include <string.h>
>  
> +#include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavfilter/buffersink.h"
> @@ -500,7 +501,7 @@ int hw_device_setup_for_encode(OutputStream *ost)
>  static int hwaccel_retrieve_data(AVCodecContext *avctx, AVFrame *input)
>  {
>      InputStream *ist = avctx->opaque;
> -    AVFrame *output = NULL;
> +    AVFrame *output = ist->hwaccel_frame;
>      enum AVPixelFormat output_format = ist->hwaccel_output_format;
>      int err;
>  
> @@ -509,9 +510,8 @@ static int hwaccel_retrieve_data(AVCodecContext *avctx, AVFrame *input)
>          return 0;
>      }
>  
> -    output = av_frame_alloc();
> -    if (!output)
> -        return AVERROR(ENOMEM);
> +    av_assert0(output);
> +    av_frame_unref(output);

If you move the allocation of the frame here, you can avoid the
allocation in case no hwaccel as well as the assert.
The av_frame_unref() above is btw unnecessary.

>  
>      output->format = output_format;
>  
> @@ -519,24 +519,19 @@ static int hwaccel_retrieve_data(AVCodecContext *avctx, AVFrame *input)
>      if (err < 0) {
>          av_log(avctx, AV_LOG_ERROR, "Failed to transfer data to "
>                 "output frame: %d.\n", err);
> -        goto fail;
> +        return err;
>      }
>  
>      err = av_frame_copy_props(output, input);
>      if (err < 0) {
>          av_frame_unref(output);
> -        goto fail;
> +        return err;
>      }
>  
>      av_frame_unref(input);
>      av_frame_move_ref(input, output);
> -    av_frame_free(&output);

I wonder whether we should support src == dst in
av_hwframe_transfer_data() (potentially with a flag). This would make
this function and the implicit copying of side-data superfluous.
(av_hwframe_transfer_data() itself could btw. make use of the fact that
it lives in libavutil and avoid allocating an AVFrame).

>  
>      return 0;
> -
> -fail:
> -    av_frame_free(&output);
> -    return err;
>  }
>  
>  int hwaccel_decode_init(AVCodecContext *avctx)
>