| Message ID | NeL6DSd--3-9@lynne.ee |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] vulkan_hevc: switch from a buffer pool to a simple malloc | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure_x86 | warning | Failed to apply patch |
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
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.
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
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
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