Closed Bug 1346665 Opened 3 years ago Closed 3 years ago

Implement PulseAudio backend for Cubeb in Rust

Categories

(Core :: Audio/Video: cubeb, enhancement, P2)

Product:
Core
Component:
Audio/Video: cubeb
Platform:
All
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kamidphish, Assigned: kamidphish)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 14 obsolete files)

P5: Vendor semver rust crates.
129.04 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
P2: Enable USE_PULSE_RUST in libcubeb.
804 bytes, patch
rillian
: review+
jbeich
: feedback+
Details | Diff | Splinter Review
P3: Import rustified cubeb_pulse.c
219.85 KB, patch
Details | Diff | Splinter Review
P4: Enable cubeb-pulse-rs crate when MOZ_PULSEAUDIO is defined. r?rillian
16.21 KB, patch
rillian
: review+
Details | Diff | Splinter Review
P6: Update from github repo support.
2.03 KB, patch
rillian
: review+
Details | Diff | Splinter Review 
Convert the pulse audio backend in cubeb_pulse.c to rust.
Assignee: nobody → dglastonbury
Depends on: 1341238
No longer depends on: 1341238
Depends on: 1341238
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
MozReview-Commit-ID: AFSzSQcNWHV
MozReview-Commit-ID: CG7srdz0MBq
Attached patch P4: Enable rust backend. (obsolete) — — Splinter Review 
MozReview-Commit-ID: 5ZQLFXNFWdB
Attachment #8847838 - Flags: feedback?(kinetik)
Rank: 22
Priority: -- → P2
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)
Attached patch P1: Update cubeb to fd0d72b (obsolete) — — Splinter Review 
MozReview-Commit-ID: 9zdVuRGdrbe
Attachment #8852267 - Flags: review?(kinetik)
MozReview-Commit-ID: 5ZQLFXNFWdB
Attachment #8852268 - Flags: review?(kinetik)
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
Attachment #8847837 - Attachment is obsolete: true
MozReview-Commit-ID: AFSzSQcNWHV
Attachment #8852270 - Flags: review?(giles)
Attachment #8847838 - Attachment is obsolete: true
Attached patch P5: Update rust crates. (obsolete) — — Splinter Review 
MozReview-Commit-ID: 5LJOupV8Pqr
Attachment #8852271 - Flags: review?(giles)
Attachment #8847840 - Attachment is obsolete: true
MozReview-Commit-ID: CG7srdz0MBq
Attachment #8852272 - Flags: review?(kinetik)
Attachment #8847839 - Attachment is obsolete: true
Attachment #8852267 - Flags: review?(kinetik) → review+
Attachment #8852268 - Flags: review?(kinetik) → review+
Attachment #8852272 - Flags: review?(kinetik) → review+
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 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 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 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+
(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.
(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...
(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.
(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 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)
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
MozReview-Commit-ID: 5ZQLFXNFWdB
Attachment #8859051 - Flags: review+
MozReview-Commit-ID: EZXJAzN3X7c
moz.build detects MOZ_PULSEAUDIO define and passes --features
cubeb_pulse_rust to cargo build.

MozReview-Commit-ID: AFSzSQcNWHV
Attachment #8859053 - Flags: review?(giles)
Import semver and semver-parser creates needed by the cubeb
pulseaudio backend.

MozReview-Commit-ID: 5LJOupV8Pqr
Attachment #8859054 - Flags: review+
MozReview-Commit-ID: CG7srdz0MBq
Attachment #8859055 - Flags: review+
Attachment #8852268 - Attachment is obsolete: true
Attachment #8852269 - Attachment is obsolete: true
Attachment #8852270 - Attachment is obsolete: true
Attachment #8852271 - Attachment is obsolete: true
Attachment #8852272 - Attachment is obsolete: true
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-
(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 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+
(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.
(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.
(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?
(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.
MozReview-Commit-ID: 5ZQLFXNFWdB
Attachment #8859444 - Flags: review?(giles)
Attachment #8859444 - Flags: feedback?(jbeich)
Attachment #8859051 - Attachment is obsolete: true
(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.
Attachment #8859444 - Flags: review?(giles) → review+
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+
MozReview-Commit-ID: EZXJAzN3X7c
Attachment #8859052 - Attachment is obsolete: true
moz.build detects MOZ_PULSEAUDIO define and passes --features
cubeb_pulse_rust to cargo build.

MozReview-Commit-ID: AFSzSQcNWHV
Attachment #8859844 - Flags: review?(giles)
Attachment #8859053 - Attachment is obsolete: true
MozReview-Commit-ID: CG7srdz0MBq
Attachment #8859845 - Flags: review?(giles)
Attachment #8859055 - Attachment is obsolete: true
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+
Attachment #8859845 - Flags: review?(giles) → review+
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
(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 ?
https://hg.mozilla.org/mozilla-central/rev/29e1433cdec5
https://hg.mozilla.org/mozilla-central/rev/d7a79e8ceb58
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
See comment 44.
Flags: needinfo?(dglastonbury)
(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)
Blocks: sb-audio
Depends on: 1284816
See Also: → 1438528
You need to log in before you can comment on or make changes to this bug.