| Message ID | 20240624153021.16399-1-post@frankplowman.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/vvc: Always set flags for the current picture | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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". >
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.
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))
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(-)