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

Run natively on Wayland when available #139

Merged
merged 4 commits into from
Aug 19, 2021
Merged

Run natively on Wayland when available #139

merged 4 commits into from
Aug 19, 2021

Conversation

WhyNotHugo
Copy link
Collaborator

Currently when running on Wayland, slack will fall back to XWayland which results in blurry rendering (if it runs at all). There is no way to configure Slack to run on Wayland

This change forces Slack to run naively on Wayland but only if that's the only an option. That is, it'll only run on Wayland if the user configurs the overrides sockets=wayland;!x11.

@flathubbot
Copy link
Contributor

Started test build 54939

@flathubbot
Copy link
Contributor

Build 54939 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/52885/com.slack.Slack.flatpakref

This flag is required for screen sharing to work under Wayland.
@flathubbot
Copy link
Contributor

Started test build 55130

@flathubbot
Copy link
Contributor

Build 55130 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/53073/com.slack.Slack.flatpakref

@WhyNotHugo
Copy link
Collaborator Author

@Eonfge Any comments on this?

Not sure if you get notifications for new PRs. 😅

@Eonfge
Copy link
Contributor

Eonfge commented Aug 3, 2021

I wasn't actually on the watch-list, but now that I'm getting more involved, I'll add myself. That said, I'm not a maintainer. 😪

@barthalion It's a bit of a dirty hack, but this kind of thing mind be worth it for now. I might be able to test it at the office tomorrow, so I'll let you know how it works on X11.

@WhyNotHugo
Copy link
Collaborator Author

Thanks! I've mostly tested the Wayland aspect of it, so proper testing on native X11 is very welcome.

@Eonfge
Copy link
Contributor

Eonfge commented Aug 4, 2021

@barthalion Testing was successful. I was able to easily video conference for hours on end. What a misery.... but X11 was not to blame! Also, if you merge this request, could you also close all other pending pull requests?

@vchernin
Copy link
Contributor

vchernin commented Aug 6, 2021

BTW Barthalion is on vacation currently, I wouldn't expect them to respond for a bit.

Also the WebRTCPipeWireCapturer screen sharing flag was already enabled in the desktop file in 8552181. So there shouldn't be any user-noticeable change for screen sharing on Wayland. PipeWire screen sharing should work in xwayland and in wayland.

However you should be able to revert that commit since this PR should be an improvement (8552181 won't work with flatpak run).

@WhyNotHugo
Copy link
Collaborator Author

Odd, if I remove WebRTCPipeWireCapturer from my changes, slack crashes (tries to connect to Xorg) when trying to screen share.

I'm under the impression that this --enable-features overrides the other one.

@vchernin
Copy link
Contributor

vchernin commented Aug 6, 2021

Odd, if I remove WebRTCPipeWireCapturer from my changes, slack crashes (tries to connect to Xorg) when trying to screen share.

Perhaps passing the arguments in two seperate places is problematic. First passing the flag for WebRTCPipeWireCapturer, and then coming back to enable Wayland means first Slack is first told to connect to Xorg and use PipeWire, but then is told to use Wayland.

Are you able to screen share using current stable without native Wayland flags (and not using flatpak run)?

This could also be a Compositor-specific issue, here on GNOME/Mutter WebRTCPipeWireCapturer on Intel has not caused crashing whether on Xorg, xwayland or Wayland.

I'm under the impression that this --enable-features overrides the other one.

That could be. In Chromium Flatpak there is similar-ish issue with chromium-flags.conf. Only the last --enable-features argument is actually used. I'm not sure this is the exact same issue (nor does Slack have chromium-flags.conf), but there are existing problems with Chromium flags.

See flathub/org.chromium.Chromium#131 (comment), the relevant system is here.

@WhyNotHugo
Copy link
Collaborator Author

It seems the same applies here: only the last --enable-features is used. So removing WebRTCPipeWireCapturer from here actually turns it off (and the fallback is xorg, so it crashes).

I'm not sure if it would not be cleaner to remove the flag from the desktop file and move it into this wrapper script too.

@vchernin
Copy link
Contributor

vchernin commented Aug 8, 2021

I'm not sure if it would not be cleaner to remove the flag from the desktop file and move it into this wrapper script too.

Do you mean to remove --enable-features=WebRTCPipeWireCapturer from the desktop file (reverting 8552181), while keeping the rest of this PR? I'd definitely suggest trying that if you haven't already, it sounds like a good idea to me.

Then again how would this affect users who want to use Slack in xwayland with PipeWire? Ideally PipeWire should work whether running in xwayland or in Wayland.

@barthalion
Copy link
Member

Sorry folks, was on vacation. Anything actionable for me here?

@vchernin
Copy link
Contributor

vchernin commented Aug 16, 2021

Welcome back :)

At least in its current state I personally think this needs more work. I am not exactly sure if this works correctly in xwayland and Wayland yet. I would try to test it but my paid Slack plan trial ran out (you need a paid plan to screen share for some reason).

I would definitely like to be able to use native Wayland mode easily with Slack, but we also need to make sure PipeWire is working for xwayland and Wayland.

It might be worth discussing using Cobalt to do this (I'm not sure if it's the right tool though).

Only the last instance of --enable-features is picked up, so to make
things clear, always limit these to the launcher script so it's obvious
what's going on.
@flathubbot
Copy link
Contributor

Started test build 56549

@flathubbot
Copy link
Contributor

Build 56549 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/54475/com.slack.Slack.flatpakref

@WhyNotHugo
Copy link
Collaborator Author

I've pushed the changes we discussed.

Now all --enable-features calls are in the wrapper script to make this more obvious. When disabling socket=x11 and enabling socket=wayland, Slack will run natively under wayland, and use pipewire for screen sharing.

Without these extra overrides, it defaults back to the previous behaviour.

@flathubbot
Copy link
Contributor

Started test build 56555

@flathubbot
Copy link
Contributor

Build 56555 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/54481/com.slack.Slack.flatpakref

@vchernin
Copy link
Contributor

I've pushed the changes we discussed.

Now all --enable-features calls are in the wrapper script to make this more obvious. When disabling socket=x11 and enabling socket=wayland, Slack will run natively under wayland, and use pipewire for screen sharing.

Without these extra overrides, it defaults back to the previous behaviour.

Definitely LGTM, I think this can be merged once X11, xwayland and Wayland are tested.

For reference to testers, PipeWire screen sharing should work when Slack is under xwayland and Wayland. When under a Wayland session Slack will default to xwayland unless you enable the Wayland socket (and disable X11). When you enable the Wayland socket Slack should mostly work, except for a lack of title bar on GNOME (but that's a separate issue).

@WhyNotHugo
Copy link
Collaborator Author

For the record, I've tested this with Wayland and XWayland. So we're just missing testing on x11 (I don't have a native x11 setup where I can test it myself, sorry).

@vchernin
Copy link
Contributor

For the record, I've tested this with Wayland and XWayland. So we're just missing testing on x11 (I don't have a native x11 setup where I can test it myself, sorry).

On gnome I quickly logged into X11, and basic things worked just fine. I couldn't test screen sharing since I lack the subscription required, but I don't see X11 screen sharing not working. For me, Electron apps under X11 have always worked normally with the PipeWire flag in all it's glory insecurity.

@WhyNotHugo
Copy link
Collaborator Author

Yeah, FWIW, this patch has no functional changes on x11; it still gets called with the same flags.

You can test with /call --test. Note that this can't be run in a conversation with Slackbot.

@vchernin
Copy link
Contributor

You can test with /call --test. Note that this can't be run in a conversation with Slackbot.

Wow, I went out of my way to get a subscription trial from slack just to test screen sharing and then it turns out I could have used that. Thanks for pointing that out! Although I guess running the test isn't as thorough as an actual call.

I tried that tool on xwayland and wayland, it succeeds and the PipeWire pop up appears.

Let me try X11 again (I have to log out again).

@vchernin
Copy link
Contributor

X11 works as expected, no pop up and it says it succeeded.

I think this is ready to merge unless someone wants to test an actual call.

@barthalion barthalion merged commit 9476404 into flathub:master Aug 19, 2021
@WhyNotHugo
Copy link
Collaborator Author

WhyNotHugo commented Aug 19, 2021

Thanks!

@Jmennius
Copy link

Jmennius commented Sep 6, 2021

Works (almost) fine for me on Fedora 34 after flatpak override --socket wayland --socket=fallback-x11 com.slack.Slack.
One issue that I can see on my system is that it doesn't have a header and the window can't be resized with a mouse (fine for me since I mostly use shortcuts to position the window and I especially like that fact that there is no header when maximized). Maybe this is something I've done in the past but I don't remember it

@vchernin
Copy link
Contributor

vchernin commented Sep 6, 2021

@Jmennius
Copy link

Jmennius commented Sep 6, 2021

@Jmennius flathub/im.riot.Riot#200 (comment)

Thanks! I understand now why wayland socket is not a default (yet).

@WhyNotHugo
Copy link
Collaborator Author

One issue that I can see on my system is that it doesn't have a header and the window can't be resized with a mouse

This is a well known issue on GNOME which the developers have confirmed they will never fix. You'll have to wait for Electron to implement client side decorations: electron/electron#27016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants