Re: [FFmpeg-devel] [PATCH] lavc/vvc: Error if SPS ID is duplicated within CVS

[Nuo Mi <nuomi2021@gmail.com>]

Hi Frank,
Thank you for the patch.

On Thu, Apr 4, 2024 at 3:01 AM Frank Plowman <post@frankplowman.com> wrote:

> Key line from the spec is:
>
> "All SPS NAL units with a particular value of sps_seq_parameter_set_id
> in a CVS shall have the same content."
>
> Prior to this patch, the VVC decoder's behaviour on encountering a
> duplicated SPS ID (within the entire bitstream, not restricted to
> a CVS) was simply to replace the entry in the SPS lookup table with the
> new data.  Illegal bitstreams with multiple SPSs in the same CVS sharing
> an ID but differing elsewhere could cause all manner of issues.
>
> The patch tracks which SPS IDs have been used in the given CVS using the
> new sps_id_used field of VVCParamSets.  If it encounters an SPS with an
> ID already in use and whose content differs from the previous SPS, it
> throws an AVERROR_INVALIDDATA.
> ---
>  libavcodec/vvc/vvc_ps.c | 29 +++++++++++++++++++++--------
>  libavcodec/vvc/vvc_ps.h |  1 +
>  2 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c
> index 301fa16400..7a8daaa6ad 100644
> --- a/libavcodec/vvc/vvc_ps.c
> +++ b/libavcodec/vvc/vvc_ps.c
> @@ -219,14 +219,23 @@ fail:
>      return NULL;
>  }
>
> -static int decode_sps(VVCParamSets *ps, const H266RawSPS *rsps, void
> *log_ctx)
> +static int decode_sps(VVCParamSets *ps, const H266RawSPS *rsps, void
> *log_ctx, int is_clvss)
>  {
>      const int sps_id        = rsps->sps_seq_parameter_set_id;
>      const VVCSPS *old_sps   = ps->sps_list[sps_id];
>      const VVCSPS *sps;
>
> -    if (old_sps && old_sps->r == rsps)
> -        return 0;
> +    if (is_clvss) {
> +        for (int sps_id = 0; sps_id < VVC_MAX_SPS_COUNT; ++sps_id)
> +            ps->sps_id_used[sps_id] = 0;
> +    }
> +
> +    if (old_sps)
>
 better use a bracket here.

> +        if (old_sps->r == rsps || !memcmp(old_sps->r, rsps,
> sizeof(*old_sps->r)))
> +            return 0;
> +        else if (ps->sps_id_used[sps_id])
> +            return AVERROR_INVALIDDATA;
> +

trailing space.

>
>
>      sps = sps_alloc(rsps, log_ctx);
>      if (!sps)
> @@ -234,6 +243,7 @@ static int decode_sps(VVCParamSets *ps, const
> H266RawSPS *rsps, void *log_ctx)
>
>      ff_refstruct_unref(&ps->sps_list[sps_id]);
>      ps->sps_list[sps_id] = sps;
> +    ps->sps_id_used[sps_id] = 1;
>
>      return 0;
>  }
> @@ -610,7 +620,7 @@ static int decode_pps(VVCParamSets *ps, const
> H266RawPPS *rpps)
>      return ret;
>  }
>
> -static int decode_ps(VVCParamSets *ps, const CodedBitstreamH266Context
> *h266, void *log_ctx)
> +static int decode_ps(VVCParamSets *ps, const CodedBitstreamH266Context
> *h266, void *log_ctx, int is_clvss)
>  {
>      const H266RawPictureHeader *ph = h266->ph;
>      const H266RawPPS *rpps;
> @@ -628,7 +638,7 @@ static int decode_ps(VVCParamSets *ps, const
> CodedBitstreamH266Context *h266, vo
>      if (!rsps)
>          return AVERROR_INVALIDDATA;
>
> -    ret = decode_sps(ps, rsps, log_ctx);
> +    ret = decode_sps(ps, rsps, log_ctx, is_clvss);
>      if (ret < 0)
>          return ret;
>
> @@ -867,13 +877,16 @@ int ff_vvc_decode_frame_ps(VVCFrameParamSets *fps,
> struct VVCContext *s)
>      int ret = 0;
>      VVCParamSets *ps                        = &s->ps;
>      const CodedBitstreamH266Context *h266   = s->cbc->priv_data;
> +    int is_clvss;
>
> -    ret = decode_ps(ps, h266, s->avctx);
> +    decode_recovery_flag(s);
> +    is_clvss = IS_CLVSS(s);
> +
> +    ret = decode_ps(ps, h266, s->avctx, is_clvss);
>      if (ret < 0)
>          return ret;
>
> -    decode_recovery_flag(s);
> -    ret = decode_frame_ps(fps, ps, h266, s->poc_tid0, IS_CLVSS(s));
> +    ret = decode_frame_ps(fps, ps, h266, s->poc_tid0, is_clvss);
>      decode_recovery_poc(s, &fps->ph);
>      return ret;
>  }
> diff --git a/libavcodec/vvc/vvc_ps.h b/libavcodec/vvc/vvc_ps.h
> index f60d8b81c6..b293043de8 100644
> --- a/libavcodec/vvc/vvc_ps.h
> +++ b/libavcodec/vvc/vvc_ps.h
> @@ -210,6 +210,7 @@ typedef struct VVCLMCS {
>
>  typedef struct VVCParamSets {
>      const VVCSPS            *sps_list[VVC_MAX_SPS_COUNT];       ///<
> RefStruct reference
> +          int                sps_id_used[VVC_MAX_SPS_COUNT];
>
The int array seems excessive for just a flag. Let's replace it with a
uint16_t.
Also, please relocate sps_id_used to the end of the structure."

>      const VVCPPS            *pps_list[VVC_MAX_PPS_COUNT];       ///<
> RefStruct reference
>      const VVCALF            *alf_list[VVC_MAX_ALF_COUNT];       ///<
> RefStruct reference
>      const H266RawAPS        *lmcs_list[VVC_MAX_LMCS_COUNT];     ///<
> RefStruct reference
> --
> 2.44.0
>
> _______________________________________________
> 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".