[FFmpeg-devel] [PATCH] vulkan_hevc: switch from a buffer pool to a simple malloc

[FFmpeg-devel] [PATCH] vulkan_hevc: switch from a buffer pool to a simple malloc

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 92 bytes --]

Simpler and more robust now that contexts are not shared between threads.

Patch attached.


[-- Attachment #2: 0001-vulkan_hevc-switch-from-a-buffer-pool-to-a-simple-ma.patch --]
[-- Type: text/x-diff, Size: 4761 bytes --]

From f476162c2b0b165f5cb23398bf6b989b4503daee Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Fri, 15 Sep 2023 01:51:49 +0200
Subject: [PATCH] vulkan_hevc: switch from a buffer pool to a simple malloc

Simpler and more robust now that contexts are not shared between threads.
---
 libavcodec/vulkan_decode.c |  2 +-
 libavcodec/vulkan_decode.h |  4 ++--
 libavcodec/vulkan_hevc.c   | 33 ++++++++++++++-------------------
 3 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/libavcodec/vulkan_decode.c b/libavcodec/vulkan_decode.c
index 3986330c81..3253bf6fef 100644
--- a/libavcodec/vulkan_decode.c
+++ b/libavcodec/vulkan_decode.c
@@ -1075,7 +1075,7 @@ int ff_vk_decode_uninit(AVCodecContext *avctx)
     /* Wait on and free execution pool */
     ff_vk_exec_pool_free(&ctx->s, &dec->exec_pool);
 
-    av_buffer_pool_uninit(&dec->tmp_pool);
+    av_freep(&dec->hevc_headers);
     av_buffer_unref(&dec->session_params);
     av_buffer_unref(&dec->shared_ref);
     av_freep(&dec->slice_off);
diff --git a/libavcodec/vulkan_decode.h b/libavcodec/vulkan_decode.h
index c50527e5f8..65e883a044 100644
--- a/libavcodec/vulkan_decode.h
+++ b/libavcodec/vulkan_decode.h
@@ -64,8 +64,8 @@ typedef struct FFVulkanDecodeContext {
     uint32_t frame_id_alloc_mask; /* For AV1 only */
 
     /* Thread-local state below */
-    AVBufferPool *tmp_pool; /* Pool for temporary data, if needed (HEVC) */
-    size_t tmp_pool_ele_size;
+    struct HEVCHeaderSet *hevc_headers;
+    size_t hevc_headers_size;
 
     uint32_t                       *slice_off;
     unsigned int                    slice_off_max;
diff --git a/libavcodec/vulkan_hevc.c b/libavcodec/vulkan_hevc.c
index 672694a19d..faf8959ca5 100644
--- a/libavcodec/vulkan_hevc.c
+++ b/libavcodec/vulkan_hevc.c
@@ -68,10 +68,11 @@ typedef struct HEVCHeaderSet {
     HEVCHeaderVPS *hvps;
 } HEVCHeaderSet;
 
-static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
-                            int nb_vps, AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
+static int alloc_hevc_header_structs(FFVulkanDecodeContext *s,
+                                     int nb_vps,
+                                     AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
 {
-    uint8_t *data_ptr;
+    uintptr_t data_ptr;
     HEVCHeaderSet *hdr;
 
     size_t base_size = sizeof(StdVideoH265SequenceParameterSet)*HEVC_MAX_SPS_COUNT +
@@ -93,22 +94,18 @@ static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
         buf_size += sizeof(HEVCHeaderVPSSet)*vps->vps_num_hrd_parameters;
     }
 
-    if (buf_size > s->tmp_pool_ele_size) {
-        av_buffer_pool_uninit(&s->tmp_pool);
-        s->tmp_pool_ele_size = 0;
-        s->tmp_pool = av_buffer_pool_init(buf_size, NULL);
-        if (!s->tmp_pool)
+    if (buf_size > s->hevc_headers_size) {
+        av_freep(&s->hevc_headers);
+        s->hevc_headers_size = 0;
+        s->hevc_headers = av_mallocz(buf_size);
+        if (!s->hevc_headers)
             return AVERROR(ENOMEM);
-        s->tmp_pool_ele_size = buf_size;
+        s->hevc_headers_size = buf_size;
     }
 
-    *data_buf = av_buffer_pool_get(s->tmp_pool);
-    if (!(*data_buf))
-        return AVERROR(ENOMEM);
-
     /* Setup pointers */
-    data_ptr = (*data_buf)->data;
-    hdr = (HEVCHeaderSet *)data_ptr;
+    hdr = s->hevc_headers;
+    data_ptr = (uintptr_t)hdr;
     hdr->hvps = (HEVCHeaderVPS *)(data_ptr + base_size);
     data_ptr += base_size + vps_size*nb_vps;
     for (int i = 0; i < nb_vps; i++) {
@@ -674,7 +671,6 @@ static int vk_hevc_create_params(AVCodecContext *avctx, AVBufferRef **buf)
     };
 
     int nb_vps = 0;
-    AVBufferRef *data_set;
     HEVCHeaderSet *hdr;
 
     AVBufferRef *tmp;
@@ -685,11 +681,11 @@ static int vk_hevc_create_params(AVCodecContext *avctx, AVBufferRef **buf)
     for (int i = 0; h->ps.vps_list[i]; i++)
         nb_vps++;
 
-    err = get_data_set_buf(dec, &data_set, nb_vps, h->ps.vps_list);
+    err = alloc_hevc_header_structs(dec, nb_vps, h->ps.vps_list);
     if (err < 0)
         return err;
 
-    hdr = (HEVCHeaderSet *)data_set->data;
+    hdr = dec->hevc_headers;
 
     h265_params_info.pStdSPSs = hdr->sps;
     h265_params_info.pStdPPSs = hdr->pps;
@@ -728,7 +724,6 @@ static int vk_hevc_create_params(AVCodecContext *avctx, AVBufferRef **buf)
     /* Create session parameters */
     ret = vk->CreateVideoSessionParametersKHR(ctx->s.hwctx->act_dev, &session_params_create,
                                               ctx->s.hwctx->alloc, par);
-    av_buffer_unref(&data_set);
     if (ret != VK_SUCCESS) {
         av_log(avctx, AV_LOG_ERROR, "Unable to create Vulkan video session parameters: %s!\n",
                ff_vk_ret2str(ret));
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

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

Re: [FFmpeg-devel] [PATCH] vulkan_hevc: switch from a buffer pool to a simple malloc

[Andreas Rheinhardt]

Lynne:
> -static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
> -                            int nb_vps, AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
> +static int alloc_hevc_header_structs(FFVulkanDecodeContext *s,
> +                                     int nb_vps,
> +                                     AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
>  {
> -    uint8_t *data_ptr;
> +    uintptr_t data_ptr;
>      HEVCHeaderSet *hdr;
>  
>      size_t base_size = sizeof(StdVideoH265SequenceParameterSet)*HEVC_MAX_SPS_COUNT +
> @@ -93,22 +94,18 @@ static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
>          buf_size += sizeof(HEVCHeaderVPSSet)*vps->vps_num_hrd_parameters;
>      }
>  
> -    if (buf_size > s->tmp_pool_ele_size) {
> -        av_buffer_pool_uninit(&s->tmp_pool);
> -        s->tmp_pool_ele_size = 0;
> -        s->tmp_pool = av_buffer_pool_init(buf_size, NULL);
> -        if (!s->tmp_pool)
> +    if (buf_size > s->hevc_headers_size) {
> +        av_freep(&s->hevc_headers);
> +        s->hevc_headers_size = 0;
> +        s->hevc_headers = av_mallocz(buf_size);

The earlier code did not zero-initialize the buffer. I thought the
set_[vsp]ps functions were enough to initialize them.

> +        if (!s->hevc_headers)
>              return AVERROR(ENOMEM);
> -        s->tmp_pool_ele_size = buf_size;
> +        s->hevc_headers_size = buf_size;
>      }
>  
> -    *data_buf = av_buffer_pool_get(s->tmp_pool);
> -    if (!(*data_buf))
> -        return AVERROR(ENOMEM);
> -
>      /* Setup pointers */
> -    data_ptr = (*data_buf)->data;
> -    hdr = (HEVCHeaderSet *)data_ptr;
> +    hdr = s->hevc_headers;
> +    data_ptr = (uintptr_t)hdr;
>      hdr->hvps = (HEVCHeaderVPS *)(data_ptr + base_size);

The standard way to get pointer that is at a certain byte offset from a
given pointer is by casting said pointer to char* and applying the
offset; casting to uintptr_t and back will work, but is actually not
guaranteed. In fact, the whole data_ptr variable seems unnecessary.
You may use FF_FIELD_AT from lavu/internal.h if you think that it is
helpful for this. (I don't think so.)

>      data_ptr += base_size + vps_size*nb_vps;
>      for (int i = 0; i < nb_vps; i++) {

LGTM apart from these nits.

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

Re: [FFmpeg-devel] [PATCH] vulkan_hevc: switch from a buffer pool to a simple malloc

[Lynne]

Sep 15, 2023, 03:00 by andreas.rheinhardt@outlook.com:

> Lynne:
>
>> -static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
>> -                            int nb_vps, AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
>> +static int alloc_hevc_header_structs(FFVulkanDecodeContext *s,
>> +                                     int nb_vps,
>> +                                     AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
>>  {
>> -    uint8_t *data_ptr;
>> +    uintptr_t data_ptr;
>>  HEVCHeaderSet *hdr;
>>  
>>  size_t base_size = sizeof(StdVideoH265SequenceParameterSet)*HEVC_MAX_SPS_COUNT +
>> @@ -93,22 +94,18 @@ static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
>>  buf_size += sizeof(HEVCHeaderVPSSet)*vps->vps_num_hrd_parameters;
>>  }
>>  
>> -    if (buf_size > s->tmp_pool_ele_size) {
>> -        av_buffer_pool_uninit(&s->tmp_pool);
>> -        s->tmp_pool_ele_size = 0;
>> -        s->tmp_pool = av_buffer_pool_init(buf_size, NULL);
>> -        if (!s->tmp_pool)
>> +    if (buf_size > s->hevc_headers_size) {
>> +        av_freep(&s->hevc_headers);
>> +        s->hevc_headers_size = 0;
>> +        s->hevc_headers = av_mallocz(buf_size);
>>
>
> The earlier code did not zero-initialize the buffer. I thought the
> set_[vsp]ps functions were enough to initialize them.
>

Better to be on the safe side, in case I've forgotten to set a reserved
field (or better yet, I should remove references to them in case they're
renamed.


>> +        if (!s->hevc_headers)
>>  return AVERROR(ENOMEM);
>> -        s->tmp_pool_ele_size = buf_size;
>> +        s->hevc_headers_size = buf_size;
>>  }
>>  
>> -    *data_buf = av_buffer_pool_get(s->tmp_pool);
>> -    if (!(*data_buf))
>> -        return AVERROR(ENOMEM);
>> -
>>  /* Setup pointers */
>> -    data_ptr = (*data_buf)->data;
>> -    hdr = (HEVCHeaderSet *)data_ptr;
>> +    hdr = s->hevc_headers;
>> +    data_ptr = (uintptr_t)hdr;
>>  hdr->hvps = (HEVCHeaderVPS *)(data_ptr + base_size);
>>
>
> The standard way to get pointer that is at a certain byte offset from a
> given pointer is by casting said pointer to char* and applying the
> offset; casting to uintptr_t and back will work, but is actually not
> guaranteed. In fact, the whole data_ptr variable seems unnecessary.
> You may use FF_FIELD_AT from lavu/internal.h if you think that it is
> helpful for this. (I don't think so.)
>

Couldn't really think of a way to remove the data_ptr field.
Do you have any ideas? If not, I'll push this later today.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] vulkan_hevc: switch from a buffer pool to a simple malloc

[Andreas Rheinhardt]

Lynne:
> Sep 15, 2023, 03:00 by andreas.rheinhardt@outlook.com:
> 
>> Lynne:
>>
>>> -static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
>>> -                            int nb_vps, AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
>>> +static int alloc_hevc_header_structs(FFVulkanDecodeContext *s,
>>> +                                     int nb_vps,
>>> +                                     AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
>>>  {
>>> -    uint8_t *data_ptr;
>>> +    uintptr_t data_ptr;
>>>  HEVCHeaderSet *hdr;
>>>  
>>>  size_t base_size = sizeof(StdVideoH265SequenceParameterSet)*HEVC_MAX_SPS_COUNT +
>>> @@ -93,22 +94,18 @@ static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
>>>  buf_size += sizeof(HEVCHeaderVPSSet)*vps->vps_num_hrd_parameters;
>>>  }
>>>  
>>> -    if (buf_size > s->tmp_pool_ele_size) {
>>> -        av_buffer_pool_uninit(&s->tmp_pool);
>>> -        s->tmp_pool_ele_size = 0;
>>> -        s->tmp_pool = av_buffer_pool_init(buf_size, NULL);
>>> -        if (!s->tmp_pool)
>>> +    if (buf_size > s->hevc_headers_size) {
>>> +        av_freep(&s->hevc_headers);
>>> +        s->hevc_headers_size = 0;
>>> +        s->hevc_headers = av_mallocz(buf_size);
>>>
>>
>> The earlier code did not zero-initialize the buffer. I thought the
>> set_[vsp]ps functions were enough to initialize them.
>>
> 
> Better to be on the safe side, in case I've forgotten to set a reserved
> field (or better yet, I should remove references to them in case they're
> renamed.
> 
> 
>>> +        if (!s->hevc_headers)
>>>  return AVERROR(ENOMEM);
>>> -        s->tmp_pool_ele_size = buf_size;
>>> +        s->hevc_headers_size = buf_size;
>>>  }
>>>  
>>> -    *data_buf = av_buffer_pool_get(s->tmp_pool);
>>> -    if (!(*data_buf))
>>> -        return AVERROR(ENOMEM);
>>> -
>>>  /* Setup pointers */
>>> -    data_ptr = (*data_buf)->data;
>>> -    hdr = (HEVCHeaderSet *)data_ptr;
>>> +    hdr = s->hevc_headers;
>>> +    data_ptr = (uintptr_t)hdr;
>>>  hdr->hvps = (HEVCHeaderVPS *)(data_ptr + base_size);
>>>
>>
>> The standard way to get pointer that is at a certain byte offset from a
>> given pointer is by casting said pointer to char* and applying the
>> offset; casting to uintptr_t and back will work, but is actually not
>> guaranteed. In fact, the whole data_ptr variable seems unnecessary.
>> You may use FF_FIELD_AT from lavu/internal.h if you think that it is
>> helpful for this. (I don't think so.)
>>
> 
> Couldn't really think of a way to remove the data_ptr field.
> Do you have any ideas? If not, I'll push this later today.

Ok, I now see that data_ptr is used for more than one line and in a way
which makes it hard to avoid. So keep it, but make it char*.

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

Re: [FFmpeg-devel] [PATCH] vulkan_hevc: switch from a buffer pool to a simple malloc

[Lynne]

Sep 15, 2023, 10:34 by andreas.rheinhardt@outlook.com:

> Lynne:
>
>> Sep 15, 2023, 03:00 by andreas.rheinhardt@outlook.com:
>>
>>> Lynne:
>>>
>>>> -static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
>>>> -                            int nb_vps, AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
>>>> +static int alloc_hevc_header_structs(FFVulkanDecodeContext *s,
>>>> +                                     int nb_vps,
>>>> +                                     AVBufferRef * const vps_list[HEVC_MAX_VPS_COUNT])
>>>>  {
>>>> -    uint8_t *data_ptr;
>>>> +    uintptr_t data_ptr;
>>>>  HEVCHeaderSet *hdr;
>>>>  
>>>>  size_t base_size = sizeof(StdVideoH265SequenceParameterSet)*HEVC_MAX_SPS_COUNT +
>>>> @@ -93,22 +94,18 @@ static int get_data_set_buf(FFVulkanDecodeContext *s, AVBufferRef **data_buf,
>>>>  buf_size += sizeof(HEVCHeaderVPSSet)*vps->vps_num_hrd_parameters;
>>>>  }
>>>>  
>>>> -    if (buf_size > s->tmp_pool_ele_size) {
>>>> -        av_buffer_pool_uninit(&s->tmp_pool);
>>>> -        s->tmp_pool_ele_size = 0;
>>>> -        s->tmp_pool = av_buffer_pool_init(buf_size, NULL);
>>>> -        if (!s->tmp_pool)
>>>> +    if (buf_size > s->hevc_headers_size) {
>>>> +        av_freep(&s->hevc_headers);
>>>> +        s->hevc_headers_size = 0;
>>>> +        s->hevc_headers = av_mallocz(buf_size);
>>>>
>>>
>>> The earlier code did not zero-initialize the buffer. I thought the
>>> set_[vsp]ps functions were enough to initialize them.
>>>
>>
>> Better to be on the safe side, in case I've forgotten to set a reserved
>> field (or better yet, I should remove references to them in case they're
>> renamed.
>>
>>
>>>> +        if (!s->hevc_headers)
>>>>  return AVERROR(ENOMEM);
>>>> -        s->tmp_pool_ele_size = buf_size;
>>>> +        s->hevc_headers_size = buf_size;
>>>>  }
>>>>  
>>>> -    *data_buf = av_buffer_pool_get(s->tmp_pool);
>>>> -    if (!(*data_buf))
>>>> -        return AVERROR(ENOMEM);
>>>> -
>>>>  /* Setup pointers */
>>>> -    data_ptr = (*data_buf)->data;
>>>> -    hdr = (HEVCHeaderSet *)data_ptr;
>>>> +    hdr = s->hevc_headers;
>>>> +    data_ptr = (uintptr_t)hdr;
>>>>  hdr->hvps = (HEVCHeaderVPS *)(data_ptr + base_size);
>>>>
>>>
>>> The standard way to get pointer that is at a certain byte offset from a
>>> given pointer is by casting said pointer to char* and applying the
>>> offset; casting to uintptr_t and back will work, but is actually not
>>> guaranteed. In fact, the whole data_ptr variable seems unnecessary.
>>> You may use FF_FIELD_AT from lavu/internal.h if you think that it is
>>> helpful for this. (I don't think so.)
>>>
>>
>> Couldn't really think of a way to remove the data_ptr field.
>> Do you have any ideas? If not, I'll push this later today.
>>
>
> Ok, I now see that data_ptr is used for more than one line and in a way
> which makes it hard to avoid. So keep it, but make it char*.
>

Changed to uint8_t *, removed the manual
struct size calculations and pushed.

Thanks for the reviews
_______________________________________________
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".