[PATCH v4 0/2] MR6069: quartz: Properly copy data to render surfaces of planar formats in VMR9.

[Francisco Casas (＠fcasas)]

Fixes Bug 23029 (Devil May Cry® 3 Special Edition) (6550) Intro Video is covered by green, transparent square for a majority of it.

Before these patches, surface data for planar formats is not copied
correctly when the application uses a custom allocator-presenter and
allocates a surface of different size than the VMR9 source.

Patch 2/2 adds support for performing this copy correctly when the
source dimensions are less or equal than the rendering surface
dimensions.

---

I have some questions:
- Should I also make it work for when the dst is smaller than the src?
- If not, should the FIXME() in 1/2 be promoted to an error? Otherwise we might have segfaults, writing out of scope.
- Should I use copy_plane(), introduced in 2/2 also in the implementation of the other formats? Changing what needs to be changed to preserve behavior of course.

--
  v4: quartz: Properly copy data to render surfaces of planar formats in VMR9.

https://gitlab.winehq.org/wine/wine/-/merge_requests/6069

[PATCH v4 1/2] quartz: Emit FIXME when the rendering surface is smaller than the source in VMR9.

[Francisco Casas]

From: Francisco Casas

---
 dlls/quartz/vmr9.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/dlls/quartz/vmr9.c b/dlls/quartz/vmr9.c
index 9d95cda9def..270277f0196 100644
--- a/dlls/quartz/vmr9.c
+++ b/dlls/quartz/vmr9.c
@@ -189,6 +189,7 @@ static HRESULT vmr_render(struct strmbase_renderer *iface, IMediaSample *sample)
     REFERENCE_TIME start_time, end_time;
     VMR9PresentationInfo info = {};
     D3DLOCKED_RECT locked_rect;
+    D3DSURFACE_DESC dst_desc;
     BYTE *data = NULL;
     HRESULT hr;
     int height;
@@ -240,6 +241,16 @@ static HRESULT vmr_render(struct strmbase_renderer *iface, IMediaSample *sample)
     info.szAspectRatio.cy = height;
     info.lpSurf = filter->surfaces[(++filter->cur_surface) % filter->num_surfaces];

+    if (FAILED(hr = IDirect3DSurface9_GetDesc(info.lpSurf, &dst_desc)))
+    {
+        ERR("Failed to get rendering surface description.\n");
+        return hr;
+    }
+
+    if (width > dst_desc.Width || abs(height) > dst_desc.Height)
+        FIXME("src surface (%ux%u) larger than rendering surface (%ux%u).\n", width, height,
+                dst_desc.Width, dst_desc.Height);
+
     if (FAILED(hr = IDirect3DSurface9_LockRect(info.lpSurf, &locked_rect, NULL, D3DLOCK_DISCARD)))
     {
         ERR("Failed to lock surface, hr %#lx.\n", hr);

[PATCH v4 2/2] quartz: Properly copy data to render surfaces of planar formats in VMR9.

[Francisco Casas]

From: Francisco Casas

Before this patch, surface data for planar formats is not copied
correctly when the application uses a custom allocator-presenter and
allocates a surface of different size than the VMR9 source.

This patch adds support for performing this copy correctly when the
source dimensions are less or equal than the rendering surface
dimensions.
---
 dlls/quartz/vmr9.c | 74 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 17 deletions(-)

diff --git a/dlls/quartz/vmr9.c b/dlls/quartz/vmr9.c
index 270277f0196..ef8c78951c8 100644
--- a/dlls/quartz/vmr9.c
+++ b/dlls/quartz/vmr9.c
@@ -181,6 +181,42 @@ static inline struct quartz_vmr *impl_from_IBaseFilter(IBaseFilter *iface)
     return CONTAINING_RECORD(iface, struct quartz_vmr, renderer.filter.IBaseFilter_iface);
 }

+static void copy_plane(BYTE **dstp, unsigned int dst_pitch, unsigned int dst_height,
+                       const BYTE **srcp, unsigned int src_pitch, int src_height)
+{
+    size_t copy_size = min(src_pitch, dst_pitch);
+    const BYTE *src = *srcp;
+    BYTE *dst = *dstp;
+    unsigned int i;
+
+    if (src_height < 0)
+    {
+        TRACE("Inverting image.\n");
+
+        src_height = -src_height;
+        src += src_height * src_pitch;
+
+        for (i = 0; i < src_height; ++i)
+        {
+            src -= src_pitch;
+            memcpy(dst, src, copy_size);
+            dst += dst_pitch;
+        }
+    }
+    else
+    {
+        for (i = 0; i < src_height; ++i)
+        {
+            memcpy(dst, src, copy_size);
+            dst += dst_pitch;
+            src += src_pitch;
+        }
+    }
+
+    *srcp += src_pitch * src_height;
+    *dstp += dst_pitch * dst_height;
+}
+
 static HRESULT vmr_render(struct strmbase_renderer *iface, IMediaSample *sample)
 {
     struct quartz_vmr *filter = impl_from_IBaseFilter(&iface->filter.IBaseFilter_iface);
@@ -257,36 +293,40 @@ static HRESULT vmr_render(struct strmbase_renderer *iface, IMediaSample *sample)
         return hr;
     }

-    if (height > 0 && bitmap_header->biCompression == BI_RGB)
+    if (bitmap_header->biCompression == mmioFOURCC('N','V','1','2'))
     {
-        BYTE *dst = (BYTE *)locked_rect.pBits + (height * locked_rect.Pitch);
+        BYTE *dst = locked_rect.pBits;
         const BYTE *src = data;

-        TRACE("Inverting image.\n");
+        copy_plane(&dst, locked_rect.Pitch, dst_desc.Height, &src, src_pitch, height);
+        copy_plane(&dst, locked_rect.Pitch, dst_desc.Height / 2, &src, src_pitch, height / 2);
+    }
+    else if (bitmap_header->biCompression == mmioFOURCC('Y','V','1','2'))
+    {
+        BYTE *dst = locked_rect.pBits;
+        const BYTE *src = data;

-        while (height--)
-        {
-            dst -= locked_rect.Pitch;
-            memcpy(dst, src, width * depth / 8);
-            src += src_pitch;
-        }
+        copy_plane(&dst, locked_rect.Pitch, dst_desc.Height, &src, src_pitch, height);
+        copy_plane(&dst, locked_rect.Pitch / 2, dst_desc.Height / 2, &src, src_pitch / 2, height / 2);
+        copy_plane(&dst, locked_rect.Pitch / 2, dst_desc.Height / 2, &src, src_pitch / 2, height / 2);
     }
-    else if (locked_rect.Pitch != src_pitch)
+    else if (height > 0 && bitmap_header->biCompression == BI_RGB)
     {
         BYTE *dst = locked_rect.pBits;
         const BYTE *src = data;

-        height = abs(height);
+        copy_plane(&dst, locked_rect.Pitch, dst_desc.Height, &src, src_pitch, -height);
+    }
+    else if (locked_rect.Pitch != src_pitch)
+    {
+        BYTE *dst = locked_rect.pBits;
+        const BYTE *src = data;

TRACE("Source pitch %u does not match dest pitch %u; copying manually.\n",
                 src_pitch, locked_rect.Pitch);

-        while (height--)
-        {
-            memcpy(dst, src, width * depth / 8);
-            src += src_pitch;
-            dst += locked_rect.Pitch;
-        }
+        height = abs(height);
+        copy_plane(&dst, locked_rect.Pitch, dst_desc.Height, &src, src_pitch, height);
     }
     else
     {

[Francisco Casas (＠fcasas)]

On Mon Jul 15 23:00:18 2024 +0000, Elizabeth Figura wrote:
> That's not important, though. The extra data that's copied past the
> pitch is alignment padding and will eventually be discarded anyway.

Okay, I updated the MR to use copy_plane() except for the fast path as suggested.

[PATCH v4 0/2] MR6069: quartz: Properly copy data to render surfaces of planar formats in VMR9. - approved

[Elizabeth Figura (＠zfigura)]

This merge request was approved by Elizabeth Figura.