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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

James Almer:
> 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@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.

No. SPS is the only exception.

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