Re: [v2 PATCH] d2d1: Support shared bitmaps created from dxgi surface
On 8 October 2016 at 19:18, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
-HRESULT d2d_bitmap_create_shared(ID2D1Factory *factory, ID3D10Device *target_device, +HRESULT d2d_bitmap_create_shared(ID2D1RenderTarget *render_target, ID3D10Device *target_device, REFIID iid, void *data, const D2D1_BITMAP_PROPERTIES *desc, struct d2d_bitmap **bitmap) { + D2D1_BITMAP_PROPERTIES d; + ID2D1Factory *factory; + + ID2D1RenderTarget_GetFactory(render_target, &factory); + ID2D1Factory_Release(factory); This is a little improper. It does save a few lines, but I'd rather just release it after we're actually done with it.
+ else if (IsEqualGUID(iid, &IID_IDXGISurface) || IsEqualGUID(iid, &IID_IDXGISurface1)) "else" is redundant here, although that's minor.
+ { + ID3D10ShaderResourceView *view; + DXGI_SURFACE_DESC surface_desc; + IDXGISurface *surface = data; + ID3D10Resource *resource; + D2D1_SIZE_U pixel_size; + HRESULT hr; + + if (FAILED(IDXGISurface_QueryInterface(surface, &IID_ID3D10Resource, (void **)&resource))) + { + WARN("Failed to get d3d resource from dxgi surface.\n"); + return E_FAIL; + } + + hr = ID3D10Device_CreateShaderResourceView(target_device, resource, NULL, &view); I think we should validate the dxgi surface's device matches the target device first, like in the IID_ID2D1Bitmap case.
It's in fact possible to share dxgi resources between different devices and supposedly even different processes, but doing that would require using IDXGIResource::GetSharedHandle(), ID3D10Device::OpenSharedResource(), and D3D10_RESOURCE_MISC_SHARED, all of which are currently unimplemented in Wine. I'm not sure if d2d1 allows it.
+ /* Shared DXGI surface. */ + desc.type = D2D1_RENDER_TARGET_TYPE_DEFAULT; + desc.pixelFormat.format = DXGI_FORMAT_B8G8R8A8_UNORM; + desc.pixelFormat.alphaMode = D2D1_ALPHA_MODE_PREMULTIPLIED; + desc.dpiX = 0.0f; + desc.dpiY = 0.0f; + desc.usage = D2D1_RENDER_TARGET_USAGE_NONE; + desc.minLevel = D2D1_FEATURE_LEVEL_DEFAULT; + + hr = ID2D1Factory_CreateDxgiSurfaceRenderTarget(factory1, surface2, &desc, &rt2); + ok(SUCCEEDED(hr), "Failed to create render target, hr %#x.\n", hr); + + bitmap_desc.pixelFormat.format = DXGI_FORMAT_B8G8R8A8_UNORM; + bitmap_desc.pixelFormat.alphaMode = D2D1_ALPHA_MODE_PREMULTIPLIED; + bitmap_desc.dpiX = 0.0f; + bitmap_desc.dpiY = 0.0f; + + hr = ID2D1RenderTarget_CreateSharedBitmap(rt2, &IID_IDXGISurface, surface2, &bitmap_desc, &bitmap2); + ok(SUCCEEDED(hr) || broken(hr == E_INVALIDARG) /* vista */, "Failed to create bitmap, hr %#x.\n", hr); It's interesting that this works. I'd expect actually drawing with that bitmap on that render target to be undefined though.
On 10.10.2016 14:15, Henri Verbeet wrote:
On 8 October 2016 at 19:18, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
-HRESULT d2d_bitmap_create_shared(ID2D1Factory *factory, ID3D10Device *target_device, +HRESULT d2d_bitmap_create_shared(ID2D1RenderTarget *render_target, ID3D10Device *target_device, REFIID iid, void *data, const D2D1_BITMAP_PROPERTIES *desc, struct d2d_bitmap **bitmap) { + D2D1_BITMAP_PROPERTIES d; + ID2D1Factory *factory; + + ID2D1RenderTarget_GetFactory(render_target, &factory); + ID2D1Factory_Release(factory); This is a little improper. It does save a few lines, but I'd rather just release it after we're actually done with it.
Okay.
+ else if (IsEqualGUID(iid, &IID_IDXGISurface) || IsEqualGUID(iid, &IID_IDXGISurface1)) "else" is redundant here, although that's minor.
Sure.
+ { + ID3D10ShaderResourceView *view; + DXGI_SURFACE_DESC surface_desc; + IDXGISurface *surface = data; + ID3D10Resource *resource; + D2D1_SIZE_U pixel_size; + HRESULT hr; + + if (FAILED(IDXGISurface_QueryInterface(surface, &IID_ID3D10Resource, (void **)&resource))) + { + WARN("Failed to get d3d resource from dxgi surface.\n"); + return E_FAIL; + } + + hr = ID3D10Device_CreateShaderResourceView(target_device, resource, NULL, &view); I think we should validate the dxgi surface's device matches the target device first, like in the IID_ID2D1Bitmap case.
Will do.
It's in fact possible to share dxgi resources between different devices and supposedly even different processes, but doing that would require using IDXGIResource::GetSharedHandle(), ID3D10Device::OpenSharedResource(), and D3D10_RESOURCE_MISC_SHARED, all of which are currently unimplemented in Wine. I'm not sure if d2d1 allows it.
This sounds like too much, with nothing that depends on this yet. I guess different device case/same process is easy to test, but if it's a general sharing rule of dxgi/d3d10/11 to restrict resource to device it was created from, tests should go to dxgi/d3d10/11 instead.
+ /* Shared DXGI surface. */ + desc.type = D2D1_RENDER_TARGET_TYPE_DEFAULT; + desc.pixelFormat.format = DXGI_FORMAT_B8G8R8A8_UNORM; + desc.pixelFormat.alphaMode = D2D1_ALPHA_MODE_PREMULTIPLIED; + desc.dpiX = 0.0f; + desc.dpiY = 0.0f; + desc.usage = D2D1_RENDER_TARGET_USAGE_NONE; + desc.minLevel = D2D1_FEATURE_LEVEL_DEFAULT; + + hr = ID2D1Factory_CreateDxgiSurfaceRenderTarget(factory1, surface2, &desc, &rt2); + ok(SUCCEEDED(hr), "Failed to create render target, hr %#x.\n", hr); + + bitmap_desc.pixelFormat.format = DXGI_FORMAT_B8G8R8A8_UNORM; + bitmap_desc.pixelFormat.alphaMode = D2D1_ALPHA_MODE_PREMULTIPLIED; + bitmap_desc.dpiX = 0.0f; + bitmap_desc.dpiY = 0.0f; + + hr = ID2D1RenderTarget_CreateSharedBitmap(rt2, &IID_IDXGISurface, surface2, &bitmap_desc, &bitmap2); + ok(SUCCEEDED(hr) || broken(hr == E_INVALIDARG) /* vista */, "Failed to create bitmap, hr %#x.\n", hr); It's interesting that this works. I'd expect actually drawing with that bitmap on that render target to be undefined though.
Undefined because same surface is used for render target and bitmap? Do you want me to change that?
On 10 October 2016 at 13:33, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
It's in fact possible to share dxgi resources between different devices and supposedly even different processes, but doing that would require using IDXGIResource::GetSharedHandle(), ID3D10Device::OpenSharedResource(), and D3D10_RESOURCE_MISC_SHARED, all of which are currently unimplemented in Wine. I'm not sure if d2d1 allows it.
This sounds like too much, with nothing that depends on this yet. Yeah, implementing resource sharing is going to be a little painful. More so because you can also share e.g. d3d9 surfaces with d3d11.
I guess different device case/same process is easy to test, but if it's a general sharing rule of dxgi/d3d10/11 to restrict resource to device it was created from, tests should go to dxgi/d3d10/11 instead.
Possibly. In general using resources from a different device is certainly not going to work correctly, but the API may not explicitly enforce that everywhere and just crash or ignore calls. It should be easy enough to test for something like CreateShaderResourceView(), but e.g. VSSetShaderResources() would have less opportunities for giving feedback about using a view from a different device, and you likely wouldn't notice anything until you tried to draw with that setup.
Undefined because same surface is used for render target and bitmap? Yeah. Generally speaking binding resources as both inputs and outputs at the same time results in undefined behaviour, although that behaviour may very well end up being "does what you'd expect" on the particular hardware and driver you're testing with at the time.
Do you want me to change that? Not necessarily, I think it's fine for the test to show that this is allowed, although I do wonder a little if the Vista failure might be related.
participants (2)
-
Henri Verbeet -
Nikolay Sivov