[FFmpeg-devel] [PATCH 1/2] avcodec/pthread_frame: Remove ff_thread_release_buffer()

[Anton Khirnov]

Quoting Andreas Rheinhardt (2023-10-20 17:41:17)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2023-10-20 16:33:06)
> >> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> >> index c02408548c..1a4339b346 100644
> >> --- a/libavcodec/av1dec.c
> >> +++ b/libavcodec/av1dec.c
> >> @@ -636,9 +636,9 @@ static int get_pixel_format(AVCodecContext *avctx)
> >>      return 0;
> >>  }
> >>  
> >> -static void av1_frame_unref(AVCodecContext *avctx, AV1Frame *f)
> >> +static void av1_frame_unref(AV1Frame *f)
> >>  {
> >> -    ff_thread_release_buffer(avctx, f->f);
> >> +    av_frame_unref(f->f);
> >>      ff_refstruct_unref(&f->hwaccel_picture_private);
> >>      ff_refstruct_unref(&f->header_ref);
> >>      f->raw_frame_header = NULL;
> >> @@ -689,7 +689,7 @@ static int av1_frame_ref(AVCodecContext *avctx, AV1Frame *dst, const AV1Frame *s
> >>      return 0;
> >>  
> >>  fail:
> >> -    av1_frame_unref(avctx, dst);
> >> +    av1_frame_unref(dst);
> >>      return AVERROR(ENOMEM);
> >>  }
> >>  
> >> @@ -699,12 +699,15 @@ static av_cold int av1_decode_free(AVCodecContext *avctx)
> >>      AV1RawMetadataITUTT35 itut_t35;
> >>  
> >>      for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++) {
> >> -        av1_frame_unref(avctx, &s->ref[i]);
> >> -        av_frame_free(&s->ref[i].f);
> >> +        if (s->ref[i].f) {
> > 
> > Wouldn't it be simpler and more consistent to check for the frame's
> > existence in av1_frame_unref()?
> > 
> 
> The frame being NULL is a very exceptional scenario: It can only happen
> if init failed. In this case it is clear that the AV1Frame is blank and
> need not be unreferenced at all. Given that av1_frame_unref() is not
> called infrequently, why should it check all the time for this
> exceptional scenario? It seems better to handle this case in the only
> codepath that needs to handle this.

Ok, as you prefer.

-- 
Anton Khirnov