Closed Bug 1841851 Opened 1 year ago Closed 1 year ago

libwebrtc framerate negotiation breaks screensharing on Wayland

Categories

(Core :: WebRTC, defect, P3)

defect

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
relnote-firefox --- 116+
firefox116 --- fixed
firefox117 --- fixed

People

(Reporter: david.turner, Assigned: jgrulich)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

System info

  • Raspberry Pi 4, aarch64, running Raspberry Pi OS based on Debian Bookworm
  • Wayland, using Wayfire compositor
  • Using the xdg-desktop-portal-wlr for screen capture
  • Pipewire+wireplumber 0.3.72 built from latest git (but I can also reproduce the problem with pipewire+wireplumber 0.3.65 installed from Bookworm packages)

Steps to reproduce:

What happens:

  • Screen sharing appears to start and the active sharing popup appears
  • But no screen content is seen on the GUM page
  • I've attached the errors seen in the pipewire log (My pipewire is modified to add a little more info here)
  • The output from libwebrtc trace is copied at the bottom

Debugging:

  • I've done a git bisect and the regression was introduced by 29e87376, which vendors the libwebrtc commit "desktop_capturer: Support frame rate negotiation via pipewire"
  • It looks like pipewire is failing to find a matching format between the xdg-desktop-portal-wlr output and the libwebrtc input.
  • Given that nobody else has noticed this in over a month I'm suspicious that this might be specific to xdg-desktop-portal-wlr

libwebrtc trace output:

[Parent 17589: Main Thread]: D/webrtc_trace (screen_capture_portal_interface.cc:34): Successfully created proxy for the portal.
[Parent 17589: Main Thread]: D/webrtc_trace (xdg_desktop_portal_utils.cc:119): Desktop session requested.
[Parent 17589: Main Thread]: D/webrtc_trace (screen_capture_portal_interface.cc:53): Initializing the session.
[Parent 17589: Main Thread]: D/webrtc_trace (screencast_portal.cc:264): Requesting sources from the screen cast session.
[Parent 17589: Main Thread]: D/webrtc_trace (screencast_portal.cc:290): Sources requested from the screen cast session.
[Parent 17589: Main Thread]: D/webrtc_trace (screencast_portal.cc:305): Subscribed to sources signal.
[Parent 17589: Main Thread]: D/webrtc_trace (screencast_portal.cc:320): Received sources signal from session.
[Parent 17589: Main Thread]: D/webrtc_trace (xdg_desktop_portal_utils.cc:155): Starting the portal session.
[Parent 17589: Main Thread]: D/webrtc_trace (screen_capture_portal_interface.cc:123): Subscribed to the start signal.
[Parent 17589: Main Thread]: D/webrtc_trace (screencast_portal.cc:359): Start signal received.
[Parent 17589: Main Thread]: D/webrtc_trace (screencast_portal.cc:426): Opening the PipeWire remote.
[Parent 17589: Main Thread]: D/webrtc_trace (egl_dmabuf.cc:476): Egl initialization succeeded
[Parent 17589: Main Thread]: D/webrtc_trace (shared_screencast_stream.cc:518): PipeWire remote opened.
[Parent 17589: Unnamed thread 7ecda7c940]: D/webrtc_trace (shared_screencast_stream.cc:257): PipeWire stream format changed.
[Parent 17589: Unnamed thread 7ecda7c940]: D/webrtc_trace (shared_screencast_stream.cc:234): PipeWire stream state error: no more input formats
[Parent 17589: Unnamed thread 7ecda7c940]: D/webrtc_trace (shared_screencast_stream.cc:197): PipeWire remote error: no more input formats

Is this something you could help look into?

Blocks: wayland
Severity: -- → S3
Flags: needinfo?(stransky)
Priority: -- → P3

Looks like it's a buffer overflow in webrtc/pipewire 🎉

The pipewire format PODs setup by webrtc to receive screen captures (screencast_stream_utils.cc:BuildFormat()) were pretty close to the 2048-byte size limit before the framerate and maxFramerate properties were added, and adding those was enough to push it over the edge and overflow the buffer. Something else presumably overwrites the overflowed portion of the format POD and pipewire ignores the invalid (overwritten) format options, which in my case means there's no matching format since the one I wanted was near the end. Other setups may or may not see the problem depending on memory layout and which format they actually use.

To confirm this hypothesis: Making all the SPA POD builder buffers 4096 bytes (in shared_screencast_stream.cc do s/1024/4096/ and s/2048/4096/) makes the problem go away.

In theory pipewire should throw an error if the builder buffer overflows, so I need to work out why this isn't happening. And then we'll need an upstream webrtc patch to increase the buffer size. I'll probably get patches out for both of these next week.

Ideally someone should test if this affects more common Wayland platforms (i.e. Asahi and x86_64+GNOME). The problematic commit didn't make it into the 115 release but I think it's in the beta branch.

Ah, it turns out when Pipewire says "if the buffer is not large enough, an appropriate error will be generated" they mean "Will fire the optional callback nobody ever uses and return an error code nobody ever checks and which is discarded by some of our macros". But it's not obviously buggy even if it is super unhelpful.

I'll file a bug report with webrtc.

I proposed a fix to upstream. I will submit a backport once it gets approved and merged so I can add a tracking commit hash for future WebRTC backports on Firefox side.

This is a simple backport of an WebRTC upstream change.

Upstream commit: 8fcc6df79daf1810cd4ecdb8d2ef1d361abfdc9c

Assignee: nobody → jgrulich
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Seems to be handled now, thanks.

Flags: needinfo?(stransky)

Comment on attachment 9343434 [details]
Bug 1841851 - WebRTC backport: PipeWire capturer: increase buffer size to avoid buffer overflow r=jib

Beta/Release Uplift Approval Request

  • User impact if declined: Screen sharing using pipewire is going to be broken in 116 without this.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This has been well understood and is simply making a buffer larger. This is a patch from upstream that we're backporting.
  • String changes made/needed: none
  • Is Android affected?: No
Attachment #9343434 - Flags: approval-mozilla-beta?
Pushed by padenot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/263be02fdeb0
WebRTC backport: PipeWire capturer: increase buffer size to avoid buffer overflow r=jib,webrtc-reviewers,mjf

Comment on attachment 9343434 [details]
Bug 1841851 - WebRTC backport: PipeWire capturer: increase buffer size to avoid buffer overflow r=jib

Switching flag to release for consideration for the next dot release

Attachment #9343434 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch

Comment on attachment 9343434 [details]
Bug 1841851 - WebRTC backport: PipeWire capturer: increase buffer size to avoid buffer overflow r=jib

Approved for the 116.0.3 dot release

Attachment #9343434 - Flags: approval-mozilla-release? → approval-mozilla-release+
QA Whiteboard: [qa-triaged]
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: