Re: [FFmpeg-devel] [PATCH v2] avocdec/hevc_ps: don't use a fixed sized buffer for parameter set raw data

[James Almer <jamrial@gmail.com>]

On 3/29/2024 2:28 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@gmail.com>
>> ---
>>   libavcodec/hevc_ps.c | 127 +++++++++++++++++++++++--------------------
>>   libavcodec/hevc_ps.h |  14 +++--
>>   2 files changed, 77 insertions(+), 64 deletions(-)
>>
>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>> index 6475d86d7d..7f74553fc1 100644
>> --- a/libavcodec/hevc_ps.c
>> +++ b/libavcodec/hevc_ps.c
>> @@ -442,47 +442,42 @@ 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);
>> -}
>> -
>> -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;
>> +    av_freep(&vps->data);
>>   }
>>   
>>   int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>>                              HEVCParamSets *ps)
>>   {
>>       int i,j;
>> -    int vps_id = 0;
>> -    ptrdiff_t nal_size;
>> -    HEVCVPS *vps = ff_refstruct_alloc_ext(sizeof(*vps), 0, NULL, uninit_vps);
>> +    int vps_id = get_bits(gb, 4);
>> +    int ret = AVERROR_INVALIDDATA;
>> +    HEVCVPS *vps;
>>   
>> +    if (ps->pps_list[vps_id]) {
>> +        const HEVCVPS *vps1 = ps->vps_list[vps_id];
>> +        if (vps1->data_size == gb->buffer_end - gb->buffer &&
>> +            !memcmp(vps1->data, gb->buffer, vps1->data_size))
>> +            return 0;
>> +    }
>> +
>> +    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);
>> +    vps->vps_id = vps_id;
>>   
>>       if (get_bits(gb, 2) != 3) { // vps_reserved_three_2bits
>>           av_log(avctx, AV_LOG_ERROR, "vps_reserved_three_2bits is not three\n");
>> @@ -579,19 +574,14 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>>               goto err;
>>       }
>>   
>> -    if (ps->vps_list[vps_id] &&
>> -        compare_vps(ps->vps_list[vps_id], vps)) {
>> -        ff_refstruct_unref(&vps);
>> -    } else {
>>           remove_vps(ps, vps_id);
>>           ps->vps_list[vps_id] = vps;
>> -    }
>>   
>>       return 0;
>>   
>>   err:
>>       ff_refstruct_unref(&vps);
>> -    return AVERROR_INVALIDDATA;
>> +    return ret;
>>   }
>>   
>>   static void decode_vui(GetBitContext *gb, AVCodecContext *avctx,
>> @@ -1294,36 +1284,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 +1337,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 +1345,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 +1364,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,
>> @@ -1762,27 +1763,35 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>>       const HEVCSPS *sps = NULL;
>>       const HEVCVPS *vps = NULL;
>>       int i, ret = 0;
>> -    unsigned int pps_id = 0;
>> -    ptrdiff_t nal_size;
>> +    unsigned int pps_id = get_ue_golomb_long(gb);
>>       unsigned log2_parallel_merge_level_minus2;
>> +    HEVCPPS *pps;
>> +
>> +    av_log(avctx, AV_LOG_DEBUG, "Decoding PPS\n");
>> +
>> +    if (pps_id >= HEVC_MAX_PPS_COUNT) {
>> +        av_log(avctx, AV_LOG_ERROR, "PPS id out of range: %d\n", pps_id);
>> +        ret = AVERROR_INVALIDDATA;
>> +        goto err;
> 
> return AVERROR_INVALIDDATA;
> goto err would use pps which is uninitialized.
> 
>> +    }
>>   
>> -    HEVCPPS *pps = ff_refstruct_alloc_ext(sizeof(*pps), 0, NULL, hevc_pps_free);
>> +    if (ps->pps_list[pps_id]) {
>> +        const HEVCPPS *pps1 = ps->pps_list[pps_id];
>> +        if (pps1->data_size == gb->buffer_end - gb->buffer &&
>> +            !memcmp(pps1->data, gb->buffer, pps1->data_size))
>> +            return 0;
>> +    }
>>   
>> +    pps = ff_refstruct_alloc_ext(sizeof(*pps), 0, NULL, hevc_pps_free);
>>       if (!pps)
>>           return AVERROR(ENOMEM);
>>   
>> -    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;
>> @@ -1795,7 +1804,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>>       pps->log2_max_transform_skip_block_size    = 2;
>>   
>>       // Coded parameters
>> -    pps_id = pps->pps_id = get_ue_golomb_long(gb);
>> +    pps->pps_id = pps_id;
>>       if (pps_id >= HEVC_MAX_PPS_COUNT) {
>>           av_log(avctx, AV_LOG_ERROR, "PPS id out of range: %d\n", pps_id);
>>           ret = AVERROR_INVALIDDATA;
> 
> You already check for this above.

Will remove.

> 
>> 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 */
> 
> You are adding comments that are already wrong at the time you add them.

Yeah, remnant of a previous version where i was doing first a data check 
then struct check.

Will fix the above stuff and push later. Thanks.

> 
>> +    uint8_t *data;
>>       int data_size;
>>   } HEVCPPS;
>>   
> 
> _______________________________________________
> 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".
_______________________________________________
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".