[FFmpeg-devel] [PATCH] lavc/vvc: Always set flags for the current picture

[FFmpeg-devel] [PATCH] lavc/vvc: Always set flags for the current picture

[Frank Plowman]

ff_vvc_frame_rpl uses the flags to detect whether a frame is in use.
Therefore, in the case of a CVSS AU (RASL/GDR with
NoOutputBeforeRecoveryFlag) with ph_non_ref_pic_flag = 1, the frame
would be freed before it is used.  Fix this by always marking the
current frame with VVC_FRAME_FLAG_SHORT_REF, as is done by the HEVC
decoder.

Additionally, add an assert0 to mitigate the effects of a frame being
freed before it is used.

Signed-off-by: Frank Plowman <post@frankplowman.com>
---
 libavcodec/vvc/refs.c   | 2 +-
 libavcodec/vvc/thread.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/vvc/refs.c b/libavcodec/vvc/refs.c
index 8b7ba639a3..26a5b0b34c 100644
--- a/libavcodec/vvc/refs.c
+++ b/libavcodec/vvc/refs.c
@@ -191,7 +191,7 @@ int ff_vvc_set_new_ref(VVCContext *s, VVCFrameContext *fc, AVFrame **frame)
     fc->ref = ref;
 
     if (s->no_output_before_recovery_flag && (IS_RASL(s) || !GDR_IS_RECOVERED(s)))
-        ref->flags = 0;
+        ref->flags = VVC_FRAME_FLAG_SHORT_REF;
     else if (ph->r->ph_pic_output_flag)
         ref->flags = VVC_FRAME_FLAG_OUTPUT;
 
diff --git a/libavcodec/vvc/thread.c b/libavcodec/vvc/thread.c
index 5b01dd2d20..e87ed5b676 100644
--- a/libavcodec/vvc/thread.c
+++ b/libavcodec/vvc/thread.c
@@ -801,6 +801,8 @@ int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext *fc)
 {
     VVCFrameThread *ft = fc->ft;
 
+    av_assert0(fc->ref->progress);
+
     ff_mutex_lock(&ft->lock);
 
     while (atomic_load(&ft->nb_scheduled_tasks) || atomic_load(&ft->nb_scheduled_listeners))
-- 
2.45.1

_______________________________________________
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] lavc/vvc: Always set flags for the current picture

[Nuo Mi]

On Mon, Jun 24, 2024 at 11:30 PM Frank Plowman <post@frankplowman.com>
wrote:

> ff_vvc_frame_rpl uses the flags to detect whether a frame is in use.
> Therefore, in the case of a CVSS AU (RASL/GDR with
> NoOutputBeforeRecoveryFlag) with ph_non_ref_pic_flag = 1, the frame
> would be freed before it is used.  Fix this by always marking the
> current frame with VVC_FRAME_FLAG_SHORT_REF, as is done by the HEVC
> decoder.
>
> Additionally, add an assert0 to mitigate the effects of a frame being
> freed before it is used.
>
> Signed-off-by: Frank Plowman <post@frankplowman.com>
> ---
>  libavcodec/vvc/refs.c   | 2 +-
>  libavcodec/vvc/thread.c | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/vvc/refs.c b/libavcodec/vvc/refs.c
> index 8b7ba639a3..26a5b0b34c 100644
> --- a/libavcodec/vvc/refs.c
> +++ b/libavcodec/vvc/refs.c
> @@ -191,7 +191,7 @@ int ff_vvc_set_new_ref(VVCContext *s, VVCFrameContext
> *fc, AVFrame **frame)
>      fc->ref = ref;
>
>      if (s->no_output_before_recovery_flag && (IS_RASL(s) ||
> !GDR_IS_RECOVERED(s)))
> -        ref->flags = 0;
> +        ref->flags = VVC_FRAME_FLAG_SHORT_REF;
>      else if (ph->r->ph_pic_output_flag)
>          ref->flags = VVC_FRAME_FLAG_OUTPUT;
>
> diff --git a/libavcodec/vvc/thread.c b/libavcodec/vvc/thread.c
> index 5b01dd2d20..e87ed5b676 100644
> --- a/libavcodec/vvc/thread.c
> +++ b/libavcodec/vvc/thread.c
> @@ -801,6 +801,8 @@ int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext
> *fc)
>  {
>      VVCFrameThread *ft = fc->ft;
>
> +    av_assert0(fc->ref->progress);
> +
>
Hi Frank,
Thank you for the patch.

We have a mission to remove all assert0s in the decoder, right? :)
If it's only for debugging, maybe we don't need it.

>      ff_mutex_lock(&ft->lock);
>
>      while (atomic_load(&ft->nb_scheduled_tasks) ||
> atomic_load(&ft->nb_scheduled_listeners))
> --
> 2.45.1
>
> _______________________________________________
> 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] lavc/vvc: Always set flags for the current picture

[Frank Plowman]

On 27/06/2024 13:52, Nuo Mi wrote:
> On Mon, Jun 24, 2024 at 11:30 PM Frank Plowman <post@frankplowman.com>
> wrote:
> 
>> ff_vvc_frame_rpl uses the flags to detect whether a frame is in use.
>> Therefore, in the case of a CVSS AU (RASL/GDR with
>> NoOutputBeforeRecoveryFlag) with ph_non_ref_pic_flag = 1, the frame
>> would be freed before it is used.  Fix this by always marking the
>> current frame with VVC_FRAME_FLAG_SHORT_REF, as is done by the HEVC
>> decoder.
>>
>> Additionally, add an assert0 to mitigate the effects of a frame being
>> freed before it is used.
>>
>> Signed-off-by: Frank Plowman <post@frankplowman.com>
>> ---
>>  libavcodec/vvc/refs.c   | 2 +-
>>  libavcodec/vvc/thread.c | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/vvc/refs.c b/libavcodec/vvc/refs.c
>> index 8b7ba639a3..26a5b0b34c 100644
>> --- a/libavcodec/vvc/refs.c
>> +++ b/libavcodec/vvc/refs.c
>> @@ -191,7 +191,7 @@ int ff_vvc_set_new_ref(VVCContext *s, VVCFrameContext
>> *fc, AVFrame **frame)
>>      fc->ref = ref;
>>
>>      if (s->no_output_before_recovery_flag && (IS_RASL(s) ||
>> !GDR_IS_RECOVERED(s)))
>> -        ref->flags = 0;
>> +        ref->flags = VVC_FRAME_FLAG_SHORT_REF;
>>      else if (ph->r->ph_pic_output_flag)
>>          ref->flags = VVC_FRAME_FLAG_OUTPUT;
>>
>> diff --git a/libavcodec/vvc/thread.c b/libavcodec/vvc/thread.c
>> index 5b01dd2d20..e87ed5b676 100644
>> --- a/libavcodec/vvc/thread.c
>> +++ b/libavcodec/vvc/thread.c
>> @@ -801,6 +801,8 @@ int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext
>> *fc)
>>  {
>>      VVCFrameThread *ft = fc->ft;
>>
>> +    av_assert0(fc->ref->progress);
>> +
>>
> Hi Frank,
> Thank you for the patch.
> 
> We have a mission to remove all assert0s in the decoder, right? :)
> If it's only for debugging, maybe we don't need it.
> 

I think assertions do have a role besides debugging in mitigating the
effects of a bug, however in this case I think it is probably not
necessary.  v2 sent.

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