| Message ID | 20240407030435.1060-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/vvc/ps: reset sps_id_used on PS uninit | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Sun, Apr 7, 2024 at 11:05 AM James Almer <jamrial@gmail.com> wrote: > Signed-off-by: James Almer <jamrial@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. } > > static void alf_coeff(int16_t *coeff, > -- > 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". >
On 4/7/2024 10:38 AM, Nuo Mi wrote: > On Sun, Apr 7, 2024 at 11:05 AM James Almer <jamrial@gmail.com> wrote: > >> Signed-off-by: James Almer <jamrial@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?
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@gmail.com> wrote: >> >>> Signed-off-by: James Almer <jamrial@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. Thanks, -- Frank
On Mon, Apr 8, 2024 at 4:37 PM Frank Plowman <post@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@gmail.com> wrote: > >> > >>> Signed-off-by: James Almer <jamrial@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. > Thanks, > -- > 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". >
On 08/04/2024 15:12, Nuo Mi wrote: > On Mon, Apr 8, 2024 at 4:37 PM Frank Plowman <post@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@gmail.com> wrote: >>>> >>>>> Signed-off-by: James Almer <jamrial@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?
On Mon, Apr 8, 2024 at 11:15 PM Frank Plowman <post@frankplowman.com> wrote: > On 08/04/2024 15:12, Nuo Mi wrote: > > On Mon, Apr 8, 2024 at 4:37 PM Frank Plowman <post@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@gmail.com> > wrote: > >>>> > >>>>> Signed-off-by: James Almer <jamrial@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.
On 09/04/2024 14:36, Nuo Mi wrote: > On Mon, Apr 8, 2024 at 11:15 PM Frank Plowman <post@frankplowman.com> wrote: > >> On 08/04/2024 15:12, Nuo Mi wrote: >>> On Mon, Apr 8, 2024 at 4:37 PM Frank Plowman <post@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@gmail.com> >> wrote: >>>>>> >>>>>>> Signed-off-by: James Almer <jamrial@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.
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@frankplowman.com> wrote: >> >>> On 08/04/2024 15:12, Nuo Mi wrote: >>>> On Mon, Apr 8, 2024 at 4:37 PM Frank Plowman <post@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@gmail.com> >>> wrote: >>>>>>> >>>>>>>> Signed-off-by: James Almer <jamrial@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.
Mostly Harmless :) On Mon, Apr 15, 2024 at 6:24 AM James Almer <jamrial@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@frankplowman.com> > wrote: > >> > >>> On 08/04/2024 15:12, Nuo Mi wrote: > >>>> On Mon, Apr 8, 2024 at 4:37 PM Frank Plowman <post@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@gmail.com> > >>> wrote: > >>>>>>> > >>>>>>>> Signed-off-by: James Almer <jamrial@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@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 --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; } static void alf_coeff(int16_t *coeff,
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/vvc/ps.c | 1 + 1 file changed, 1 insertion(+)