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

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

[James Almer]

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

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

[Nuo Mi]

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".
>
_______________________________________________
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] avcodec/vvc/ps: reset sps_id_used on PS uninit

[James Almer]

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?
_______________________________________________
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] avcodec/vvc/ps: reset sps_id_used on PS uninit

[Frank Plowman]

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
_______________________________________________
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] avcodec/vvc/ps: reset sps_id_used on PS uninit

[Nuo Mi]

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".
>
_______________________________________________
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] avcodec/vvc/ps: reset sps_id_used on PS uninit

[Frank Plowman]

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?
_______________________________________________
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] avcodec/vvc/ps: reset sps_id_used on PS uninit

[Nuo Mi]

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.

_______________________________________________
> 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] avcodec/vvc/ps: reset sps_id_used on PS uninit

[Frank Plowman]

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.

-- 
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] avcodec/vvc/ps: reset sps_id_used on PS uninit

[James Almer]

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

Re: [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@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".
>
_______________________________________________
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".