Quoting Cameron Gutman (2022-01-10 09:17:37)
>
> > On Jan 9, 2022, at 3:24 AM, Anton Khirnov <an...@khirnov.net> wrote:
> >
> > Quoting Cameron Gutman (2022-01-03 01:33:19)
> >> Signed-off-by: Cameron Gutman <aicomman...@gmail.com>
> >> ---
> >> libavutil/hwcontext_videotoolbox.c | 25 +++++++++++++++++++++++++
> >> 1 file changed, 25 insertions(+)
> >>
> >> diff --git a/libavutil/hwcontext_videotoolbox.c
> >> b/libavutil/hwcontext_videotoolbox.c
> >> index 0a8dbe9f33..026127d412 100644
> >> --- a/libavutil/hwcontext_videotoolbox.c
> >> +++ b/libavutil/hwcontext_videotoolbox.c
> >> @@ -711,6 +711,30 @@ fail:
> >> return err;
> >> }
> >>
> >> +static int vt_map_from(AVHWFramesContext *hwfc, AVFrame *dst,
> >> + const AVFrame *src, int flags)
> >> +{
> >> + int err;
> >> +
> >> + if (dst->format == AV_PIX_FMT_NONE)
> >> + dst->format = hwfc->sw_format;
> >> + else if (dst->format != hwfc->sw_format)
> >> + return AVERROR(ENOSYS);
> >> +
> >> + err = vt_map_frame(hwfc, dst, src, flags);
> >> + if (err)
> >> + return err;
> >> +
> >> + dst->width = src->width;
> >> + dst->height = src->height;
> >> +
> >> + err = av_frame_copy_props(dst, src);
> >> + if (err)
> >> + return err;
> >
> > Don't you need to unmap the frame in this error path?
> >
>
> Maybe? It's complicated...
>
> The mapping is still written to dst and it will be unmapped when
> av_frame_unref() is called on dst. However, if the caller wants to try again
> with same dst frame for some reason, then it looks like it will leak the first
> "failed" mapping. AFAICT, another call to ff_hwframe_map_create() will
> overwrite dst->buf[0] without unrefing it first, but that makes sense given
> that the docs say "dst should be blank" (arguably that "should" ought to be a
> "must" in this case). However, this isn't the full story.
>
> None of the existing map_from() implementations (VAAPI, DRM, DXVA2) actually
> unmap when av_frame_copy_props() fails. The only ones that don't have this bug
> are OpenCL and Vulkan, and that's only because they forgot to call
> av_frame_copy_props() entirely!
>
> Ideally, you'd have nothing modified in dst when av_hwframe_map() fails,
> but that isn't the case in practice. Much of the mapping process involves
> irreversible changes to the dst frame, including overwriting dst->format,
> dst->width, dst->height, dst->data, dst->linesize, even partially copied
> frame properties prior to av_frame_copy_props() failing.
>
> Given these limitations, it seems like the only sane thing to do with a dst
> frame after failure of av_hwframe_map() (other than ENOSYS) is to unref/free.
> The frame is basically in an undefined state after such a failure. If that's
> the case, then we don't actually have a leak here since av_frame_unref()
> will do the right thing and free the old mapping.
Right, but who will call this av_frame_unref(). The doxy for
av_hwframe_map() does not specify what precisely happens on failure, but
I would expect it to clean dst.
--
Anton Khirnov
_______________________________________________
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".
