Signed-off-by: Dmitry Timoshkov dmitry@baikal.ru --- dlls/d2d1/bitmap.c | 65 ++++++++++++++++++++++++++++++++++++++-- dlls/d2d1/d2d1_private.h | 1 + 2 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/dlls/d2d1/bitmap.c b/dlls/d2d1/bitmap.c index 971e3c7ff6b..bab3575c431 100644 --- a/dlls/d2d1/bitmap.c +++ b/dlls/d2d1/bitmap.c @@ -226,12 +226,73 @@ static HRESULT STDMETHODCALLTYPE d2d_bitmap_GetSurface(ID2D1Bitmap1 *iface, IDXG return *surface ? S_OK : D2DERR_INVALID_CALL; }
+static unsigned int d3d11_cpu_access_from_d2d1_map_options(map_options) +{ + unsigned int cpu_access = 0; + + if (map_options & D2D1_MAP_OPTIONS_READ) + cpu_access |= D3D11_CPU_ACCESS_READ; + if (map_options & D2D1_MAP_OPTIONS_WRITE) + cpu_access |= D3D11_CPU_ACCESS_WRITE; + if (map_options &= ~(D2D1_MAP_OPTIONS_READ | D2D1_MAP_OPTIONS_WRITE)) + FIXME("Unhandled map options flags %#x.\n", map_options); + + return cpu_access; +} + +static unsigned int d3d11_map_from_d2d1_map_options(map_options) +{ + unsigned int map = 0; + + if (map_options & D2D1_MAP_OPTIONS_READ) + map |= D3D11_MAP_READ; + if (map_options & D2D1_MAP_OPTIONS_WRITE) + map |= D3D11_MAP_WRITE; + if (map_options &= ~(D2D1_MAP_OPTIONS_READ | D2D1_MAP_OPTIONS_WRITE)) + FIXME("Unhandled map options flags %#x.\n", map_options); + + return map; +} + static HRESULT STDMETHODCALLTYPE d2d_bitmap_Map(ID2D1Bitmap1 *iface, D2D1_MAP_OPTIONS options, D2D1_MAPPED_RECT *mapped_rect) { - FIXME("iface %p, options %#x, mapped_rect %p stub!\n", iface, options, mapped_rect); + struct d2d_bitmap *bitmap = impl_from_ID2D1Bitmap1(iface); + ID3D11Device *device; + ID3D11DeviceContext *context; + D3D11_TEXTURE2D_DESC texture_desc; + ID3D11Texture2D *staging_texture; + D3D11_MAPPED_SUBRESOURCE mapped_subresource; + HRESULT hr;
- return E_NOTIMPL; + TRACE("iface %p, options %#x, mapped_rect %p.\n", iface, options, mapped_rect); + + ID3D11Resource_GetDevice(bitmap->resource, &device); + ID3D11Device_GetImmediateContext(device, &context); + ID3D11Texture2D_GetDesc((ID3D11Texture2D *)bitmap->resource, &texture_desc); + + texture_desc.Usage = D3D11_USAGE_STAGING; + texture_desc.BindFlags = 0; + texture_desc.CPUAccessFlags = d3d11_cpu_access_from_d2d1_map_options(options); + ID3D11Device_CreateTexture2D(device, &texture_desc, NULL, &staging_texture); + ID3D11DeviceContext_CopyResource(context, (ID3D11Resource *)staging_texture, bitmap->resource); + if (SUCCEEDED(hr = ID3D11DeviceContext_Map(context, (ID3D11Resource *)staging_texture, 0, + d3d11_map_from_d2d1_map_options(options), 0, &mapped_subresource))) + { + mapped_rect->pitch = mapped_subresource.RowPitch; + mapped_rect->bits = mapped_subresource.pData; + bitmap->staging_resource = (ID3D11Resource *)staging_texture; + } + else + { + WARN("Failed to map resource, hr %#lx.\n", hr); + ID3D11Texture2D_Release(staging_texture); + } + + ID3D11DeviceContext_Release(context); + ID3D11Device_Release(device); + + return hr; }
static HRESULT STDMETHODCALLTYPE d2d_bitmap_Unmap(ID2D1Bitmap1 *iface) diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index aa8e8569455..da5f9b5a90a 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -382,6 +382,7 @@ struct d2d_bitmap ID3D11RenderTargetView *rtv; IDXGISurface *surface; ID3D11Resource *resource; + ID3D11Resource *staging_resource; D2D1_SIZE_U pixel_size; D2D1_PIXEL_FORMAT format; float dpi_x;
On 5/12/22 16:23, Dmitry Timoshkov wrote:
static HRESULT STDMETHODCALLTYPE d2d_bitmap_Map(ID2D1Bitmap1 *iface, D2D1_MAP_OPTIONS options, D2D1_MAPPED_RECT *mapped_rect) {
- FIXME("iface %p, options %#x, mapped_rect %p stub!\n", iface, options, mapped_rect);
- struct d2d_bitmap *bitmap = impl_from_ID2D1Bitmap1(iface);
- ID3D11Device *device;
- ID3D11DeviceContext *context;
- D3D11_TEXTURE2D_DESC texture_desc;
- ID3D11Texture2D *staging_texture;
- D3D11_MAPPED_SUBRESOURCE mapped_subresource;
- HRESULT hr;
- return E_NOTIMPL;
- TRACE("iface %p, options %#x, mapped_rect %p.\n", iface, options, mapped_rect);
- ID3D11Resource_GetDevice(bitmap->resource, &device);
- ID3D11Device_GetImmediateContext(device, &context);
- ID3D11Texture2D_GetDesc((ID3D11Texture2D *)bitmap->resource, &texture_desc);
- texture_desc.Usage = D3D11_USAGE_STAGING;
- texture_desc.BindFlags = 0;
- texture_desc.CPUAccessFlags = d3d11_cpu_access_from_d2d1_map_options(options);
- ID3D11Device_CreateTexture2D(device, &texture_desc, NULL, &staging_texture);
- ID3D11DeviceContext_CopyResource(context, (ID3D11Resource *)staging_texture, bitmap->resource);
- if (SUCCEEDED(hr = ID3D11DeviceContext_Map(context, (ID3D11Resource *)staging_texture, 0,
d3d11_map_from_d2d1_map_options(options), 0, &mapped_subresource)))- {
mapped_rect->pitch = mapped_subresource.RowPitch;mapped_rect->bits = mapped_subresource.pData;bitmap->staging_resource = (ID3D11Resource *)staging_texture;- }
- else
- {
WARN("Failed to map resource, hr %#lx.\n", hr);ID3D11Texture2D_Release(staging_texture);- }
- ID3D11DeviceContext_Release(context);
- ID3D11Device_Release(device);
- return hr; }
According to docs, Map() only works for D2D1_BITMAP_OPTIONS_CPU_READ, that potentially simplifies things. Also Unmap() in patch 2 will not write anything back.
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/12/22 16:23, Dmitry Timoshkov wrote:
static HRESULT STDMETHODCALLTYPE d2d_bitmap_Map(ID2D1Bitmap1 *iface, D2D1_MAP_OPTIONS options, D2D1_MAPPED_RECT *mapped_rect) {
- FIXME("iface %p, options %#x, mapped_rect %p stub!\n", iface, options, mapped_rect);
- struct d2d_bitmap *bitmap = impl_from_ID2D1Bitmap1(iface);
- ID3D11Device *device;
- ID3D11DeviceContext *context;
- D3D11_TEXTURE2D_DESC texture_desc;
- ID3D11Texture2D *staging_texture;
- D3D11_MAPPED_SUBRESOURCE mapped_subresource;
- HRESULT hr;
- return E_NOTIMPL;
- TRACE("iface %p, options %#x, mapped_rect %p.\n", iface, options, mapped_rect);
- ID3D11Resource_GetDevice(bitmap->resource, &device);
- ID3D11Device_GetImmediateContext(device, &context);
- ID3D11Texture2D_GetDesc((ID3D11Texture2D *)bitmap->resource, &texture_desc);
- texture_desc.Usage = D3D11_USAGE_STAGING;
- texture_desc.BindFlags = 0;
- texture_desc.CPUAccessFlags = d3d11_cpu_access_from_d2d1_map_options(options);
- ID3D11Device_CreateTexture2D(device, &texture_desc, NULL, &staging_texture);
- ID3D11DeviceContext_CopyResource(context, (ID3D11Resource *)staging_texture, bitmap->resource);
- if (SUCCEEDED(hr = ID3D11DeviceContext_Map(context, (ID3D11Resource *)staging_texture, 0,
d3d11_map_from_d2d1_map_options(options), 0, &mapped_subresource)))- {
mapped_rect->pitch = mapped_subresource.RowPitch;mapped_rect->bits = mapped_subresource.pData;bitmap->staging_resource = (ID3D11Resource *)staging_texture;- }
- else
- {
WARN("Failed to map resource, hr %#lx.\n", hr);ID3D11Texture2D_Release(staging_texture);- }
- ID3D11DeviceContext_Release(context);
- ID3D11Device_Release(device);
- return hr; }
According to docs, Map() only works for D2D1_BITMAP_OPTIONS_CPU_READ, that potentially simplifies things.
Thanks for the review. I guess that you suggest to drop the from_d2d1_map_options() helpers and always use READ access, is that correct, or there are other things to get into account for simplification?
Also Unmap() in patch 2 will not write anything back.
Should it? I don't see that mentioned in MSDN, and implying that only READ access is supposed to be supported by ::Map() it means that bitmap memory can't be modified by user.
On 5/12/22 19:49, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/12/22 16:23, Dmitry Timoshkov wrote:
static HRESULT STDMETHODCALLTYPE d2d_bitmap_Map(ID2D1Bitmap1 *iface, D2D1_MAP_OPTIONS options, D2D1_MAPPED_RECT *mapped_rect) {
- FIXME("iface %p, options %#x, mapped_rect %p stub!\n", iface, options, mapped_rect);
- struct d2d_bitmap *bitmap = impl_from_ID2D1Bitmap1(iface);
- ID3D11Device *device;
- ID3D11DeviceContext *context;
- D3D11_TEXTURE2D_DESC texture_desc;
- ID3D11Texture2D *staging_texture;
- D3D11_MAPPED_SUBRESOURCE mapped_subresource;
- HRESULT hr;
- return E_NOTIMPL;
- TRACE("iface %p, options %#x, mapped_rect %p.\n", iface, options, mapped_rect);
- ID3D11Resource_GetDevice(bitmap->resource, &device);
- ID3D11Device_GetImmediateContext(device, &context);
- ID3D11Texture2D_GetDesc((ID3D11Texture2D *)bitmap->resource, &texture_desc);
- texture_desc.Usage = D3D11_USAGE_STAGING;
- texture_desc.BindFlags = 0;
- texture_desc.CPUAccessFlags = d3d11_cpu_access_from_d2d1_map_options(options);
- ID3D11Device_CreateTexture2D(device, &texture_desc, NULL, &staging_texture);
- ID3D11DeviceContext_CopyResource(context, (ID3D11Resource *)staging_texture, bitmap->resource);
- if (SUCCEEDED(hr = ID3D11DeviceContext_Map(context, (ID3D11Resource *)staging_texture, 0,
d3d11_map_from_d2d1_map_options(options), 0, &mapped_subresource)))- {
mapped_rect->pitch = mapped_subresource.RowPitch;mapped_rect->bits = mapped_subresource.pData;bitmap->staging_resource = (ID3D11Resource *)staging_texture;- }
- else
- {
WARN("Failed to map resource, hr %#lx.\n", hr);ID3D11Texture2D_Release(staging_texture);- }
- ID3D11DeviceContext_Release(context);
- ID3D11Device_Release(device);
- return hr; }
According to docs, Map() only works for D2D1_BITMAP_OPTIONS_CPU_READ, that potentially simplifies things.
Thanks for the review. I guess that you suggest to drop the from_d2d1_map_options() helpers and always use READ access, is that correct, or there are other things to get into account for simplification?
If it has CPU_READ I'd think you can map it directly, without a staging resource.
Another implication is that bitmap resource might have to be made inaccessible after Map(), e.g. it's possible you can't use it in drawing commands, right away, or it will fail on EndDraw(), who knows.
Also Unmap() in patch 2 will not write anything back.
Should it? I don't see that mentioned in MSDN, and implying that only READ access is supposed to be supported by ::Map() it means that bitmap memory can't be modified by user.
It also says that it's supported in effects for DISCARD | WRITE, and there is a movement in this area on patch list, so might not be entirely theoretical.
But anyway, it's impossible to guess, this needs some tests for different options, maybe CreateBitmap() fails for anything than READ or NONE. In that case you don't need to handle anything else, and if Map only works for bitmaps with CPU_READ, you probably can map resource without a copy. Another question is what happens with CreateBitmapFromDxgiSurface if you give it unmappable surface, but CPU_READ option.
Nikolay Sivov nsivov@codeweavers.com wrote:
According to docs, Map() only works for D2D1_BITMAP_OPTIONS_CPU_READ, that potentially simplifies things.
Thanks for the review. I guess that you suggest to drop the from_d2d1_map_options() helpers and always use READ access, is that correct, or there are other things to get into account for simplification?
If it has CPU_READ I'd think you can map it directly, without a staging resource.
That's a good point, thanks.
Another implication is that bitmap resource might have to be made inaccessible after Map(), e.g. it's possible you can't use it in drawing commands, right away, or it will fail on EndDraw(), who knows.
Also Unmap() in patch 2 will not write anything back.
Should it? I don't see that mentioned in MSDN, and implying that only READ access is supposed to be supported by ::Map() it means that bitmap memory can't be modified by user.
It also says that it's supported in effects for DISCARD | WRITE, and there is a movement in this area on patch list, so might not be entirely theoretical.
But anyway, it's impossible to guess, this needs some tests for different options, maybe CreateBitmap() fails for anything than READ or NONE. In that case you don't need to handle anything else, and if Map only works for bitmaps with CPU_READ, you probably can map resource without a copy.
The application I have here passes a bitmap without CPU_READ flag, so ::Map() fails, and that's the reason for creating a staging texture. Initially I had the code that calls ::Map() for the bitmap->resource, and if that fails then creates a staging texture. What approach would be better: check that bitmap was created with CPU_READ flag, or always try to map bitmap->resource and fallback to a staging texture if that fails?
Another question is what happens with CreateBitmapFromDxgiSurface if you give it unmappable surface, but CPU_READ option.
Is that accepatable to leave this question aside for the initial implementation?
On 5/12/22 21:10, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
According to docs, Map() only works for D2D1_BITMAP_OPTIONS_CPU_READ, that potentially simplifies things.
Thanks for the review. I guess that you suggest to drop the from_d2d1_map_options() helpers and always use READ access, is that correct, or there are other things to get into account for simplification?
If it has CPU_READ I'd think you can map it directly, without a staging resource.
That's a good point, thanks.
Another implication is that bitmap resource might have to be made inaccessible after Map(), e.g. it's possible you can't use it in drawing commands, right away, or it will fail on EndDraw(), who knows.
Also Unmap() in patch 2 will not write anything back.
Should it? I don't see that mentioned in MSDN, and implying that only READ access is supposed to be supported by ::Map() it means that bitmap memory can't be modified by user.
It also says that it's supported in effects for DISCARD | WRITE, and there is a movement in this area on patch list, so might not be entirely theoretical.
But anyway, it's impossible to guess, this needs some tests for different options, maybe CreateBitmap() fails for anything than READ or NONE. In that case you don't need to handle anything else, and if Map only works for bitmaps with CPU_READ, you probably can map resource without a copy.
The application I have here passes a bitmap without CPU_READ flag, so ::Map() fails, and that's the reason for creating a staging texture. Initially I had the code that calls ::Map() for the bitmap->resource, and if that fails then creates a staging texture. What approach would be better: check that bitmap was created with CPU_READ flag, or always try to map bitmap->resource and fallback to a staging texture if that fails?
Bitmap always references same resource, and resource parameters can't change, so in principle you'll know which path will work when bitmap is created.
It's probably fine to have support only for READ map, and have a fixme, with error return value otherwise.
For CPU_READ flag, we probably should have a fixme too.
Note that you don't release staging texture on unmap, and d3d11_map_from_d2d1_map_options() seems wrong because D3D11_MAP_* is not a mask.
Another question is what happens with CreateBitmapFromDxgiSurface if you give it unmappable surface, but CPU_READ option.
Is that accepatable to leave this question aside for the initial implementation?
I quickly tested this with existing tests that are using swapchain buffer. It fails to map on windows, but with this patch it works. So there is already a discrepancy.
Nikolay Sivov nsivov@codeweavers.com wrote:
It also says that it's supported in effects for DISCARD | WRITE, and there is a movement in this area on patch list, so might not be entirely theoretical.
But anyway, it's impossible to guess, this needs some tests for different options, maybe CreateBitmap() fails for anything than READ or NONE. In that case you don't need to handle anything else, and if Map only works for bitmaps with CPU_READ, you probably can map resource without a copy.
The application I have here passes a bitmap without CPU_READ flag, so ::Map() fails, and that's the reason for creating a staging texture. Initially I had the code that calls ::Map() for the bitmap->resource, and if that fails then creates a staging texture. What approach would be better: check that bitmap was created with CPU_READ flag, or always try to map bitmap->resource and fallback to a staging texture if that fails?
Bitmap always references same resource, and resource parameters can't change, so in principle you'll know which path will work when bitmap is created.
So, to sum it up, ::Map() should have dedicated code paths for bitmaps with and without CPU_READ flag.
It's probably fine to have support only for READ map, and have a fixme, with error return value otherwise.
For CPU_READ flag, we probably should have a fixme too.
Note that you don't release staging texture on unmap, and d3d11_map_from_d2d1_map_options() seems wrong because D3D11_MAP_* is not a mask.
Thanks for noticing these bugs, good catch.
Another question is what happens with CreateBitmapFromDxgiSurface if you give it unmappable surface, but CPU_READ option.
Is that accepatable to leave this question aside for the initial implementation?
I quickly tested this with existing tests that are using swapchain buffer. It fails to map on windows, but with this patch it works. So there is already a discrepancy.
Probably having a fixme or a comment mentioning that scenario could also be acceptable?
On 5/12/22 22:47, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
It also says that it's supported in effects for DISCARD | WRITE, and there is a movement in this area on patch list, so might not be entirely theoretical.
But anyway, it's impossible to guess, this needs some tests for different options, maybe CreateBitmap() fails for anything than READ or NONE. In that case you don't need to handle anything else, and if Map only works for bitmaps with CPU_READ, you probably can map resource without a copy.
The application I have here passes a bitmap without CPU_READ flag, so ::Map() fails, and that's the reason for creating a staging texture. Initially I had the code that calls ::Map() for the bitmap->resource, and if that fails then creates a staging texture. What approach would be better: check that bitmap was created with CPU_READ flag, or always try to map bitmap->resource and fallback to a staging texture if that fails?
Bitmap always references same resource, and resource parameters can't change, so in principle you'll know which path will work when bitmap is created.
So, to sum it up, ::Map() should have dedicated code paths for bitmaps with and without CPU_READ flag.
Not just the flag, bitmap could be created from existing resource.
I suggest for now limit this to READ, with a staging resource, that we can later optimize.
Please also add a test for bitmap configuration that your application uses. Also what happens on a Map() on already mapped bitmap is also interesting, right now you'll leak a texture.
It's probably fine to have support only for READ map, and have a fixme, with error return value otherwise.
For CPU_READ flag, we probably should have a fixme too.
Note that you don't release staging texture on unmap, and d3d11_map_from_d2d1_map_options() seems wrong because D3D11_MAP_* is not a mask.
Thanks for noticing these bugs, good catch.
Another question is what happens with CreateBitmapFromDxgiSurface if you give it unmappable surface, but CPU_READ option.
Is that accepatable to leave this question aside for the initial implementation?
I quickly tested this with existing tests that are using swapchain buffer. It fails to map on windows, but with this patch it works. So there is already a discrepancy.
Probably having a fixme or a comment mentioning that scenario could also be acceptable?
Ideally in a form of a todo test.
-
Dmitry Timoshkov -
Nikolay Sivov