Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 11/13] ffv1dec: reference the current packet into the main context
Date: Thu, 13 Mar 2025 18:48:01 +0100
Message-ID: <AS8P250MB0744E15CBF40D1BADF2BF7C98FD32@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <2321de1c-92a4-4e8e-97a0-e794690b4e0c@lynne.ee>

Lynne:
> On 13/03/2025 05:57, Andreas Rheinhardt wrote:
>> Lynne:
>>>
>>>
>>> On 13/03/2025 02:24, Andreas Rheinhardt wrote:
>>>> Lynne:
>>>>> On 10/03/2025 18:42, Lynne wrote:
>>>>>> On 10/03/2025 04:14, Andreas Rheinhardt wrote:
>>>>>>> Lynne:
>>>>>>>> ---
>>>>>>>>     libavcodec/ffv1.h    |  3 +++
>>>>>>>>     libavcodec/ffv1dec.c | 19 +++++++++++++++++--
>>>>>>>>     2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h
>>>>>>>> index 8c0e71284d..860a5c14b1 100644
>>>>>>>> --- a/libavcodec/ffv1.h
>>>>>>>> +++ b/libavcodec/ffv1.h
>>>>>>>> @@ -174,6 +174,9 @@ typedef struct FFV1Context {
>>>>>>>>          * NOT shared between frame threads.
>>>>>>>>          */
>>>>>>>>         uint8_t           frame_damaged;
>>>>>>>> +
>>>>>>>> +    /* Reference to the current packet */
>>>>>>>> +    AVPacket *pkt_ref;
>>>>>>>>     } FFV1Context;
>>>>>>>>     int ff_ffv1_common_init(AVCodecContext *avctx, FFV1Context *s);
>>>>>>>> diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
>>>>>>>> index eaa21eebdf..6396f22f79 100644
>>>>>>>> --- a/libavcodec/ffv1dec.c
>>>>>>>> +++ b/libavcodec/ffv1dec.c
>>>>>>>> @@ -469,6 +469,10 @@ static av_cold int decode_init(AVCodecContext
>>>>>>>> *avctx)
>>>>>>>>         f->pix_fmt = AV_PIX_FMT_NONE;
>>>>>>>>         f->configured_pix_fmt = AV_PIX_FMT_NONE;
>>>>>>>> +    f->pkt_ref = av_packet_alloc();
>>>>>>>> +    if (!f->pkt_ref)
>>>>>>>> +        return AVERROR(ENOMEM);
>>>>>>>> +
>>>>>>>>         if ((ret = ff_ffv1_common_init(avctx, f)) < 0)
>>>>>>>>             return ret;
>>>>>>>> @@ -701,6 +705,10 @@ static int decode_frame(AVCodecContext *avctx,
>>>>>>>> AVFrame *rframe,
>>>>>>>>         /* Start */
>>>>>>>>         if (hwaccel) {
>>>>>>>> +        ret = av_packet_ref(f->pkt_ref, avpkt);
>>>>>>>> +        if (ret < 0)
>>>>>>>> +            return ret;
>>>>>>>> +
>>>>>>>>             ret = hwaccel->start_frame(avctx, avpkt->data, avpkt-
>>>>>>>>> size);
>>>>>>>>             if (ret < 0)
>>>>>>>>                 return ret;
>>>>>>>> @@ -720,15 +728,21 @@ static int decode_frame(AVCodecContext
>>>>>>>> *avctx,
>>>>>>>> AVFrame *rframe,
>>>>>>>>                 uint32_t len;
>>>>>>>>                 ret = find_next_slice(avctx, avpkt->data,
>>>>>>>> buf_end, i,
>>>>>>>>                                       &pos, &len);
>>>>>>>> -            if (ret < 0)
>>>>>>>> +            if (ret < 0) {
>>>>>>>> +                av_packet_unref(f->pkt_ref);
>>>>>>>>                     return ret;
>>>>>>>> +            }
>>>>>>>>                 buf_end -= len;
>>>>>>>>                 ret = hwaccel->decode_slice(avctx, pos, len);
>>>>>>>> -            if (ret < 0)
>>>>>>>> +            if (ret < 0) {
>>>>>>>> +                av_packet_unref(f->pkt_ref);
>>>>>>>>                     return ret;
>>>>>>>> +            }
>>>>>>>>             }
>>>>>>>> +
>>>>>>>> +        av_packet_unref(f->pkt_ref);
>>>>>>>>         } else {
>>>>>>>>             ret = decode_slices(avctx, c, avpkt);
>>>>>>>>             if (ret < 0)
>>>>>>>> @@ -827,6 +841,7 @@ static av_cold int
>>>>>>>> ffv1_decode_close(AVCodecContext *avctx)
>>>>>>>>         ff_progress_frame_unref(&s->last_picture);
>>>>>>>>         av_refstruct_unref(&s->hwaccel_last_picture_private);
>>>>>>>> +    av_packet_free(&s->pkt_ref);
>>>>>>>>         ff_ffv1_close(s);
>>>>>>>>         return 0;
>>>>>>>
>>>>>>> Why not simply use a const AVPacket*?
>>>>>>
>>>>>> No reason. Fixed locally.
>>>>>> Thanks.
>>>>>
>>>>> *reverted this change.
>>>>> We need to ref the packet, since we map its memory and let the GPU use
>>>>> it directly without copying the contents. 6k16bit content at 24fps is
>>>>> typically around 2Gbps when compressed, so avoiding copies is
>>>>> important.
>>>>
>>>> How long does the hwaccel need this data?
>>>
>>> Until the frame has been asynchronously decoded. We give an output frame
>>> with a semaphore that receivers need to wait on to determine when
>>> that is.

Does this mean that the frames the caller of avcodec_receive_frame()
receives are not completely decoded yet?

>>>
>>> On the decoder-side, the hardware has a fixed number of queues where
>>> submissions can be sent to asynchronously. We treat it as a ring buffer
>>> and keep a reference to all resources our side for each submission,
>>> until we need to reuse the slot, at which point we wait on the frame
>>> decoding to complete (which it usually has), and we release all
>>> resources used.
>>>
>>> Output frames also have a bit of state that has to be freed once the
>>> frame is marked (unreferenced) by the decoder as no longer being needed
>>> as a reference, this is done in the FFHWAccel.free_frame_priv callback.
>>> There, we have to wait for the last internal use of the frame to be
>>> finished (done via the vp->wait_semaphores() call in vulkan_decode.c).
>>>
>>> This is valid for both ASIC hardware decoders and a compute shader based
>>> implementation, since the two share the same code, except for decode
>>> submissions.
>>
>> 1. If you need a reference to the packet's data, then reference
>> AVPacket.buf, not the whole AVPacket. This avoids allocating a spare
>> AVPacket as well as copying side data.
>> 2. It sounds very wrong and fragile that the decoder has to keep a
>> reference because the hwaccel might need it. There may be future
>> hwaccels that don't need such a reference etc. It seems better to extend
>> e.g. the start_frame callback and pass a reference to the input data (no
>> need to change this for all other start_frame calls; they can pass NULL
>> until needed).
>> 3. If the user closes the decoder (which is allowed at any time, even
>> without draining the decoder), ff_codec_close() uninitializes the
>> hwaccel after calling the decoder's close function; the latter
>> unreferences the reference to the packet. Is this really safe?
> 
> I wanted to avoid having to change the FFHWAccel API for a single
> hwaccel, so I opted to simply add a field to the codec private context.
> I'll add an argument to start_frame and resubmit.
> 
> I don't see why it wouldn't be safe, upon uninit, all submissions are
> waited on to complete before unreferencing all resources they held.
> The output frames themselves have no references to any resources used
> during decoding, so all resources are guaranteed to be freed at uninit.

The problem is that codec close might free the packet's data and the
decoder could still process it (or what guarantees that it doesn't?).

- Andreas

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

  reply	other threads:[~2025-03-13 17:48 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-10  3:08 [FFmpeg-devel] [PATCH 01/13] vulkan: rename ff_vk_set_descriptor_image to ff_vk_shader_update_img Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 02/13] vulkan: add ff_vk_create_imageview Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 03/13] vulkan: copy host-mapping buffer code from hwcontext Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 04/13] vulkan: workaround BGR storage image undefined behaviour Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 05/13] vulkan_decode: support software-defined decoders Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 06/13] vulkan_decode: support multiple image views Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 07/13] vulkan_decode: adjust number of async contexts created Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 08/13] ffv1enc_vulkan: refactor shaders slightly to support sharing Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 09/13] vulkan: unify handling of BGR and simplify ffv1_rct Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 10/13] ffv1dec: add support for hwaccels Lynne
2025-03-10  3:08 ` [FFmpeg-devel] [PATCH 11/13] ffv1dec: reference the current packet into the main context Lynne
2025-03-10  3:08   ` [FFmpeg-devel] [PATCH 12/13] vulkan: add ff_vk_exec_add_dep_wait_sem() Lynne
2025-03-10  3:08   ` [FFmpeg-devel] [PATCH 13/13] ffv1: add a Vulkan-based decoder Lynne
2025-03-10  3:14   ` [FFmpeg-devel] [PATCH 11/13] ffv1dec: reference the current packet into the main context Andreas Rheinhardt
2025-03-10 17:42     ` Lynne
2025-03-13  0:30       ` Lynne
2025-03-13  1:24         ` Andreas Rheinhardt
2025-03-13  1:56           ` Lynne
2025-03-13  4:57             ` Andreas Rheinhardt
2025-03-13 12:51               ` Lynne
2025-03-13 17:48                 ` Andreas Rheinhardt [this message]
2025-03-13 18:45                   ` Lynne

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AS8P250MB0744E15CBF40D1BADF2BF7C98FD32@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM \
    --to=andreas.rheinhardt@outlook.com \
    --cc=ffmpeg-devel@ffmpeg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git