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

[Lynne <dev@lynne.ee>]

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