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".
next prev parent 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