1346665 - Implement PulseAudio backend for Cubeb in Rust

Status: RESOLVED FIXED

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

Description

[u480271]

Convert the pulse audio backend in cubeb_pulse.c to rust.

Comment 1

[u480271]

Attachment: 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

Comment 2

[u480271]

Attachment: P2: Enable cubeb-pulse-rs crate on Linux.

MozReview-Commit-ID: AFSzSQcNWHV

Comment 3

[u480271]

Attachment: P3: Update from github repo support.

MozReview-Commit-ID: CG7srdz0MBq

Comment 4

[u480271]

Attachment: P4: Enable rust backend.

MozReview-Commit-ID: 5ZQLFXNFWdB

Comment 5

[Matthew Gregan [:kinetik]]

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.

Comment 6

[Jan Beich]

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)

Comment 7

[u480271]

Attachment: P1: Update cubeb to fd0d72b

MozReview-Commit-ID: 9zdVuRGdrbe

Comment 8

[u480271]

Attachment: P2: Enable USE_PULSE_RUST in libcubeb.

MozReview-Commit-ID: 5ZQLFXNFWdB

Comment 9

[u480271]

Attachment: 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

Comment 10

[u480271]

Attachment: P4: Enable cubeb-pulse-rs crate on Linux.

MozReview-Commit-ID: AFSzSQcNWHV

Comment 11

[u480271]

Attachment: P5: Update rust crates.

MozReview-Commit-ID: 5LJOupV8Pqr

Comment 12

[u480271]

Attachment: P6: Update from github repo support.

MozReview-Commit-ID: CG7srdz0MBq

Comment 13

[Ralph Giles (:rillian)]

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

[Ralph Giles (:rillian)]

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

[Ralph Giles (:rillian)]

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

[Ralph Giles (:rillian)]

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.

Comment 17

[u480271]

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

Comment 18

[u480271]

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

Comment 19

[u480271]

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

Comment 20

[u480271]

(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

[Ralph Giles (:rillian)]

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.

Comment 22

[u480271]

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.

Comment 23

[u480271]

Attachment: P2: Enable USE_PULSE_RUST in libcubeb.

MozReview-Commit-ID: 5ZQLFXNFWdB

Comment 24

[u480271]

Attachment: P3: Import rustified cubeb_pulse.c

MozReview-Commit-ID: EZXJAzN3X7c

Comment 25

[u480271]

Attachment: 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

Comment 26

[u480271]

Attachment: P5: Vendor semver rust crates.

Import semver and semver-parser creates needed by the cubeb
pulseaudio backend.

MozReview-Commit-ID: 5LJOupV8Pqr

Comment 27

[u480271]

Attachment: P6: Update from github repo support.

MozReview-Commit-ID: CG7srdz0MBq

Comment 28

[Jan Beich]

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.

Comment 29

[Jan Beich]

(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

[Ralph Giles (:rillian)]

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.

Comment 31

[u480271]

(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

[Ralph Giles (:rillian)]

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

Comment 33

[u480271]

(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?

Comment 34

[u480271]

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

Comment 35

[u480271]

Attachment: P2: Enable USE_PULSE_RUST in libcubeb.

MozReview-Commit-ID: 5ZQLFXNFWdB

Comment 36

[Ralph Giles (:rillian)]

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

Comment 37

[Jan Beich]

Comment on attachment 8859444 [details] [diff] [review]
P2: Enable USE_PULSE_RUST in libcubeb.

pulse-rust now works fine on FreeBSD. Thank you!

Comment 38

[u480271]

Try push with code compiling but backend disabled.

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

Comment 39

[u480271]

Attachment: P3: Import rustified cubeb_pulse.c

MozReview-Commit-ID: EZXJAzN3X7c

Comment 40

[u480271]

Attachment: 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

Comment 41

[u480271]

Attachment: P6: Update from github repo support.

MozReview-Commit-ID: CG7srdz0MBq

Comment 42

[Ralph Giles (:rillian)]

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! :-)

Comment 43

[Pulsebot]

Pushed by [e-mail]:
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

[Jan Beich]

(In reply to Pulsebot from comment #43)
> Pushed by [e-mail]:
> 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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29e1433cdec5
https://hg.mozilla.org/mozilla-central/rev/d7a79e8ceb58

Comment 46

[Jan Beich]

See comment 44.

Comment 47

[u480271]

(In reply to Jan Beich from comment #44)
> (In reply to Pulsebot from comment #43)
> > Pushed by [e-mail]:
> > 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.