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

[James Almer]

On 11/18/2021 8:05 PM, Andreas Rheinhardt wrote:
> 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".

Ah, i remember now i was going to rewrite this patch to match what i was 
doing in "ffmpeg: Allocate (In|Out)putStream.filter_frame early", so 
I'll do that.

> 
>>>
>>>>          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".
> 
> _______________________________________________
> 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".
>