[FFmpeg-devel] [PATCH] hwcontext_vulkan: fix exporting multi-plane DRM modifiers

[Russell Greene]

Is this documented anywhere? Should I maybe have a fixed sized stack
buffer and fallback to an allocation if it's just a rule of thumb that
it'll be less than some known small number. I just somehow doubt this
is in the vulkan spec or any sort of guaranteed....

On Thu, May 1, 2025 at 6:10 PM Lynne <dev at lynne.ee> wrote:
>
> On 01/05/2025 07:05, Russell Greene wrote:
> > From: Russell Greene <russell at shotover.com>
> >
> > Previously, it was assumed that `drmFormatModifierPlaneCount` was one
> > for all modifiers when exporting, which is not always the case, in
> > particular for AMD GPUs and maybe others.
> >
> > Fetch the number of memory planes and fill the structs appropriately in this situation.
> >
> > The encoded stream is still bad in the case whre modifers are involved,
> > but I think this patch still stands on its own and I suspect that may be a driver bug.
> >
> > A potential improvement that could be make is to cache the format
> > information, so we can avoid the two GetPhysicalDeviceFormatProperties2
> > calls for each export, as well as the allocation. I doubt this is very
> > expensive, but seemed worth noting.
> >
> > Signed-off-by: Russell Greene <russellgreene8 at gmail.com>
> > ---
> >   libavutil/hwcontext_vulkan.c | 76 +++++++++++++++++++++++++++++++-----
> >   1 file changed, 67 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
> > index ade0235ef1..d14fa4655b 100644
> > --- a/libavutil/hwcontext_vulkan.c
> > +++ b/libavutil/hwcontext_vulkan.c
> > @@ -3787,6 +3787,17 @@ static inline uint32_t vulkan_fmt_to_drm(VkFormat vkfmt)
> >       return DRM_FORMAT_INVALID;
> >   }
> >
> > +#define MAX_MEMORY_PLANES 4
> > +static VkImageAspectFlags plane_index_to_aspect(int plane) {
> > +    if (plane == 0) return VK_IMAGE_ASPECT_MEMORY_PLANE_0_BIT_EXT;
> > +    if (plane == 1) return VK_IMAGE_ASPECT_MEMORY_PLANE_1_BIT_EXT;
> > +    if (plane == 2) return VK_IMAGE_ASPECT_MEMORY_PLANE_2_BIT_EXT;
> > +    if (plane == 3) return VK_IMAGE_ASPECT_MEMORY_PLANE_3_BIT_EXT;
> > +
> > +    av_assert2 (false && "Invalid plane index");
> > +    return VK_IMAGE_ASPECT_MEMORY_PLANE_0_BIT_EXT;
> > +}
> > +
> >   static int vulkan_map_to_drm(AVHWFramesContext *hwfc, AVFrame *dst,
> >                                const AVFrame *src, int flags)
> >   {
> > @@ -3855,14 +3866,65 @@ static int vulkan_map_to_drm(AVHWFramesContext *hwfc, AVFrame *dst,
> >
> >       drm_desc->nb_layers = planes;
> >       for (int i = 0; i < drm_desc->nb_layers; i++) {
> > -        VkSubresourceLayout layout;
> > -        VkImageSubresource sub = {
> > -            .aspectMask = VK_IMAGE_ASPECT_MEMORY_PLANE_0_BIT_EXT,
> > +        VkDrmFormatModifierPropertiesListEXT modp = {
> > +            .sType = VK_STRUCTURE_TYPE_DRM_FORMAT_MODIFIER_PROPERTIES_LIST_EXT,
> > +        };
> > +        VkFormatProperties2 fmtp = {
> > +            .sType = VK_STRUCTURE_TYPE_FORMAT_PROPERTIES_2,
> > +            .pNext = &modp,
> >           };
> >           VkFormat plane_vkfmt = av_vkfmt_from_pixfmt(hwfc->sw_format)[i];
> >
> > -        drm_desc->layers[i].format    = vulkan_fmt_to_drm(plane_vkfmt);
> > -        drm_desc->layers[i].nb_planes = 1;
> > +        drm_desc->layers[i].format = vulkan_fmt_to_drm(plane_vkfmt);
> > +
> > +        /* query drmFormatModifierCount by keeping pDrmFormatModifierProperties NULL */
> > +        vk->GetPhysicalDeviceFormatProperties2(hwctx->phys_dev, plane_vkfmt, &fmtp);
> > +
> > +        modp.pDrmFormatModifierProperties =
> > +            av_calloc(modp.drmFormatModifierCount, sizeof(*modp.pDrmFormatModifierProperties));
> > +        if (!modp.pDrmFormatModifierProperties) {
> > +            err = AVERROR(ENOMEM);
> > +            goto end;
> > +        }
> > +        vk->GetPhysicalDeviceFormatProperties2(hwctx->phys_dev, plane_vkfmt, &fmtp);
> > +
> > +        VkDrmFormatModifierPropertiesEXT *mod_props = NULL;
> > +        for (uint32_t i = 0; i < modp.drmFormatModifierCount; ++i) {
> > +            VkDrmFormatModifierPropertiesEXT *m = &modp.pDrmFormatModifierProperties[i];
> > +            if (m->drmFormatModifier == drm_mod.drmFormatModifier) {
> > +                mod_props = m;
> > +                break;
> > +            }
> > +        }
> > +
> > +        if (!mod_props) {
> > +            av_free(modp.pDrmFormatModifierProperties);
> > +            av_log(hwfc, AV_LOG_ERROR, "Cannot fetch modifier properties for modifier "PRIu64"!\n",
> > +                   drm_mod.drmFormatModifier);
> > +            err = AVERROR_EXTERNAL;
> > +            goto end;
> > +        }
> > +        drm_desc->layers[i].nb_planes = mod_props->drmFormatModifierPlaneCount;
> > +        av_free(modp.pDrmFormatModifierProperties);
> > +
> > +        if (drm_desc->layers[i].nb_planes > MAX_MEMORY_PLANES) {
> > +            av_log(hwfc, AV_LOG_ERROR, "Too many memory planes for DRM format!\n");
> > +            err = AVERROR_EXTERNAL;
> > +            goto end;
> > +        }
> > +
> > +        for (int j = 0; j < drm_desc->layers[i].nb_planes; j++) {
> > +            VkSubresourceLayout layout;
> > +            VkImageSubresource sub = {
> > +                .aspectMask = plane_index_to_aspect(j),
> > +            };
> > +
> > +            drm_desc->layers[i].planes[j].object_index = FFMIN(i, drm_desc->nb_objects - 1);
> > +
> > +            vk->GetImageSubresourceLayout(hwctx->act_dev, f->img[i], &sub, &layout);
> > +            drm_desc->layers[i].planes[j].offset = layout.offset;
> > +            drm_desc->layers[i].planes[j].pitch  = layout.rowPitch;
> > +        }
> >
> >           if (drm_desc->layers[i].format == DRM_FORMAT_INVALID) {
> >               av_log(hwfc, AV_LOG_ERROR, "Cannot map to DRM layer, unsupported!\n");
> > @@ -3870,14 +3932,10 @@ static int vulkan_map_to_drm(AVHWFramesContext *hwfc, AVFrame *dst,
> >               goto end;
> >           }
> >
> > -        drm_desc->layers[i].planes[0].object_index = FFMIN(i, drm_desc->nb_objects - 1);
> >
> >           if (f->tiling == VK_IMAGE_TILING_OPTIMAL)
> >               continue;
> >
> > -        vk->GetImageSubresourceLayout(hwctx->act_dev, f->img[i], &sub, &layout);
> > -        drm_desc->layers[i].planes[0].offset = layout.offset;
> > -        drm_desc->layers[i].planes[0].pitch  = layout.rowPitch;
> >       }
> >
> >       dst->width   = src->width;
>
> You don't need allocation for this, you can just use a fixed number
> large enough for a few coefficients. From memory, most software uses 16
> modifiers.
>
> The dmabuf export code is still very much in a bad shape. I wrote code
> which made the decoder output dmabuf-backed VkImages, and ran into this
> issue.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".