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

Implement AudioParam.setValueCurveAtTime #22897 #23259

Merged
merged 1 commit into from Apr 27, 2019

Conversation

Projects
None yet
7 participants
@Akhilesh1996
Copy link
Contributor

commented Apr 25, 2019

Updated audioparam.rs to send the new UserAutomationEvent SetValueCurveAtTime to the audio engine and updated test results.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22897 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 25, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented Apr 25, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/audioparam.rs, components/script/dom/webidls/AudioParam.webidl
  • @KiChjang: components/script/dom/audioparam.rs, components/script/dom/webidls/AudioParam.webidl
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@highfive highfive assigned Manishearth and unassigned jdm Apr 25, 2019

@@ -1,2 +1,124 @@
[audioparam-setValueCurveAtTime.html]
expected: ERROR
[X Max error for test 12 at offset 15876 is not less than or equal to 0.0000037194. Got 0.4649144411087036.]
expected: FAIL

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 25, 2019

Member

Most of these tests shouldn't be failing. Are you sure the servo-media code is doing the right thing?

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 25, 2019

Author Contributor

I was also confused about the same. We get the expected output while running the code from the servo/media directory as I printed out the values and time to verify. But I got similar amount of failures when I ran the tests for linearRampToValueAtTime, exponentialRampToValueAtTime etc so I thought this was common.

I also noticed that a lot of error were of the following type:
[X Discontinuity at index is not equal to 7938. Got 7940.]
expected: FAIL

[X Discontinuity at index is not equal to 6615. Got 6617.]
expected: FAIL

[X Discontinuity at index is not equal to 11907. Got 11909.]
expected: FAIL

[X Discontinuity at index is not equal to 9261. Got 9263.]
expected: FAIL
There is a mismatch by 2 for all of these.

Is there somewhere I can look to fix these or is this something I don't have to look into?

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 25, 2019

Member

This could be a timing synchronization bug which wouldn't be your fault.

You're right that most seem minor, though the Max error ones don't seem so minor.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 25, 2019

Member

Filed #23271, ignore this issue for now.

start_time: Finite<f64>,
end_time: Finite<f64>,
) -> DomRoot<AudioParam> {
self.message_node(AudioNodeMessage::SetParam(

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 25, 2019

Member

This needs to do a bunch of input validation, see the "description" column of the arguments table on https://webaudio.github.io/web-audio-api/#dom-audioparam-setvaluecurveattime

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 25, 2019

Author Contributor

For this, I based my code on the other processing algorithms that are already present in AudioParam.rs because they had the same kind of validation description. Can you give me some pointers or some example that I can look into so that I can fix the same?

Thanks.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 25, 2019

Member

You can see examples of such checks on https://github.com/ferjm/servo/blob/webaudio/components/script/dom/audionode.rs#L100

(Looks like the other AudioParam functions are missing validation too, I should add that)

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 25, 2019

Member

You can see examples of such checks on https://github.com/ferjm/servo/blob/webaudio/components/script/dom/audionode.rs#L100

(Looks like the other AudioParam functions are missing validation too, I should add that)

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 25, 2019

Member

do the same thing done in #23270

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 25, 2019

Author Contributor

I have added the validations and pushed the code.

Thanks.

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 26, 2019

Author Contributor

I think I have corrected the error, can you take a look now?

@Manishearth Manishearth changed the title Finished: Implement AudioParam.setValueCurveAtTime #22897 Implement AudioParam.setValueCurveAtTime #22897 Apr 25, 2019

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Make sure your code passes ./mach test-tidy

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Please squash your commits

@Akhilesh1996 Akhilesh1996 force-pushed the Akhilesh1996:master branch from 97485d3 to 9d4c4ef Apr 25, 2019

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

commented Apr 25, 2019

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

@Manishearth
Copy link
Member

left a comment

You seem to have included unrelated changes

0:06.39 INFO Starting http server on 127.0.0.1:8001
0:06.39 INFO Starting http server on 127.0.0.1:8000
0:06.40 INFO Starting https server on 127.0.0.1:8443
0:08.11 SUITE_START: web-platform-test - running 1 tests

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 25, 2019

Member

This file shouldn't be there

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 25, 2019

Author Contributor

I think I made a mistake while squashing commits. I will try to find where I went wrong

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 26, 2019

Member

This file still needs to be removed

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 26, 2019

Author Contributor

Done.

This comment has been minimized.

Copy link
@Akhilesh1996

Akhilesh1996 Apr 26, 2019

Author Contributor

Hi Manish,
Are any other changes needed for this?

@Akhilesh1996 Akhilesh1996 force-pushed the Akhilesh1996:master branch 3 times, most recently from 0a08419 to b270330 Apr 26, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

☔️ The latest upstream changes (presumably #23270) made this pull request unmergeable. Please resolve the merge conflicts.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

📌 Commit b270330 has been approved by Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

🌲 The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Oh, this pull request has merge conflicts, please rebase over master.

@Akhilesh1996

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Oh, this pull request has merge conflicts, please rebase over master.

Given that I'm working on the forked repo, and I don't have the changes that you made. I'm not sure how I can rebase. Can I just accommodate the changes you have made to AudioParam.webidl to my file manually so that the merge conflict is resolved?

Or there is a resolute conflicts button on this page, can I just edit in that? Will that work?

Thanks.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

git fetch origin
git rebase origin/master

git mergetool
git add
git rebase --continue

git push -f <forkname> <branchname>
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

/webaudio/idlharness.https.window.html has some new test passes that need to be incorporated, and the change to the result for /webaudio/the-audio-api/the-iirfilternode-interface/iirfilter.html should be reverted.

@jdm

This comment has been minimized.

@Akhilesh1996

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

/webaudio/idlharness.https.window.html has some new test passes that need to be incorporated, and the change to the result for /webaudio/the-audio-api/the-iirfilternode-interface/iirfilter.html should be reverted.

Thanks for the comments Josh.
I have reverted the result for IIRFilter. Now for the /webaudio/idlharness.https.window.html, I can run ./mach test-wpt tests/wpt/web-platform-tests/webaudio/idlharness.https.window.html and will updated the test results. Is that all? OR am I missing something?

@Akhilesh1996

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Along with the changes from https://build.servo.org/builders/linux-rel-css/builds/12060/steps/shell__4/logs/filtered-wpt-errorsummary.log.

Where should I incorporate these changes?

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

That log is showing test results for /webaudio/the-audio-api/the-audioparam-interface/audioparam-method-chaining.html and /webaudio/the-audio-api/the-audioparam-interface/audioparam-exceptional-values.html that need to be updated. That can happen the same way you described updating /webaudio/idlharness.https.window.html.

@Akhilesh1996 Akhilesh1996 force-pushed the Akhilesh1996:master branch from aed143b to b519a0b Apr 27, 2019

@Akhilesh1996

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

That log is showing test results for /webaudio/the-audio-api/the-audioparam-interface/audioparam-method-chaining.html and /webaudio/the-audio-api/the-audioparam-interface/audioparam-exceptional-values.html that need to be updated. That can happen the same way you described updating /webaudio/idlharness.https.window.html.

I have run the tests in the WebAudio folder and updated all the test results. Can you verify if its okay now?

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@bors-servo r=Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

📌 Commit b519a0b has been approved by Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

⌛️ Testing commit b519a0b with merge 06c8563...

bors-servo added a commit that referenced this pull request Apr 27, 2019

Auto merge of #23259 - Akhilesh1996:master, r=Manishearth
Implement AudioParam.setValueCurveAtTime #22897

<!-- Please describe your changes on the following line: -->
Updated audioparam.rs to send the new UserAutomationEvent SetValueCurveAtTime to the audio engine and updated test results.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #22897 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/23259)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

💔 Test failed - mac-rel-wpt3

@CYBAI

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

⌛️ Testing commit b519a0b with merge 10bee0d...

bors-servo added a commit that referenced this pull request Apr 27, 2019

Auto merge of #23259 - Akhilesh1996:master, r=Manishearth
Implement AudioParam.setValueCurveAtTime #22897

<!-- Please describe your changes on the following line: -->
Updated audioparam.rs to send the new UserAutomationEvent SetValueCurveAtTime to the audio engine and updated test results.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #22897 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/23259)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

@bors-servo bors-servo merged commit b519a0b into servo:master Apr 27, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.