Implement PulseAudio backend for Cubeb in Rust

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: cubeb
P2
normal
Rank:
22
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: kamidphish, Assigned: kamidphish)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
All
Linux
Points:
---
Dependency tree / graph

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+
Jan Beich (away from May 25 to June 11)
: 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
(Assignee)

Description

3 months ago
Convert the pulse audio backend in cubeb_pulse.c to rust.
(Assignee)

Updated

3 months ago
Assignee: nobody → dglastonbury
(Assignee)

Updated

3 months ago
Depends on: 1341238
No longer depends on: 1341238
Depends on: 1341238
(Assignee)

Comment 1

3 months 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

3 months ago
Created attachment 8847838 [details] [diff] [review]
P2: Enable cubeb-pulse-rs crate on Linux.

MozReview-Commit-ID: AFSzSQcNWHV
(Assignee)

Comment 3

3 months ago
Created attachment 8847839 [details] [diff] [review]
P3: Update from github repo support.

MozReview-Commit-ID: CG7srdz0MBq
(Assignee)

Comment 4

3 months ago
Created attachment 8847840 [details] [diff] [review]
P4: Enable rust backend.

MozReview-Commit-ID: 5ZQLFXNFWdB
(Assignee)

Updated

3 months ago
Attachment #8847838 - Flags: feedback?(kinetik)

Updated

2 months ago
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)
(Assignee)

Comment 7

2 months 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 months 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 months 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 months ago
Attachment #8847837 - Attachment is obsolete: true
(Assignee)

Comment 10

2 months 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 months ago
Attachment #8847838 - Attachment is obsolete: true
(Assignee)

Comment 11

2 months ago
Created attachment 8852271 [details] [diff] [review]
P5: Update rust crates.

MozReview-Commit-ID: 5LJOupV8Pqr
Attachment #8852271 - Flags: review?(giles)
(Assignee)

Updated

2 months ago
Attachment #8847840 - Attachment is obsolete: true
(Assignee)

Comment 12

2 months 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 months ago
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+
(Assignee)

Comment 17

2 months 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 months 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 months 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 months 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 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

a month 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

a month 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

a month ago
Created attachment 8859052 [details] [diff] [review]
P3: Import rustified cubeb_pulse.c

MozReview-Commit-ID: EZXJAzN3X7c
(Assignee)

Comment 25

a month 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

a month 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

a month ago
Created attachment 8859055 [details] [diff] [review]
P6: Update from github repo support.

MozReview-Commit-ID: CG7srdz0MBq
Attachment #8859055 - Flags: review+
(Assignee)

Updated

a month ago
Attachment #8852268 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8852269 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8852270 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8852271 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
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+
(Assignee)

Comment 31

a month 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.
(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

a month 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

a month 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

a month 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

a month ago
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+
(Assignee)

Comment 38

a month ago
Try push with code compiling but backend disabled.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=665911053e326d33e5c4b1042987b1be29486f32
(Assignee)

Comment 39

a month ago
Created attachment 8859842 [details] [diff] [review]
P3: Import rustified cubeb_pulse.c

MozReview-Commit-ID: EZXJAzN3X7c
(Assignee)

Updated

a month ago
Attachment #8859052 - Attachment is obsolete: true
(Assignee)

Comment 40

a month 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

a month ago
Attachment #8859053 - Attachment is obsolete: true
(Assignee)

Comment 41

a month ago
Created attachment 8859845 [details] [diff] [review]
P6: Update from github repo support.

MozReview-Commit-ID: CG7srdz0MBq
Attachment #8859845 - Flags: review?(giles)
(Assignee)

Updated

a month ago
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+

Comment 43

a month 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
(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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29e1433cdec5
https://hg.mozilla.org/mozilla-central/rev/d7a79e8ceb58
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
See comment 44.
Flags: needinfo?(dglastonbury)
(Assignee)

Comment 47

a month 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 month ago
Blocks: 1104619

Updated

a month ago
Depends on: 1284816
You need to log in before you can comment on or make changes to this bug.