[FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Frank Plowman]

When the SPS associated with a particular SPS ID changes, invalidate all
the PPSs which use that SPS ID.  Fixes crashes with illegal bitstreams.
This is done in the CBS, rather than in libavcodec/vvc/ps.c like the SPS
ID reuse validation, as parts of the CBS parsing process for PPSs
depend on the SPS being referred to.

Signed-off-by: Frank Plowman <post@frankplowman.com>
---
 libavcodec/cbs_h2645.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index c5f167334d..e2389f124e 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -789,9 +789,28 @@ static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \
 }
 
 cbs_h266_replace_ps(6, VPS, vps, vps_video_parameter_set_id)
-cbs_h266_replace_ps(6, SPS, sps, sps_seq_parameter_set_id)
 cbs_h266_replace_ps(6, PPS, pps, pps_pic_parameter_set_id)
 
+static int cbs_h266_replace_sps(CodedBitstreamContext *ctx,
+                                CodedBitstreamUnit *unit)
+{
+    CodedBitstreamH266Context *priv = ctx->priv_data;
+    H266RawSPS *sps = unit->content;
+    unsigned int id = sps->sps_seq_parameter_set_id;
+    int err = ff_cbs_make_unit_refcounted(ctx, unit);
+    if (err < 0)
+        return err;
+    av_assert0(unit->content_ref);
+    if (priv->sps[id] && memcmp(priv->sps[id], unit->content_ref, sizeof(*priv->sps[id]))) {
+        for (unsigned int i = 0; i < VVC_MAX_PPS_COUNT; i++) {
+            if (priv->pps[i] && priv->pps[i]->pps_seq_parameter_set_id == id)
+                ff_refstruct_unref(&priv->pps[i]);
+        }
+    }
+    ff_refstruct_replace(&priv->sps[id], unit->content_ref);
+    return 0;
+}
+
 static int cbs_h266_replace_ph(CodedBitstreamContext *ctx,
                                CodedBitstreamUnit *unit,
                                H266RawPictureHeader *ph)
-- 
2.45.1

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

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Christophe Gisquet]

Le ven. 14 juin 2024, 11:39, Frank Plowman <post@frankplowman.com> a écrit :

> When the SPS associated with a particular SPS ID changes, invalidate all
> the PPSs which use that SPS ID.  Fixes crashes with illegal bitstreams.
> This is done in the CBS, rather than in libavcodec/vvc/ps.c like the SPS
> ID reuse validation, as parts of the CBS parsing process for PPSs
> depend on the SPS being referred to.
>

I am uncertain about this. I have no definite knowledge nor proof, but I
would have thought these are persistent, IE it's legal to update some of
them, their validity depending on something else.

Wondering if the tested streams are thus conformant.

But I don't know the actual rule. Maybe finding an EOB/EOS NUT? Related to
some particular shape of a clean random access point, that would require
retransmitting VPS/SPS/PPS/APS/... ?

Asking Benjamin Bross might be a better option here.

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

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Nuo Mi]

On Sat, Jun 15, 2024 at 2:35 PM Christophe Gisquet <
christophe.gisquet@gmail.com> wrote:

> Le ven. 14 juin 2024, 11:39, Frank Plowman <post@frankplowman.com> a
> écrit :
>
> > When the SPS associated with a particular SPS ID changes, invalidate all
> > the PPSs which use that SPS ID.  Fixes crashes with illegal bitstreams.
> > This is done in the CBS, rather than in libavcodec/vvc/ps.c like the SPS
> > ID reuse validation, as parts of the CBS parsing process for PPSs
> > depend on the SPS being referred to.
> >
>
> I am uncertain about this. I have no definite knowledge nor proof, but I
> would have thought these are persistent, IE it's legal to update some of
> them, their validity depending on something else.
>

> Wondering if the tested streams are thus conformant.
>
> But I don't know the actual rule. Maybe finding an EOB/EOS NUT? Related to
> some particular shape of a clean random access point, that would require
> retransmitting VPS/SPS/PPS/APS/... ?
>
> Asking Benjamin Bross might be a better option here.
>
Hi Chris,
spec said sps should not change in a CVS.  Frank has some patches to fix a
similar issue.
https://github.com/FFmpeg/FFmpeg/commit/2d79ae3f8a3306d24afe43ba505693a8dbefd21b


Hi Frank,
Did it crash before your error hand code in ps.c?
Could you send me the clip?

Thank you


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

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Frank Plowman]

n 15/06/2024 13:24, Nuo Mi wrote:
> On Sat, Jun 15, 2024 at 2:35 PM Christophe Gisquet <
> christophe.gisquet@gmail.com> wrote:
> 
>> Le ven. 14 juin 2024, 11:39, Frank Plowman <post@frankplowman.com> a
>> écrit :
>>
>>> When the SPS associated with a particular SPS ID changes, invalidate all
>>> the PPSs which use that SPS ID.  Fixes crashes with illegal bitstreams.
>>> This is done in the CBS, rather than in libavcodec/vvc/ps.c like the SPS
>>> ID reuse validation, as parts of the CBS parsing process for PPSs
>>> depend on the SPS being referred to.
>>>
>>
>> I am uncertain about this. I have no definite knowledge nor proof, but I
>> would have thought these are persistent, IE it's legal to update some of
>> them, their validity depending on something else.
>>
> 
>> Wondering if the tested streams are thus conformant.
>>
>> But I don't know the actual rule. Maybe finding an EOB/EOS NUT? Related to
>> some particular shape of a clean random access point, that would require
>> retransmitting VPS/SPS/PPS/APS/... ?
>>
>> Asking Benjamin Bross might be a better option here.
>>
> Hi Chris,
> spec said sps should not change in a CVS.  Frank has some patches to fix a
> similar issue.
> https://github.com/FFmpeg/FFmpeg/commit/2d79ae3f8a3306d24afe43ba505693a8dbefd21b
> 
> 
> Hi Frank,
> Did it crash before your error hand code in ps.c?
> Could you send me the clip?
> 
> Thank you
> 

Hi both,

Thank you for your reviews.

An example of a crashing bitstream which is fixed by this patch is ID
295 available here: https://github.com/ffvvc/tests/pull/43.  The
relevant part of the bitstream is a sequence of NAL units

AU (decode_order=5)
18. SPS
    sps_seq_parameter_set_id = 0
    sps_ctb_log2_size_y = 5
19. PPS
    pps_pic_parameter_set_id = 0
    pps_seq_parameter_set_id = 0
20. IDR_N_LP
    ph_pic_order_cnt_lsb = 0
    NoOutputBeforeRecoveryFlag = 1
    ph_pic_parameter_set_id = 0

AU (decode_order=6)
21. AUD
22. VPS
23. SPS
    sps_seq_parameter_set_id = 0
    sps_ctb_log2_size_y = 7
24. PREFIX_APS
25. IDR_N_LP
    ph_pic_order_cnt_lsb = 0
    NoOutputBeforeRecoveryFlag = 1
    ph_pic_parameter_set_id = 0

The layout of SPSs alone is legal (not covered by the checks introduced
in 2d79ae3f8a3306d24afe43ba505693a8dbefd21b) as the second AU is a CLVSS
AU.  As a result, the bitstream crashes both before and after
2d79ae3f8a3306d24afe43ba505693a8dbefd21b.  What this patch does is
produce an error when the VCL NAL unit in the second AU (25.) tries to
use PPS ID 0, as the SPS NAL unit that PPS was defined with reference to
(18.) is no longer available.

Christophe, is my interpretation of your point correct when I say you
are suggesting that the above sequence may be legal, so long as the PPS
still satisfies the new bounds etc. derived from the second SPS?  I did
consider this, and I think it may be possible to implement by delaying
CBS element validation and inference until libavcodec/vvc/ps.c.
However, there are no bitstreams in the conformance suite which contain
such a structure and this is different to how the native HEVC decoder
behaves (see libavcodec/hevc/ps.c:72).

All the best,
-- 
Frank
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Nuo Mi]

On Sun, Jun 16, 2024 at 12:38 AM Frank Plowman <post@frankplowman.com>
wrote:

> n 15/06/2024 13:24, Nuo Mi wrote:
> > On Sat, Jun 15, 2024 at 2:35 PM Christophe Gisquet <
> > christophe.gisquet@gmail.com> wrote:
> >
> >> Le ven. 14 juin 2024, 11:39, Frank Plowman <post@frankplowman.com> a
> >> écrit :
> >>
> >>> When the SPS associated with a particular SPS ID changes, invalidate
> all
> >>> the PPSs which use that SPS ID.  Fixes crashes with illegal bitstreams.
> >>> This is done in the CBS, rather than in libavcodec/vvc/ps.c like the
> SPS
> >>> ID reuse validation, as parts of the CBS parsing process for PPSs
> >>> depend on the SPS being referred to.
> >>>
> >>
> >> I am uncertain about this. I have no definite knowledge nor proof, but I
> >> would have thought these are persistent, IE it's legal to update some of
> >> them, their validity depending on something else.
> >>
> >
> >> Wondering if the tested streams are thus conformant.
> >>
> >> But I don't know the actual rule. Maybe finding an EOB/EOS NUT? Related
> to
> >> some particular shape of a clean random access point, that would require
> >> retransmitting VPS/SPS/PPS/APS/... ?
> >>
> >> Asking Benjamin Bross might be a better option here.
> >>
> > Hi Chris,
> > spec said sps should not change in a CVS.  Frank has some patches to fix
> a
> > similar issue.
> >
> https://github.com/FFmpeg/FFmpeg/commit/2d79ae3f8a3306d24afe43ba505693a8dbefd21b
> >
> >
> > Hi Frank,
> > Did it crash before your error hand code in ps.c?
> > Could you send me the clip?
> >
> > Thank you
> >
>
> Hi both,
>
> Thank you for your reviews.
>
> An example of a crashing bitstream which is fixed by this patch is ID
> 295 available here: https://github.com/ffvvc/tests/pull/43.  The
> relevant part of the bitstream is a sequence of NAL units
>
> AU (decode_order=5)
> 18. SPS
>     sps_seq_parameter_set_id = 0
>     sps_ctb_log2_size_y = 5
> 19. PPS
>     pps_pic_parameter_set_id = 0
>     pps_seq_parameter_set_id = 0
> 20. IDR_N_LP
>     ph_pic_order_cnt_lsb = 0
>     NoOutputBeforeRecoveryFlag = 1
>     ph_pic_parameter_set_id = 0
>
> AU (decode_order=6)
> 21. AUD
> 22. VPS
> 23. SPS
>     sps_seq_parameter_set_id = 0
>     sps_ctb_log2_size_y = 7
> 24. PREFIX_APS
> 25. IDR_N_LP
>     ph_pic_order_cnt_lsb = 0
>     NoOutputBeforeRecoveryFlag = 1
>     ph_pic_parameter_set_id = 0
>
> The layout of SPSs alone is legal (not covered by the checks introduced
> in 2d79ae3f8a3306d24afe43ba505693a8dbefd21b) as the second AU is a CLVSS
> AU.  As a result, the bitstream crashes both before and after
> 2d79ae3f8a3306d24afe43ba505693a8dbefd21b.  What this patch does is
> produce an error when the VCL NAL unit in the second AU (25.) tries to
> use PPS ID 0, as the SPS NAL unit that PPS was defined with reference to
> (18.) is no longer available.
>
> Christophe, is my interpretation of your point correct when I say you
> are suggesting that the above sequence may be legal, so long as the PPS
> still satisfies the new bounds etc. derived from the second SPS?  I did
> consider this, and I think it may be possible to implement by delaying
> CBS element validation and inference until libavcodec/vvc/ps.c.
> However, there are no bitstreams in the conformance suite which contain
> such a structure and this is different to how the native HEVC decoder
> behaves (see libavcodec/hevc/ps.c:72).
>
Pretty solid analysis, thank you, Frank

>
> All the best,
> --
> Frank
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Mark Thompson]

On 15/06/2024 17:37, Frank Plowman wrote:
> n 15/06/2024 13:24, Nuo Mi wrote:
>> On Sat, Jun 15, 2024 at 2:35 PM Christophe Gisquet <
>> christophe.gisquet@gmail.com> wrote:
>>
>>> Le ven. 14 juin 2024, 11:39, Frank Plowman <post@frankplowman.com> a
>>> écrit :
>>>
>>>> When the SPS associated with a particular SPS ID changes, invalidate all
>>>> the PPSs which use that SPS ID.  Fixes crashes with illegal bitstreams.
>>>> This is done in the CBS, rather than in libavcodec/vvc/ps.c like the SPS
>>>> ID reuse validation, as parts of the CBS parsing process for PPSs
>>>> depend on the SPS being referred to.
>>>>
>>>
>>> I am uncertain about this. I have no definite knowledge nor proof, but I
>>> would have thought these are persistent, IE it's legal to update some of
>>> them, their validity depending on something else.
>>>
>>
>>> Wondering if the tested streams are thus conformant.
>>>
>>> But I don't know the actual rule. Maybe finding an EOB/EOS NUT? Related to
>>> some particular shape of a clean random access point, that would require
>>> retransmitting VPS/SPS/PPS/APS/... ?
>>>
>>> Asking Benjamin Bross might be a better option here.
>>>
>> Hi Chris,
>> spec said sps should not change in a CVS.  Frank has some patches to fix a
>> similar issue.
>> https://github.com/FFmpeg/FFmpeg/commit/2d79ae3f8a3306d24afe43ba505693a8dbefd21b
>>
>>
>> Hi Frank,
>> Did it crash before your error hand code in ps.c?
>> Could you send me the clip?
>>
>> Thank you
>>
> 
> Hi both,
> 
> Thank you for your reviews.
> 
> An example of a crashing bitstream which is fixed by this patch is ID
> 295 available here: https://github.com/ffvvc/tests/pull/43.  The
> relevant part of the bitstream is a sequence of NAL units
> 
> AU (decode_order=5)
> 18. SPS
>     sps_seq_parameter_set_id = 0
>     sps_ctb_log2_size_y = 5
> 19. PPS
>     pps_pic_parameter_set_id = 0
>     pps_seq_parameter_set_id = 0
> 20. IDR_N_LP
>     ph_pic_order_cnt_lsb = 0
>     NoOutputBeforeRecoveryFlag = 1
>     ph_pic_parameter_set_id = 0
> 
> AU (decode_order=6)
> 21. AUD
> 22. VPS
> 23. SPS
>     sps_seq_parameter_set_id = 0
>     sps_ctb_log2_size_y = 7
> 24. PREFIX_APS
> 25. IDR_N_LP
>     ph_pic_order_cnt_lsb = 0
>     NoOutputBeforeRecoveryFlag = 1
>     ph_pic_parameter_set_id = 0
> 
> The layout of SPSs alone is legal (not covered by the checks introduced
> in 2d79ae3f8a3306d24afe43ba505693a8dbefd21b) as the second AU is a CLVSS
> AU.  As a result, the bitstream crashes both before and after
> 2d79ae3f8a3306d24afe43ba505693a8dbefd21b.  What this patch does is
> produce an error when the VCL NAL unit in the second AU (25.) tries to
> use PPS ID 0, as the SPS NAL unit that PPS was defined with reference to
> (18.) is no longer available.
> 
> Christophe, is my interpretation of your point correct when I say you
> are suggesting that the above sequence may be legal, so long as the PPS
> still satisfies the new bounds etc. derived from the second SPS?  I did
> consider this, and I think it may be possible to implement by delaying
> CBS element validation and inference until libavcodec/vvc/ps.c.
> However, there are no bitstreams in the conformance suite which contain
> such a structure and this is different to how the native HEVC decoder
> behaves (see libavcodec/hevc/ps.c:72).

Is there some requirement in H.266 that in a single stream the PPS precedes the SPS?

Currently we effectively require that for a single stream because we use the SPS to enforce constraints on the PPS in both H.265 and H.266, but I'm not seeing a hard dependency and it looks like it will currently work on later stream starts as long as the parameters don't change too much.

In H.264 there is an explicit dependency because you need chroma_format_idc to parse scaling lists, but again this will usually work because it's unlikely to change inline.

If that is not required then this will discard the PPS of a stream where the SPS follows the PPS.  (Though I admit that before this it was only dubiously working because the bounds checking might be wrong.)

Thanks,

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

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Anton Khirnov]

Quoting Mark Thompson (2024-06-16 17:26:26)
> Is there some requirement in H.266 that in a single stream the PPS precedes the SPS?
> 
> Currently we effectively require that for a single stream because we use the SPS to enforce constraints on the PPS in both H.265 and H.266, but I'm not seeing a hard dependency and it looks like it will currently work on later stream starts as long as the parameters don't change too much.
> 
> In H.264 there is an explicit dependency because you need chroma_format_idc to parse scaling lists, but again this will usually work because it's unlikely to change inline.

IIRC the spec requirement is about PPS activation, not order in the
bytestream. Our decoder does impose constraints on the order, and I
don't recall it ever causing problems, but it does not follow from the
spec.

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

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Nuo Mi]

On Sun, Jun 16, 2024 at 11:26 PM Mark Thompson <sw@jkqxz.net> wrote:

> On 15/06/2024 17:37, Frank Plowman wrote:
> > n 15/06/2024 13:24, Nuo Mi wrote:
> >> On Sat, Jun 15, 2024 at 2:35 PM Christophe Gisquet <
> >> christophe.gisquet@gmail.com> wrote:
> >>
> >>> Le ven. 14 juin 2024, 11:39, Frank Plowman <post@frankplowman.com> a
> >>> écrit :
> >>>
> >>>> When the SPS associated with a particular SPS ID changes, invalidate
> all
> >>>> the PPSs which use that SPS ID.  Fixes crashes with illegal
> bitstreams.
> >>>> This is done in the CBS, rather than in libavcodec/vvc/ps.c like the
> SPS
> >>>> ID reuse validation, as parts of the CBS parsing process for PPSs
> >>>> depend on the SPS being referred to.
> >>>>
> >>>
> >>> I am uncertain about this. I have no definite knowledge nor proof, but
> I
> >>> would have thought these are persistent, IE it's legal to update some
> of
> >>> them, their validity depending on something else.
> >>>
> >>
> >>> Wondering if the tested streams are thus conformant.
> >>>
> >>> But I don't know the actual rule. Maybe finding an EOB/EOS NUT?
> Related to
> >>> some particular shape of a clean random access point, that would
> require
> >>> retransmitting VPS/SPS/PPS/APS/... ?
> >>>
> >>> Asking Benjamin Bross might be a better option here.
> >>>
> >> Hi Chris,
> >> spec said sps should not change in a CVS.  Frank has some patches to
> fix a
> >> similar issue.
> >>
> https://github.com/FFmpeg/FFmpeg/commit/2d79ae3f8a3306d24afe43ba505693a8dbefd21b
> >>
> >>
> >> Hi Frank,
> >> Did it crash before your error hand code in ps.c?
> >> Could you send me the clip?
> >>
> >> Thank you
> >>
> >
> > Hi both,
> >
> > Thank you for your reviews.
> >
> > An example of a crashing bitstream which is fixed by this patch is ID
> > 295 available here: https://github.com/ffvvc/tests/pull/43.  The
> > relevant part of the bitstream is a sequence of NAL units
> >
> > AU (decode_order=5)
> > 18. SPS
> >     sps_seq_parameter_set_id = 0
> >     sps_ctb_log2_size_y = 5
> > 19. PPS
> >     pps_pic_parameter_set_id = 0
> >     pps_seq_parameter_set_id = 0
> > 20. IDR_N_LP
> >     ph_pic_order_cnt_lsb = 0
> >     NoOutputBeforeRecoveryFlag = 1
> >     ph_pic_parameter_set_id = 0
> >
> > AU (decode_order=6)
> > 21. AUD
> > 22. VPS
> > 23. SPS
> >     sps_seq_parameter_set_id = 0
> >     sps_ctb_log2_size_y = 7
> > 24. PREFIX_APS
> > 25. IDR_N_LP
> >     ph_pic_order_cnt_lsb = 0
> >     NoOutputBeforeRecoveryFlag = 1
> >     ph_pic_parameter_set_id = 0
> >
> > The layout of SPSs alone is legal (not covered by the checks introduced
> > in 2d79ae3f8a3306d24afe43ba505693a8dbefd21b) as the second AU is a CLVSS
> > AU.  As a result, the bitstream crashes both before and after
> > 2d79ae3f8a3306d24afe43ba505693a8dbefd21b.  What this patch does is
> > produce an error when the VCL NAL unit in the second AU (25.) tries to
> > use PPS ID 0, as the SPS NAL unit that PPS was defined with reference to
> > (18.) is no longer available.
> >
> > Christophe, is my interpretation of your point correct when I say you
> > are suggesting that the above sequence may be legal, so long as the PPS
> > still satisfies the new bounds etc. derived from the second SPS?  I did
> > consider this, and I think it may be possible to implement by delaying
> > CBS element validation and inference until libavcodec/vvc/ps.c.
> > However, there are no bitstreams in the conformance suite which contain
> > such a structure and this is different to how the native HEVC decoder
> > behaves (see libavcodec/hevc/ps.c:72).
>
> Is there some requirement in H.266 that in a single stream the PPS
> precedes the SPS?
>
No, the spec only states that when decoding the picture header, we need the
corresponding PPS and SPS.
If we strictly follow the spec, we should delay the PPS-derived process
when decoding the picture header, but it may be very complex.

7.4.3.4 Sequence parameter set RBSP semantics
An SPS RBSP shall be available to the decoding process, by inclusion in at
least one AU with TemporalId equal to 0 or
provided through external means, prior to it being referenced by either of
the following:
– a PH NAL unit having a ph_pic_parameter_set_id that refers to a PPS with
pps_seq_parameter_set_id equal to the
value of sps_seq_parameter_set_id in the SPS RBSP,
– a coded slice NAL unit having sh_picture_header_in_slice_header_flag
equal to 1 with a ph_pic_parameter_set_id
that refers to a PPS with pps_seq_parameter_set_id equal to the value of
sps_seq_parameter_set_id in the SPS RBSP

>
> Currently we effectively require that for a single stream because we use
> the SPS to enforce constraints on the PPS in both H.265 and H.266, but I'm
> not seeing a hard dependency and it looks like it will currently work on
> later stream starts as long as the parameters don't change too much.
>
> In H.264 there is an explicit dependency because you need
> chroma_format_idc to parse scaling lists, but again this will usually work
> because it's unlikely to change inline.
>
> If that is not required then this will discard the PPS of a stream where
> the SPS follows the PPS.  (Though I admit that before this it was only
> dubiously working because the bounds checking might be wrong.)
>
> Thanks,
>
> - Mark
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Nuo Mi]

Hi all,
I will merge this tomorrow if there are no objections.

Thank you.

On Mon, Jun 17, 2024 at 9:23 PM Nuo Mi <nuomi2021@gmail.com> wrote:

>
>
> On Sun, Jun 16, 2024 at 11:26 PM Mark Thompson <sw@jkqxz.net> wrote:
>
>> On 15/06/2024 17:37, Frank Plowman wrote:
>> > n 15/06/2024 13:24, Nuo Mi wrote:
>> >> On Sat, Jun 15, 2024 at 2:35 PM Christophe Gisquet <
>> >> christophe.gisquet@gmail.com> wrote:
>> >>
>> >>> Le ven. 14 juin 2024, 11:39, Frank Plowman <post@frankplowman.com> a
>> >>> écrit :
>> >>>
>> >>>> When the SPS associated with a particular SPS ID changes, invalidate
>> all
>> >>>> the PPSs which use that SPS ID.  Fixes crashes with illegal
>> bitstreams.
>> >>>> This is done in the CBS, rather than in libavcodec/vvc/ps.c like the
>> SPS
>> >>>> ID reuse validation, as parts of the CBS parsing process for PPSs
>> >>>> depend on the SPS being referred to.
>> >>>>
>> >>>
>> >>> I am uncertain about this. I have no definite knowledge nor proof,
>> but I
>> >>> would have thought these are persistent, IE it's legal to update some
>> of
>> >>> them, their validity depending on something else.
>> >>>
>> >>
>> >>> Wondering if the tested streams are thus conformant.
>> >>>
>> >>> But I don't know the actual rule. Maybe finding an EOB/EOS NUT?
>> Related to
>> >>> some particular shape of a clean random access point, that would
>> require
>> >>> retransmitting VPS/SPS/PPS/APS/... ?
>> >>>
>> >>> Asking Benjamin Bross might be a better option here.
>> >>>
>> >> Hi Chris,
>> >> spec said sps should not change in a CVS.  Frank has some patches to
>> fix a
>> >> similar issue.
>> >>
>> https://github.com/FFmpeg/FFmpeg/commit/2d79ae3f8a3306d24afe43ba505693a8dbefd21b
>> >>
>> >>
>> >> Hi Frank,
>> >> Did it crash before your error hand code in ps.c?
>> >> Could you send me the clip?
>> >>
>> >> Thank you
>> >>
>> >
>> > Hi both,
>> >
>> > Thank you for your reviews.
>> >
>> > An example of a crashing bitstream which is fixed by this patch is ID
>> > 295 available here: https://github.com/ffvvc/tests/pull/43.  The
>> > relevant part of the bitstream is a sequence of NAL units
>> >
>> > AU (decode_order=5)
>> > 18. SPS
>> >     sps_seq_parameter_set_id = 0
>> >     sps_ctb_log2_size_y = 5
>> > 19. PPS
>> >     pps_pic_parameter_set_id = 0
>> >     pps_seq_parameter_set_id = 0
>> > 20. IDR_N_LP
>> >     ph_pic_order_cnt_lsb = 0
>> >     NoOutputBeforeRecoveryFlag = 1
>> >     ph_pic_parameter_set_id = 0
>> >
>> > AU (decode_order=6)
>> > 21. AUD
>> > 22. VPS
>> > 23. SPS
>> >     sps_seq_parameter_set_id = 0
>> >     sps_ctb_log2_size_y = 7
>> > 24. PREFIX_APS
>> > 25. IDR_N_LP
>> >     ph_pic_order_cnt_lsb = 0
>> >     NoOutputBeforeRecoveryFlag = 1
>> >     ph_pic_parameter_set_id = 0
>> >
>> > The layout of SPSs alone is legal (not covered by the checks introduced
>> > in 2d79ae3f8a3306d24afe43ba505693a8dbefd21b) as the second AU is a CLVSS
>> > AU.  As a result, the bitstream crashes both before and after
>> > 2d79ae3f8a3306d24afe43ba505693a8dbefd21b.  What this patch does is
>> > produce an error when the VCL NAL unit in the second AU (25.) tries to
>> > use PPS ID 0, as the SPS NAL unit that PPS was defined with reference to
>> > (18.) is no longer available.
>> >
>> > Christophe, is my interpretation of your point correct when I say you
>> > are suggesting that the above sequence may be legal, so long as the PPS
>> > still satisfies the new bounds etc. derived from the second SPS?  I did
>> > consider this, and I think it may be possible to implement by delaying
>> > CBS element validation and inference until libavcodec/vvc/ps.c.
>> > However, there are no bitstreams in the conformance suite which contain
>> > such a structure and this is different to how the native HEVC decoder
>> > behaves (see libavcodec/hevc/ps.c:72).
>>
>> Is there some requirement in H.266 that in a single stream the PPS
>> precedes the SPS?
>>
> No, the spec only states that when decoding the picture header, we need
> the corresponding PPS and SPS.
> If we strictly follow the spec, we should delay the PPS-derived process
> when decoding the picture header, but it may be very complex.
>
> 7.4.3.4 Sequence parameter set RBSP semantics
> An SPS RBSP shall be available to the decoding process, by inclusion in at
> least one AU with TemporalId equal to 0 or
> provided through external means, prior to it being referenced by either of
> the following:
> – a PH NAL unit having a ph_pic_parameter_set_id that refers to a PPS with
> pps_seq_parameter_set_id equal to the
> value of sps_seq_parameter_set_id in the SPS RBSP,
> – a coded slice NAL unit having sh_picture_header_in_slice_header_flag
> equal to 1 with a ph_pic_parameter_set_id
> that refers to a PPS with pps_seq_parameter_set_id equal to the value of
> sps_seq_parameter_set_id in the SPS RBSP
>
>>
>> Currently we effectively require that for a single stream because we use
>> the SPS to enforce constraints on the PPS in both H.265 and H.266, but I'm
>> not seeing a hard dependency and it looks like it will currently work on
>> later stream starts as long as the parameters don't change too much.
>>
>> In H.264 there is an explicit dependency because you need
>> chroma_format_idc to parse scaling lists, but again this will usually work
>> because it's unlikely to change inline.
>>
>> If that is not required then this will discard the PPS of a stream where
>> the SPS follows the PPS.  (Though I admit that before this it was only
>> dubiously working because the bounds checking might be wrong.)
>>
>> Thanks,
>>
>> - Mark
>> _______________________________________________
>> 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".

Re: [FFmpeg-devel] [PATCH] lavc/vvc: Invalidate PPSs which refer to a changed SPS

[Nuo Mi]

On Wed, Jun 19, 2024 at 4:53 PM Nuo Mi <nuomi2021@gmail.com> wrote:

> Hi all,
> I will merge this tomorrow if there are no objections.
>
>>
>>> Applied.
Thank you, Frank, Christophe, Mark, and Anton, for a good discussion.
_______________________________________________
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".