Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for XDG Shell on Wayland #1190

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

marius-pelegrin-arm
Copy link
Contributor

Hi !

Inspired by the PR #698, here is a working support for XDG Shell on Wayland. It is not a replacement of the classic Wayland Shell by XDG Shell but an addition : both are now supported, but XDG Shell is the default choice if both are available, as it is recommended in the Wayland official documentation.

This PR works and can be tested, it is however not finished yet as choices need to be made about the link model. Currently, in GFXReconstruct, the libwayland is only loaded at runtime if necessary. However, Wayland XDG Shell structs, functions and constants are not available in libwayland. They are instead expected to be directly included in the project using sources generated by wayland-scanner.
The issue is these structs, functions and constants depends on the one available in libwayland. So what happens is that if libwayland is not available at link time, the compilation fails.

I see several solutions to this, and there are probably several that I do not see, but here is, in my opinion, the best solution. We could, as I did temporarily in the PR, link against libwayland at link time instead of loading it at runtime. If we compile the XDG Shell sources, it means that cmake found Wayland anyway, so there's no more assumption on the platform or the user environment if we link to libwayland when it is found. The only "loss" is a performance cost - the libwayland will be loaded at startup even if Wayland is finally not used.

If this solution is chosen, the WaylandLoader class becomes useless (as everything is already linked, thus loaded when the program starts) and should be removed. I can do that work if this solution is chosen.

Of course, let me know if I haven't seen another issue that linking against libwayland could cause or if you have a better solution to this issue.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 3680.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 2951 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 2951 passed.

@bradgrantham-lunarg bradgrantham-lunarg added the P1 Prevents an important capture from being replayed label Oct 30, 2023
@marius-pelegrin-arm marius-pelegrin-arm marked this pull request as draft January 18, 2024 12:49
Add support for XDG Shell on Wayland in addition to the classic Wayland
shell and make XDG Shell the default choice when both are available.

Change-Id: Ib5521c699275e23060c4e7ae05696ed8fc3f7221
Add Python script `generate_wayland.py` that generates source
code from a Wayland XML protocol. This script has the same
role as the `wayland-scanner` tool, except that the sources
generated use the GFXReconstruct `WaylandLoader` instead of
directly using `libwayland-client.so` symbols.

The current state now is:
- If the machine that compiles the code does not support
Wayland (no `libwayland-client.so`) then the Wayland code
is simply not compiled and the executable will not support
Wayland (as done until now)
- If the machine that compiles the code supports Wayland
and the executable is ran on a machine that supports
Wayland, then the `libwayland-client.so` of the client
will be loaded using `dlopen` and the generated protocol
sources will initialize protocol constants (interfaces).
- If the machine that compiles the code supports Wayland
but the executable is ran on a machine that does not
support Wayland, then the WaylandLoader will remain
uninitialized and thus not call the generated protocol
sources and not attempt to initialize the protocol
constants.

This way, the binaries will be able no matter the state of
the support of Wayland on the machine that runs them.

Change-Id: I9b3878a71b74002859f4edda6d9a6fe180278a8f
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 137703.

@marius-pelegrin-arm marius-pelegrin-arm marked this pull request as ready for review February 22, 2024 08:07
@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3754 running.

@marius-pelegrin-arm
Copy link
Contributor Author

Hello !

So the solution I propose in the end is given with these two commits.

The first commit is the one that I proposed here several months ago (with some changes for merge conflicts). Wayland is linked statically, but an issue found with this solution is that the generated binary will not run on a machine that does not have libwayland.so if it was compiled on a machine that has libwayland.so.

The second commit solves this issue and goes in the opposite direction. The main issue here is that wayland-scanner generates sources assuming that libwayland is linked statically: this is the main error.
So I added wayland-protocols as a submodule of GFXReconstruct, and created a python script generate_wayland.py that parses xdg-shell.xml to create sources (as would the wayland-scanner do) but that works with libwayland.so being loaded dynamically.
The script is generic enough to work with other wayland protocols if we ever need it, and the resulting "dynamic table" is stored and filled at runtime in the WaylandLoader class.

I think this solution is the best that can be made, because doing so has no cost on performances - everything is loaded once, at runtime, only if wayland is used -, do not break any portability - nothing is linked statically so the binary just checks that everything is available when asked at runtime and fails gracefully in all cases and is as future proof as the Vulkan generators already integrated in GFXReconstruct.

If you have any question or remark, do not hesitate !

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3754 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 138247.

@lunarpapillo
Copy link
Contributor

The LunarG CI failure # 3754 seemed to be due to an old version of git on the system that didn't correctly resolve SSL to get the submodule. I've fixed the issue and restarted.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3759 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3759 passed.

Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything in the code parts of the PR look good. The only change I request (but am happy to do for you if you'd rather) is moving the wayland-protocols submodule into a 'download on demand' dependency, ie the generate_wayland.py script would download the protocols repo when it is being called.

Currently, the strategy for dependencies is to vendor anything that isn't changing often and submodule the things that do change often. Vulkan-Headers updates every week or so, with content that gfxrecon uses. Whereas most of the dependencies are either not updated nearly as frequently, or are in a stable state which doesn't make sense to update all the time.

Wayland-protocols from my understanding is very much like Vulkan-Headers, (more like the gitlab vulkan repo actually) and updates frequently. However, gfxrecon-replay isn't going to be adding new protocols every other week, so a submodule isn't a great fit.

Thus, I would think that download on demand would allow anyone who wants to add a protocol to do so easily while not increasing the checked in repo size or excess traffic to wayland-protocols.

@bradgrantham-lunarg
Copy link
Contributor

@charles-lunarg when this lands, could you also check out #1245 to see if this PR resolves that issue?

Changes requested by LunarG: Remove wayland-protocols submodule
and ownload it from generate_wayland.py instead.

Change-Id: I29932c82d0c4b847f94e47dd191e6fcb4406157a
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 146153.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3825 running.

@marius-pelegrin-arm
Copy link
Contributor Author

There are multiple ways to do this so I just used the subprocess python module to launch git commands as already done in some other scripts in the repository. If the repository exists, it just launch git pull inside of it. I still use the same path (external/wayland-protocols) to store the repository, and added it to the gitignore.
If you had something else in mind / something to add/modify, you can do it if you want =)

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3825 passed.

Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look great now.

I found an example capture that resized and can confirm that resizing works as expected.

@charles-lunarg charles-lunarg merged commit 75dc7dd into LunarG:dev Mar 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Prevents an important capture from being replayed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants