Aug. 10, 2026
4:41 p.m.
On Thu Aug 6 15:36:23 2026 +0000, Stian Low wrote:
> > Hi, really sorry for the late review, I promise I wanted to get to it sooner.
> No worries. Thanks for review. It's a heftier merge and your review
> helped minimize further.
> Most of all your suggests have been pushed and I will cleanup commit
> messages soon after I wrap up testing latest push for Last Hope game for
> Unreal Engine:
> - https://bugs.winehq.org/show_bug.cgi?id=59067#c7
> MR-10567 has no effect for Last Hope which uses swap effect 0x2
> (FLIP_DISCARD) and `WINED3DFMT_R10G10B10A2_UNORM = 52` for both
> swapchain and rtv formats so the rotate logic should be skipped.
> ```
> 0264:fixme:d3d:wined3d_swapchain_rendertarget_view_gl_rotate swapchain
> backbuffer_count 2, format 52
> 014c:fixme:d3d:adapter_gl_create_rendertarget_view swap rtv format_id 52
> ```
> However rotate is still called and logs for condition that should never
> happen which will also be fixed for next push:
> `0264:fixme:d3d:wined3d_swapchain_rendertarget_view_gl_rotate Skipping
> rotate for view_gl 00007F63F9314E10 with gl view name 0 which should not occur.`
> Unnecessary rotate calls do not break anything and are practically
> skipped despite the logs but better to not call at all. The game bug
> otherwise seems unrelated to this merge request despite resembling it at
> first glance.
Maybe in the future splitting as separate merge requests each incremental improvement from original trivial patch vs combining all incremental improvements into this single merge request is a better approach in order to avoid what seems to have become an over-bloated mess.
> Hi, really sorry for the late review, I promise I wanted to get to it sooner.
Thanks also for patience and late reply. Most requests were addressed and resolved last week and with latest pushes.
Commit messages will be corrected after finalizing additional game tests that are failing with backtrace for `wined3d_swapchain_rendertarget_view_gl_rotate` because `wined3d_swapchain_resize_buffers` destroys previous underlying rtv textures while rotation happens so maybe some extra safety mechanisms are needed.
Skipping texture destroys at the end of `wined3d_swapchain_resize_buffers` fixes crashes but resize is not properly so applied.
Whatever gap that these game tests expose needs to be filled otherwise tests passing is misleading and will cause regressions.
Also the reason for vulkan tests failing has been determined:
`vk_blitter_clear_rendertargets` delays blitting for `ClearRenderTargetView` which may cause some order of operations discrepancy which leads to srgb correction not being applied properly.
Forcing `attachment_count > 0` for `vk_blitter_clear_rendertargets` forces immediate blitting and allows all vulkan tests to pass for srgb `ClearRenderTargetView`.
> 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.
Resolved. Will also remove for commit messages for next push once app tests are complete.
> This needs ok(hr == S_OK). Or does it not actually work on native for some reason?
Resolved.
d3d10core status check copied from `test_swapchain_views()` which seems to be the appropriate way of handling it:
`ok(hr == S_OK || broken(hr == E_INVALIDARG), "Failed to create render target view, hr %#lx.\n", hr);`
d3d11 passes on Windows 11 so `ok(hr == S_OK)` has been added for it as suggested.
> Also, generally we split d3d10core/tests and d3d11/tests into their own commits, and label them accordingly, not using "wined3d/tests".
Resolved. Git logs show some `wined3d/tests` messages but suggest seems much more common.
> 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).
Resolved. I figured a wrapper struct only for swapchain rtvs might be more optimal vs adding extra memory per every rtvs but performance diffs are probably negligible and using `wined3d_rendertarget_view` as list directly may have wider use cases eventually.
> 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.
Is the concern that views may be initialized after swapchain has already been rotated/offset?
If so then I added a test case that creates `backbuffer_1_rtv` right after initial present/rotate which passes testsfor the existing impl vs handling any extra offsets which seems consistent with Windows 11 test results.
```
+ hr = ID3D10Device_CreateRenderTargetView(device, (ID3D10Resource *)backbuffer_0, &rtv_desc, &backbuffer_1_rtv_srgb);
+ ok(hr == S_OK || broken(hr == E_INVALIDARG), "Failed to create render target view, hr %#lx.\n", hr);
+ if (SUCCEEDED(hr))
+ {
+ ID3D10Device_ClearRenderTargetView(device, backbuffer_1_rtv_srgb, grey);
+ color = get_texture_color(backbuffer_0, 320, 240); /* grey */
+ todo_wine ok(compare_color(color, 0x80bcbcbc, 1), "Got unexpected srgb color 0x%08x.\n", color);
+ }
```
> I would also not assign the existing texture_vk variable as that creates a bit of a footgun.
Resolved. Variable renamed and scoped within loop to reduce any risks of confusion.
> /* TODO: cleanup? */What do we need to clean up here? Note that Vulkan views will get deleted when the view object is destroyed.
Resolved. Comments left for review just in case problematic.
> You're not using these contexts for anything?
Nope. Thanks for catching. Copied from vk impl which uses contexts and accidentally overlooked maybe as wined3d sync related.
> 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.
I'll keep testing more apps/games in the meantime but so far it still seems to be working and passing all tests.
> 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?
Anything that helps wined3d and d3d12 better align always seems worthwhile to me but original impl kept consistent with existing rotate functions for now.
If indexing is more favorable then perhaps the existing rotate functions should also be changed for consistency between them all and to resemble how d3d12 already uses explicit indexing.
Unless its critical I've left it for a later merge request to handle wined3d and d3d12 alignment more specifically.
> 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?
Swapchains support arbitrary number of rtv views per non-arbitray number of backbuffers with specific count so the list seems necessary unless I'm missing something.
However the iteration of the backbuffer indicies per rtv view may be handled via indices vs reassigning but whatever difference seems very negligible because only UINTs are reassigned which is consistent with how texture rotation is already handled.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/10567#note_148492