wayland-hwc: Drop dummy window hack & QtMir integration improvements #6
Conversation
- The 1x1 dummy buffer causes QtMir to only draw that buffer, not the attached surfaces. Remove the hack. - Commit setting the appID of the window earlier to avoid races.
Set Android properties to determine whether a window has been spawned or not. This is useful for the Waydroid launcher to determine whether the launch has been successful or not, and to exit cleanly in this case.
|
The window spawn state tracking is necessary for this feature: waydroid/waydroid#418 |
- Might not be needed after all. Also try to prevent fresh crashes. - Replace the dummy-window hack with an surfaces wrapped in an EGL window.
There is no wayland-egl in external/wayland, provide it here.
Prevents a crash in (Qt)Mir.
fredldotme
changed the title
Compositors which have decorations around the windows might trigger a toplevel xdg window to be closed from "outside", handle those cases.
The window's resources might have been freed during a toplevel XDG window close, but the struct itself still exists. Remove it.
|
The question remains whether the EGL window change is actually necessary. |
|
Wouldn't either setting xdg-surface's window geometry or using viewporter to extend the single pixel buffer into desired size be enough there? I'm not sure what wayland-egl brings to the table. |
|
The wayland-egl stuff was an experiment, sadly failed. Problem with the single pixel solution is that it might work for one or two compositors, but some might expose weird behavior, like QtMir does where the 1px buffer blocks view to the (sub)surfaces. The reasoning behind it is also a little problematic as apps being able to decide where they want to place themselves is precisely something that Wayland tries to counteract. Nothing guarantees us that GNOME Shell/Mutter or some other major compositor kills that happy accident in an update due to trying to stay true to the protocol. |
|
Well, the xdg-surface needs some buffer because otherwise it will stay unmapped. In subsurface mode HWC essentially offloads compositing to the Wayland compositor via subsurfaces, and since nothing on Android side guarantees that the first layer always starts at 0x0 you need to have a dummy buffer regardless of whether you're trying to hackily position your windows or not. [edit] Does this commit - d8ad35e - fix the problem with QtMir? (it's in a branch I'm in the middle of cleaning up right now) |
|
I have now posted the window geometry patch as a pull request (#18) and also posted a change that makes use of the dummy buffer to provide window background (#12). It will still be desirable to position each window's layers independently of global coordinate space, but that will require some more involved changes. Your PR didn't really change anything in that regard - it just replicated pretty much the same hack by creating the buffer in a different way. I'll be interested in whether either #12 or #18 (or maybe both combined) fix your QtMir issues though, and from what I can see this PR still has interesting and useful changes in it that could be split out. |
| @@ -412,10 +415,15 @@ destroy_window(struct window *window, bool keep) | |||
| window->isActive = false; | |||
| else | |||
| free(window); | |||
|
|
|||
| if (!window->appID.empty()) { | |||
| // Window is closed, don't bother | ||
| if (!window->isActive) | ||
| if (window && !window->isActive) { | ||
| free(window); |
There was a problem hiding this comment.
You're not removing the window from the list, which leads to crash. Also, I'd rather use destroy_window instead of freeing directly.
| @@ -1006,6 +1006,9 @@ static int hwc_open(const struct hw_module_t* module, const char* name, | |||
| if (!property_get_bool("persist.waydroid.cursor_on_subsurface", false)) | |||
| pdev->display->cursor_surface = | |||
| wl_compositor_create_surface(pdev->display->compositor); | |||
| else | |||
| pdev->display->cursor_surface = nullptr; | |||
There was a problem hiding this comment.
I'm not really well-versed in C++, but since the display is allocated with struct display *display = new struct display();, this should mean that all its members get value-initialized, which in case of pointers initializes them to nullptr already. Is that not the case here?
attached surfaces. Remove the hack.