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

Build with PipeWire support (stable branch) #24

Merged
merged 14 commits into from
Jun 28, 2021
Merged

Build with PipeWire support (stable branch) #24

merged 14 commits into from
Jun 28, 2021

Conversation

vchernin
Copy link
Collaborator

@vchernin vchernin commented Jun 7, 2021

Fixes screen sharing on Wayland.
See flathub/com.slack.Slack#118.

Steps:

If your Mattermost deployment includes an app that screen shares, please install the beta branch and attempt to screen share on Wayland.

Note, PipeWire screencasting functionality was never explictly tested, as we didn't have access to an app that screen shares from within Mattermost desktop.

If you want to test this change please test the beta branch.

flatpak remote-add flathub-beta https://flathub.org/beta-repo/flathub-beta.flatpakrepo
flatpak install flathub-beta com.mattermost.Desktop

@flathubbot
Copy link
Contributor

Started test build 50226

@flathubbot
Copy link
Contributor

Build 50226 failed

I could only obtain a canonical copy of the first image from Archive.org. The second and third images were taken from Flathub's cache which has a lower resolution than the originals.
I could not find any other suitable images from Mattermost itself.
@flathubbot
Copy link
Contributor

Started test build 50227

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/48299/com.mattermost.Desktop.flatpakref

@SemaiCZE
Copy link
Collaborator

SemaiCZE commented Jun 7, 2021

Hi @vchernin,
thanks for the PR. Did you test it? Don't you need to update the desktop exec command https://github.com/flathub/com.mattermost.Desktop/blob/master/com.mattermost.Desktop.json#L39 with --enable-features=WebRTCPipeWireCapturer flag like the Slack?

@vchernin
Copy link
Collaborator Author

vchernin commented Jun 7, 2021

Hi @vchernin,

thanks for the PR. Did you test it? Don't you need to update the desktop exec command https://github.com/flathub/com.mattermost.Desktop/blob/master/com.mattermost.Desktop.json#L39 with --enable-features=WebRTCPipeWireCapturer flag like the Slack?

Yeah, you're right, I'm going to change the desktop file too.

The reason I haven't yet is because I haven't actually been able to test screen sharing yet. I looked through the list of Mattermost calling plugins, and I don't really have easy access to most of them. The only one I could actually test, Jitsi, can't screen share, it has a bug preventing it from sharing on any platform. Side note I've actually encountered the same issue with Element so it seems like a common embedded Jitsi bug.

If you are anyone else is able to test this PR (once I've pushed that last commit) that would be most helpful. Even if no one can I think it's probably ok to merge as we aren't changing anything massive, and I couldn't find any regressions in Slack which has basically the exact same problem.

Edit: I will also add on the current release version is on Electron 7, which almost certainly isn't enough for PipeWire. I think we could just test the beta branch though, since that should be on Electron 12.

@SemaiCZE
Copy link
Collaborator

SemaiCZE commented Jun 7, 2021

The reason I haven't yet is because I haven't actually been able to test screen sharing yet. I looked through the list of Mattermost calling plugins, and I don't really have easy access to most of them. The only one I could actually test, Jitsi, can't screen share, it has a bug preventing it from sharing on any platform. Side note I've actually encountered the same issue with Element so it seems like a common embedded Jitsi bug.

Yeah, Mattermost instance at my work (which is the reason I got into flatpacking the app) has only Zoom integration and it basically just generates the links for the Zoom app, no other connection to the Mattermost.

Edit: I will also add on the current release version is on Electron 7, which almost certainly isn't enough for PipeWire. I think we could just test the beta branch though, since that should be on Electron 12.

Sure, we can upgrade the Electron. We can also do some releases on the beta branch if you want, that's not a problem.

@flathubbot
Copy link
Contributor

Started test build 50267

@flathubbot
Copy link
Contributor

Build 50267 failed

@flathubbot
Copy link
Contributor

Started test build 50270

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/48339/com.mattermost.Desktop.flatpakref

@vchernin vchernin changed the title Build with PipeWire support Build with PipeWire support (stable branch) Jun 7, 2021
@vchernin
Copy link
Collaborator Author

vchernin commented Jun 7, 2021

See this PR's description for an overview of what I'm doing now.

I'm currently working on getting element-hq/element-web#25 working. It appears the upstream location of the icon.svg file was changed, and I've been trying to figure out how exactly. When that's fixed hopefully element-hq/element-web#25 should work.

When that's done @SemaiCZE could you merge element-hq/element-web#25 and element-hq/element-web#26?
Once those are done I think we can wait until mattermost updates their stable builds, and then we should merge this PR (#24).

There isn't really a point to this PR (#24) until Mattermost updates their stable Electron version.

@flathubbot
Copy link
Contributor

Started test build 50628

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/48684/com.mattermost.Desktop.flatpakref

@SemaiCZE
Copy link
Collaborator

@vchernin I've just made the icon to work and merged your PR's into the beta branch. Please test that too:

flatpak --user install https://dl.flathub.org/build-repo/48683/com.mattermost.Desktop.flatpakref

For me the basic things seems to work ok.
Thanks!

@vchernin
Copy link
Collaborator Author

@SemaiCZE That repo seems to work for me as well, thanks for fixing the icon!

I also made element-hq/element-web#27 to update the beta branch to the latest RC.

@SemaiCZE
Copy link
Collaborator

Well, I don't know if it's the best fix possible, but I really don't want to dig out the icon from some binary resource file. I guess I can handle to keep the icon inside the repo 🙂

@flathubbot
Copy link
Contributor

Started test build 50766

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/48820/com.mattermost.Desktop.flatpakref

@vchernin vchernin mentioned this pull request Jun 11, 2021
@flathubbot
Copy link
Contributor

Started test build 50772

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/48826/com.mattermost.Desktop.flatpakref

@vchernin vchernin mentioned this pull request Jun 11, 2021
@flathubbot
Copy link
Contributor

Started test build 51726

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/49744/com.mattermost.Desktop.flatpakref

@vchernin
Copy link
Collaborator Author

Seems to be fine for me (I can't actually test logged in behaviour anymore as my free trial ran out, but I'd be surprised if anything broke since last time).

@SemaiCZE This PR should be ready to merge now. I still can't say if PipeWire screen sharing support is actually useful, since neither of us have access to apps that try to screen share from within Mattermost itself. There really isn't any downside though as far as I'm aware.

Note when Mattermost is released with Electron 13, we should update PipeWire to 0.3. Starting with Electron 13/Chromium 91 PipeWire 0.3 is build by default, so like Chromium, Mattermost should probably use it too.

I also might make another PR to enable Wayland support by default and to see if Mattermost can be updated via x-checker-data.

@vchernin vchernin marked this pull request as ready for review June 23, 2021 23:51
@SemaiCZE
Copy link
Collaborator

@vchernin sorry, but I'll get into this after the weekend. Now I'm really busy organizing wild water races.

@vchernin
Copy link
Collaborator Author

@SemaiCZE No worries at all, wild water races sounds fun!

@SemaiCZE SemaiCZE merged commit a0668e1 into flathub:master Jun 28, 2021
@SemaiCZE
Copy link
Collaborator

@vchernin It's published. Thank you for your work.

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.

4 participants