[FFmpeg-devel] [PATCH v2] avocdec/hevc_ps: don't use a fixed sized buffer for parameter set raw data
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Fri Mar 29 19:28:31 EET 2024
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 | 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.
> 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.
> + uint8_t *data;
> int data_size;
> } HEVCPPS;
>
More information about the ffmpeg-devel
mailing list