New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebAudio API #21158

Merged
merged 87 commits into from Jul 31, 2018

Conversation

@ferjm
Member

ferjm commented Jul 10, 2018

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #6710
  • There are tests for these changes

This PR adds basic support for the WebAudio API using servo-media with GStreamer as the audio backend.

There are still some major stuff to fix like:

  • Detach ArrayBuffer during the AudioBuffer "acquire the content" operation. I am naively using JS_StealArrayBufferContents() directly, because it is what Gecko uses, but this should probably be part of the rust-mozjs TypedArray API. And, in any case, I am not even sure if that's the proper way to do it. According to the results of the WPTs it may not even be right since this assertion is failing in some cases. I need to dig more about this.
  • Disable the GStreamer dependency on Android. Unfortunately gstreamer-rs requires an NDK version upgrade, so we need to disable this for Android until then. I tried adding different features to servo-media, but I am currently hitting this issue

I still need to run servo-tidy, change the servo-media dependency to use the git repo and add/fix some comments and TODOs.

The remaining feature work should be done in future PRs.

Note that most of the failing WPTs are failing because we don't implement the tested features yet (we only implement a few AudioNodes) and we have no OfflineAudioContext support, which most WPTs rely on.


This change is Reviewable

@ferjm ferjm requested a review from nox Jul 10, 2018

@highfive

This comment has been minimized.

highfive commented Jul 10, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/periodicwave.rs, components/script/dom/webidls/OscillatorNode.webidl, components/script/Cargo.toml, components/script/dom/webidls/AudioBuffer.webidl, components/script/dom/mod.rs and 22 more
  • @jgraham: tests/wpt/include.ini
  • @KiChjang: components/script/dom/periodicwave.rs, components/script/dom/webidls/OscillatorNode.webidl, components/script/Cargo.toml, components/script/dom/webidls/AudioBuffer.webidl, components/script/dom/mod.rs and 22 more
@highfive

This comment has been minimized.

highfive commented Jul 10, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 10, 2018

r? @nox

@highfive highfive assigned nox and unassigned pcwalton Jul 10, 2018

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 10, 2018

We should use a git dependency for servo-media, not a path dependency.

Either that or use a versioned dep but I'm okay with sticking to git while we're still working on it.

For the servo media stuff, how about we set things via stuff like https://github.com/ferjm/servo/blob/d4ca6a43854116ad8ca3438ea311a65b2c4d6f60/components/script/Cargo.toml#L26 ?

@Manishearth

Reviewed 4 of 175 files at r1.
Reviewable status: 4 of 175 files reviewed, 11 unresolved discussions (waiting on @ferjm and @nox)


components/script/Cargo.toml, line 89 at r1 (raw file):

servo_config = {path = "../config"}
servo_geometry = {path = "../geometry" }
servo_media = {path = "../../../media/servo-media"}

Should be a git or path dep


components/script/lib.rs, line 16 at r1 (raw file):

#![allow(non_snake_case)]

#![recursion_limit = "128"]

This snuck in in one of my commits, we don't need it. This was an accident.


components/script/dom/audiobuffer.rs, line 26 at r1 (raw file):

pub struct AudioBuffer {
    reflector_: Reflector,
    js_channels: DomRefCell<Vec<JSAudioChannel>>,

@nox should have a look at the arraybuffer code in this file


components/script/dom/audiobuffer.rs, line 38 at r1 (raw file):

    #[allow(unrooted_must_root)]
    #[allow(unsafe_code)]
    pub fn new_inherited(cx: *mut JSContext,

safe functions should never take raw pointers. We should pass global here.


components/script/dom/audiobuffer.rs, line 86 at r1 (raw file):

    }

    pub fn Constructor(window: &Window,

spec link please


components/script/dom/audiobuffer.rs, line 89 at r1 (raw file):

                       options: &AudioBufferOptions) -> Fallible<DomRoot<AudioBuffer>> {
        if options.numberOfChannels > MAX_CHANNEL_COUNT {
            return Err(Error::NotSupported);

spec doesn't mention that we should error here?


components/script/dom/audiobuffer.rs, line 95 at r1 (raw file):

    #[allow(unsafe_code)]
    fn restore_js_channel_data(&self, cx: *mut JSContext) -> bool {

must be unsafe if we're operating on a raw pointer

(it is a safe operation to construct invalid raw pointers)

Though it's no big deal since this is private anywayn


components/script/dom/audiobuffersourcenode.rs, line 45 at r1 (raw file):

    #[allow(unrooted_must_root)]
    fn new_inherited(
        window: &Window,

nit: rustfmt this


components/script/dom/audiobuffersourcenode.rs, line 123 at r1 (raw file):

                let buffer = buffer.acquire_contents();
                self.source_node.node().message(
                    AudioNodeMessage::AudioBufferSourceNode(

this seems to be missing from the spec? perhaps we should add it.x


components/script/dom/audiocontext.rs, line 80 at r1 (raw file):

        if self.context.is_allowed_to_start() {
            // Step 6.
            self.context.resume();

should we be queueing a statechange event here?


components/script/dom/audiocontext.rs, line 140 at r1 (raw file):

                            atom!("statechange"),
                            &window
                            );

nit: rustfmt

@Manishearth

Reviewed 169 of 175 files at r1.
Reviewable status: 173 of 175 files reviewed, 22 unresolved discussions (waiting on @ferjm and @nox)


components/script/dom/audionode.rs, line 78 at r1 (raw file):

        if output >= self.NumberOfOutputs() ||
            input >= destination.NumberOfInputs() {

nit: rustfmt


components/script/dom/audionode.rs, line 82 at r1 (raw file):

            }

        // XXX Check previous connections.

nit: this is unnecessary, connect_ports handles this and the spec doesn't ask for an error


components/script/dom/audionode.rs, line 178 at r1 (raw file):

    fn SetChannelCountMode(&self, value: ChannelCountMode) -> ErrorResult {
        self.channel_count_mode.set(value);

we need to also message the nodes with SetChannelCountMode (you can message_node this), same for the count and interpretationx

And we must verify validity, we can do this by writing all the checks from https://webaudio.github.io/web-audio-api/#AudioNode-attributes, or we can have the SetChannelCountMode messages respond with an error state. I prefer the former.x


components/script/dom/audioparam.rs, line 78 at r1 (raw file):

    fn Value(&self) -> Finite<f32> {
        // XXX

can you file issues on servo-media for all these todos and link to them in the comment?


components/script/dom/audioscheduledsourcenode.rs, line 46 at r1 (raw file):

impl AudioScheduledSourceNodeMethods for AudioScheduledSourceNode {
    // https://webaudio.github.io/web-audio-api/#dom-audioscheduledsourcenode-onended
    event_handler!(ended, GetOnended, SetOnended);

we should dispatch this when we reach the end (or have a fixme comment for the same)


components/script/dom/audioscheduledsourcenode.rs, line 56 at r1 (raw file):

        self.node.message(
            AudioNodeMessage::AudioScheduledSourceNode(AudioScheduledSourceNodeMessage::Start(*when))
            );

nit: rustfmt


components/script/dom/baseaudiocontext.rs, line 55 at r1 (raw file):

#[derive(JSTraceable)]
#[allow(unrooted_must_root)]

I think this should be marked #[must_root]


components/script/dom/baseaudiocontext.rs, line 127 at r1 (raw file):

    }

    #[allow(unrooted_must_root)]

I'm not sure if we should be doing this?


components/script/dom/macros.rs, line 631 at r1 (raw file):

        handle_potential_webgl_error!($context, $call, ());
    };
}

nit: no newline at end of file


components/script/dom/periodicwave.rs, line 15 at r1 (raw file):

#[dom_struct]
pub struct PeriodicWave {

given that we don't yet support SetPeriodicWave perhaps we should just leave this unimplemented?


components/script/dom/webidls/PeriodicWave.webidl, line 5 at r1 (raw file):

 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
/*
 * The origin of this IDL file is

ditto re: unimplemented stuff should be removed

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 10, 2018

Reviewed everything except BaseAudioContext and AudioBuffer. I may start pushing fixes for some of these issues.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 10, 2018

Pushed some fixes for the basic stuff to this PR directly

@ferjm

This comment has been minimized.

Member

ferjm commented Jul 11, 2018

We should use a git dependency for servo-media, not a path dependency.

Yes. I mentioned this as pending task in the description :).

For the servo media stuff, how about we set things via stuff like https://github.com/ferjm/servo/blob/d4ca6a43854116ad8ca3438ea311a65b2c4d6f60/components/script/Cargo.toml#L26 ?

Yes, that was the plan. Ideally we should be able to do something like #20475. But since that's currently not possible, I wanted to set servo-media as a dependency with different features (to use the dummy or the gst backend) depending on the platform. But I hit this issue. I am trying to find a workaround. Setting default-features = false as suggested here does not work. Having different git branches with different default features does not work either. So I am afraid that I may end up:

  • creating a new servo-media-dummy crate with the dummy backend as the default one and using stuff from servo-media or servo-media-dummy depending on the platform, like:
#[cfg(not(target_os = "android"))]
use servo_media::audio::buffer_source_node::AudioBuffer as ServoMediaAudioBuffer;
#[cfg(target_os = "android")]
use servo_media_dummy::audio::buffer_source_node::AudioBuffer as ServoMediaAudioBuffer;
  • or messing up the entire patch with cfg clauses and unimplemented!() like:
#[cfg(not(target_os = "android"))]
pub fn Foo() {
  // Use servo-media stuff
}

#[cfg(target_os = "android")]
pub fn Foo() {
  unimplemented!()
}

I am really looking forward to hear about cleaner options :)

@ferjm

Reviewable status: 161 of 175 files reviewed, 16 unresolved discussions (waiting on @Manishearth, @ferjm, and @nox)


components/script/dom/audiobuffer.rs, line 89 at r1 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

spec doesn't mention that we should error here?

No, it doesn't. But I did what Gecko and Blink do for this case. This may be missing from the spec. What's the alternative, down-mixing in this case?


components/script/dom/audiobuffersourcenode.rs, line 123 at r1 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

this seems to be missing from the spec? perhaps we should add it.x

It's there, step 5.


components/script/dom/audiocontext.rs, line 80 at r1 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

should we be queueing a statechange event here?

BaseAudioContext.resume() takes care of that here


components/script/dom/audionode.rs, line 178 at r1 (raw file):

And we must verify validity, we can do this by writing all the checks from https://webaudio.github.io/web-audio-api/#AudioNode-attributes, or we can have the SetChannelCountMode messages respond with an error state. I prefer the former.x

Note that with the set of nodes that we currently implement the two only constraint that applies are the ones for OfflineAudioContext (which we can implement by elimination) and the one for channel count and AudioDestinationNodes. In the latter case the constraint was already enforced by the if condition at the top of the function. The error thrown was not correct for AudioDestinationNodes though, it should be IndexSize instead of NotSupported. I fixed that and I left comments in the code so we don't forget about the other nodes.


components/script/dom/baseaudiocontext.rs, line 127 at r1 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

I'm not sure if we should be doing this?

I took this pattern from the HTMLMediaElement implementation that has similar needs.

@Manishearth

Reviewable status: 157 of 173 files reviewed, 13 unresolved discussions (waiting on @Manishearth, @ferjm, and @nox)


components/script/dom/audiobuffer.rs, line 89 at r1 (raw file):

Previously, ferjm (Fernando Jiménez Moreno) wrote…

No, it doesn't. But I did what Gecko and Blink do for this case. This may be missing from the spec. What's the alternative, down-mixing in this case?

I think we should get this added to the spec then


components/script/dom/audionode.rs, line 178 at r1 (raw file):

Previously, ferjm (Fernando Jiménez Moreno) wrote…

And we must verify validity, we can do this by writing all the checks from https://webaudio.github.io/web-audio-api/#AudioNode-attributes, or we can have the SetChannelCountMode messages respond with an error state. I prefer the former.x

Note that with the set of nodes that we currently implement the two only constraint that applies are the ones for OfflineAudioContext (which we can implement by elimination) and the one for channel count and AudioDestinationNodes. In the latter case the constraint was already enforced by the if condition at the top of the function. The error thrown was not correct for AudioDestinationNodes though, it should be IndexSize instead of NotSupported. I fixed that and I left comments in the code so we don't forget about the other nodes.

Sounds good

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 11, 2018

servo/media#84 fixes it for android. Once merged, you'll have to make a minor change -- script/Cargo.toml will have to call it dependencies.servo-media with a dash

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 12, 2018

Okay, so one problem is that the travis builders are on trusty, and libgstreamer1.0-dev is pretty old (1.2.4) on that outdated distro.

Perhaps there's a PPA that we can get this from? I'm not sure how this works. cc @sdroege

@sdroege

This comment has been minimized.

sdroege commented Jul 12, 2018

Check the travis.yml of the gstreamer bindings. Basically a tarball with 1.14.1 binaries built for trusty. Not ideal but simple

@ferjm

Reviewable status: 156 of 176 files reviewed, 13 unresolved discussions (waiting on @Manishearth, @ferjm, and @nox)


components/script/dom/audiobuffer.rs, line 89 at r1 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

I think we should get this added to the spec then

Actually, it is there

.travis.yml Outdated
@@ -53,6 +59,9 @@ matrix:
- ccache
- libdbus-glib-1-dev
- libedit-dev
- libglib2.0-dev
- libxml2-dev
- libgtk-3-dev

This comment has been minimized.

@sdroege

sdroege Jul 13, 2018

You don't need libxml2 and gtk in your case, that's only needed for some of the (gtk-based) example applications in the GStreamer bindings repo :)

.travis.yml Outdated
@@ -17,7 +17,13 @@ matrix:
- sudo add-apt-repository 'deb http://apt.llvm.org/precise/ llvm-toolchain-precise-3.9 main' -y
- sudo apt-get update -q
- sudo apt-get install clang-3.9 llvm-3.9 llvm-3.9-runtime -y
- sudo apt-get install libgstreamer1.0-dev libgstreamer-plugins-base1.0-dev -y
- curl -L https://people.freedesktop.org/~slomo/gstreamer.tar.gz | tar xz

This comment has been minimized.

@sdroege

sdroege Jul 13, 2018

I would recommend hosting this yourself as I might update this at any point to a newer version as required for my own uses of it. But if there's an update it shouldn't really affect how it works so maybe that's acceptable for you

@sdroege

This comment has been minimized.

sdroege commented Jul 13, 2018

Disable the GStreamer dependency on Android. Unfortunately gstreamer-rs requires an NDK version upgrade, so we need to disable this for Android until then.

@ferjm Can you give some details why older NDK versions won't work and what exactly is needed?

@ferjm

This comment has been minimized.

Member

ferjm commented Jul 13, 2018

@ferjm Can you give some details why older NDK versions won't work and what exactly is needed?

I honestly don't remember the details of why I needed to upgrade from r12b to r16b to make gstreamer-rs build on Android. I need to revisit all that work soon, so I'll probably have more details then.

@ferjm ferjm force-pushed the ferjm:webaudio branch from 8cc7c2c to 2a6e95a Jul 13, 2018

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 13, 2018

Fixed the duplicate dependency thing, should be green on travis now

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 13, 2018

Oh uh we need to figure out Appveyor too. Though we can also disable that for now

@sdroege

This comment has been minimized.

sdroege commented Jul 13, 2018

Oh uh we need to figure out Appveyor too. Though we can also disable that for now

https://gstreamer.freedesktop.org/data/pkg/windows/1.14.1/ the two .msi files :)

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

⌛️ Testing commit 2daf9ff with merge e8554b5...

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 31, 2018

@bors-servo delegate+

bors-servo added a commit that referenced this pull request Jul 31, 2018

Auto merge of #21158 - ferjm:webaudio, r=manishearth,nox,ferjm
WebAudio API

- [X] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #6710
- [X] There are tests for these changes

This PR adds basic support for the WebAudio API using [servo-media](https://github.com/servo/media) with GStreamer as the audio backend.

There are still some major stuff to fix like:

- [x] Detach ArrayBuffer during the [AudioBuffer "acquire the content" operation](https://webaudio.github.io/web-audio-api/#acquire-the-content). I am naively using `JS_StealArrayBufferContents()` directly, because it is what Gecko uses, but this should probably be part of the [rust-mozjs](https://github.com/servo/rust-mozjs) [TypedArray](https://github.com/servo/rust-mozjs/blob/master/src/typedarray.rs) API. And, in any case, I am not even sure if that's the proper way to do it. According to the results of the WPTs it may not even be right since [this assertion](https://github.com/servo/rust-mozjs/blob/master/src/typedarray.rs#L285) is failing in some cases. I need to dig more about this.
- [x] Disable the GStreamer dependency on Android. Unfortunately gstreamer-rs requires an NDK version upgrade, so we need to disable this for Android until then. I tried adding [different features to servo-media](servo/media#79), but I am currently hitting [this issue](rust-lang/cargo#1197)

I still need to run servo-tidy, change the servo-media dependency to use the git repo and add/fix some comments and TODOs.

The remaining feature work should be done in future PRs.

Note that most of the failing WPTs are failing because we don't implement the tested features yet (we only implement a few AudioNodes) and we have no OfflineAudioContext support, which most WPTs rely on.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21158)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

✌️ @ferjm can now approve this pull request

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

💔 Test failed - android

@ferjm

This comment has been minimized.

Member

ferjm commented Jul 31, 2018

@bors-servo retry

  • Disk space issues?
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

💔 Test failed - android

@ferjm

This comment has been minimized.

Member

ferjm commented Jul 31, 2018

install: error writing '/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/apk/jniLibs/armeabi-v7a/libservo.so': No space left on device
install: failed to extend '/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/apk/jniLibs/armeabi-v7a/libservo.so': No space left on device
make: *** [/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/apk/jniLibs/armeabi-v7a/libservo.so] Error 1
make: *** Deleting file `/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/apk/jniLibs/armeabi-v7a/libservo.so'
@jdm

This comment has been minimized.

Member

jdm commented Jul 31, 2018

@bors-servo retry

  • cleaned up build machine
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

⌛️ Testing commit 2daf9ff with merge 0051597...

bors-servo added a commit that referenced this pull request Jul 31, 2018

Auto merge of #21158 - ferjm:webaudio, r=manishearth,nox,ferjm
WebAudio API

- [X] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #6710
- [X] There are tests for these changes

This PR adds basic support for the WebAudio API using [servo-media](https://github.com/servo/media) with GStreamer as the audio backend.

There are still some major stuff to fix like:

- [x] Detach ArrayBuffer during the [AudioBuffer "acquire the content" operation](https://webaudio.github.io/web-audio-api/#acquire-the-content). I am naively using `JS_StealArrayBufferContents()` directly, because it is what Gecko uses, but this should probably be part of the [rust-mozjs](https://github.com/servo/rust-mozjs) [TypedArray](https://github.com/servo/rust-mozjs/blob/master/src/typedarray.rs) API. And, in any case, I am not even sure if that's the proper way to do it. According to the results of the WPTs it may not even be right since [this assertion](https://github.com/servo/rust-mozjs/blob/master/src/typedarray.rs#L285) is failing in some cases. I need to dig more about this.
- [x] Disable the GStreamer dependency on Android. Unfortunately gstreamer-rs requires an NDK version upgrade, so we need to disable this for Android until then. I tried adding [different features to servo-media](servo/media#79), but I am currently hitting [this issue](rust-lang/cargo#1197)

I still need to run servo-tidy, change the servo-media dependency to use the git repo and add/fix some comments and TODOs.

The remaining feature work should be done in future PRs.

Note that most of the failing WPTs are failing because we don't implement the tested features yet (we only implement a few AudioNodes) and we have no OfflineAudioContext support, which most WPTs rely on.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21158)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

💔 Test failed - linux-rel-css

@ferjm

This comment has been minimized.

Member

ferjm commented Jul 31, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 31, 2018

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: manishearth,nox,ferjm
Pushing 0051597 to master...

@bors-servo bors-servo merged commit 2daf9ff into servo:master Jul 31, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@ferjm ferjm added this to Done in WebAudio Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment