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