Re: [FFmpeg-devel] [PATCH] lavu/videotoolbox: add support for memory mapping frames

[Anton Khirnov <anton@khirnov.net>]

Quoting Cameron Gutman (2022-01-10 09:17:37)
> 
> > On Jan 9, 2022, at 3:24 AM, Anton Khirnov <anton@khirnov.net> wrote:
> > 
> > Quoting Cameron Gutman (2022-01-03 01:33:19)
> >> Signed-off-by: Cameron Gutman <aicommander@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-request@ffmpeg.org with subject "unsubscribe".