[FFmpeg-devel] [PATCH] avcodec/vvc/ps: reset sps_id_used on PS uninit

[Nuo Mi]

Mostly Harmless :)

On Mon, Apr 15, 2024 at 6:24 AM James Almer <jamrial at gmail.com> wrote:

> On 4/13/2024 7:18 AM, Frank Plowman wrote:
> >
> >
> > On 09/04/2024 14:36, Nuo Mi wrote:
> >> On Mon, Apr 8, 2024 at 11:15 PM Frank Plowman <post at frankplowman.com>
> wrote:
> >>
> >>> On 08/04/2024 15:12, Nuo Mi wrote:
> >>>> On Mon, Apr 8, 2024 at 4:37 PM Frank Plowman <post at frankplowman.com>
> >>> wrote:
> >>>>
> >>>>> On 07/04/2024 15:48, James Almer wrote:
> >>>>>> On 4/7/2024 10:38 AM, Nuo Mi wrote:
> >>>>>>> On Sun, Apr 7, 2024 at 11:05 AM James Almer <jamrial at gmail.com>
> >>> wrote:
> >>>>>>>
> >>>>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
> >>>>>>>> ---
> >>>>>>>>    libavcodec/vvc/ps.c | 1 +
> >>>>>>>>    1 file changed, 1 insertion(+)
> >>>>>>>>
> >>>>>>>> diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c
> >>>>>>>> index 3c71c34bae..83ee75fb62 100644
> >>>>>>>> --- a/libavcodec/vvc/ps.c
> >>>>>>>> +++ b/libavcodec/vvc/ps.c
> >>>>>>>> @@ -912,6 +912,7 @@ void ff_vvc_ps_uninit(VVCParamSets *ps)
> >>>>>>>>            ff_refstruct_unref(&ps->sps_list[i]);
> >>>>>>>>        for (int i = 0; i < FF_ARRAY_ELEMS(ps->pps_list); i++)
> >>>>>>>>            ff_refstruct_unref(&ps->pps_list[i]);
> >>>>>>>> +    ps->sps_id_used = 0;
> >>>>>>>>
> >>>>>>> Hi James,
> >>>>>>> thank you for the patch.
> >>>>>>>
> >>>>>>> Is this really necessary?
> >>>>>>> vvc_ps_uninit will be called by vvc_decode_free,
> >>>>>>> We are not supposed to use any member of VVCParamSets after
> >>>>>>> vvc_decode_free.
> >>>>>>
> >>>>>> My bad, i thought it was also called on every flush() call.
> >>>>>>
> >>>>>> Something like the following:
> >>>>>>
> >>>>>>> diff --git a/libavcodec/vvc/dec.c b/libavcodec/vvc/dec.c
> >>>>>>> index eb447604fe..463536512e 100644
> >>>>>>> --- a/libavcodec/vvc/dec.c
> >>>>>>> +++ b/libavcodec/vvc/dec.c
> >>>>>>> @@ -963,6 +963,8 @@ static av_cold void
> >>>>>>> vvc_decode_flush(AVCodecContext *avctx)
> >>>>>>>           ff_vvc_flush_dpb(last);
> >>>>>>>       }
> >>>>>>>
> >>>>>>> +    s->ps->sps_id_used = 0;
> >>>>>>> +
> >>>>>>>       s->eos = 1;
> >>>>>>>   }
> >>>>>>
> >>>>>> Should be done on FFCodec.flush() (like when seeking) as the
> previous
> >>>>>> state is no longer valid, right?
> >>>>>
> >>>>> Yes I agree, I think this is needed.  Cases where the random access
> >>>>> point has no leading pictures should be covered by the existing
> logic as
> >>>>> these always fall at the start of a CVS, but I think this is needed
> to
> >>>>> cover the case in which there are leading pictures.
> >>>>>
> >>>> This patch isn't necessary.
> >>>> Leading pictures won't carry SPS.
> >>>> IDR, CRA, and GDR will carry SPS, but they will also start a new CVS,
> >>> which
> >>>> will covered by the current logic.
> >>>>
> >>>
> >>> I may be misunderstanding the spec, NoOutputBeforeRecoveryFlag is set
> >>> for pictures which have no leading pictures, no?  In any case take, for
> >>> instance, a CRA picture.  In most cases, CRA pictures have
> >>> NoOutputBeforeRecoveryFlag=0, therefore are not CLVSS pictures and
> >>> sps_id_used is not reset by the existing logic.  Do we not need to
> reset
> >>> sps_id_used when seeking to a CRA then?
> >>>
> >> After seeking, we'll set s->last_eos to 1.
> >> For a CRA, decode_recovery_flag will set
> s->no_output_before_recovery_flag
> >> to s->last_eos.
> >> So no_output_before_recovery_flag will be 1, not 0.
> >
> > I see, my mistake.  Yes in this case I do not think sps_id_used must be
> > explicitly reset.
>
> Should i revert the commit then, or is it harmless?
> This you discussed above could also be documented somewhere in the code.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>