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

tdesktop: 2.3.0 -> 2.4.3 and enable webrtc #100450

Merged
merged 2 commits into from Oct 16, 2020
Merged

Conversation

oxalica
Copy link
Contributor

@oxalica oxalica commented Oct 13, 2020

Motivation for this change

Bump version.

According to telegramdesktop/tdesktop#8483 (comment), webrtc is now a require dependency, and a special fork tg_owt is used.
So this PR also packaged tg_owt and enabled webrtc.

Fix #98994. Video calls work for me but there's no sound. Audio also works.
@mkg20001 for more test.

There's another attempt #100062 to package (official?) libwebrtc. It is how Arch package tdesktop but seems to be quite complicated, and it also need to patch tdesktop.
Well, I think the tg_owt fork may be more suitable here like we already use bundled rlottie and libtgvoip, but we can also change it later when libwebrtc is in nixpkgs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    • 637.5M -> 653.5M
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @primeos @abbradar

Copy link
Member

@mkg20001 mkg20001 left a comment

Choose a reason for hiding this comment

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

Calls work with and w/o video as intended

@oxalica
Copy link
Contributor Author

oxalica commented Oct 16, 2020

Now I patched cmake files to fix include directories (with install interface) and correctly install headers.

There's still a small issue that we need to nuke reference to include directory of tg_owt, because there is __FILE__ in headers expanding to include directory of tg_owt store path and going to .rodata of the executable.

@ofborg ofborg bot requested a review from primeos October 16, 2020 12:18
Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

Well, this clearly isn't ideal but given the circumstances I think that it's pretty good. I briefly tested tdesktop and everything seems fine (I didn't test calls though... But that part should already be covered anyway).

Would be nice if we could upstream/drop our patch in the future though.

@oxalica huge thanks for this!

@primeos primeos merged commit 4c59c0b into NixOS:master Oct 16, 2020
@primeos
Copy link
Member

primeos commented Oct 25, 2020

Might be fixed by 2.4.4 (https://github.com/telegramdesktop/tdesktop/releases/tag/v2.4.4: "Several crash fixes.", 253c9cb).

I only tested it for <5 minutes though. Anyway, if that doesn't fix it this should go into a new issue with additional information.

@oxalica
Copy link
Contributor Author

oxalica commented Oct 25, 2020

@volth

2.4.3 segfaults in 5-10 minutes after start :(

More information like nixpkgs git revision and stderr output?
I use it every day but never see it crashes.

@oxalica oxalica deleted the bump/tdesktop branch November 24, 2020 13:36
@bb010g
Copy link
Member

bb010g commented Nov 25, 2020

@volth Does 2.4.7 work for you?

dotlambda pushed a commit to dotlambda/nixpkgs that referenced this pull request Jan 28, 2021
Enables WebRTC by packaging tg_owt.

(cherry picked from commit 4c59c0b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tdesktop: Needs libwebrtc for video calls (or crashes)
5 participants