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

vesktop: add webrtc support on wayland #321710

Closed
wants to merge 1 commit into from
Closed

Conversation

Mikilio
Copy link
Contributor

@Mikilio Mikilio commented Jun 22, 2024

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot requested review from Scrumplex, getchoo, pluiedev and vgskye June 22, 2024 09:29
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jun 22, 2024
@JohnRTitor
Copy link
Contributor

Result of nixpkgs-review pr 321710 run on x86_64-linux 1

1 package built:
  • vesktop

@JohnRTitor
Copy link
Contributor

Perhaps WebRtcPipeWireCamera too?

Additionally I think a commandLineArgs override could be provided like we do for google-chrome

@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 22, 2024

Is this to "screen share" that recently introduced pipewire camera output? Because if it has something to do with camera selection, I'm not sure if this will work on any discord client. See issues like this

@JohnRTitor
Copy link
Contributor

This will help when video calling via Vesktop I believe.

image

I wonder if it is available on the electron version Vesktop uses.

@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 22, 2024

I don't think this is on by default as most of my programs still use the v4l2l backend. I'd love that feature to be easily addable by the override you mentioned, but with something like vesktop I believe the defaults should work for the most naive user. I will test if this work without having to enable anything and let you know.

@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 22, 2024

Related: feature request

@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 22, 2024

It seems that feature is not implemented yet and Vesktop itself only uses these flags which is equivalent to my PR. I suggest not adding anything extra until the Vesktop devs add it.

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

i'm a bit confused here. what exactly does this flag solve? from my testing, both audio and video sharing work perfectly fine without this - as intended through venmic. is this not the case for you?

in any case, the commit message should also be changed. this does not "add webrtc support". webrtc is already enabled by default for this build of electron, and this flag doesn't enable it

@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 22, 2024

On wayland this is necessary or screen share will not work. As mentioned above the vesktop flatpak also enables this on ozone. I have changed the commit message to better explain this.

@Mikilio Mikilio changed the title vesktop: add webrtc support vesktop: add webrtc support on wayland Jun 22, 2024
@Mikilio Mikilio requested a review from getchoo June 22, 2024 16:51
Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

Screensharing now properly works on Wayland with Pipewire, approved.

@pluiedev
Copy link
Member

I've been streaming things with Vesktop on Plasma 6 Wayland and it's always worked fine. Can you share more info on what you mean by WebRTC & screensharing not working?

@pyrox0
Copy link
Member

pyrox0 commented Jun 22, 2024

I've been using Vesktop for several months now without this flag on SwayWM, and I've had no issues screensharing and using WebRTC. What problem is this PR solving?

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

On wayland this is necessary or screen share will not work

as reported by others here and through my own testing, this doesn't seem to be the case. this should be handled by venmic - not electron directly - and it has been supported for months. this PR doesn't seem to be actually fixing anything

as mentioned above the vesktop flatpak also enables this on ozone.

could you send a link to this? reading your previous comments, you don't actually mention this anywhere. the closest is a link you provided to the .env.example in the upstream repo here, but the --enable-webrtc-pipewire-capturer flag from there doesn't seem to be used in the flatpak (you can search for it here) and the WebRTCPipeWireCapturer flag you're adding here isn't either (search results)

I have changed the commit message to better explain this

the commit message still doesn't explain this at all. the only change you made is adding "on wayland", but once again webrtc has always worked. this is enabling the webrtc pipewire capturer

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 22, 2024
@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 23, 2024

Well it doesn't work for me and this PR solves it, I apologize for making assumptions about the Flatpak.
While I can definitely confirm that this PR solves my issue (as I wouldn't make a PR for any other reason) it seems true that the issue may not be what it seems, and I may need help figuring it out.
Here some information:

  • I use hyprland which comes with its own xdg portal
  • When I click the screen share button on vesktop it opens the source picker from the xdg portal as expected.
  • When I chose a source nothing happens (vesktop doesn't start screen share as if I never pressed the button)
  • The button becomes unresponsive until I restart vesktop

To me, this all seems like venmic may be crashing.

All this does not happen when enabling the Flag in my PR.

Should I make an issue instead? I can do with an overlay for now.

@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 23, 2024

Also, it seems like here vesktop uses the environment variable ELECTRON_LAUNCH_FLAGS to start modules. This variable is not defined on my system nor in this package. So it really does look to me like the issue is caused by a missing flag in the package.

@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 23, 2024

Can you maybe reproduce the issue on wayland if you unset ELECTRON_LAUNCH_FLAGS?

@getchoo @pyrox0 @pluiedev

@Mikilio
Copy link
Contributor Author

Mikilio commented Jun 23, 2024

Scratch this. I can not reproduce the error anymore. Vesktop works without setting the flag now and I think I get what you meant with electron not being involved in screen sharing. Unfortunately, we will never find out why I had that issue, and now I don't. Maybe something like my external monitors or my docking station being wonky.

I will close this PR and reopen if I can reproduce this again. I apologize for the commotion.

@Mikilio Mikilio closed this Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants