fix: use StartUpdating method for PipeWire capturer #38833

aiddya · 2023-06-18T21:30:14Z

Description of Change

Fixed a crash related to PipeWire capturer by adapting to Chromium's interface changes. Chromium expects a call to
NativeDesktopMediaList::StartUpdating with an implementation of DesktopMediaListObserver for delegated capturers like PipeWire. This interface allows listening to user permission events and listing sources only after the user has made a choice on the permission dialog.

The interface has been implemented by an inner class to allow listening to screen and window capture permissions concurrently using two instances of the class. A patch that was resetting the capturer on the first refresh has been changed to exclude PipeWire. PipeWire capturer object will follow the lifecycle of NativeDesktopMediaList, as is the case in Chromium.

Fixes #37463

Checklist

PR description included and stakeholders cc'd
npm test passes
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed a crash when listing desktop capture sources on Wayland with PipeWire.

welcome · 2023-06-18T21:30:17Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

sunknudsen · 2023-06-20T10:38:02Z

Not the right person to add value to this PR, but can’t wait for issue to be fixed. Thanks for working on this @aiddya!

byquanton · 2023-06-20T21:16:47Z

With this PR applied, it crashes somewhere else.

Thread 46 "DesktopMediaLis" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff518656c0 (LWP 1905627)]
NativeDesktopMediaList::Worker::Refresh (this=0x2e3c0195d380, view_dialog_id=@0x2e3c0299af18: -1, 
    update_thumnails=true) at ../../chrome/browser/media/webrtc/native_desktop_media_list.cc:271

Before:

Thread 43 "DesktopMediaLis" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff530686c0 (LWP 1911851)]
webrtc::BaseCapturerPipeWire::CaptureFrame (this=<optimized out>) at ../../third_party/webrtc/modules/desktop_capture/linux/wayland/base_capturer_pipewire.cc:163

aiddya · 2023-06-21T01:15:17Z

@byquanton Can you share the following information?

How did you test this patch? Can you share the electron code as a fiddle?
Did you apply this patch on main or a branch?
What OS and OS version did you test this with?
What desktop environment are you using? If it's not GNOME or KDE, what portal implementation are you using?
Did you run electron with any flags?
Did you test this on Wayland?

byquanton · 2023-06-21T07:24:13Z

@byquanton Can you share the following information?

1. How did you test this patch? Can you share the electron code as a fiddle?

2. Did you apply this patch on main or a branch?

3. What OS and OS version did you test this with?

4. What desktop environment are you using? If it's not GNOME or KDE, what portal implementation are you using?

5. Did you run electron with any flags?

6. Did you test this on Wayland?

I ran Element with it.
Applied it on main
Fedora 38
GNOME (xdg-desktop-portal-gnome)
Without any Flags
Wayland

aiddya · 2023-06-21T16:05:35Z

@byquanton I have no idea what Element is doing and its code base is too large to debug.

However, the crash message that you're seeing is expected if you don't apply the updated desktop_media_list.patch. Did you run gclient sync -f to reapply the Chomium patches after you applied my patch? Can you verify that chrome/browser/media/webrtc/native_desktop_media_list.cc reflects the changes made in this PR?

aiddya · 2023-06-23T05:30:26Z

I pushed a commit to fix thumbnail generation. I've validated the fixes using a more extensive test case that lists sources, displays thumbnails and initiates a stream using the source ID: https://gist.github.com/aiddya/d368bc2cb7f72a9b030b918ee1332299

byquanton · 2023-06-23T20:14:39Z

@byquanton I have no idea what Element is doing and its code base is too large to debug.

However, the crash message that you're seeing is expected if you don't apply the updated desktop_media_list.patch. Did you run gclient sync -f to reapply the Chomium patches after you applied my patch? Can you verify that chrome/browser/media/webrtc/native_desktop_media_list.cc reflects the changes made in this PR?

Rebuild it again, and now it works.

sunknudsen · 2023-06-30T10:51:25Z

Hey @ckerr and @VerteDinde, has one of you had a chance to look into this PR?

Screen sharing is currently broken when using Linux/Wayland on Element, Jitsi and Signal which is affecting thousands is not millions of users.

Sending love to the universe… looking forward to a fix. 🙏

theofficialgman · 2023-06-30T14:33:28Z

This also affects https://github.com/SpacingBat3/WebCord with 35K installs from pi-apps alone with thousands more from other avenues. I agree this is likely affecting millions of users/application installs.

ToasterUwU · 2023-06-30T15:40:20Z

This also affects Armcord and Vencord Desktop. Which are both Linux focused Discord Clients.

VerteDinde · 2023-07-03T04:33:26Z

@aiddya Thanks for the PR! Sorry, I was out of the office when you submitted the PR originally, I'll take a look at it this week 🙇‍♀️

codebytere

Thanks for working on this!

patches/chromium/desktop_media_list.patch

codebytere · 2023-07-03T08:01:53Z

shell/browser/api/electron_api_desktop_capturer.cc

+    have_selection_ = false;
+
+    // PipeWire returns a single source, so index is not relevant.
+    std::move(this->update_callback_).Run();


This new logic applies cross-platform, though - is there a situation where we'd need this index?

Not unless Chromium makes significant changes/refactors. PipeWire is the only delegated capturer right now. As a result, this code path will not be executed on other platforms.

Secondly, according to Chromium documentation, delegated capturers manage their own UI for source selection and will return a single source. This logic should hold even if there's a new delegated capturer in the future.

shell/browser/api/electron_api_desktop_capturer.cc

Fixed a crash related to PipeWire capturer by adapting to Chromium's interface changes. Chromium expects a call to `NativeDesktopMediaList::StartUpdating` with an implementation of `DesktopMediaListObserver` for delegated capturers like PipeWire. This interface allows listening to user permission events and listing sources only after the user has made a choice on the permission dialog. The interface has been implemented by an inner class to allow listening to screen and window capture permissions concurrently using two instances of the class. A patch that was resetting the capturer on the first refresh has been changed to exclude PipeWire. PipeWire capturer object will follow the lifecycle of `NativeDesktopMediaList`, as is the case in Chromium. Fixes electron#37463

The PipeWire stream starts after the dialog is dismissed. If the sources are listed immediately afterwards, the thumbnail may not have been generated by that time. Explicitly wait for both thumbnail generation and a selection on the source dialog before listing sources.

sunknudsen · 2023-07-15T14:32:55Z

Hey!

Are others experiencing following issues on Ubuntu 22.04.1 LTS (or equivalent) and Electron 25.3.0 (which implements this PR)?

First, calling desktopCapturer.getSources triggers screen share prompt asking user to allow specific window (wasn’t the case before… screen share prompt was only triggered by navigator.mediaDevices.getUserMedia… that said, window names and thumbnails were not displayed which likely explains why prompt was not required).

const sources = await desktopCapturer.getSources({
  types: ["window"],
})

Then, calling navigator.mediaDevices.getUserMedia using following constraints triggers screen share prompt (again).

const constraints = {
    audio: false,
    video: {
      mandatory: {
        chromeMediaSource: "desktop",
        chromeMediaSourceId: sourceId, // Source ID from `sources`
        maxFrameRate: 24,
      },
    },
  }
}

await navigator.mediaDevices.getUserMedia(constraints)

This is pretty bad for user experience… and wasn’t the case on previous “working” versions of Electron (running on Ubuntu 22.04.1 LTS).

Second, when window is selected, moving app window causes app to crash with following error.

Dropping DMA-BUF modifier: 72057594037927935 and trying to renegotiate stream parameters
Segmentation fault (core dumped)

Third (niche use case), implementation is completely broken on Tails 5.15.1.

Puzzled…

ToasterUwU · 2023-07-15T14:36:50Z

@sunknudsen i think you are talking about the same thing that was mentioned in this very same PR earlier. There already is the Issue #39043 for resolving this bad experience stuff.

sunknudsen · 2023-07-15T14:39:41Z

@ToasterUwU Did you experience segmentation fault during testing?

ToasterUwU · 2023-07-15T14:44:08Z

@sunknudsen I have only tried it once for testing if it fixed the screenshare issue on a Discord Client, have not checked if it actually spits any errors while running. You can see the entire 40 second video i recorded of me trying it in Vencord/Vesktop#40, just so you know what exactly i did. It worked for me, even tho it was tedious to use. So if there was an error, it wasnt a fatal one (like Segmentation Fault typically is).

Im on PikaOS 23.04, which is Ubuntu based.

aiddya · 2023-07-15T15:57:39Z

@sunknudsen You should not see the second popup if the portal implementation you're using supports screencast restoration. That's supported by xdg-desktop-portal 1.12 and xdg-desktop-portal-gnome 42.

I tested moving and resizing the shared window with Electron 25.3.0, but I haven't seen any crash on Fedora 38. Do you see any of these issues on Chrome/Chromium? You can test using Mozilla's gUM example page.

trop · 2023-07-16T04:36:24Z

@aiddya has manually backported this PR to "23-x-y", please check out #39116

trop · 2023-07-16T04:37:55Z

@aiddya has manually backported this PR to "22-x-y", please check out #39117

sunknudsen · 2023-07-17T22:44:14Z

Hey @aiddya and @VerteDinde, thanks for helping out… please see #39131. Ideas?

* fix: use StartUpdating method for PipeWire capturer Fixed a crash related to PipeWire capturer by adapting to Chromium's interface changes. Chromium expects a call to `NativeDesktopMediaList::StartUpdating` with an implementation of `DesktopMediaListObserver` for delegated capturers like PipeWire. This interface allows listening to user permission events and listing sources only after the user has made a choice on the permission dialog. The interface has been implemented by an inner class to allow listening to screen and window capture permissions concurrently using two instances of the class. A patch that was resetting the capturer on the first refresh has been changed to exclude PipeWire. PipeWire capturer object will follow the lifecycle of `NativeDesktopMediaList`, as is the case in Chromium. Fixes electron#37463 * fix: wait for thumbnails from PipeWire when necessary The PipeWire stream starts after the dialog is dismissed. If the sources are listed immediately afterwards, the thumbnail may not have been generated by that time. Explicitly wait for both thumbnail generation and a selection on the source dialog before listing sources.

aiddya requested a review from a team as a code owner June 18, 2023 21:30

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 18, 2023

aiddya changed the title ~~fix: use StartUpdating method for PipeWire capturer (#37463)~~ fix: use StartUpdating method for PipeWire capturer Jun 18, 2023

aiddya force-pushed the pipewire-crash-fix branch from 0c32734 to ec7bb90 Compare June 18, 2023 21:33

codebytere added the semver/patch backwards-compatible bug fixes label Jun 19, 2023

codebytere requested review from ckerr and VerteDinde June 19, 2023 13:04

codebytere added target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Jun 19, 2023

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 19, 2023

smartfrigde mentioned this pull request Jun 23, 2023

[BUG] New Screenshare Implementation doesnt work on Wayland Vencord/Vesktop#40

Closed

orowith2os mentioned this pull request Jul 1, 2023

aur arch builds do not work on wayland ArmCord/ArmCord#416

Closed

codebytere reviewed Jul 3, 2023

View reviewed changes

aiddya force-pushed the pipewire-crash-fix branch from c769e9a to 747579c Compare July 6, 2023 02:38

aiddya added 2 commits July 5, 2023 19:49

trop bot added merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. and removed in-flight/24-x-y in-flight/25-x-y in-flight/26-x-y labels Jul 11, 2023

infra-monkey mentioned this pull request Jul 14, 2023

Slack crashes in share screen attempt in Wayland (regression) flathub/com.slack.Slack#196

Closed

aiddya deleted the pipewire-crash-fix branch July 15, 2023 15:47

aiddya mentioned this pull request Jul 16, 2023

fix: use StartUpdating method for PipeWire capturer #39116

Merged

trop bot added in-flight/23-x-y and removed needs-manual-bp/23-x-y labels Jul 16, 2023

aiddya mentioned this pull request Jul 16, 2023

fix: use StartUpdating method for PipeWire capturer #39117

Closed

trop bot added in-flight/22-x-y and removed in-flight/22-x-y labels Jul 16, 2023

trop bot added merged/23-x-y PR was merged to the "23-x-y" branch. and removed in-flight/23-x-y labels Jul 24, 2023

theofficialgman mentioned this pull request Aug 2, 2023

Screenshare window instead of whole screen SpacingBat3/WebCord#409

Closed

aiddya mentioned this pull request Aug 2, 2023

fix: use generic capturer to list both screens and windows when possible #39111

Merged

3 tasks

This was referenced Sep 1, 2023

fix: use generic capturer to list both screens and windows when possible #39710

Merged

fix: use generic capturer to list both screens and windows when possible #39711

Merged

commit-lint bot mentioned this pull request Sep 21, 2023

[pull] main from electron:main Blaquez-home/electron#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use StartUpdating method for PipeWire capturer #38833

fix: use StartUpdating method for PipeWire capturer #38833

aiddya commented Jun 18, 2023 •

edited

welcome bot commented Jun 18, 2023

sunknudsen commented Jun 20, 2023

byquanton commented Jun 20, 2023

aiddya commented Jun 21, 2023

byquanton commented Jun 21, 2023

aiddya commented Jun 21, 2023

aiddya commented Jun 23, 2023

byquanton commented Jun 23, 2023

sunknudsen commented Jun 30, 2023

theofficialgman commented Jun 30, 2023

ToasterUwU commented Jun 30, 2023

VerteDinde commented Jul 3, 2023 •

edited

codebytere left a comment

codebytere Jul 3, 2023

aiddya Jul 6, 2023

sunknudsen commented Jul 15, 2023 •

edited

ToasterUwU commented Jul 15, 2023

sunknudsen commented Jul 15, 2023

ToasterUwU commented Jul 15, 2023

aiddya commented Jul 15, 2023

trop bot commented Jul 16, 2023

trop bot commented Jul 16, 2023

sunknudsen commented Jul 17, 2023

fix: use StartUpdating method for PipeWire capturer #38833

fix: use StartUpdating method for PipeWire capturer #38833

Conversation

aiddya commented Jun 18, 2023 • edited

Description of Change

Checklist

Release Notes

welcome bot commented Jun 18, 2023

sunknudsen commented Jun 20, 2023

byquanton commented Jun 20, 2023

aiddya commented Jun 21, 2023

byquanton commented Jun 21, 2023

aiddya commented Jun 21, 2023

aiddya commented Jun 23, 2023

byquanton commented Jun 23, 2023

sunknudsen commented Jun 30, 2023

theofficialgman commented Jun 30, 2023

ToasterUwU commented Jun 30, 2023

VerteDinde commented Jul 3, 2023 • edited

codebytere left a comment

Choose a reason for hiding this comment

codebytere Jul 3, 2023

Choose a reason for hiding this comment

aiddya Jul 6, 2023

Choose a reason for hiding this comment

sunknudsen commented Jul 15, 2023 • edited

ToasterUwU commented Jul 15, 2023

sunknudsen commented Jul 15, 2023

ToasterUwU commented Jul 15, 2023

aiddya commented Jul 15, 2023

trop bot commented Jul 16, 2023

trop bot commented Jul 16, 2023

sunknudsen commented Jul 17, 2023

aiddya commented Jun 18, 2023 •

edited

VerteDinde commented Jul 3, 2023 •

edited

sunknudsen commented Jul 15, 2023 •

edited