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

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

[Cameron Gutman]

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;
+
+    return 0;
+}
+
 static int vt_device_create(AVHWDeviceContext *ctx, const char *device,
                             AVDictionary *opts, int flags)
 {
@@ -736,6 +760,7 @@ const HWContextType ff_hwcontext_type_videotoolbox = {
     .transfer_get_formats = vt_transfer_get_formats,
     .transfer_data_to     = vt_transfer_data_to,
     .transfer_data_from   = vt_transfer_data_from,
+    .map_from             = vt_map_from,
 
     .pix_fmts = (const enum AVPixelFormat[]){ AV_PIX_FMT_VIDEOTOOLBOX, AV_PIX_FMT_NONE },
 };
-- 
2.25.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".

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

[Aman Karmani]

On Sun, Jan 2, 2022 at 4:33 PM Cameron Gutman <aicommander@gmail.com> wrote:

> 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;
> +
> +    return 0;
> +}
> +
>  static int vt_device_create(AVHWDeviceContext *ctx, const char *device,
>                              AVDictionary *opts, int flags)
>  {
> @@ -736,6 +760,7 @@ const HWContextType ff_hwcontext_type_videotoolbox = {
>      .transfer_get_formats = vt_transfer_get_formats,
>      .transfer_data_to     = vt_transfer_data_to,
>      .transfer_data_from   = vt_transfer_data_from,
> +    .map_from             = vt_map_from,


Thanks for this!

Does this add support for hwdownload filter? Or what's the best way to test
this patch?

Aman


>
>      .pix_fmts = (const enum AVPixelFormat[]){ AV_PIX_FMT_VIDEOTOOLBOX,
> AV_PIX_FMT_NONE },
>  };
> --
> 2.25.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".
>
_______________________________________________
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".

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

[Cameron Gutman]

On 1/2/22 19:21, Aman Karmani wrote:
> 
> 
> On Sun, Jan 2, 2022 at 4:33 PM Cameron Gutman <aicommander@gmail.com <mailto:aicommander@gmail.com>> wrote:
> 
>     Signed-off-by: Cameron Gutman <aicommander@gmail.com <mailto: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;
>     +
>     +    return 0;
>     +}
>     +
>      static int vt_device_create(AVHWDeviceContext *ctx, const char *device,
>                                  AVDictionary *opts, int flags)
>      {
>     @@ -736,6 +760,7 @@ const HWContextType ff_hwcontext_type_videotoolbox = {
>          .transfer_get_formats = vt_transfer_get_formats,
>          .transfer_data_to     = vt_transfer_data_to,
>          .transfer_data_from   = vt_transfer_data_from,
>     +    .map_from             = vt_map_from,
> 
> 
> Thanks for this!
> 
> Does this add support for hwdownload filter? Or what's the best way to test this patch?
> 
> Aman
> 
> 

I'm calling the C API directly (av_hwframe_map) from my application, but you can test it
with the command-line tool using something like this:

ffmpeg -hwaccel videotoolbox -hwaccel_output_format videotoolbox_vld -i 1280x720.h264 -vf "hwmap,format=nv12" 1280x720.yuv

Replacing hwmap (which uses map_from) with hwdownload (which uses transfer_data_from)
yields the same decoded YUV output, as expected.



Cam

> 
>          .pix_fmts = (const enum AVPixelFormat[]){ AV_PIX_FMT_VIDEOTOOLBOX, AV_PIX_FMT_NONE },
>      };
>     -- 
>     2.25.1
> 
>     _______________________________________________
>     ffmpeg-devel mailing list
>     ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
>     https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
>     To unsubscribe, visit link above, or email
>     ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
> 
_______________________________________________
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".

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

[Aman Karmani]

On Sun, Jan 2, 2022 at 6:22 PM Cameron Gutman <aicommander@gmail.com> wrote:

> On 1/2/22 19:21, Aman Karmani wrote:
> >
> >
> > On Sun, Jan 2, 2022 at 4:33 PM Cameron Gutman <aicommander@gmail.com
> <mailto:aicommander@gmail.com>> wrote:
> >
> >     Signed-off-by: Cameron Gutman <aicommander@gmail.com <mailto:
> 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;
> >     +
> >     +    return 0;
> >     +}
> >     +
> >      static int vt_device_create(AVHWDeviceContext *ctx, const char
> *device,
> >                                  AVDictionary *opts, int flags)
> >      {
> >     @@ -736,6 +760,7 @@ const HWContextType
> ff_hwcontext_type_videotoolbox = {
> >          .transfer_get_formats = vt_transfer_get_formats,
> >          .transfer_data_to     = vt_transfer_data_to,
> >          .transfer_data_from   = vt_transfer_data_from,
> >     +    .map_from             = vt_map_from,
> >
> >
> > Thanks for this!
> >
> > Does this add support for hwdownload filter? Or what's the best way to
> test this patch?
> >
> > Aman
> >
> >
>
> I'm calling the C API directly (av_hwframe_map) from my application, but
> you can test it
> with the command-line tool using something like this:
>
> ffmpeg -hwaccel videotoolbox -hwaccel_output_format videotoolbox_vld -i
> 1280x720.h264 -vf "hwmap,format=nv12" 1280x720.yuv
>
> Replacing hwmap (which uses map_from) with hwdownload (which uses
> transfer_data_from)
> yields the same decoded YUV output, as expected.


Thanks, I didn't realize hwmap used a different route.

Patch applied to master.

Aman


>
>
>
> Cam
>
> >
> >          .pix_fmts = (const enum AVPixelFormat[]){
> AV_PIX_FMT_VIDEOTOOLBOX, AV_PIX_FMT_NONE },
> >      };
> >     --
> >     2.25.1
> >
> >     _______________________________________________
> >     ffmpeg-devel mailing list
> >     ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> >     https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> >
> >     To unsubscribe, visit link above, or email
> >     ffmpeg-devel-request@ffmpeg.org <mailto:
> ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
> >
>
_______________________________________________
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".

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

[Anton Khirnov]

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?

-- 
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".

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

[Cameron Gutman]

> 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.


Cam

> -- 
> 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".

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

[Anton Khirnov]

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".

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

[Cameron Gutman]

> On Jan 17, 2022, at 3:07 AM, Anton Khirnov <anton@khirnov.net> wrote:
> 
> 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.
> 

Whoever allocated the frame will unref/free it when they are done with it.

For example, vf_hwmap.c does this:

        dst = av_frame_alloc();
        if (!dst) {
            av_frame_free(&src);
            return NULL;
        }

        err = av_hwframe_map(dst, src, ctx->mode);
        if (err) {
            av_log(avctx, AV_LOG_ERROR, "Failed to map frame to "
                   "software: %d.\n", err);
            av_frame_free(&src);
            av_frame_free(&dst);
            return NULL;
        }

So the mapping will be freed in the call to av_frame_free(&dst) in this case.

Where we might run into issues with leaks is if someone does:

        /* Try NV12 first */
        dst->format = AV_PIX_FMT_NV12;
        err = av_hwframe_map(dst, src, AV_HWFRAME_MAP_READ);
        if (err) {
            /* That didn't work. Now try YUV420P */
            dst->format = AV_PIX_FMT_YUV420P;
            err = av_hwframe_map(dst, src, AV_HWFRAME_MAP_READ);
        }

Assuming that first av_hwframe_map() fails in av_frame_copy_props(), the second
call (if successful) will cause the first mapping to be leaked. For this case,
it makes sense to clear dst on failure.

The docs say "dst should be blank", so you could also argue that the second
call to av_hwframe_map() is incorrect, since it is not guaranteed that dst is
blank at that point (and today it is guaranteed _not_ to be blank for any of
the map_from() implementations in the case that av_frame_copy_props() fails).

If we do want dst to be cleared upon failure, a good place to do it might be in
av_hwframe_map() itself (since none of the existing map_from() implementations
do it themselves). That would formalize the behavior of cleaning dst on failure
rather than leaving the various hwcontext implementations to handle it how they
choose.

> -- 
> 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".

_______________________________________________
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".