[FFmpeg-devel] [PATCH] avocdec/hevc_ps: don't use a fixed sized buffer for parameter set raw data
James Almer
jamrial at gmail.com
Fri Mar 29 18:17:00 EET 2024
On 3/29/2024 1:10 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Allocate it instead, and use it to compare sets instead of the parsed struct.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> libavcodec/hevc_ps.c | 84 ++++++++++++++++++++++----------------------
>> libavcodec/hevc_ps.h | 14 +++++---
>> 2 files changed, 51 insertions(+), 47 deletions(-)
>>
>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>> index 6475d86d7d..e417039d76 100644
>> --- a/libavcodec/hevc_ps.c
>> +++ b/libavcodec/hevc_ps.c
>> @@ -442,20 +442,18 @@ static int decode_hrd(GetBitContext *gb, int common_inf_present,
>> return 0;
>> }
>>
>> -static void uninit_vps(FFRefStructOpaque opaque, void *obj)
>> +static void hevc_vps_free(FFRefStructOpaque opaque, void *obj)
>> {
>> HEVCVPS *vps = obj;
>>
>> av_freep(&vps->hdr);
>> + av_freep(&vps->data);
>> }
>>
>> static int compare_vps(const HEVCVPS *vps1, const HEVCVPS *vps2)
>> {
>> - if (!memcmp(vps1, vps2, offsetof(HEVCVPS, hdr)))
>> - return !vps1->vps_num_hrd_parameters ||
>> - !memcmp(vps1->hdr, vps2->hdr, vps1->vps_num_hrd_parameters * sizeof(*vps1->hdr));
>> -
>> - return 0;
>> + return vps1->data_size == vps2->data_size &&
>> + !memcmp(vps1->data, vps2->data, vps1->data_size);
>> }
>>
>> int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>> @@ -463,25 +461,20 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>> {
>> int i,j;
>> int vps_id = 0;
>> - ptrdiff_t nal_size;
>> - HEVCVPS *vps = ff_refstruct_alloc_ext(sizeof(*vps), 0, NULL, uninit_vps);
>> + int ret = AVERROR_INVALIDDATA;
>> + HEVCVPS *vps = ff_refstruct_alloc_ext(sizeof(*vps), 0, NULL, hevc_vps_free);
>>
>> if (!vps)
>> return AVERROR(ENOMEM);
>>
>> av_log(avctx, AV_LOG_DEBUG, "Decoding VPS\n");
>>
>> - nal_size = gb->buffer_end - gb->buffer;
>> - if (nal_size > sizeof(vps->data)) {
>> - av_log(avctx, AV_LOG_WARNING, "Truncating likely oversized VPS "
>> - "(%"PTRDIFF_SPECIFIER" > %"SIZE_SPECIFIER")\n",
>> - nal_size, sizeof(vps->data));
>> - vps->data_size = sizeof(vps->data);
>> - } else {
>> - vps->data_size = nal_size;
>> + vps->data_size = gb->buffer_end - gb->buffer;
>> + vps->data = av_memdup(gb->buffer, vps->data_size);
>> + if (!vps->data) {
>> + ret = AVERROR(ENOMEM);
>> + goto err;
>> }
>> - memcpy(vps->data, gb->buffer, vps->data_size);
>> -
>> vps_id = vps->vps_id = get_bits(gb, 4);
>>
>> if (get_bits(gb, 2) != 3) { // vps_reserved_three_2bits
>> @@ -591,7 +584,7 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>>
>> err:
>> ff_refstruct_unref(&vps);
>> - return AVERROR_INVALIDDATA;
>> + return ret;
>> }
>>
>> static void decode_vui(GetBitContext *gb, AVCodecContext *avctx,
>> @@ -1294,36 +1287,43 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id,
>> return 0;
>> }
>>
>> +static void hevc_sps_free(FFRefStructOpaque opaque, void *obj)
>> +{
>> + HEVCSPS *sps = obj;
>> +
>> + av_freep(&sps->data);
>> +}
>> +
>> +static int compare_sps(const HEVCSPS *sps1, const HEVCSPS *sps2)
>> +{
>> + return sps1->data_size == sps2->data_size &&
>> + !memcmp(sps1->data, sps2->data, sps1->data_size);
>> +}
>> +
>> int ff_hevc_decode_nal_sps(GetBitContext *gb, AVCodecContext *avctx,
>> HEVCParamSets *ps, int apply_defdispwin)
>> {
>> - HEVCSPS *sps = ff_refstruct_allocz(sizeof(*sps));
>> + HEVCSPS *sps = ff_refstruct_alloc_ext(sizeof(*sps), 0, NULL, hevc_sps_free);
>> unsigned int sps_id;
>> int ret;
>> - ptrdiff_t nal_size;
>>
>> if (!sps)
>> return AVERROR(ENOMEM);
>>
>> av_log(avctx, AV_LOG_DEBUG, "Decoding SPS\n");
>>
>> - nal_size = gb->buffer_end - gb->buffer;
>> - if (nal_size > sizeof(sps->data)) {
>> - av_log(avctx, AV_LOG_WARNING, "Truncating likely oversized SPS "
>> - "(%"PTRDIFF_SPECIFIER" > %"SIZE_SPECIFIER")\n",
>> - nal_size, sizeof(sps->data));
>> - sps->data_size = sizeof(sps->data);
>> - } else {
>> - sps->data_size = nal_size;
>> + sps->data_size = gb->buffer_end - gb->buffer;
>> + sps->data = av_memdup(gb->buffer, sps->data_size);
>> + if (!sps->data) {
>> + ret = AVERROR(ENOMEM);
>> + goto err;
>> }
>> - memcpy(sps->data, gb->buffer, sps->data_size);
>>
>> ret = ff_hevc_parse_sps(sps, gb, &sps_id,
>> apply_defdispwin,
>> ps->vps_list, avctx);
>> if (ret < 0) {
>> - ff_refstruct_unref(&sps);
>> - return ret;
>> + goto err;
>> }
>>
>> if (avctx->debug & FF_DEBUG_BITSTREAM) {
>> @@ -1340,7 +1340,7 @@ int ff_hevc_decode_nal_sps(GetBitContext *gb, AVCodecContext *avctx,
>> * original one.
>> * otherwise drop all PPSes that depend on it */
>> if (ps->sps_list[sps_id] &&
>> - !memcmp(ps->sps_list[sps_id], sps, sizeof(*sps))) {
>> + compare_sps(ps->sps_list[sps_id], sps)) {
>> ff_refstruct_unref(&sps);
>> } else {
>> remove_sps(ps, sps_id);
>> @@ -1348,6 +1348,9 @@ int ff_hevc_decode_nal_sps(GetBitContext *gb, AVCodecContext *avctx,
>> }
>>
>> return 0;
>> +err:
>> + ff_refstruct_unref(&sps);
>> + return ret;
>> }
>>
>> static void hevc_pps_free(FFRefStructOpaque unused, void *obj)
>> @@ -1364,6 +1367,7 @@ static void hevc_pps_free(FFRefStructOpaque unused, void *obj)
>> av_freep(&pps->tile_pos_rs);
>> av_freep(&pps->tile_id);
>> av_freep(&pps->min_tb_addr_zs_tab);
>> + av_freep(&pps->data);
>> }
>>
>> static void colour_mapping_octants(GetBitContext *gb, HEVCPPS *pps, int inp_depth,
>> @@ -1773,16 +1777,12 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>>
>> av_log(avctx, AV_LOG_DEBUG, "Decoding PPS\n");
>>
>> - nal_size = gb->buffer_end - gb->buffer;
>> - if (nal_size > sizeof(pps->data)) {
>> - av_log(avctx, AV_LOG_WARNING, "Truncating likely oversized PPS "
>> - "(%"PTRDIFF_SPECIFIER" > %"SIZE_SPECIFIER")\n",
>> - nal_size, sizeof(pps->data));
>> - pps->data_size = sizeof(pps->data);
>> - } else {
>> - pps->data_size = nal_size;
>> + pps->data_size = gb->buffer_end - gb->buffer;
>> + pps->data = av_memdup(gb->buffer, pps->data_size);
>> + if (!pps->data) {
>> + ret = AVERROR_INVALIDDATA;
>> + goto err;
>> }
>> - memcpy(pps->data, gb->buffer, pps->data_size);
>>
>> // Default values
>> pps->loop_filter_across_tiles_enabled_flag = 1;
>> diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
>> index 0d8eaf2b3e..bed406770f 100644
>> --- a/libavcodec/hevc_ps.h
>> +++ b/libavcodec/hevc_ps.h
>> @@ -172,11 +172,11 @@ typedef struct HEVCVPS {
>> int vps_num_ticks_poc_diff_one; ///< vps_num_ticks_poc_diff_one_minus1 + 1
>> int vps_num_hrd_parameters;
>>
>> - uint8_t data[4096];
>> - int data_size;
>> - /* Put this at the end of the structure to make it easier to calculate the
>> + /* Keep this at the end of the structure to make it easier to calculate the
>> * size before this pointer, which is used for memcmp */
>> HEVCHdrParams *hdr;
>> + uint8_t *data;
>> + int data_size;
>> } HEVCVPS;
>>
>> typedef struct ScalingList {
>> @@ -299,7 +299,9 @@ typedef struct HEVCSPS {
>>
>> int qp_bd_offset;
>>
>> - uint8_t data[4096];
>> + /* Keep this at the end of the structure to make it easier to calculate the
>> + * size before this pointer, which is used for memcmp */
>> + uint8_t *data;
>> int data_size;
>> } HEVCSPS;
>>
>> @@ -434,7 +436,9 @@ typedef struct HEVCPPS {
>> int *min_tb_addr_zs; ///< MinTbAddrZS
>> int *min_tb_addr_zs_tab;///< MinTbAddrZS
>>
>> - uint8_t data[4096];
>> + /* Keep this at the end of the structure to make it easier to calculate the
>> + * size before this pointer, which is used for memcmp */
>> + uint8_t *data;
>> int data_size;
>> } HEVCPPS;
>>
>
> This is vastly overcomplicated: If you already have the complete data of
> the earlier PS, all you need is comparing the data before you even parse
> the new parameter set.
Need to get sps_id from the new sps buffer, which requires calling
ff_hevc_parse_sps(). Same for pps to get pps_id. Only vps has its id as
the very first element.
More information about the ffmpeg-devel
mailing list