Hi, really sorry for the late review, I promise I wanted to get to it sooner. This looks structurally much closer to what it should, and is much easier to read, so thank you for that and for your patience. General comment: subject lines should be a grammatical sentence, and we don't need the MR number tag anywhere in the commit. I'd suggest titles like: 1/6: d3d10core/tests, d3d11/tests: Test toggling sRGB for swapchain views. 2/6: wined3d: Track swapchain views in a list. 3/6: wined3d/vk: Create views for all backbuffers. 4/6: wined3d/gl: Create views for all backbuffers. 5/6: wined3d/gl: Disable GL_FRAMEBUFFER_SRGB for drawable FBO blits. 6/6: wined3d: Rotate swapchain texture views. 1/6: ``` + /* Test SRGB rtv for UNORM swapchain common for Unity games for Win10 (bug #45364, MR #10567) */ ``` Generally we don't put bug numbers in the code. Certainly I don't think there's a reason to put merge request numbers in the code. ``` + hr = ID3D10Device_CreateRenderTargetView(device, (ID3D10Resource *)backbuffer_0, &rtv_desc, &backbuffer_0_rtv_srgb); + if (SUCCEEDED(hr)) ``` This needs ok(hr == S_OK). Or does it not actually work on native for some reason? Also, generally we split d3d10core/tests and d3d11/tests into their own commits, and label them accordingly, not using "wined3d/tests". 2/6: We don't need a separate wined3d_swapchain_rendertarget_view struct; we can just put the list entry directly in wined3d_rendertarget_view (and it doesn't need to be backend-specific either). 3/6: ``` + context = context_acquire(resource->device, NULL, 0); ``` Better to just not release the context earlier. ``` + texture_vk = wined3d_texture_vk(swapchain->back_buffers[i]); + if ((view_vk->vk_image_view[i] = wined3d_view_vk_create_vk_image_view(wined3d_context_vk(context), + desc, texture_vk, format_vk, COLOR_FIXUP_IDENTITY, true, vk_usage))) + continue; ``` That's not quite right though; we need to offset the back buffer index by the index of this backbuffer. I would also not assign the existing texture_vk variable as that creates a bit of a footgun. ``` + /* TODO: cleanup? */ ``` What do we need to clean up here? Note that Vulkan views will get deleted when the view object is destroyed. 4/6: ``` + context = context_acquire(resource->device, NULL, 0); ``` You're not using these contexts for anything? 5/6: This one I need more time on, sorry. I've been staring at the spec all afternoon and it seems a bit hairier than we originally thought. 6/6: Rotating views works, buuuut, what if we instead just store an index and increment it, and use that indexed view instead of using view 0? Actually, for that matter, we could store the index in the swapchain, and then we wouldn't actually even have to make lists of views at all? Does this work or am I not thinking this through enough? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10567#note_147765