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

[Andreas Rheinhardt]

James Almer:
> On 11/18/2021 5:35 AM, Andreas Rheinhardt wrote:
>> 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.
> 
> Here where? You mean in hw_device_setup_for_encode() above?
> 

"Here" as in "exactly the same place where the packet is currently
allocated".

>>
>>>         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)
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".