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.