Closed Bug 1391523 Opened 7 years ago Closed 7 years ago

Prototype cubeb client/server split for testing

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: u480271, Assigned: u480271)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 12 obsolete files)

125.74 KB, patch
Details | Diff | Splinter Review
3.42 KB, patch
Details | Diff | Splinter Review
72.19 KB, patch
Details | Diff | Splinter Review
2.17 KB, patch
Details | Diff | Splinter Review
5.85 KB, patch
Details | Diff | Splinter Review
2.96 MB, patch
rillian
: review+
Details | Diff | Splinter Review
8.96 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
u480271
: review+
Details
59 bytes, text/x-review-board-request
u480271
: review+
Details
59 bytes, text/x-review-board-request
u480271
: review+
Details
59 bytes, text/x-review-board-request
u480271
: review+
Details
59 bytes, text/x-review-board-request
u480271
: review+
Details
59 bytes, text/x-review-board-request
u480271
: review+
Details
Contains prototype of cubeb remoting using client/server split + communication of data callbacks via shmem.

Is not ready for release.

Can be turned on by compiling with:

MOZCONFIG: ac_add_options --enable-cubeb-remoting
prejs.js: user_pref("media.cubeb.sandbox", true);
Assignee: nobody → dglastonbury
Blocks: 1362223
Status: NEW → ASSIGNED
The idea is to land this so other people can try out the feature and help shake out and fix bugs.
Attached patch P1: Import of cubeb-rs (obsolete) — Splinter Review
MozReview-Commit-ID: GoqaGftNNLT
Attachment #8898608 - Flags: review?(kinetik)
Attached patch P2: Remove cubeb build steps. (obsolete) — Splinter Review
MozReview-Commit-ID: Dt9eV5ONGlI
Attachment #8898609 - Flags: review?(kinetik)
Attached patch P3: Import audioipc crates. (obsolete) — Splinter Review
MozReview-Commit-ID: Ci2xbenAd8i
Attachment #8898610 - Flags: review?(kinetik)
MozReview-Commit-ID: KN7FLoVsJW5
Attachment #8898611 - Flags: review?(kinetik)
Attached patch P5: Compile in audioipc crates (obsolete) — Splinter Review
MozReview-Commit-ID: 6BC0fu1rrEE
Enable client/server using: media.cubeb.sandbox = true

This pref is only read at start up, so requires restarting firefox for
a change in value to take effect.

MozReview-Commit-ID: 36L4vR5QEDJ
Attachment #8898613 - Flags: review?(jyavenard)
--enable-cubeb-remoting

MozReview-Commit-ID: 7xTShZIOeRI
MozReview-Commit-ID: gRgeFhQV4G
Attachment #8898612 - Flags: review?(giles)
Attachment #8898614 - Flags: review?(giles)
Attachment #8898615 - Flags: review?(giles)
(In reply to Dan Glastonbury :kamidphish from comment #7)
> Created attachment 8898613 [details] [diff] [review]
> P6: Integrate audioipc into CubebUtils.
> 
> Enable client/server using: media.cubeb.sandbox = true
> 
> This pref is only read at start up, so requires restarting firefox for
> a change in value to take effect.
> 
> MozReview-Commit-ID: 36L4vR5QEDJ

Requesting review from :jya because we discussed when to read prefs during start up and he worked on MediaPrefs.
Attachment #8898613 - Flags: review?(padenot)
Hi Nathan, 

Can you review the rust build system changes for Ralph (who is out on PTO), or recommend someone else?

thanks,
Dan
Flags: needinfo?(nfroyd)
Blocks: 1390385
--enable-cubeb-remoting

MozReview-Commit-ID: 7xTShZIOeRI
Attachment #8898644 - Flags: review?(giles)
Attachment #8898614 - Attachment is obsolete: true
Attachment #8898614 - Flags: review?(giles)
Comment on attachment 8898613 [details] [diff] [review]
P6: Integrate audioipc into CubebUtils.

Review of attachment 8898613 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/CubebUtils.cpp
@@ +246,5 @@
>        sCubebBackendName[value.Length()] = 0;
>      }
> +  } else if (strcmp(aPref, PREF_CUBEB_SANDBOX) == 0) {
> +    bool cubebSandbox = false;
> +    Preferences::GetBool(PREF_CUBEB_SANDBOX, &cubebSandbox);

Can't you use MediaPref here?
I see that to read that pref you do so later. Why not here too?

@@ +388,5 @@
>  
> +  bool cubebSandbox = MediaPrefs::CubebSandbox();
> +  MOZ_LOG(gCubebLog, LogLevel::Info, ("%s: %s", PREF_CUBEB_SANDBOX, cubebSandbox ? "true" : "false"));
> +
> +  int rv = (cubebSandbox)

unecessary parenthesis.
Attachment #8898613 - Flags: review?(jyavenard) → review+
Comment on attachment 8898615 [details] [diff] [review]
P8: cargo update -p gkrust-shared && ./mach vendor rust

Review of attachment 8898615 [details] [diff] [review]:
-----------------------------------------------------------------

This patch has a bunch of rlibs and compilation artifacts in it; is that an artifact of the development process, or is that really how the upstream package was released?

Either way, it needs to be addressed before this patch can be committed.
Attachment #8898615 - Flags: review?(giles) → review-
Comment on attachment 8898612 [details] [diff] [review]
P5: Compile in audioipc crates

Review of attachment 8898612 [details] [diff] [review]:
-----------------------------------------------------------------

These changes are not bad, but I guess all the bits commented on below actually appear in P7.  It'd be better to have changes that go together all in the same patch.

::: toolkit/library/gtest/rust/Cargo.toml
@@ +7,5 @@
>  
> +[dependencies]
> +mp4parse-gtest = { path = "../../../../dom/media/gtest" }
> +nsstring-gtest = { path = "../../../../xpcom/rust/nsstring/gtest" }
> +gkrust-shared = { path = "../../rust/shared" }

Did these get moved accidentally, or semi-deliberately?  I don't know that they need to be moved around...

@@ +13,5 @@
>  [features]
>  bindgen = ["gkrust-shared/bindgen"]
>  servo = ["gkrust-shared/servo"]
>  quantum_render = ["gkrust-shared/quantum_render"]
> +cubeb-remoting = ["gkrust-shared/cubeb-remoting"]

This feature is introduced here...

::: toolkit/library/rust/shared/Cargo.toml
@@ +15,5 @@
>  cubeb-pulse = { path = "../../../../media/libcubeb/cubeb-pulse-rs", optional = true, features=["pulse-dlopen"] }
>  encoding_c = "0.7.1"
>  encoding_glue = { path = "../../../../intl/encoding_glue" }
> +audioipc-client = { path = "../../../../media/audioipc/client" }
> +audioipc-server = { path = "../../../../media/audioipc/server" }

...but these crates are not optional, nor is the feature declared below...

::: toolkit/library/rust/shared/lib.rs
@@ +15,5 @@
>  extern crate cubeb_pulse;
>  extern crate encoding_c;
>  extern crate encoding_glue;
> +extern crate audioipc_client;
> +extern crate audioipc_server;

...nor are these crates conditionally included (compare the cubeb_pulse crate above).
Attachment #8898612 - Flags: review?(giles) → review-
Comment on attachment 8898644 [details] [diff] [review]
P7: Enable cubeb remoting through configure.

Review of attachment 8898644 [details] [diff] [review]:
-----------------------------------------------------------------

This is the right idea, but I think almost everything in this patch needs to be shuffled around.

::: dom/media/CubebUtils.cpp
@@ +60,5 @@
>  namespace mozilla {
>  
>  namespace {
>  
> +#ifdef MOZ_CUBEB_REMOTING

I have no comment on whether the changes to this file are OK; you may want to have :kinetik review these?  Should these changes have been in P6 anyway?

::: toolkit/library/rust/Cargo.toml
@@ +13,5 @@
>  gecko_debug = ["gkrust-shared/gecko_debug"]
>  simd-accel = ["gkrust-shared/simd-accel"]
>  no-static-ideograph-encoder-tables = ["gkrust-shared/no-static-ideograph-encoder-tables"]
>  # parallel-utf8 = ["gkrust-shared/parallel-utf8"]
> +cubeb-remoting = ["gkrust-shared/cubeb-remoting"]

This change, the other Cargo.toml change, and the lib.rs change should probably go in P5.

::: toolkit/moz.configure
@@ +1250,5 @@
> +    if value:
> +        return True
> +
> +set_config('MOZ_CUBEB_REMOTING', cubeb_remoting)
> +set_define('MOZ_CUBEB_REMOTING', cubeb_remoting)

It is a little weird to have the configure option for controlling the compilation of code be introduced in the patch after said code has already been committed, especially since P6 (at least) contains references to MOZ_CUBEB_REMOTING.  Is it possible that the patches could be reordered so bisecting would work correctly?
Attachment #8898644 - Flags: review?(giles) → review-
(In reply to Dan Glastonbury :kamidphish from comment #11)
> Can you review the rust build system changes for Ralph (who is out on PTO)

Done.  Please feel free to tag me for the reviews after the revisions are made if Ralph's not back yet.
Flags: needinfo?(nfroyd)
I'm back online. Although not in my usual timezone, I'm still happy to do reviews.
Comment on attachment 8898608 [details] [diff] [review]
P1: Import of cubeb-rs

Review of attachment 8898608 [details] [diff] [review]:
-----------------------------------------------------------------

We'll need to come up with a script (or some other solution) to update this over time, but that can be done later.
Attachment #8898608 - Flags: review?(kinetik) → review+
Comment on attachment 8898609 [details] [diff] [review]
P2: Remove cubeb build steps.

Review of attachment 8898609 [details] [diff] [review]:
-----------------------------------------------------------------

We'll have to make the update.sh script apply this on each import once we have a script.
Attachment #8898609 - Flags: review?(kinetik) → review+
Attachment #8898610 - Flags: review?(kinetik) → review+
Attachment #8898611 - Flags: review?(kinetik) → review+
(In reply to Nathan Froyd [:froydnj] from comment #17)
> (In reply to Dan Glastonbury :kamidphish from comment #11)
> > Can you review the rust build system changes for Ralph (who is out on PTO)
> 
> Done.  Please feel free to tag me for the reviews after the revisions are
> made if Ralph's not back yet.

Thanks for the review, Nathan. I'll fold the patches as you suggest.  I had planned to fold patches together for landing but left them as is for review.
(In reply to Nathan Froyd [:froydnj] from comment #14)
> Comment on attachment 8898615 [details] [diff] [review]
> P8: cargo update -p gkrust-shared && ./mach vendor rust
> 
> Review of attachment 8898615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch has a bunch of rlibs and compilation artifacts in it; is that an
> artifact of the development process, or is that really how the upstream
> package was released?
> 
> Either way, it needs to be addressed before this patch can be committed.

This is literal the output of running ./mach vendor rust turned into a patch, so it's whatever is served up by crates.io.

Ralph, do you have any suggestions on how to proceed?  Do I manually exclude the compilation artifacts in third_party/?
Flags: needinfo?(giles)
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> ::: dom/media/CubebUtils.cpp
> @@ +246,5 @@
> >        sCubebBackendName[value.Length()] = 0;
> >      }
> > +  } else if (strcmp(aPref, PREF_CUBEB_SANDBOX) == 0) {
> > +    bool cubebSandbox = false;
> > +    Preferences::GetBool(PREF_CUBEB_SANDBOX, &cubebSandbox);
> 
> Can't you use MediaPref here?
> I see that to read that pref you do so later. Why not here too?

I'll give calling MediaPrefs::CubebSandbox() here.

I wasn't sure.  I had to put this code into PrefChanged and call RegisterCallbackAndCall because:
a) if the pref is globally enabled (eg. via all.js) the value is available and no change is received,
b) if the pref isn't defined globally (eg. via users pref.js) the value isn't available and a pref change
   is received when the user's profile directory is read.

Can I handle pref changes via MediaPrefs?
MozReview-Commit-ID: 6BC0fu1rrEE
Attachment #8899314 - Flags: review?(giles)
Attachment #8898612 - Attachment is obsolete: true
Attachment #8898644 - Attachment is obsolete: true
Fold this patch into P5 before landing.

MozReview-Commit-ID: gRgeFhQV4G
Attachment #8898615 - Attachment is obsolete: true
Enable client/server using: media.cubeb.sandbox = true

This pref is only read at start up, so requires restarting firefox for
a change in value to take effect.

MozReview-Commit-ID: 36L4vR5QEDJ
Attachment #8899316 - Flags: review?(padenot)
Attachment #8898613 - Attachment is obsolete: true
Attachment #8898613 - Flags: review?(padenot)
(In reply to Matthew Gregan [:kinetik] from comment #19)
> Comment on attachment 8898608 [details] [diff] [review]
> P1: Import of cubeb-rs
> 
> Review of attachment 8898608 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We'll need to come up with a script (or some other solution) to update this
> over time, but that can be done later.

Yup.  I'll do that in a follow up patch once we get this one landed.
Comment on attachment 8899316 [details] [diff] [review]
P7: Integrate audioipc into CubebUtils. r?jya,padenot

Review of attachment 8899316 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/CubebUtils.cpp
@@ +394,5 @@
> +  MOZ_LOG(gCubebLog, LogLevel::Info, ("%s: %s", PREF_CUBEB_SANDBOX, cubebSandbox ? "true" : "false"));
> +
> +  int rv = cubebSandbox
> +    ? audioipc_client_init(&sCubebContext, sBrandName)
> +    : cubeb_init(&sCubebContext, sBrandName, sCubebBackendName.get());

This silently falls back to in process audio, right? Is that the right policy for testing?
Attachment #8899316 - Flags: review?(padenot) → review+
(In reply to Dan Glastonbury :kamidphish from comment #22)

> This is literal the output of running ./mach vendor rust turned into a
> patch, so it's whatever is served up by crates.io.
> 
> Ralph, do you have any suggestions on how to proceed?  Do I manually exclude
> the compilation artifacts in third_party/?

Manually removing them is the thing to do for this patch, but we should figure out where they came from. `mach vendor` is supposed to add everything that changes in third_party/rust. And warn if there are any outstanding changes before you start. However `cargo package` doesn't have good hygene around what's included in the crates.io upload, so it's pretty easy for extra files in the local repository directory to end up in the published crate.
Flags: needinfo?(giles)
Attachment #8899314 - Flags: review?(giles) → review+
MozReview-Commit-ID: GoqaGftNNLT
Attachment #8898608 - Attachment is obsolete: true
MozReview-Commit-ID: Dt9eV5ONGlI
Attachment #8898609 - Attachment is obsolete: true
MozReview-Commit-ID: Ci2xbenAd8i
Attachment #8898610 - Attachment is obsolete: true
MozReview-Commit-ID: KN7FLoVsJW5
Attachment #8898611 - Attachment is obsolete: true
MozReview-Commit-ID: 6BC0fu1rrEE
Attachment #8899314 - Attachment is obsolete: true
Fold this patch into P5 before landing.

MozReview-Commit-ID: gRgeFhQV4G
Attachment #8900137 - Flags: review?(giles)
Attachment #8899315 - Attachment is obsolete: true
Enable client/server using: media.cubeb.sandbox = true

This pref is only read at start up, so requires restarting firefox for
a change in value to take effect.

MozReview-Commit-ID: 36L4vR5QEDJ
Attachment #8899316 - Attachment is obsolete: true
Rank: 15
Priority: -- → P1
(In reply to Dan Glastonbury :kamidphish from comment #23)
> (In reply to Jean-Yves Avenard [:jya] from comment #13)
> > ::: dom/media/CubebUtils.cpp
> > @@ +246,5 @@
> > >        sCubebBackendName[value.Length()] = 0;
> > >      }
> > > +  } else if (strcmp(aPref, PREF_CUBEB_SANDBOX) == 0) {
> > > +    bool cubebSandbox = false;
> > > +    Preferences::GetBool(PREF_CUBEB_SANDBOX, &cubebSandbox);
> > 
> > Can't you use MediaPref here?
> > I see that to read that pref you do so later. Why not here too?
> 
> I'll give calling MediaPrefs::CubebSandbox() here.
> 
> I wasn't sure.  I had to put this code into PrefChanged and call
> RegisterCallbackAndCall because:
> a) if the pref is globally enabled (eg. via all.js) the value is available
> and no change is received,
> b) if the pref isn't defined globally (eg. via users pref.js) the value
> isn't available and a pref change
>    is received when the user's profile directory is read.
> 
> Can I handle pref changes via MediaPrefs?

I'm confused.

This is already what MediaPrefs does: register a callback and get modifications. So a MediaPref can be read on any threads, and you're guaranteed to have the latest value.
Comment on attachment 8900137 [details] [diff] [review]
P6: cargo update -p gkrust-shared && ./mach vendor rust. r?rillian

Review of attachment 8900137 [details] [diff] [review]:
-----------------------------------------------------------------

The moves the current copy of backtrace to bactrace-sys-0.1.10, gcc to gcc-0.3.42, and rustc-demangle to rustc-demangle-0.1.4 to maintain geckodriver's deps while updating to the latest for cubeb. It would be nice to not do that. Can you please see if geckodriver (r?ato) can update at the same time, or if your code will work with the currently-vendored releases and update both later? The minor versions match so they're probably compatible. Geckodriver changes would be r?ato.

The audioipc-server crate depends on both lazycell 0.4 and mio, which in turn depends on lazycell 0.5, so this patch vendors both versions. The audioipc crate should be updates to use the same version as mio, but it's a different minor version so some source changes may be necessary. If so, that can happen in a follow-up bug.

Otherwise, LGTM.
Attachment #8900137 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) | needinfo me from comment #38)
> Comment on attachment 8900137 [details] [diff] [review]
> P6: cargo update -p gkrust-shared && ./mach vendor rust. r?rillian
> 
> Review of attachment 8900137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The moves the current copy of backtrace to bactrace-sys-0.1.10, gcc to
> gcc-0.3.42, and rustc-demangle to rustc-demangle-0.1.4 to maintain
> geckodriver's deps while updating to the latest for cubeb. It would be nice
> to not do that. Can you please see if geckodriver (r?ato) can update at the
> same time, or if your code will work with the currently-vendored releases
> and update both later? The minor versions match so they're probably
> compatible. Geckodriver changes would be r?ato.
> 
> The audioipc-server crate depends on both lazycell 0.4 and mio, which in
> turn depends on lazycell 0.5, so this patch vendors both versions. The
> audioipc crate should be updates to use the same version as mio, but it's a
> different minor version so some source changes may be necessary. If so, that
> can happen in a follow-up bug.
> 
> Otherwise, LGTM.

Thanks Ralph, I'll look at trying to sort out the crate versions to match what is already in tree.
(In reply to Ralph Giles (:rillian) | needinfo me from comment #38)
> Comment on attachment 8900137 [details] [diff] [review]
> P6: cargo update -p gkrust-shared && ./mach vendor rust. r?rillian
> 
> Review of attachment 8900137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The moves the current copy of backtrace to bactrace-sys-0.1.10, gcc to
> gcc-0.3.42, and rustc-demangle to rustc-demangle-0.1.4 to maintain
> geckodriver's deps while updating to the latest for cubeb. It would be nice
> to not do that. Can you please see if geckodriver (r?ato) can update at the
> same time, or if your code will work with the currently-vendored releases
> and update both later? The minor versions match so they're probably
> compatible. Geckodriver changes would be r?ato.
> 
> The audioipc-server crate depends on both lazycell 0.4 and mio, which in
> turn depends on lazycell 0.5, so this patch vendors both versions. The
> audioipc crate should be updates to use the same version as mio, but it's a
> different minor version so some source changes may be necessary. If so, that
> can happen in a follow-up bug.
> 
> Otherwise, LGTM.

Ralph,

I fixed up issues with versions here by hand-editing toolkit/library/rust/Cargo.lock and toolkit/library/gtest/rust/Cargo.lock to use the same versions as geckodriver.

Interesting that geckodriver has:
backtrace 0.3.2
backtrace-sys 0.1.10

and gkrust had:
backtrace 0.3.2
backtrace-sys 0.1.12

(backtrace is the root dep pulling in backtrace-sys, gcc, and rustc-demangle)

I rolled mio back to version 0.6.9 to avoid pulling in lazycell 0.5.1.
Flags: needinfo?(giles)
That sounds better, thanks. Maybe there is some better way to do these updates, since some deps seem to vary already without pulling separate versions.
Flags: needinfo?(giles)
I uploaded patches to mozreview to enable auto-land.  Doesn't look like it carries r= from the commit messages. I'm going to r=me to carry the r+s from all previous reviews.
Attachment #8901039 - Flags: review?(kinetik) → review+
Attachment #8901040 - Flags: review?(kinetik) → review+
Attachment #8901041 - Flags: review?(kinetik) → review+
Attachment #8901042 - Flags: review?(kinetik) → review+
Attachment #8901043 - Flags: review?(giles) → review+
Attachment #8901044 - Flags: review?(padenot)
Attachment #8901044 - Flags: review?(jyavenard)
Attachment #8901044 - Flags: review+
Attachment #8901039 - Flags: review+ → review?(dglastonbury)
Attachment #8901040 - Flags: review+ → review?(dglastonbury)
Attachment #8901041 - Flags: review+ → review?(dglastonbury)
Attachment #8901042 - Flags: review+ → review?(dglastonbury)
Attachment #8901043 - Flags: review+ → review?(dglastonbury)
Attachment #8901044 - Flags: review?(dglastonbury)
Attachment #8901044 - Flags: review+
Comment on attachment 8901039 [details]
Bug 1391523 - P1: Import of cubeb-rs.

https://reviewboard.mozilla.org/r/172508/#review177794

Carry r=kinetik.
Attachment #8901039 - Flags: review?(dglastonbury) → review+
Comment on attachment 8901040 [details]
Bug 1391523 - P2: Remove cubeb build steps.

https://reviewboard.mozilla.org/r/172510/#review177798

Carry r=kinetik.
Attachment #8901040 - Flags: review?(dglastonbury) → review+
Comment on attachment 8901041 [details]
Bug 1391523 - P3: Import audioipc crates.

https://reviewboard.mozilla.org/r/172512/#review177802

Carry r=kinetik.
Attachment #8901041 - Flags: review?(dglastonbury) → review+
Comment on attachment 8901042 [details]
Bug 1391523 - P4: Adjust audioipc to refer to in-tree cubeb-rs.

https://reviewboard.mozilla.org/r/172514/#review177804

Carry r=kinetik.
Attachment #8901042 - Flags: review?(dglastonbury) → review+
Comment on attachment 8901043 [details]
Bug 1391523 - P5: Compile in audioipc crates.

https://reviewboard.mozilla.org/r/172516/#review177806

Carry r=rillian.
Attachment #8901043 - Flags: review?(dglastonbury) → review+
Comment on attachment 8901044 [details]
Bug 1391523 - P6: Integrate audioipc into CubebUtils.

https://reviewboard.mozilla.org/r/172518/#review177808

Carry r=jya,padenot.
Attachment #8901044 - Flags: review?(dglastonbury) → review+
Pushed by dglastonbury@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93b668b34f1c
P1: Import of cubeb-rs. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/405ced5bce0b
P2: Remove cubeb build steps. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/a39241b3e7b1
P3: Import audioipc crates. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/6518b242a2a3
P4: Adjust audioipc to refer to in-tree cubeb-rs. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/bdae736da908
P5: Compile in audioipc crates. r=kamidphish
https://hg.mozilla.org/integration/autoland/rev/ce8749002d07
P6: Integrate audioipc into CubebUtils. r=kamidphish
See Also: → 1410704
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: