On Wed Aug 12 04:16:09 2026 +0000, Elizabeth Figura wrote:
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. I'll have to take a closer look how to properly fix it.
Forcing `attachment_count > 0` for `vk_blitter_clear_rendertargets` forces immediate blitting and allows all vulkan tests to pass for srgb `ClearRenderTargetView`. Right, we need to be rotating locations as well, which is a bit annoying. Forcing backbuffers into TEXTURE_RGB on a present and invalidating !TEXTURE_RGB is probably best. Though forcing an immediate clear for a swapchain texture seems good enough for a stopgap. 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);` Ah, it works on 7 and newer then presumably. Copying the comment would be helpful there. I think a broken() should always be annotated with some sort of explanation, personally. Resolved. Git logs show some `wined3d/tests` messages but suggest seems much more common. Hrm, where? I checked `git log --grep` and didn't see any. 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. That's a fair concern. I don't think there'll be enough RTVs for it to really matter, though, yeah.
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. Not per se—as you observe, that already works because the textures are rotated. My comment was about creating a view on anything but the first backbuffer. E.g. the view list for backbuffer 0 goes [0, 1, 2, 3]; the view list for backbuffer 1 goes [1, 2, 3, 0], where each view is the index of texture we want to create a view of.
/* 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. Since there's nothing to do please go ahead and delete the comment.
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. Thanks for the patience. Also, thanks a lot to Henri for helping me understand this. I believe that this patch is correct as-is, no further review needed.
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. Sure, we can leave it like this for now.
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. Well, with this scheme, any usage of the views would go through the swapchain's index. We don't need to touch the views because we're instead touching the thing that the views use. Original patch has been part of staging for 3 months now which makes up the majority of this merge's duration, during which staging users have only reported games fixed and none bugged.
Maybe a better approach to this merge would have been to marked the few tests that failed as todo to be resolved later vs combining the entire effort into 1 merge request which again, has become so bloated that gitlab struggles to load the comments anymore.
I realize staging probably represents a more narrow user test pool overall but proably a higher proportionality of gamers for real world apps/games most relevant to the original patch so maybe it should have been merged despite a few extra test cases failing which seem much more edge case vs real world cases. I understand the thought, and I recognize the value of pushing a hack now and fixing the bug properly later. It's something we avoid, though, not just in d3d but in broader Wine, and it's for the reason that these hacks make things a lot harder for maintainers (and d3d is a hard beast to maintain even under the best of conditions). It's also common for contributors to get their hack committed and then disappear, never actualizing the "fix it later" part; it takes at least an amount of trust to do that. I apologize that it's been open so long; ultimaately there hasn't actually been that many rounds of review, and the delay is mostly my fault. I am sorry for that; I have been dealing with some health issues which certainly make it harder, although I don't like to use that as an excuse. FWIW, it's possible that gitlab simply struggles due to the large number of pushes. I've seen merge requests with more comments load fine. I don't know if you're pushing for the sake of CI, but you can run CI by pushing to a local fork, which also avoids generating lots of notifications for reviewers. Thanks for review. I'll address whatever remains that needs resolving this week for next review.
(and d3d is a hard beast to maintain even under the best of conditions).
Tis a hard beast to tame.
It's also common for contributors to get their hack committed and then disappear, never actualizing the "fix it later" part; it takes at least an amount of trust to do that.
Understood. Maybe someday I'll earn trust for some leniency privileges.
I apologize that it's been open so long; ultimaately there hasn't actually been that many rounds of review,
Agreed review of latest complex/complete solution has only actually been under review for about a month because the orig simpler patch was left unchanged for most of the duration while its brewed in staging.
and the delay is mostly my fault. I am sorry for that; I have been dealing with some health issues which certainly make it harder, although I don't like to use that as an excuse.
Sounds perfectly reasonable/legit to me. Please be/stay well.
FWIW, it's possible that gitlab simply struggles due to the large number of pushes. I've seen merge requests with more comments load fine. I don't know if you're pushing for the sake of CI, but you can run CI by pushing to a local fork, which also avoids generating lots of notifications for reviewers.
Thanks for insights. I indeed have partly been pushing excessively to set CI expectations for me and to compare diffs between my local tests vs server side. I'll ease up more now that expectations seem steadier and diffs between local and server side seem mostly resolved. Apologies for spam it may have caused. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10567#note_148692