[FFmpeg-devel,v3] lavu/hwcontext_vaapi: Use vaMapBuffer2 for mapping image buffers

This allows some optimizations in driver, such as not having to read
back the data if write-only mapping is requested.
---
v3: Fix another warning

 libavutil/hwcontext_vaapi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

On Fri, 27 Oct 2023 12:00:10 +0200
David Rosca <nowrep@gmail.com> wrote:

> This allows some optimizations in driver, such as not having to read
> back the data if write-only mapping is requested.
> ---
> v3: Fix another warning
> 
>  libavutil/hwcontext_vaapi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
> index 558fed94c6..86b0852c12 100644
> --- a/libavutil/hwcontext_vaapi.c
> +++ b/libavutil/hwcontext_vaapi.c
> @@ -799,6 +799,9 @@ static int vaapi_map_frame(AVHWFramesContext
> *hwfc, VAStatus vas;
>      void *address = NULL;
>      int err, i;
> +#if VA_CHECK_VERSION(1, 21, 0)
> +    uint32_t vaflags = 0;
> +#endif
>  
>      surface_id = (VASurfaceID)(uintptr_t)src->data[3];
>      av_log(hwfc, AV_LOG_DEBUG, "Map surface %#x.\n", surface_id);
> @@ -882,7 +885,15 @@ static int vaapi_map_frame(AVHWFramesContext
> *hwfc, }
>      }
>  
> +#if VA_CHECK_VERSION(1, 21, 0)
> +    if (flags & AV_HWFRAME_MAP_READ || !(flags &
> AV_HWFRAME_MAP_OVERWRITE))
> +        vaflags |= VA_MAPBUFFER_FLAG_READ;
> +    if (flags & AV_HWFRAME_MAP_WRITE)
> +        vaflags |= VA_MAPBUFFER_FLAG_WRITE;
> +    vas = vaMapBuffer2(hwctx->display, map->image.buf, &address,
> vaflags); +#else
>      vas = vaMapBuffer(hwctx->display, map->image.buf, &address);
> +#endif
>      if (vas != VA_STATUS_SUCCESS) {
>          av_log(hwfc, AV_LOG_ERROR, "Failed to map image from surface
> " "%#x: %d (%s).\n", surface_id, vas, vaErrorStr(vas));

LGTM. Thanks.

--phil

On 27/10/2023 11:00, David Rosca wrote:
> This allows some optimizations in driver, such as not having to read
> back the data if write-only mapping is requested.
> ---
> v3: Fix another warning
> 
>   libavutil/hwcontext_vaapi.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
> index 558fed94c6..86b0852c12 100644
> --- a/libavutil/hwcontext_vaapi.c
> +++ b/libavutil/hwcontext_vaapi.c
> @@ -799,6 +799,9 @@ static int vaapi_map_frame(AVHWFramesContext *hwfc,
>       VAStatus vas;
>       void *address = NULL;
>       int err, i;
> +#if VA_CHECK_VERSION(1, 21, 0)
> +    uint32_t vaflags = 0;
> +#endif
>   
>       surface_id = (VASurfaceID)(uintptr_t)src->data[3];
>       av_log(hwfc, AV_LOG_DEBUG, "Map surface %#x.\n", surface_id);
> @@ -882,7 +885,15 @@ static int vaapi_map_frame(AVHWFramesContext *hwfc,
>           }
>       }
>   
> +#if VA_CHECK_VERSION(1, 21, 0)
> +    if (flags & AV_HWFRAME_MAP_READ || !(flags & AV_HWFRAME_MAP_OVERWRITE))
> +        vaflags |= VA_MAPBUFFER_FLAG_READ;

I don't understand where the !overwrite has come from in this condition?

If the user requested write-only but not overwrite then they're expecting to write some pixels within the image (such as adding an overlay), but don't want to read anything.

> +    if (flags & AV_HWFRAME_MAP_WRITE)
> +        vaflags |= VA_MAPBUFFER_FLAG_WRITE;
> +    vas = vaMapBuffer2(hwctx->display, map->image.buf, &address, vaflags);
> +#else
>       vas = vaMapBuffer(hwctx->display, map->image.buf, &address);
> +#endif
>       if (vas != VA_STATUS_SUCCESS) {
>           av_log(hwfc, AV_LOG_ERROR, "Failed to map image from surface "
>                  "%#x: %d (%s).\n", surface_id, vas, vaErrorStr(vas));

Please add a note that there is a compatibility layer in libva so that MapBuffer2 calls MapBuffer if the driver doesn't expose it directly, so this does work with older drivers.  (The patch looked wrong before I realised that.)

Thanks,

- Mark

On Fri, Oct 27, 2023 at 7:14 PM Mark Thompson <sw@jkqxz.net> wrote:
>
> On 27/10/2023 11:00, David Rosca wrote:
> > This allows some optimizations in driver, such as not having to read
> > back the data if write-only mapping is requested.
> > ---
> > v3: Fix another warning
> >
> >   libavutil/hwcontext_vaapi.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
> > index 558fed94c6..86b0852c12 100644
> > --- a/libavutil/hwcontext_vaapi.c
> > +++ b/libavutil/hwcontext_vaapi.c
> > @@ -799,6 +799,9 @@ static int vaapi_map_frame(AVHWFramesContext *hwfc,
> >       VAStatus vas;
> >       void *address = NULL;
> >       int err, i;
> > +#if VA_CHECK_VERSION(1, 21, 0)
> > +    uint32_t vaflags = 0;
> > +#endif
> >
> >       surface_id = (VASurfaceID)(uintptr_t)src->data[3];
> >       av_log(hwfc, AV_LOG_DEBUG, "Map surface %#x.\n", surface_id);
> > @@ -882,7 +885,15 @@ static int vaapi_map_frame(AVHWFramesContext *hwfc,
> >           }
> >       }
> >
> > +#if VA_CHECK_VERSION(1, 21, 0)
> > +    if (flags & AV_HWFRAME_MAP_READ || !(flags & AV_HWFRAME_MAP_OVERWRITE))
> > +        vaflags |= VA_MAPBUFFER_FLAG_READ;
>
> I don't understand where the !overwrite has come from in this condition?

This logic is a couple lines ahead in the vaCreateImage path. If
AV_HWFRAME_MAP_OVERWRITE isn't set, it will call vaGetImage to read
the image data. And as vaDeriveImage + vaMapBuffer is read+write
mapping, I think the same logic needs to be applied to vaMapBuffer2
too.

>
> If the user requested write-only but not overwrite then they're expecting to write some pixels within the image (such as adding an overlay), but don't want to read anything.

Exactly for this case the read is needed. If the user writes only some
(not all) pixels of the image, then the rest of the image will be
invalid if a driver implements the mapping using staging texture
(which is what Mesa does).

>
> > +    if (flags & AV_HWFRAME_MAP_WRITE)
> > +        vaflags |= VA_MAPBUFFER_FLAG_WRITE;
> > +    vas = vaMapBuffer2(hwctx->display, map->image.buf, &address, vaflags);
> > +#else
> >       vas = vaMapBuffer(hwctx->display, map->image.buf, &address);
> > +#endif
> >       if (vas != VA_STATUS_SUCCESS) {
> >           av_log(hwfc, AV_LOG_ERROR, "Failed to map image from surface "
> >                  "%#x: %d (%s).\n", surface_id, vas, vaErrorStr(vas));
>
> Please add a note that there is a compatibility layer in libva so that MapBuffer2 calls MapBuffer if the driver doesn't expose it directly, so this does work with older drivers.  (The patch looked wrong before I realised that.)
>
> Thanks,
>
> - Mark
> _______________________________________________
> 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".

On 27/10/2023 19:46, David Rosca wrote:
> On Fri, Oct 27, 2023 at 7:14 PM Mark Thompson <sw@jkqxz.net> wrote:
>> On 27/10/2023 11:00, David Rosca wrote:
>>> This allows some optimizations in driver, such as not having to read
>>> back the data if write-only mapping is requested.
>>> ---
>>> v3: Fix another warning
>>>
>>>    libavutil/hwcontext_vaapi.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
>>> index 558fed94c6..86b0852c12 100644
>>> --- a/libavutil/hwcontext_vaapi.c
>>> +++ b/libavutil/hwcontext_vaapi.c
>>> @@ -799,6 +799,9 @@ static int vaapi_map_frame(AVHWFramesContext *hwfc,
>>>        VAStatus vas;
>>>        void *address = NULL;
>>>        int err, i;
>>> +#if VA_CHECK_VERSION(1, 21, 0)
>>> +    uint32_t vaflags = 0;
>>> +#endif
>>>
>>>        surface_id = (VASurfaceID)(uintptr_t)src->data[3];
>>>        av_log(hwfc, AV_LOG_DEBUG, "Map surface %#x.\n", surface_id);
>>> @@ -882,7 +885,15 @@ static int vaapi_map_frame(AVHWFramesContext *hwfc,
>>>            }
>>>        }
>>>
>>> +#if VA_CHECK_VERSION(1, 21, 0)
>>> +    if (flags & AV_HWFRAME_MAP_READ || !(flags & AV_HWFRAME_MAP_OVERWRITE))
>>> +        vaflags |= VA_MAPBUFFER_FLAG_READ;
>>
>> I don't understand where the !overwrite has come from in this condition?
> 
> This logic is a couple lines ahead in the vaCreateImage path. If
> AV_HWFRAME_MAP_OVERWRITE isn't set, it will call vaGetImage to read
> the image data. And as vaDeriveImage + vaMapBuffer is read+write
> mapping, I think the same logic needs to be applied to vaMapBuffer2
> too.

The case is not the same as the one earlier, because ...

>>
>> If the user requested write-only but not overwrite then they're expecting to write some pixels within the image (such as adding an overlay), but don't want to read anything.
> 
> Exactly for this case the read is needed. If the user writes only some
> (not all) pixels of the image, then the rest of the image will be
> invalid if a driver implements the mapping using staging texture
> (which is what Mesa does).

... that is not what the documentation says the function has to do:

"""

/**
  * Map data store of the buffer into the client's address space
  * this interface could be used to convey the operation hint
  * backend driver could use these hint to optimize the implementations
  */

/** \brief VA_MAPBUFFER_FLAG_DEFAULT is used when there are no flag specified
  * same as VA_MAPBUFFER_FLAG_READ | VA_MAPBUFFER_FLAG_WRITE.
  */

/** \brief application will read the surface after map */
#define VA_MAPBUFFER_FLAG_READ    1
/** \brief application will write the surface after map */
#define VA_MAPBUFFER_FLAG_WRITE   2

VAStatus vaMapBuffer2(
     VADisplay dpy,
     VABufferID buf_id,  /* in */
     void **pbuf,        /* out */
     uint32_t flags      /* in */
);

"""

There is no suggestion here that setting WRITE & !READ implies that the user has to completely overwrite the surface.

A user reading this would reasonably set write-only in the case where they want to add an overlay, but your interpretation would make the driver discard the rest of the image and give an unexpected result.

If you believe the function is intended to work this way then can you submit a patch to libva to update the documentation to say what you expect?  (Which can also be seen by other driver implementers, so that they can comment on whether the additional meaning you assign to the flags is something they would support as well.)

Alternatively, it sounds like you would want to add an OVERWRITE flag to libva, for exactly the same reasons that it exists already in ffmpeg.

>>> +    if (flags & AV_HWFRAME_MAP_WRITE)
>>> +        vaflags |= VA_MAPBUFFER_FLAG_WRITE;
>>> +    vas = vaMapBuffer2(hwctx->display, map->image.buf, &address, vaflags);
>>> +#else
>>>        vas = vaMapBuffer(hwctx->display, map->image.buf, &address);
>>> +#endif
>>>        if (vas != VA_STATUS_SUCCESS) {
>>>            av_log(hwfc, AV_LOG_ERROR, "Failed to map image from surface "
>>>                   "%#x: %d (%s).\n", surface_id, vas, vaErrorStr(vas));
>>
>> Please add a note that there is a compatibility layer in libva so that MapBuffer2 calls MapBuffer if the driver doesn't expose it directly, so this does work with older drivers.  (The patch looked wrong before I realised that.)
>>

Thanks,

- Mark