[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 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?