diff mbox series

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

Message ID 20240406134546.61454-1-post@frankplowman.com
State New
Headers show
Series [FFmpeg-devel,v2] lavc/vvc: Error if SPS ID is duplicated within CVS | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Frank Plowman April 6, 2024, 1:46 p.m. UTC 
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.

Signed-off-by: Frank Plowman <post@frankplowman.com>
---
Changes since v1:
* Track which SPS IDs are in use using a bit field rather than a bool
  array.
* Move sps_id_used to the end of VVCParamSets.
* Style: remove blank line & add braces for clarity.

 libavcodec/vvc/vvc_ps.c | 28 ++++++++++++++++++++--------
 libavcodec/vvc/vvc_ps.h |  2 ++
 2 files changed, 22 insertions(+), 8 deletions(-)

Comments

Nuo Mi April 7, 2024, 2:39 a.m. UTC | #1 
On Sat, Apr 6, 2024 at 9:46 PM 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.
>
> Signed-off-by: Frank Plowman <post@frankplowman.com>
> ---
> Changes since v1:
> * Track which SPS IDs are in use using a bit field rather than a bool
>   array.
> * Move sps_id_used to the end of VVCParamSets.
> * Style: remove blank line & add braces for clarity.
>
Pushed.
Thank you, Frank

>
>  libavcodec/vvc/vvc_ps.c | 28 ++++++++++++++++++++--------
>  libavcodec/vvc/vvc_ps.h |  2 ++
>  2 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c
> index 301fa16400..7e967180d2 100644
> --- a/libavcodec/vvc/vvc_ps.c
> +++ b/libavcodec/vvc/vvc_ps.c
> @@ -219,14 +219,22 @@ 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) {
> +        ps->sps_id_used = 0;
> +    }
> +
> +    if (old_sps) {
> +        if (old_sps->r == rsps || !memcmp(old_sps->r, rsps,
> sizeof(*old_sps->r)))
> +            return 0;
> +        else if (ps->sps_id_used & (1 << sps_id))
> +            return AVERROR_INVALIDDATA;
> +    }
>
>      sps = sps_alloc(rsps, log_ctx);
>      if (!sps)
> @@ -234,6 +242,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 |= (1 << sps_id);
>
>      return 0;
>  }
> @@ -610,7 +619,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 +637,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 +876,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;
> +
> +    decode_recovery_flag(s);
> +    is_clvss = IS_CLVSS(s);
>
> -    ret = decode_ps(ps, h266, s->avctx);
> +    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..710673ebed 100644
> --- a/libavcodec/vvc/vvc_ps.h
> +++ b/libavcodec/vvc/vvc_ps.h
> @@ -214,6 +214,8 @@ typedef struct VVCParamSets {
>      const VVCALF            *alf_list[VVC_MAX_ALF_COUNT];       ///<
> RefStruct reference
>      const H266RawAPS        *lmcs_list[VVC_MAX_LMCS_COUNT];     ///<
> RefStruct reference
>      const VVCScalingList    *scaling_list[VVC_MAX_SL_COUNT];    ///<
> RefStruct reference
> +    // Bit field of SPS IDs used in the current CVS
> +          uint16_t           sps_id_used;
>  } VVCParamSets;
>
>  typedef struct VVCFrameParamSets {
> --
> 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".
>
Anton Khirnov April 7, 2024, 6:15 a.m. UTC | #2 
Quoting Frank Plowman (2024-04-06 15:46:09)
> 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.

I wonder if it wouldn't be better to do what H264/HEVC do, which is
replace the SPS, invalidate the PPSes that depend on the old one, and
start a new CVS.
Nuo Mi April 7, 2024, 12:13 p.m. UTC | #3 
On Sun, Apr 7, 2024 at 2:15 PM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Frank Plowman (2024-04-06 15:46:09)
> > 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.
>
> I wonder if it wouldn't be better to do what H264/HEVC do, which is
> replace the SPS, invalidate the PPSes that depend on the old one, and
> start a new CVS.
>
Consider two scenarios:
If the first SPS is incorrect, the entire CVS is undecodable because the
key frame is wrong, no matter what we do.
If the second SPS is incorrect, H.264/HEVC can't recover until the next CVS
because it replaces the correct SPS with the wrong one. However, the
current VVC logic allows for recovery in such cases.
Therefore, in the second case, the current logic may have benefits.

>
> --
> Anton Khirnov
> _______________________________________________
> 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".
>
Anton Khirnov April 7, 2024, 12:16 p.m. UTC | #4 
Quoting Nuo Mi (2024-04-07 14:13:58)
> On Sun, Apr 7, 2024 at 2:15 PM Anton Khirnov <anton@khirnov.net> wrote:
> 
> > Quoting Frank Plowman (2024-04-06 15:46:09)
> > > 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.
> >
> > I wonder if it wouldn't be better to do what H264/HEVC do, which is
> > replace the SPS, invalidate the PPSes that depend on the old one, and
> > start a new CVS.
> >
> Consider two scenarios:
> If the first SPS is incorrect, the entire CVS is undecodable because the
> key frame is wrong, no matter what we do.
> If the second SPS is incorrect, H.264/HEVC can't recover until the next CVS
> because it replaces the correct SPS with the wrong one. However, the
> current VVC logic allows for recovery in such cases.
> Therefore, in the second case, the current logic may have benefits.

Could the new SPS not signal the start of a new CVS?
Nuo Mi April 7, 2024, 1:04 p.m. UTC | #5 
On Sun, Apr 7, 2024 at 8:16 PM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Nuo Mi (2024-04-07 14:13:58)
> > On Sun, Apr 7, 2024 at 2:15 PM Anton Khirnov <anton@khirnov.net> wrote:
> >
> > > Quoting Frank Plowman (2024-04-06 15:46:09)
> > > > 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.
> > >
> > > I wonder if it wouldn't be better to do what H264/HEVC do, which is
> > > replace the SPS, invalidate the PPSes that depend on the old one, and
> > > start a new CVS.
> > >
> > Consider two scenarios:
> > If the first SPS is incorrect, the entire CVS is undecodable because the
> > key frame is wrong, no matter what we do.
> > If the second SPS is incorrect, H.264/HEVC can't recover until the next
> CVS
> > because it replaces the correct SPS with the wrong one. However, the
> > current VVC logic allows for recovery in such cases.
> > Therefore, in the second case, the current logic may have benefits.
>
> Could the new SPS not signal the start of a new CVS?
>
Yes.
Take AUD_A_Broadcom_3.bit as an example, the nal order is

SPS
PPS
IDR_N_LP
TRAIL
MORE TRAILS...
SPS
PPS
CRA
TRAIL
MORE TRAILS...
SPS
PPS
IDR_N_LP

The SPS before CRA will not start a new CVS, unless you seek to the CRA.

>
> --
> Anton Khirnov
> _______________________________________________
> 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".
>
diff mbox series

Patch

diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c
index 301fa16400..7e967180d2 100644
--- a/libavcodec/vvc/vvc_ps.c
+++ b/libavcodec/vvc/vvc_ps.c
@@ -219,14 +219,22 @@  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) {
+        ps->sps_id_used = 0;
+    }
+
+    if (old_sps) {
+        if (old_sps->r == rsps || !memcmp(old_sps->r, rsps, sizeof(*old_sps->r)))
+            return 0;
+        else if (ps->sps_id_used & (1 << sps_id))
+            return AVERROR_INVALIDDATA;
+    }
 
     sps = sps_alloc(rsps, log_ctx);
     if (!sps)
@@ -234,6 +242,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 |= (1 << sps_id);
 
     return 0;
 }
@@ -610,7 +619,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 +637,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 +876,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;
+
+    decode_recovery_flag(s);
+    is_clvss = IS_CLVSS(s);
 
-    ret = decode_ps(ps, h266, s->avctx);
+    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..710673ebed 100644
--- a/libavcodec/vvc/vvc_ps.h
+++ b/libavcodec/vvc/vvc_ps.h
@@ -214,6 +214,8 @@  typedef struct VVCParamSets {
     const VVCALF            *alf_list[VVC_MAX_ALF_COUNT];       ///< RefStruct reference
     const H266RawAPS        *lmcs_list[VVC_MAX_LMCS_COUNT];     ///< RefStruct reference
     const VVCScalingList    *scaling_list[VVC_MAX_SL_COUNT];    ///< RefStruct reference
+    // Bit field of SPS IDs used in the current CVS
+          uint16_t           sps_id_used;
 } VVCParamSets;
 
 typedef struct VVCFrameParamSets {