Status
()
People
(Reporter: kamidphish, Assigned: kamidphish)
Tracking
(Blocks: 1 bug)
Firefox Tracking Flags
(firefox55 fixed)
Details
Attachments
(5 attachments, 14 obsolete attachments)
129.04 KB,
patch
|
kamidphish
:
review+
|
Details | Diff | Splinter Review |
804 bytes,
patch
|
rillian
:
review+
jbeich
:
feedback+
|
Details | Diff | Splinter Review |
219.85 KB,
patch
|
Details | Diff | Splinter Review | |
16.21 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
2.03 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
Convert the pulse audio backend in cubeb_pulse.c to rust.
(Assignee) | ||
Updated•2 years ago
|
Assignee: nobody → dglastonbury
(Assignee) | ||
Comment 1•2 years ago
|
||
Created attachment 8847837 [details] [diff] [review] P1: Import rustified cubeb_pulse.c Makes use of libpulse-sys crate from crates.io. Includes as a sub-crate because it will be replaced in a follow up patch. MozReview-Commit-ID: EZXJAzN3X7c
(Assignee) | ||
Comment 2•2 years ago
|
||
Created attachment 8847838 [details] [diff] [review] P2: Enable cubeb-pulse-rs crate on Linux. MozReview-Commit-ID: AFSzSQcNWHV
(Assignee) | ||
Comment 3•2 years ago
|
||
Created attachment 8847839 [details] [diff] [review] P3: Update from github repo support. MozReview-Commit-ID: CG7srdz0MBq
(Assignee) | ||
Comment 4•2 years ago
|
||
Created attachment 8847840 [details] [diff] [review] P4: Enable rust backend. MozReview-Commit-ID: 5ZQLFXNFWdB
(Assignee) | ||
Updated•2 years ago
|
Attachment #8847838 -
Flags: feedback?(kinetik)
Updated•2 years ago
|
Rank: 22
Priority: -- → P2
Comment 5•2 years ago
|
||
Comment on attachment 8847838 [details] [diff] [review] P2: Enable cubeb-pulse-rs crate on Linux. Review of attachment 8847838 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine, but personally I'd prefer it to live in a subdirectory of media/libcubeb.
Attachment #8847838 -
Flags: feedback?(kinetik) → feedback+
Comment on attachment 8847838 [details] [diff] [review] P2: Enable cubeb-pulse-rs crate on Linux. PulseAudio is used by default on many Tier3 platforms that lack native backend as well, see bug 952828. > +#[cfg(target_os = "linux")] [...] > +[target.'cfg(target_os = "linux")'.dependencies] Can you convert the conditionals to depend on MOZ_PULSEAUDIO instead? $ ./mach build [...] ../../build/unix/gold/ld: error: obj-x86_64-unknown-freebsd12.0/toolkit/library/../../media/libcubeb/src/cubeb.o: requires dynamic R_X86_64_PC32 reloc against 'pulse_rust_init' which may overflow at runtime; recompile with -fPIC ../../build/unix/gold/ld: error: read-only segment has dynamic relocations obj-x86_64-unknown-freebsd12.0/toolkit/library/../../media/libcubeb/src/cubeb.o:media/libcubeb/src/cubeb.c:function cubeb_init: warning: undefined reference to 'pulse_rust_init' clang++: error: linker command failed with exit code 1 (use -v to see invocation)
(Assignee) | ||
Comment 7•2 years ago
|
||
Created attachment 8852267 [details] [diff] [review] P1: Update cubeb to fd0d72b MozReview-Commit-ID: 9zdVuRGdrbe
Attachment #8852267 -
Flags: review?(kinetik)
(Assignee) | ||
Comment 8•2 years ago
|
||
Created attachment 8852268 [details] [diff] [review] P2: Enable USE_PULSE_RUST in libcubeb. MozReview-Commit-ID: 5ZQLFXNFWdB
Attachment #8852268 -
Flags: review?(kinetik)
(Assignee) | ||
Comment 9•2 years ago
|
||
Created attachment 8852269 [details] [diff] [review] P3: Import rustified cubeb_pulse.c Makes use of libpulse-sys crate from crates.io. Includes as a sub-crate because it will be replaced in a follow up patch. MozReview-Commit-ID: EZXJAzN3X7c
(Assignee) | ||
Updated•2 years ago
|
Attachment #8847837 -
Attachment is obsolete: true
(Assignee) | ||
Comment 10•2 years ago
|
||
Created attachment 8852270 [details] [diff] [review] P4: Enable cubeb-pulse-rs crate on Linux. MozReview-Commit-ID: AFSzSQcNWHV
Attachment #8852270 -
Flags: review?(giles)
(Assignee) | ||
Updated•2 years ago
|
Attachment #8847838 -
Attachment is obsolete: true
(Assignee) | ||
Comment 11•2 years ago
|
||
Created attachment 8852271 [details] [diff] [review] P5: Update rust crates. MozReview-Commit-ID: 5LJOupV8Pqr
Attachment #8852271 -
Flags: review?(giles)
(Assignee) | ||
Updated•2 years ago
|
Attachment #8847840 -
Attachment is obsolete: true
(Assignee) | ||
Comment 12•2 years ago
|
||
Created attachment 8852272 [details] [diff] [review] P6: Update from github repo support. MozReview-Commit-ID: CG7srdz0MBq
Attachment #8852272 -
Flags: review?(kinetik)
(Assignee) | ||
Updated•2 years ago
|
Attachment #8847839 -
Attachment is obsolete: true
Updated•2 years ago
|
Attachment #8852267 -
Flags: review?(kinetik) → review+
Updated•2 years ago
|
Attachment #8852268 -
Flags: review?(kinetik) → review+
Updated•2 years ago
|
Attachment #8852272 -
Flags: review?(kinetik) → review+
Comment 13•2 years ago
|
||
Comment on attachment 8852270 [details] [diff] [review] P4: Enable cubeb-pulse-rs crate on Linux. Review of attachment 8852270 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/library/rust/Cargo.lock @@ +210,5 @@ > "syn 0.11.8 (registry+https://github.com/rust-lang/crates.io-index)", > ] > > [[package]] > +name = "cubeb-ffi" I guess the actual source import for this is in the previous, unreviewed patch? It's confusing having the update split like this, but I suppose it's ok. The build should break if any of this metadata is incorrec.t ::: toolkit/library/rust/shared/Cargo.toml @@ -15,2 @@ > [features] > -default = [] What does this do? It seems like it might have side-effects.
Comment 14•2 years ago
|
||
Comment on attachment 8852272 [details] [diff] [review] P6: Update from github repo support. Review of attachment 8852272 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/cubeb-pulse-rs/README_MOZILLA @@ +2,5 @@ > +git repository using the update.sh script. The only changes > +made were those applied by update.sh and the addition of > +Makefile.in build files for the Mozilla build system. > + > +The cubeb-pulse-rs git repository is: git://github.com/djg/cubeb-pulse-rs.git Please list an https url here. The git protocol is unauthenticated and shouldn't be used.
Comment 15•2 years ago
|
||
Comment on attachment 8852269 [details] [diff] [review] P3: Import rustified cubeb_pulse.c Review of attachment 8852269 [details] [diff] [review]: ----------------------------------------------------------------- You probably don't want the Cargo.lock files checked in for crates you're maintaining in-tree. The dependencies should be tracked at the gkrust level. ::: media/cubeb-pulse-rs/cubeb-ffi/build.rs @@ +1,1 @@ > +extern crate cmake; Does this work on try? I think cmake would be a new build dependency for us.
Comment 16•2 years ago
|
||
Comment on attachment 8852271 [details] [diff] [review] P5: Update rust crates. Review of attachment 8852271 [details] [diff] [review]: ----------------------------------------------------------------- r=me for vendoring semver and semver parser. Please write a more specific commit message. Something like: > Bug 1346665 - Vendor semver rust crates. r=rillian > > Import semver and semver-parser crates needed by the cubeb > pulseaudio backend.
Attachment #8852271 -
Flags: review?(giles) → review+
(Assignee) | ||
Comment 17•2 years ago
|
||
(In reply to Jan Beich from comment #6) > Comment on attachment 8847838 [details] [diff] [review] > P2: Enable cubeb-pulse-rs crate on Linux. > > PulseAudio is used by default on many Tier3 platforms that lack native > backend as well, see bug 952828. > > > +#[cfg(target_os = "linux")] > [...] > > +[target.'cfg(target_os = "linux")'.dependencies] > > Can you convert the conditionals to depend on MOZ_PULSEAUDIO instead? Hi Jan, Thanks for the heads up. I'll look into making moz.build configure the crate based upon MOZ_PULSEAUDIO.
(Assignee) | ||
Comment 18•2 years ago
|
||
(In reply to Ralph Giles (:rillian) | needinfo me from comment #15) > Comment on attachment 8852269 [details] [diff] [review] > P3: Import rustified cubeb_pulse.c > > Review of attachment 8852269 [details] [diff] [review]: > ----------------------------------------------------------------- > > You probably don't want the Cargo.lock files checked in for crates you're > maintaining in-tree. The dependencies should be tracked at the gkrust level. > > ::: media/cubeb-pulse-rs/cubeb-ffi/build.rs > @@ +1,1 @@ > > +extern crate cmake; > > Does this work on try? I think cmake would be a new build dependency for us. That shouldn't be there...
(Assignee) | ||
Comment 19•2 years ago
|
||
(In reply to Dan Glastonbury :kamidphish from comment #18) > (In reply to Ralph Giles (:rillian) | needinfo me from comment #15) > > ::: media/cubeb-pulse-rs/cubeb-ffi/build.rs > > @@ +1,1 @@ > > > +extern crate cmake; > > > > Does this work on try? I think cmake would be a new build dependency for us. > > That shouldn't be there... Ok. build.rs isn't being used. I'll remove it.
(Assignee) | ||
Comment 20•2 years ago
|
||
(In reply to Ralph Giles (:rillian) | needinfo me from comment #13) > Comment on attachment 8852270 [details] [diff] [review] > P4: Enable cubeb-pulse-rs crate on Linux. > > Review of attachment 8852270 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/library/rust/Cargo.lock > @@ +210,5 @@ > > "syn 0.11.8 (registry+https://github.com/rust-lang/crates.io-index)", > > ] > > > > [[package]] > > +name = "cubeb-ffi" > > I guess the actual source import for this is in the previous, unreviewed > patch? It's confusing having the update split like this, but I suppose it's > ok. The build should break if any of this metadata is incorrec.t This is a sub-crate in the cubeb-pulse-rust crate. > ::: toolkit/library/rust/shared/Cargo.toml > @@ -15,2 @@ > > [features] > > -default = [] > > What does this do? It seems like it might have side-effects. That shouldn't have been removed. Although everything compiles without it.
Comment 21•2 years ago
|
||
Comment on attachment 8852270 [details] [diff] [review] P4: Enable cubeb-pulse-rs crate on Linux. Review of attachment 8852270 [details] [diff] [review]: ----------------------------------------------------------------- Ok, that all sounds good. Unsetting review for now. Please request again when you're ready with a revised patch.
Attachment #8852270 -
Flags: review?(giles)
(Assignee) | ||
Comment 22•2 years ago
|
||
Comment on attachment 8852267 [details] [diff] [review] P1: Update cubeb to fd0d72b This patch is no long required as Cubeb has been updated in another bug.
Attachment #8852267 -
Attachment is obsolete: true
(Assignee) | ||
Comment 23•2 years ago
|
||
Created attachment 8859051 [details] [diff] [review] P2: Enable USE_PULSE_RUST in libcubeb. MozReview-Commit-ID: 5ZQLFXNFWdB
Attachment #8859051 -
Flags: review+
(Assignee) | ||
Comment 24•2 years ago
|
||
Created attachment 8859052 [details] [diff] [review] P3: Import rustified cubeb_pulse.c MozReview-Commit-ID: EZXJAzN3X7c
(Assignee) | ||
Comment 25•2 years ago
|
||
Created attachment 8859053 [details] [diff] [review] P4: Enable cubeb-pulse-rs crate when MOZ_PULSEAUDIO is defined. r?rillian moz.build detects MOZ_PULSEAUDIO define and passes --features cubeb_pulse_rust to cargo build. MozReview-Commit-ID: AFSzSQcNWHV
Attachment #8859053 -
Flags: review?(giles)
(Assignee) | ||
Comment 26•2 years ago
|
||
Created attachment 8859054 [details] [diff] [review] P5: Vendor semver rust crates. Import semver and semver-parser creates needed by the cubeb pulseaudio backend. MozReview-Commit-ID: 5LJOupV8Pqr
Attachment #8859054 -
Flags: review+
(Assignee) | ||
Comment 27•2 years ago
|
||
Created attachment 8859055 [details] [diff] [review] P6: Update from github repo support. MozReview-Commit-ID: CG7srdz0MBq
Attachment #8859055 -
Flags: review+
(Assignee) | ||
Updated•2 years ago
|
Attachment #8852268 -
Attachment is obsolete: true
(Assignee) | ||
Updated•2 years ago
|
Attachment #8852269 -
Attachment is obsolete: true
(Assignee) | ||
Updated•2 years ago
|
Attachment #8852270 -
Attachment is obsolete: true
(Assignee) | ||
Updated•2 years ago
|
Attachment #8852271 -
Attachment is obsolete: true
(Assignee) | ||
Updated•2 years ago
|
Attachment #8852272 -
Attachment is obsolete: true
Comment 28•2 years ago
|
||
Comment on attachment 8859051 [details] [diff] [review] P2: Enable USE_PULSE_RUST in libcubeb. > + if CONFIG['OS_ARCH'] == 'Linux': Any particular reason to build but not expose "pulse-rust" on some platforms? toolkit/library/rust/moz.build has only MOZ_PULSEAUDIO check. After dropping the check media.cubeb.backend="pulse-rust" works fine on FreeBSD. For Telemetry (see bug 1280630) you may need to list "pulse-rust" in dom/media/CubebUtils.cpp.
Attachment #8859051 -
Flags: feedback-
Comment 29•2 years ago
|
||
(In reply to Jan Beich from comment #28) > After dropping the check s/check/line/. MOZ_PULSEAUDIO alone should be enough to limit "pulse-rust" to Linux. Android and, IIRC, Firefox OS are OS_ARCH=Linux + OS_TARGET=Android.
Comment 30•2 years ago
|
||
Comment on attachment 8859053 [details] [diff] [review] P4: Enable cubeb-pulse-rs crate when MOZ_PULSEAUDIO is defined. r?rillian Review of attachment 8859053 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/library/rust/Cargo.lock @@ +682,5 @@ > "smallvec 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)", > ] > > [[package]] > +name = "semver" These changes should really be part of the next patch in the sequence.
Attachment #8859053 -
Flags: review?(giles) → review+
(Assignee) | ||
Comment 31•2 years ago
|
||
(In reply to Jan Beich from comment #29) > (In reply to Jan Beich from comment #28) > > After dropping the check > > s/check/line/. MOZ_PULSEAUDIO alone should be enough to limit "pulse-rust" > to Linux. Android and, IIRC, Firefox OS are OS_ARCH=Linux + > OS_TARGET=Android. Thanks for your feedback. It's reasonable to not exclude FreeBSD, for example. I'll remove the explicit Linux check. Looking at the condition for Gonk, I should change the features in cubeb-pulse-rs Cargo.toml to support static linking of PulseAudio symbols.
Comment 32•2 years ago
|
||
(In reply to Dan Glastonbury :kamidphish from comment #31) > Looking at the condition for Gonk, I should change the features in > cubeb-pulse-rs Cargo.toml to support static linking of PulseAudio symbols. I thought gonk bindings were tier-3 at this point, or is there a user besides FFOS? If so we're not obligated to maintain them, and should probably remove them altogether if no one's volunteering.
(Assignee) | ||
Comment 33•2 years ago
|
||
(In reply to Ralph Giles (:rillian) | needinfo me from comment #30) > Comment on attachment 8859053 [details] [diff] [review] > P4: Enable cubeb-pulse-rs crate when MOZ_PULSEAUDIO is defined. r?rillian > > Review of attachment 8859053 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/library/rust/Cargo.lock > @@ +682,5 @@ > > "smallvec 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)", > > ] > > > > [[package]] > > +name = "semver" > > These changes should really be part of the next patch in the sequence. I plan to flatten the patches on landing. I can do that now, if you like?
(Assignee) | ||
Comment 34•2 years ago
|
||
(In reply to Ralph Giles (:rillian) | needinfo me from comment #32) > (In reply to Dan Glastonbury :kamidphish from comment #31) > > > Looking at the condition for Gonk, I should change the features in > > cubeb-pulse-rs Cargo.toml to support static linking of PulseAudio symbols. > > I thought gonk bindings were tier-3 at this point, or is there a user > besides FFOS? If so we're not obligated to maintain them, and should > probably remove them altogether if no one's volunteering. OK. I'll scratch that plan then.
(Assignee) | ||
Comment 35•2 years ago
|
||
Created attachment 8859444 [details] [diff] [review] P2: Enable USE_PULSE_RUST in libcubeb. MozReview-Commit-ID: 5ZQLFXNFWdB
Attachment #8859444 -
Flags: review?(giles)
Attachment #8859444 -
Flags: feedback?(jbeich)
(Assignee) | ||
Updated•2 years ago
|
Attachment #8859051 -
Attachment is obsolete: true
Comment 36•2 years ago
|
||
(In reply to Dan Glastonbury :kamidphish from comment #33) > I plan to flatten the patches on landing. I can do that now, if you like? If you're going to flatten when you land then it's not important to fix it.
Updated•2 years ago
|
Attachment #8859444 -
Flags: review?(giles) → review+
Comment 37•2 years ago
|
||
Comment on attachment 8859444 [details] [diff] [review] P2: Enable USE_PULSE_RUST in libcubeb. pulse-rust now works fine on FreeBSD. Thank you!
Attachment #8859444 -
Flags: feedback?(jbeich) → feedback+
(Assignee) | ||
Comment 38•2 years ago
|
||
Try push with code compiling but backend disabled. https://treeherder.mozilla.org/#/jobs?repo=try&revision=665911053e326d33e5c4b1042987b1be29486f32
(Assignee) | ||
Comment 39•2 years ago
|
||
Created attachment 8859842 [details] [diff] [review] P3: Import rustified cubeb_pulse.c MozReview-Commit-ID: EZXJAzN3X7c
(Assignee) | ||
Updated•2 years ago
|
Attachment #8859052 -
Attachment is obsolete: true
(Assignee) | ||
Comment 40•2 years ago
|
||
Created attachment 8859844 [details] [diff] [review] P4: Enable cubeb-pulse-rs crate when MOZ_PULSEAUDIO is defined. r?rillian moz.build detects MOZ_PULSEAUDIO define and passes --features cubeb_pulse_rust to cargo build. MozReview-Commit-ID: AFSzSQcNWHV
Attachment #8859844 -
Flags: review?(giles)
(Assignee) | ||
Updated•2 years ago
|
Attachment #8859053 -
Attachment is obsolete: true
(Assignee) | ||
Comment 41•2 years ago
|
||
Created attachment 8859845 [details] [diff] [review] P6: Update from github repo support. MozReview-Commit-ID: CG7srdz0MBq
Attachment #8859845 -
Flags: review?(giles)
(Assignee) | ||
Updated•2 years ago
|
Attachment #8859055 -
Attachment is obsolete: true
Comment 42•2 years ago
|
||
Comment on attachment 8859844 [details] [diff] [review] P4: Enable cubeb-pulse-rs crate when MOZ_PULSEAUDIO is defined. r?rillian Review of attachment 8859844 [details] [diff] [review]: ----------------------------------------------------------------- I often forget about the gtest crate too. Solution: write gtests so you think of running them locally! :-)
Attachment #8859844 -
Flags: review?(giles) → review+
Updated•2 years ago
|
Attachment #8859845 -
Flags: review?(giles) → review+
Comment 43•2 years ago
|
||
Pushed by dglastonbury@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/29e1433cdec5 P1: Enable USE_PULSE_RUST in libcubeb. r=kinetik,rillian https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a79e8ceb58 P2: Import rustified cubeb_pulse.c. r=kinetik,rillian
Comment 44•2 years ago
|
||
(In reply to Pulsebot from comment #43) > Pushed by dglastonbury@mozilla.com: > https://hg.mozilla.org/integration/mozilla-inbound/rev/29e1433cdec5 > P1: Enable USE_PULSE_RUST in libcubeb. r=kinetik,rillian [...] > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk': > + DEFINES['DISABLE_LIBPULSE_DLOPEN'] = True Can you re-apply bug 1357323 ?
Comment 45•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29e1433cdec5 https://hg.mozilla.org/mozilla-central/rev/d7a79e8ceb58
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee) | ||
Comment 47•2 years ago
|
||
(In reply to Jan Beich from comment #44) > (In reply to Pulsebot from comment #43) > > Pushed by dglastonbury@mozilla.com: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/29e1433cdec5 > > P1: Enable USE_PULSE_RUST in libcubeb. r=kinetik,rillian > [...] > > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk': > > + DEFINES['DISABLE_LIBPULSE_DLOPEN'] = True > > Can you re-apply bug 1357323 ? Yes. I've opened Bug 1358938.
Flags: needinfo?(dglastonbury)
Updated•a year ago
|
See Also: → bug 1438528
You need to log in
before you can comment on or make changes to this bug.
Description
•