Skip to content
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

Add AudioListener/AudioPanner DOM interfaces #21502

Merged
merged 12 commits into from Aug 25, 2018

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Aug 23, 2018

Seems to work.

I'll need some changes to the servo-media side to support the panner
node getters as well as the older setPosition()/etc APIs. I'll get to
those later.

r? @ferjm


This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/mod.rs, components/script/dom/audioscheduledsourcenode.rs, components/script/dom/audiodestinationnode.rs, components/script/dom/audionode.rs, components/script/dom/webidls/AudioListener.webidl and 5 more
  • @KiChjang: components/script/dom/mod.rs, components/script/dom/audioscheduledsourcenode.rs, components/script/dom/audiodestinationnode.rs, components/script/dom/audionode.rs, components/script/dom/webidls/AudioListener.webidl and 5 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 23, 2018
@highfive
Copy link

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@Manishearth Manishearth force-pushed the listener branch 2 times, most recently from 35b8104 to 9a52aac Compare August 23, 2018 22:13
@Manishearth
Copy link
Member Author

Tested, the following example works:

<script type="text/javascript">
let ctx = new AudioContext();
let osc = ctx.createOscillator();
let panner = new PannerNode(ctx, {"coneOuterAngle": 0, "positionX": 100, "positionY": 0, "positionZ": 100, "refDistance": 100, "rolloffFactor": 0.01});
osc.connect(panner);
panner.connect(ctx.destination);
osc.start();
panner.positionX.linearRampToValueAtTime(-100, 0.2);
panner.positionX.linearRampToValueAtTime(100, 0.2);
panner.positionX.linearRampToValueAtTime(-100, 0.4);
panner.positionX.linearRampToValueAtTime(-100, 0.4);
panner.positionX.linearRampToValueAtTime(100, 0.6);
panner.positionX.linearRampToValueAtTime(-100, 0.6);
panner.positionX.linearRampToValueAtTime(100, 0.8);
panner.positionX.linearRampToValueAtTime(100, 0.8);
panner.positionX.linearRampToValueAtTime(-100, 1.0);
panner.positionX.linearRampToValueAtTime(100, 1.0);
panner.positionX.linearRampToValueAtTime(-100, 1.2);
panner.positionX.linearRampToValueAtTime(-100, 1.2);
panner.positionX.linearRampToValueAtTime(100, 1.4);
panner.positionX.linearRampToValueAtTime(-100, 1.4);
panner.positionX.linearRampToValueAtTime(100, 1.6);
panner.positionX.linearRampToValueAtTime(100, 1.6);
</script>

(should I check this in to tests/html?)

@Manishearth
Copy link
Member Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit c62216d with merge 3dd46ec...

bors-servo pushed a commit that referenced this pull request Aug 23, 2018
Add AudioListener/AudioPanner DOM interfaces

Seems to work.

I'll need some changes to the servo-media side to support the panner
node getters as well as the older `setPosition()`/etc APIs. I'll get to
those later.

r? @ferjm

<!-- 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/21502)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 23, 2018
Copy link
Contributor

@ferjm ferjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

We need to add the channelCount and channelCountMode constraints for PannerNode.

Likewise, even though we do not support HRTF yet, we should also add the autiomationRate constraints for PannerNode and AudioListener AudioParams.

Thanks!

window,
context,
node,
ParamType::Position(ParamDir::Y),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ParamDir::Z

window,
context,
node,
ParamType::Forward(ParamDir::Y),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParamDir::Z

window,
context,
node,
ParamType::Up(ParamDir::Y),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParamDir::Z

@@ -297,6 +304,15 @@ impl BaseAudioContextMethods for BaseAudioContext {
})
}

/// https://webaudio.github.io/web-audio-api/#dom-baseaudiocontext-destination
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change link to https://webaudio.github.io/web-audio-api/#dom-baseaudiocontext-listener, please

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 24, 2018
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Aug 24, 2018
@Manishearth
Copy link
Member Author

We don't have autiomationrate constraints, the automationrate is simply ignored (this code already exists in the media crate)

@Manishearth
Copy link
Member Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit e7ed310 with merge 7196cdf...

bors-servo pushed a commit that referenced this pull request Aug 24, 2018
Add AudioListener/AudioPanner DOM interfaces

Seems to work.

I'll need some changes to the servo-media side to support the panner
node getters as well as the older `setPosition()`/etc APIs. I'll get to
those later.

r? @ferjm

<!-- 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/21502)
<!-- Reviewable:end -->
@Manishearth
Copy link
Member Author

This should cause a bunch of new WPT failures since previously some tests didn't fully run at all, but that's okay.

At some point we need to switch gears to WPT and whittle those down, most of them aren't much work.

@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 24, 2018
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 24, 2018
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 24, 2018
@Manishearth
Copy link
Member Author

@bors-servo r=ferjm

@bors-servo
Copy link
Contributor

📌 Commit 25332f0 has been approved by ferjm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 24, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit 25332f0 with merge e793d94...

bors-servo pushed a commit that referenced this pull request Aug 24, 2018
Add AudioListener/AudioPanner DOM interfaces

Seems to work.

I'll need some changes to the servo-media side to support the panner
node getters as well as the older `setPosition()`/etc APIs. I'll get to
those later.

r? @ferjm

<!-- 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/21502)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 24, 2018
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 24, 2018
@Manishearth
Copy link
Member Author

@bors-servo r=ferjm

  • needed to run update-manifest because I changed interfaces.html

@bors-servo
Copy link
Contributor

📌 Commit 04e60e6 has been approved by ferjm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 24, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit 04e60e6 with merge d827370...

bors-servo pushed a commit that referenced this pull request Aug 24, 2018
Add AudioListener/AudioPanner DOM interfaces

Seems to work.

I'll need some changes to the servo-media side to support the panner
node getters as well as the older `setPosition()`/etc APIs. I'll get to
those later.

r? @ferjm

<!-- 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/21502)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt1

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 25, 2018
@Manishearth
Copy link
Member Author

@bors-servo retry

{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "document.open should throw an InvalidStateError with XML document even when there is an active parser executing script", "test": "/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-exception-vs-return-xml.window.html", "line": 48140, "action": "test_result", "expected": "FAIL"}

seems intermittent, let's find out

@bors-servo
Copy link
Contributor

@bors-servo
Copy link
Contributor

@bors-servo bors-servo merged commit 04e60e6 into servo:master Aug 25, 2018
@bors-servo bors-servo mentioned this pull request Aug 25, 2018
3 tasks
@ferjm ferjm added this to Done in WebAudio Nov 29, 2018
@Manishearth Manishearth deleted the listener branch May 7, 2019 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-tests-failed The changes caused existing tests to fail.
Projects
Status: Done
WebAudio
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants