[PATCH 1/2] d3dx9/tests: Add tests for using D3DXLoadSurfaceFromSurface() with default render target.
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- dlls/d3dx9_36/tests/surface.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/dlls/d3dx9_36/tests/surface.c b/dlls/d3dx9_36/tests/surface.c index 37c488b5ad..469c864f30 100644 --- a/dlls/d3dx9_36/tests/surface.c +++ b/dlls/d3dx9_36/tests/surface.c @@ -861,7 +861,28 @@ static void test_D3DXLoadSurface(IDirect3DDevice9 *device) ok(hr == D3D_OK, "D3DXLoadSurfaceFromSurface returned %#x, expected %#x\n", hr, D3D_OK); IDirect3DSurface9_Release(newsurf); - } else skip("Failed to create multisampled render target\n"); + } else skip("Failed to create multisampled render target.\n"); + + hr = IDirect3DDevice9_GetRenderTarget(device, 0, &newsurf); + ok(hr == D3D_OK, "IDirect3DDevice9_GetRenderTarget returned %#x, expected %#x.\n", hr, D3D_OK); + + hr = D3DXLoadSurfaceFromSurface(newsurf, NULL, NULL, surf, NULL, NULL, D3DX_FILTER_NONE, 0); + todo_wine + ok(hr == D3D_OK, "D3DXLoadSurfaceFromSurface returned %#x, expected %#x.\n", hr, D3D_OK); + hr = D3DXLoadSurfaceFromSurface(surf, NULL, NULL, newsurf, NULL, NULL, D3DX_FILTER_NONE, 0); + todo_wine + ok(hr == D3D_OK, "D3DXLoadSurfaceFromSurface returned %#x, expected %#x.\n", hr, D3D_OK); + hr = D3DXLoadSurfaceFromSurface(surf, NULL, NULL, newsurf, NULL, NULL, D3DX_FILTER_NONE, 0xff000000); + todo_wine + ok(hr == D3D_OK, "D3DXLoadSurfaceFromSurface returned %#x, expected %#x.\n", hr, D3D_OK); + hr = D3DXLoadSurfaceFromSurface(surf, NULL, NULL, newsurf, NULL, NULL, D3DX_FILTER_TRIANGLE | D3DX_FILTER_MIRROR, 0); + todo_wine + ok(hr == D3D_OK, "D3DXLoadSurfaceFromSurface returned %#x, expected %#x.\n", hr, D3D_OK); + hr = D3DXLoadSurfaceFromSurface(surf, NULL, NULL, newsurf, NULL, NULL, D3DX_FILTER_LINEAR, 0); + todo_wine + ok(hr == D3D_OK, "D3DXLoadSurfaceFromSurface returned %#x, expected %#x.\n", hr, D3D_OK); + + IDirect3DSurface9_Release(newsurf); check_release((IUnknown*)surf, 0); -- 2.19.1
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- dlls/d3dx9_36/surface.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/d3dx9_36/surface.c b/dlls/d3dx9_36/surface.c index 0a6e20e8f1..a44398cf9e 100644 --- a/dlls/d3dx9_36/surface.c +++ b/dlls/d3dx9_36/surface.c @@ -1916,7 +1916,10 @@ HRESULT WINAPI D3DXLoadSurfaceFromSurface(IDirect3DSurface9 *dst_surface, rect = *src_rect; if (FAILED(IDirect3DSurface9_LockRect(src_surface, &lock, NULL, D3DLOCK_READONLY))) + { + FIXME("Called for unlockable source surface.\n"); return D3DXERR_INVALIDDATA; + } hr = D3DXLoadSurfaceFromMemory(dst_surface, dst_palette, dst_rect, lock.pBits, SrcDesc.Format, lock.Pitch, src_palette, &rect, filter, color_key); -- 2.19.1
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- Do you want to take a shot at implementing the missing functionality? I imagine it's just a matter of creating a temporary D3DPOOL_SYSTEMMEM surface, filling it with the contents of the render target with GetRenderTargetData() and locking that surface instead, if the original source surface is a render target.
On Fri, 16 Nov 2018 at 18:38, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
Do you want to take a shot at implementing the missing functionality? I imagine it's just a matter of creating a temporary D3DPOOL_SYSTEMMEM surface, filling it with the contents of the render target with GetRenderTargetData() and locking that surface instead, if the original source surface is a render target.
Actually, I have some patches touching this area.
On 11/16/18 18:08, Matteo Bruni wrote:
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- Do you want to take a shot at implementing the missing functionality? I imagine it's just a matter of creating a temporary D3DPOOL_SYSTEMMEM surface, filling it with the contents of the render target with GetRenderTargetData() and locking that surface instead, if the original source surface is a render target.
Yes, I could, but I thought it must be a bit more complicated than that. We probably want to use d3d9 device StretchBlt or UpdateTexture when possible (no colorkey and other restrictions pass) for performance reasons. If we can't use that and can't lock source rectangle should we assume that it is necessarily render target and use GetRenderTargetData(), or rather use StretchRect()/UpdateTexture() with staging texture's surface? We also need to care about possibly unlockable destination surface. It concerns some other D3DX surface manipulating functions which I did not test yet for ability to workaround this case. So I would suggest the following steps: 1. Add a code path for StrecthBlt (which would be a priority over locking the texture). I guess this should cover the majority of practical cases. 2. Use a staging texture if texture is not lockable; 3. Fix output to unlockable texture through staging texture (with some tests for the other functions touching this).
On Fri, Nov 16, 2018 at 5:38 PM Paul Gofman <gofmanp(a)gmail.com> wrote:
On 11/16/18 18:08, Matteo Bruni wrote:
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- Do you want to take a shot at implementing the missing functionality? I imagine it's just a matter of creating a temporary D3DPOOL_SYSTEMMEM surface, filling it with the contents of the render target with GetRenderTargetData() and locking that surface instead, if the original source surface is a render target.
Yes, I could, but I thought it must be a bit more complicated than that. We probably want to use d3d9 device StretchBlt or UpdateTexture when possible (no colorkey and other restrictions pass) for performance reasons. If we can't use that and can't lock source rectangle should we assume that it is necessarily render target and use GetRenderTargetData(), or rather use StretchRect()/UpdateTexture() with staging texture's surface?
I was thinking of just calling GetDesc() on the source surface and checking for D3DUSAGE_RENDERTARGET. I'm not sure that using StretchRect() / UpdateSurface() is that useful in practice, e.g. the default filter would disqualify both of them immediately. Also StretchRect() has pretty weird / inconsistent restrictions on its use. That said, if you want to add a fast path for the cases that are supported by those I certainly won't oppose it :)
We also need to care about possibly unlockable destination surface. It concerns some other D3DX surface manipulating functions which I did not test yet for ability to workaround this case. So I would suggest the following steps:
1. Add a code path for StrecthBlt (which would be a priority over locking the texture). I guess this should cover the majority of practical cases.
2. Use a staging texture if texture is not lockable;
3. Fix output to unlockable texture through staging texture (with some tests for the other functions touching this).
Sounds good. I'd probably wait for Henri's patches before venturing too far along the plan.
On Fri, 16 Nov 2018 at 21:37, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
It concerns some other D3DX surface manipulating functions which I did not test yet for ability to workaround this case. So I would suggest the following steps:
1. Add a code path for StrecthBlt (which would be a priority over locking the texture). I guess this should cover the majority of practical cases.
2. Use a staging texture if texture is not lockable;
3. Fix output to unlockable texture through staging texture (with some tests for the other functions touching this).
Sounds good. I'd probably wait for Henri's patches before venturing too far along the plan.
My patches are largely along those lines.
On 11/16/18 21:07, Matteo Bruni wrote:
On Fri, Nov 16, 2018 at 5:38 PM Paul Gofman <gofmanp(a)gmail.com> wrote:
On 11/16/18 18:08, Matteo Bruni wrote:
I was thinking of just calling GetDesc() on the source surface and checking for D3DUSAGE_RENDERTARGET. I'm not sure that using StretchRect() / UpdateSurface() is that useful in practice, e.g. the default filter would disqualify both of them immediately. As I understand filter (unlike color key) is not supposed to do anything if surface is not resized and pixel format is the same, and I believe it is rather frequent use case. Apart from that default filter is not currently supported (ignored with FIXME) in d3dx9 anyway, so it could continue to be not supported using a quicker StretchRect() for a time being without any functionality loss, while this would probably be not a nice solution. Also StretchRect() has pretty weird / inconsistent restrictions on its use. That said, if you want to add a fast path for the cases that are supported by those I certainly won't oppose it :)
Sounds good. I'd probably wait for Henri's patches before venturing too far along the plan.
So if Henri is preparing something in this area I won't be touching it for now.
On Fri, Nov 16, 2018 at 9:18 PM Paul Gofman <gofmanp(a)gmail.com> wrote:
On 11/16/18 21:07, Matteo Bruni wrote:
On Fri, Nov 16, 2018 at 5:38 PM Paul Gofman <gofmanp(a)gmail.com> wrote:
On 11/16/18 18:08, Matteo Bruni wrote:
I was thinking of just calling GetDesc() on the source surface and checking for D3DUSAGE_RENDERTARGET. I'm not sure that using StretchRect() / UpdateSurface() is that useful in practice, e.g. the default filter would disqualify both of them immediately. As I understand filter (unlike color key) is not supposed to do anything if surface is not resized and pixel format is the same, and I believe it is rather frequent use case.
That's probably true.
Apart from that default filter is not currently supported (ignored with FIXME) in d3dx9 anyway, so it could continue to be not supported using a quicker StretchRect() for a time being without any functionality loss, while this would probably be not a nice solution.
Right, no, I'd rather not extend the hack further.
participants (4)
-
Henri Verbeet -
Matteo Bruni -
Matteo Bruni -
Paul Gofman