search

webrtc-rs / webrtc

A pure Rust implementation of WebRTC
https://webrtc.rs
Apache License 2.0
4.08k stars 363 forks source link

Broaden compatibility for embedded platforms #546

Closed coder0xff closed 5 months ago

coder0xff commented 6 months ago

I'm working on compiling WebRTC on the Xtensa (ESP32s3) platform. Tokio was recently updated to support more platforms, but certain features are not available (and may never be). The "process" and "signal" features aren't used by WebRTC. Rather than use the "full" feature group, the relevant Cargo.toml's have been updated to list the set of features included in "full", excluding the "process" and "signal" features. WebRTC builds ok against Tokio with those features disabled.

ivmarkov commented 6 months ago
  • Comprehensive migration to portable_atomic

Why do you need that? ESP IDF does support regular atomics (up to 32 bits, that is, U64 and bigger are not supported). Further on, any platform claiming to support Rust STD, must also support the standard atomics.

So in a way, it boils down to the following: Is WebRTC having a hard-dependency on STD? If yes, then there is no need to change core::sync::atomics to portable-atomics. And I assume the answer here is "yes" anyway, as tokio itself (on which WebRTC has a hard dependency, isn't it) has a hard requirement on STD, no?

ivmarkov commented 6 months ago

... or to put it in a different way, portable-atomic is a thing only when:

coder0xff commented 6 months ago

@ivmarkov

U64 and bigger are not supported

Yup

ivmarkov commented 6 months ago

@ivmarkov

U64 and bigger are not supported

Yup

Even AtomicU64 is used here and there, perhaps it would be cleaner if you just fix these places only. Because it is a bit weird to see - say - use portable_atomic::AtomicUsize next to use std::sync::Arc which itself unconditionally uses core::sync::AtomicUsize, and not portable_atomic::AtomicUsize. (Though I suspect the latter might anyway use core::sync::AtomicUsize, if the platform supports it - but - difficult to say as the portable-atomic code is a bit involved.)

If nothing else this would reduce the patch surface significantly.

coder0xff commented 6 months ago

Readability is king. Consistency is easier for the reader to reference and understand. Separately, while this PR is triggered by my work on ESP32s3, it shouldn't introduce specific knowledge about that platform beyond what's absolutely necessary. Choosing this atomic over that makes assumptions about constraints that may not apply to other platforms. That connotation is reflected in the PR's title. That Arc uses AtomicUSize is an implementation detail that has nothing to do with WebRTC.

ivmarkov commented 6 months ago

Readability is king. Consistency is easier for the reader to reference and understand. Separately, while this PR is triggered by my work on ESP32s3, it shouldn't introduce specific knowledge about that platform beyond what's absolutely necessary. Choosing this atomic over that makes assumptions about constraints that may not apply to other platforms. That connotation is reflected in the PR's title. That Arc uses AtomicUSize is an implementation detail that has nothing to do with WebRTC.

I'm trying to help this PR get through, but in the end, I'm not a maintainer of the WebRTC crate, so - yes - my opinions and personal bias I guess don't matter that much in this context. I'll stop here.

coder0xff commented 6 months ago

Hi @rainliu. Can I have a review, please?

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 61.70%. Comparing base (af5c059) to head (54aef3b).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #546 +/- ## ========================================== + Coverage 61.68% 61.70% +0.01% ========================================== Files 529 529 Lines 48827 48827 Branches 12275 12276 +1 ========================================== + Hits 30120 30127 +7 + Misses 9544 9543 -1 + Partials 9163 9157 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

coder0xff commented 6 months ago

Is the failure of the clippy check unrelated to my PR? It's about using Arc::cloned, which I haven't touched. Should I attempt to fix it?

rainliu commented 6 months ago

yes, please fix clippy warning. thanks.

coder0xff commented 6 months ago

@rainliu I think (hope) it's good now. Would you rerun the checks please?

Notes: Since I can't run the checks myself, not even in my own fork (no way to manually trigger it), when I initially created this PR I first did a PR in my own fork. The PR within my own fork allowed triggering the checks, and I could confirm they passed. I then squash-merged within my fork's master, and then used my fork's master to open this upstream PR (where the clippy check subsequently failed, which I fail to understand.) To try again, I basically did a reset to replicate the same workflow.

$ git remote -v
origin  https://github.com/coder0xff/webrtc.git (fetch)
origin  https://github.com/coder0xff/webrtc.git (push)
upstream        https://github.com/webrtc-rs/webrtc.git (fetch)
upstream        https://github.com/webrtc-rs/webrtc.git (push)
$ git checkout master                       # master tracks my fork: remotes/origin/master
$ git reset --hard remotes/upstream/master  # wipe changes from my fork
$ git push --force                          # this closed this PR (upstream #546)
$ git checkout portable_atomic              # I kept the original branch, wise in hindsight
$ git rebase remotes/upstream/master        # pull in the latest from upstream
$ git push --force                          # rebased unsquashed commits back onto my fork

I opened a new PR within my fork to merge portable_atomic into master as before, waited for the checks to pass, then squash-merged. Now that my forks master is ahead of upstream's master, I can reopen this PR. So yes, hopefully the checks pass this time. I wonder if there's a better way to do all this though. Please educate me.

coder0xff commented 6 months ago

Hi. Would you please trigger the workflows?

marcbrevoort-cyberhive commented 5 months ago

Is there anything that's holding this up?