Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
From: Lynne <dev@lynne.ee>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH v4 15/16] FFHWAccel: add buffer_ref argument to start_frame
Date: Fri, 14 Mar 2025 03:23:54 +0100
Message-ID: <bc0843d4-8c1c-4cf6-8e05-f8dc43cb92ca@lynne.ee> (raw)
In-Reply-To: <444b61c3-18c4-4653-86a9-0a113edfac50@lynne.ee>

On 14/03/2025 00:03, Lynne wrote:
> 
> 
> On 13/03/2025 23:08, Andreas Rheinhardt wrote:
>> Lynne:
>>> This commit adds a reference to the buffer as an argument to
>>> start_frame, and adapts all existing code.
>>>
>>> This allows for asynchronous hardware accelerators to skip
>>> copying packet data by referencing it.
>>> ---
>>> diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c
>>> index da8fdc5935..20ef821819 100644
>>> --- a/libavcodec/hevc/hevcdec.c
>>> +++ b/libavcodec/hevc/hevcdec.c
>>> @@ -3401,7 +3401,10 @@ static int hevc_frame_start(HEVCContext *s, 
>>> HEVCLayerContext *l,
>>>           goto fail;
>>>       if (s->avctx->hwaccel) {
>>> -        ret = FF_HW_CALL(s->avctx, start_frame, NULL, 0);
>>> +        AVCodecInternal *avci = s->avctx->internal;
>>> +        AVPacket *avpkt = avci->in_pkt;
>>> +        ret = FF_HW_CALL(s->avctx, start_frame,
>>> +                         avpkt->buf, NULL, 0);
>>>           if (ret < 0)
>>>               goto fail;
>>>       }
>>> diff --git a/libavcodec/hwaccel_internal.h b/libavcodec/ 
>>> hwaccel_internal.h
>>> index 77df4e0904..4eb74f0b34 100644
>>> --- a/libavcodec/hwaccel_internal.h
>>> +++ b/libavcodec/hwaccel_internal.h
>>> @@ -52,11 +52,13 @@ typedef struct FFHWAccel {
>>>        * Otherwise, this means the whole frame is available at this 
>>> point.
>>>        *
>>>        * @param avctx the codec context
>>> +     * @param buf_ref the frame data buffer reference (optional)
>>>        * @param buf the frame data buffer base
>>>        * @param buf_size the size of the frame in bytes
>>>        * @return zero if successful, a negative value otherwise
>>>        */
>>> -    int (*start_frame)(AVCodecContext *avctx, const uint8_t *buf, 
>>> uint32_t buf_size);
>>> +    int (*start_frame)(AVCodecContext *avctx, AVBufferRef *buf_ref,
>>
>> const AVBufferRef*, we are not passing ownership
> 
> Done locally.
> 
>> Why did you not make this the last parameter (given that only very few
>> hwaccels will access it)?
> 
> It made more sense to have it upfront as it backs up the data given.
> I can change it, but I'd rather not, unless you have a strong opinion.
> 
>>> +                       const uint8_t *buf, uint32_t buf_size);
>>>       /**
>>>        * Callback for parameter data (SPS/PPS/VPS etc).
>>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
>>> index bd1b502e50..f3d940671e 100644
>>> --- a/libavcodec/mjpegdec.c
>>> +++ b/libavcodec/mjpegdec.c
>>> @@ -808,7 +808,7 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>>>           if (!s->hwaccel_picture_private)
>>>               return AVERROR(ENOMEM);
>>> -        ret = hwaccel->start_frame(s->avctx, s->raw_image_buffer,
>>> +        ret = hwaccel->start_frame(s->avctx, NULL, s->raw_image_buffer,
>>>                                      s->raw_image_buffer_size);
>>>           if (ret < 0)
>>>               return ret;
>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
>>> index ffe0710470..c21f220680 100644
>>> --- a/libavcodec/mpeg12dec.c
>>> +++ b/libavcodec/mpeg12dec.c
>>> @@ -1361,7 +1361,7 @@ static int mpeg_field_start(Mpeg1Context *s1, 
>>> const uint8_t *buf, int buf_size)
>>>       }
>>>       if (avctx->hwaccel) {
>>> -        if ((ret = FF_HW_CALL(avctx, start_frame, buf, buf_size)) < 0)
>>> +        if ((ret = FF_HW_CALL(avctx, start_frame, NULL, buf, 
>>> buf_size)) < 0)
>>>               return ret;
>>>       } else if (s->codec_tag == MKTAG('V', 'C', 'R', '2')) {
>>>           // Exchange UV
>>> diff --git a/libavcodec/nvdec_av1.c b/libavcodec/nvdec_av1.c
>>> index 6b408edb87..9b9be702d5 100644
>>> --- a/libavcodec/nvdec_vp9.c
>>> +++ b/libavcodec/nvdec_vp9.c
>>> @@ -29,7 +29,8 @@
>>>   #include "internal.h"
>>>   #include "vp9shared.h"
>>> -static int nvdec_vp9_start_frame(AVCodecContext *avctx, const 
>>> uint8_t *buffer, uint32_t size)
>>> +static int nvdec_vp9_start_frame(AVCodecContext *avctx, AVBufferRef 
>>> *buffer_ref,
>>> +                                 const uint8_t *buffer, uint32_t size)
>>>   {
>>>       VP9SharedContext *h = avctx->priv_data;
>>>       const AVPixFmtDescriptor *pixdesc = av_pix_fmt_desc_get(avctx- 
>>> >sw_pix_fmt);
>>> diff --git a/libavcodec/proresdec.c b/libavcodec/proresdec.c
>>> index 01caa611a0..edc46e1442 100644
>>> --- a/libavcodec/proresdec.c
>>> +++ b/libavcodec/proresdec.c
>>> @@ -793,7 +793,7 @@ static int decode_frame(AVCodecContext *avctx, 
>>> AVFrame *frame,
>>>       if (HWACCEL_MAX && avctx->hwaccel) {
>>>           const FFHWAccel *hwaccel = ffhwaccel(avctx->hwaccel);
>>> -        ret = hwaccel->start_frame(avctx, NULL, 0);
>>> +        ret = hwaccel->start_frame(avctx, avpkt->buf, NULL, 0);
>>
>> Passing an AVBufferRef for NULL data seems fishy.
> 
> I misinterpreted what NULL means.
> Changed this to be NULL.

Looked into proresdec.c and vp9.c, both of them had done the same,
start_frame(NULL, 0), and the code was copy pasted between both. But the 
size of the packet is already known in advance.
Changed to start_frame(avpkt->buf, avpkt->data, avpkt->size) for both.
mjpegdec.c is the only user of the API which has a genuine need to have 
buf as NULL, 0.

This isn't as niche as it sounds. This enables us to add a fast path for 
D3D12, Vulkan and likely VAAPI if anyone is interested.
_______________________________________________
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-14  2:24 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-13 17:03 [FFmpeg-devel] [PATCH v4 00/16] Add a Vulkan compute based FFv1 hwaccel Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 01/16] pixfmt: add AV_PIX_FMT_GBRAP32 Lynne
2025-03-14 15:13   ` Michael Niedermayer
2025-03-14 15:59     ` Lynne
2025-03-14 18:34       ` Marton Balint
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 02/16] vulkan: rename ff_vk_set_descriptor_image to ff_vk_shader_update_img Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 03/16] vulkan: add ff_vk_create_imageview Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 04/16] vulkan: copy host-mapping buffer code from hwcontext Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 05/16] hwcontext_vulkan: use the common host map function to map frame data Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 06/16] vulkan: workaround BGR storage image undefined behaviour Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 07/16] vulkan_decode: support software-defined decoders Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 08/16] vulkan_decode: support multiple image views Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 09/16] vulkan_decode: adjust number of async contexts created Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 10/16] ffv1enc_vulkan: refactor shaders slightly to support sharing Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 11/16] vulkan: unify handling of BGR and simplify ffv1_rct Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 12/16] vulkan: add ff_vk_exec_add_dep_wait_sem() Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 13/16] vulkan: add support for AV_PIX_FMT_GBRAP32 Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 14/16] ffv1dec: add support for hwaccels Lynne
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 15/16] FFHWAccel: add buffer_ref argument to start_frame Lynne
2025-03-13 22:08   ` Andreas Rheinhardt
2025-03-13 23:03     ` Lynne
2025-03-14  2:23       ` Lynne [this message]
2025-03-13 17:03 ` [FFmpeg-devel] [PATCH v4 16/16] ffv1: add a Vulkan-based decoder 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=bc0843d4-8c1c-4cf6-8e05-f8dc43cb92ca@lynne.ee \
    --to=dev@lynne.ee \
    --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