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

Fri, 29 Mar 2024 10:36:51 -0700 

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 <jamr...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to