On Fri Mar 1 15:53:00 2024 +0000, Alexandros Frantzis wrote:
As I was writing my reply below, a concern that's been forming in my mind with the list based approach is that we are making various assumptions about the states we can and cannot get into and which I feel: 1. make the code more fragile (since these assumptions may be invalidated) 2. they are somewhat complex to think through, so I don't feel super confident about the correctness in the first place. An array based approach (or any approach that has local only release state) seems to me to be much more straightforward.
Also, as you're removing the surfaces from the global list, maybe you
don't even need a separate entry, and instead can use the same to link them into the "release" surface list? I'm not completely sure I follow all the references there though so maybe I'm missing something. I think that would be valid to do *if* we use the release list only for the `update_context_drawables` case (but read below). For `wgl_context_make_current` that's invalid since we are not removing the drawables from the global list. Even for `update_context_drawables` it's not obviously correct, since in the second to last commit there is:
update_context_drawables(new, old, release) { if (ctx->draw == old || ctx->new_draw == old) { if (ctx->new_draw) wayland_gl_drawable_array_add(release, ctx->new_draw); ... } ... }This blurs the situation a bit, since theoretically we could release `new_draw (!= old)` because `draw == old`, but I *think* that's not a case we can actually get into. That is, I think that any valid `new_draw` is guaranteed to have been the most recent one for the HWND and thus the one we just released (thus `== old`)
You can initialize the list entries on creation, and always
list_remove + list_add_tail to make sure it's not added twice. Sure, that would be required to ensure they are added to the release list properly, but the issue is that we do want to release the drawables multiple times since they have been acquired multiple times (e.g., ctx->new_draw and ctx->new_read). That's why I mentioned that with this approach we also need some kind of release count. I am not particularly fond of including fields in the structs for such particular use cases that seem to be more of an "external" concern, but all that aside, it dawned on me that another danger of this approach is that it's potentially thread unsafe. The per-drawable "release" state needs to be accessed outside of the drawable lock (by design):
lock(); list_remove(&drawable->release_entry); /* or possibly reuse drawable->entry */ list_add_tail(&release_list, &drawable->release_entry); ++drawable->release_count; unlock(); /* This needs to occur outside of lock, so other threads could in theory mess up with drawable->release_entry|count. */ LIST_FOR_EACH_ENTRY(drawable, &release_list ...) while (drawable->release_count--) wayland_gl_drawable_release(drawable);What I don't like very much is the use of some non-Wine helper, it
strongly couples the code to wayland libraries and makes it more difficult to refactor later. That's a fair point, although perhaps the solution here is for Wine to grow such a utility :) A simple growable array is generally useful, and from a quick look around the codebase there seem to be several `realloc`-based sequences that would be candidates for it. If you feel that this could be a way forward I am happy to iterate on such a utility (header only like `wine/list.h`), but otherwise I can also get rid of `wl_array` and create some simple ad-hoc solution within `opengl.c`. (I think an even better utility would be a mixed stack/heap based dynamic array (optionally starts on the stack to avoid allocation for the most common cases, moves to heap on the first reallocation), but perhaps that's a step too far.)
Maybe it would be a bit simpler if the contexts didn't keep a reference on their surfaces. It doesn't look very useful as you are actually replacing the pointers in the contexts when the drawables are destroyed.
Though this is actually true for the new_read/new_draw ones, and there's arguably still the read/draw pointers which aren't replaced until the next swap, but perhaps you could move these "destroyed but still in use" drawables in a separate global list if they need to be kept alive for some reason.
Or maybe find a different way, because the four drawable pointers per context, the references and global locking feels quite complicated and difficult to follow.