Mostly Harmless :) On Mon, Apr 15, 2024 at 6:24 AM James Almer <jamr...@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 <p...@frankplowman.com> > wrote: > >> > >>> On 08/04/2024 15:12, Nuo Mi wrote: > >>>> On Mon, Apr 8, 2024 at 4:37 PM Frank Plowman <p...@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 <jamr...@gmail.com> > >>> wrote: > >>>>>>> > >>>>>>>> Signed-off-by: James Almer <jamr...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".