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.

Sign up for GitHub

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

Jump to bottom

Implement AnalyserNode #21712

Merged
merged 8 commits into from Sep 19, 2018
Merged

Implement AnalyserNode #21712

merged 8 commits into from Sep 19, 2018

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Sep 14, 2018 •
edited

Needs servo/media#127 to land (and a dependency
update)

r? @ferjm

realtimeanalyser-fft-scaling.html, the test that actually checks for some level of FFT correctness, sadly doesn't work since we don't process nodes not connected to the destination.

However I locally fixed the test to work differently and it passed. We'll fix the processing model eventually.

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/analysernode.rs, components/script/dom/mod.rs, components/script/dom/webidls/AnalyserNode.webidl, components/script/dom/bindings/trace.rs, components/script/dom/baseaudiocontext.rs and 1 more
  • @KiChjang: components/script/dom/analysernode.rs, components/script/dom/mod.rs, components/script/dom/webidls/AnalyserNode.webidl, components/script/dom/bindings/trace.rs, components/script/dom/baseaudiocontext.rs and 1 more

@highfive
Copy link

warning Warning warning

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 14, 2018
@servo-wpt-sync servo-wpt-sync mentioned this pull request Sep 14, 2018
Open
@servo-wpt-sync
Copy link
Collaborator

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#85.

@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#85.

@Manishearth Manishearth mentioned this pull request Sep 14, 2018
Closed
@Manishearth
Copy link
Member Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit e92f144 with merge 46920a8...

bors-servo pushed a commit that referenced this pull request Sep 14, 2018
Auto merge of #21712 - Manishearth:analysernode, r=<try>
46920a8 
Implement AnalyserNode

<s>Needs servo/media#127 to land (and a dependency
update)</s>

r? @ferjm

realtimeanalyser-fft-scaling.html, the test that actually checks for some level of FFT correctness, sadly doesn't work since we don't process nodes not connected to the destination.

However I locally fixed the test to work differently and it passed. We'll fix the processing model eventually.

<!-- 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/21712)
<!-- Reviewable:end -->
@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#85.

@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 Sep 14, 2018
@Manishearth
Copy link
Member Author

@bors-servo try=wpt

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 14, 2018
@bors-servo
Copy link
Contributor

⌛ Trying commit 1bde2ef with merge a28dda0...

bors-servo pushed a commit that referenced this pull request Sep 14, 2018
Auto merge of #21712 - Manishearth:analysernode, r=<try>
a28dda0 
Implement AnalyserNode

<s>Needs servo/media#127 to land (and a dependency
update)</s>

r? @ferjm

realtimeanalyser-fft-scaling.html, the test that actually checks for some level of FFT correctness, sadly doesn't work since we don't process nodes not connected to the destination.

However I locally fixed the test to work differently and it passed. We'll fix the processing model eventually.

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

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 14, 2018
@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#85.

ferjm
ferjm approved these changes Sep 17, 2018
View reviewed changes
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.

r=me

(remove the patched servo-media before merging, please)

@Manishearth
Copy link
Member Author

@bors-servo r=ferjm

@bors-servo
Copy link
Contributor

📌 Commit 1bde2ef has been approved by ferjm

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

@bors-servo r=ferjm

missed a WPT update outside of the the-audio-api folder

@bors-servo
Copy link
Contributor

📌 Commit cc32e12 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 Sep 18, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit cc32e12 with merge 75a986e...

bors-servo pushed a commit that referenced this pull request Sep 18, 2018
Auto merge of #21712 - Manishearth:analysernode, r=ferjm
75a986e 
Implement AnalyserNode

<s>Needs servo/media#127 to land (and a dependency
update)</s>

r? @ferjm

realtimeanalyser-fft-scaling.html, the test that actually checks for some level of FFT correctness, sadly doesn't work since we don't process nodes not connected to the destination.

However I locally fixed the test to work differently and it passed. We'll fix the processing model eventually.

<!-- 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/21712)
<!-- Reviewable:end -->
@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#85.

@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt4

@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 Sep 18, 2018
@jdm
Copy link
Member

jdm commented Sep 18, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Testing commit cc32e12 with merge a02c43d...

bors-servo pushed a commit that referenced this pull request Sep 18, 2018
Auto merge of #21712 - Manishearth:analysernode, r=ferjm
a02c43d 
Implement AnalyserNode

<s>Needs servo/media#127 to land (and a dependency
update)</s>

r? @ferjm

realtimeanalyser-fft-scaling.html, the test that actually checks for some level of FFT correctness, sadly doesn't work since we don't process nodes not connected to the destination.

However I locally fixed the test to work differently and it passed. We'll fix the processing model eventually.

<!-- 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/21712)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Sep 18, 2018
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@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 Sep 18, 2018
@jdm
Copy link
Member

jdm commented Sep 18, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

⚡ Previous build results for android, android-x86, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-wpt1, mac-rel-wpt2...

@bors-servo
Copy link
Contributor

☀️ 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: ferjm
Pushing a02c43d to master...

@bors-servo bors-servo merged commit cc32e12 into servo:master Sep 19, 2018
@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1537316339325.

This was referenced Sep 19, 2018
Merged
Merged
jdm pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 20, 2018
@Manishearth @jdm
Set default channel count for AnalyserNode to 2
1d1d732 
This was deliberately changed
(WebAudio/web-audio-api#1397 ) but the tests
have not been updated

Upstreamed from servo/servo#21712 [ci skip]
@servo-wpt-sync servo-wpt-sync mentioned this pull request Sep 20, 2018
Merged
@Manishearth Manishearth deleted the analysernode branch October 1, 2018 07:51
@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
Reviewers

@ferjm ferjm ferjm approved these changes

Assignees

@ferjm ferjm

Labels
S-tests-failed The changes caused existing tests to fail.
Projects
WebAudio
Status: Done
WebAudio
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Manishearth @highfive @servo-wpt-sync @bors-servo @jdm @ferjm