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

Cameron Gutman aicommander at gmail.com
Mon Jan 10 10:17:37 EET 2022


> On Jan 9, 2022, at 3:24 AM, Anton Khirnov <anton at khirnov.net> wrote:
> 
> Quoting Cameron Gutman (2022-01-03 01:33:19)
>> Signed-off-by: Cameron Gutman <aicommander at 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.


Cam

> -- 
> Anton Khirnov



More information about the ffmpeg-devel mailing list