[PATCH v3 0/2] MR6069: quartz: Properly copy data to render surfaces of planar formats in VMR9.
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. -- v3: quartz: Properly copy data to render surfaces of planar formats in VMR9. https://gitlab.winehq.org/wine/wine/-/merge_requests/6069
From: Francisco Casas <fcasas(a)codeweavers.com> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6069
From: Francisco Casas <fcasas(a)codeweavers.com> 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 | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/dlls/quartz/vmr9.c b/dlls/quartz/vmr9.c index 270277f0196..ddfdd940419 100644 --- a/dlls/quartz/vmr9.c +++ b/dlls/quartz/vmr9.c @@ -181,6 +181,25 @@ 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, unsigned int src_height) +{ + const BYTE *src = *srcp; + BYTE *dst = *dstp; + unsigned int i; + + for (i = 0; i < src_height; ++i) + { + memcpy(dst, src, src_pitch); + dst += dst_pitch; + src += src_pitch; + } + dst += (dst_height - src_height) * dst_height; + + *srcp = src; + *dstp = dst; +} + static HRESULT vmr_render(struct strmbase_renderer *iface, IMediaSample *sample) { struct quartz_vmr *filter = impl_from_IBaseFilter(&iface->filter.IBaseFilter_iface); @@ -257,7 +276,24 @@ 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 = locked_rect.pBits; + const BYTE *src = data; + + 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; + + 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 (height > 0 && bitmap_header->biCompression == BI_RGB) { BYTE *dst = (BYTE *)locked_rect.pBits + (height * locked_rect.Pitch); const BYTE *src = data; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6069
* 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.
I would at least use it for the manual cases, but leaving the memcpy() fast path seems like a good idea.
On second thought, that might not be a good idea, since for the manual cases sometimes the amount of data that needs to be copied (`width * depth / 8`) won't be the same as `src_pitch` (which is aligned to each word) and I am not sure adding an additional parameter to copy_plane() is worth it, same for inverting the image vertically. I limited myself to just fix the format then. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6069#note_76296
On Mon Jul 15 22:33:24 2024 +0000, Francisco Casas wrote:
* 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.
I would at least use it for the manual cases, but leaving the memcpy() fast path seems like a good idea. On second thought, that might not be a good idea, since for the manual cases sometimes the amount of data that needs to be copied (`width * depth / 8`) won't be the same as `src_pitch` (which is aligned to each word) and I am not sure adding an additional parameter to copy_plane() is worth it, same for inverting the image vertically. I limited myself to just fix the format then. That's not important, though. The extra data that's copied past the pitch is alignment padding and will eventually be discarded anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6069#note_76297
participants (3)
-
Elizabeth Figura (＠zfigura) -
Francisco Casas
-
Francisco Casas (＠fcasas)