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

Reply via email to