Skip to content

/ media

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 missing functionality of AudioBufferSourceNode #293

Merged
merged 6 commits into from Aug 20, 2019
Merged

Implement missing functionality of AudioBufferSourceNode #293

merged 6 commits into from Aug 20, 2019
+237 −53

Conversation

@collares
Copy link
Contributor

collares commented Aug 19, 2019 •
edited

This is most of the fix for Servo's #22363. It is broken into four pieces. Let me know if I can help in the reviewing process.
collares added 3 commits Jul 20, 2019
@collares
Track AudioBuffer's sample rate
2b8a444
@collares
AudioBufferSourceNode: Support playbackRate and looping
14fa409
@collares
AudioBufferSourceNode: Support start parameters (offset, duration)
21556e8
@collares collares mentioned this pull request Aug 19, 2019
Merged
4 of 5 tasks complete
@ferjm ferjm self-requested a review Aug 19, 2019
bors-servo added a commit to servo/servo that referenced this pull request Aug 19, 2019
@bors-servo
Auto merge of #23997 - collares:audiobuffersourcenode, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c4fedf1 
Implement missing functionality of AudioBufferSourceNode

<!-- Please describe your changes on the following line: -->
This is a tiny PR to support [Media #293](servo/media#293). Last time I ran WPT there was only one failing AudioBufferSourceNode test (which I will debug later this week). Once I rebased, however, I started getting unrelated servo-media build failures:

```
error[E0599]: no method named `shutdown_audio_context` found for type `std::sync::Arc<servo_media::ServoMedia>` in the current scope
   --> components/script/dom/baseaudiocontext.rs:566:14

error[E0599]: no method named `shutdown_player` found for type `std::sync::Arc<servo_media::ServoMedia>` in the current scope
    --> components/script/dom/htmlmediaelement.rs:1866:18
```

I wanted to get this PR up so servo/media#293 can be tested. When the unrelated build failures are fixed, I will run `./mach update-wpt` and update this PR.

---
<!-- 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` only reports unrelated errors (broken servo/media master?)
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #22363

<!-- 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/23997)
<!-- Reviewable:end -->
collares added 2 commits Aug 19, 2019
@collares
AudioBufferSourceNode: Subsample-accurate start times
Loading status checks…
bcd9d6a
@collares
Bugfix: Don't trigger onended prematurely if we are looping
Loading status checks…
3fe6ba1
@ferjm
ferjm approved these changes Aug 20, 2019
View changes
Copy link
Member

ferjm left a comment

Excellent work. Thanks!
audio/buffer_source_node.rs Show resolved Hide resolved
audio/buffer_source_node.rs Show resolved Hide resolved
audio/buffer_source_node.rs Outdated Show resolved Hide resolved
@ferjm
ferjm reviewed Aug 20, 2019
View changes
audio/buffer_source_node.rs Show resolved Hide resolved
@ferjm
ferjm reviewed Aug 20, 2019
View changes
audio/buffer_source_node.rs Outdated Show resolved Hide resolved
@collares
Address review comments, add loop messages
Loading status checks…
198f060
@collares
Copy link
Contributor Author

collares commented Aug 20, 2019 •
edited

Addressed review comments! There are two remaining relevant WPT failures:

  1. I have no idea what's happening with sub-sample-buffer-stitching.html. If I force the test to run (it timeouts on a dev build), we seem to be huge values at the end of the test (of the order of 1e20), but the test only contanis samples in the range [-1, 1]. I don't see how any part of my code could generate such large values.

  2. buffer-resampling.html fails because we interpolate the last sample with 0 when we output a sample corresponding to buffer.len() - 1 < buffer_pos < buffer.len(). The test wants us to hold the last sample constant for such values of buffer_pos, but it is unclear to me if this is the right thing to do.

If you are OK with the above failures, this PR is ready to be committed.
@ferjm
Copy link
Member

ferjm commented Aug 20, 2019

Thanks! Could you file follow-up issues for the remaining work (i.e. all the XXX comments that you added to the code) so we don't lose track of it, please?

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2019

📌 Commit 198f060 has been approved by ferjm
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2019

⌛ Testing commit 198f060 with merge 86b9a28...
bors-servo added a commit that referenced this pull request Aug 20, 2019
@bors-servo
Auto merge of #293 - collares:looping-playbackrate, r=ferjm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
86b9a28 
Implement missing functionality of AudioBufferSourceNode

This is most of the fix for [Servo's #22363](servo/servo#22363). It is broken into four pieces. Let me know if I can help in the reviewing process.
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2019

☀️ Test successful - checks-travis
Approved by: ferjm
Pushing 86b9a28 to master...
@bors-servo bors-servo merged commit 198f060 into servo:master Aug 20, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@collares collares deleted the collares:looping-playbackrate branch Aug 20, 2019
bors-servo added a commit to servo/servo that referenced this pull request Aug 21, 2019
@bors-servo
Auto merge of #23997 - collares:audiobuffersourcenode, r=ferjm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5cbef0f 
Implement missing functionality of AudioBufferSourceNode

<!-- Please describe your changes on the following line: -->
This is a tiny PR to support [Media #293](servo/media#293). Last time I ran WPT there was only one failing AudioBufferSourceNode test (which I will debug later this week). Once I rebased, however, I started getting unrelated servo-media build failures:

```
error[E0599]: no method named `shutdown_audio_context` found for type `std::sync::Arc<servo_media::ServoMedia>` in the current scope
   --> components/script/dom/baseaudiocontext.rs:566:14

error[E0599]: no method named `shutdown_player` found for type `std::sync::Arc<servo_media::ServoMedia>` in the current scope
    --> components/script/dom/htmlmediaelement.rs:1866:18
```

I wanted to get this PR up so servo/media#293 can be tested. When the unrelated build failures are fixed, I will run `./mach update-wpt` and update this PR.

---
<!-- 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` only reports unrelated errors (broken servo/media master?)
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #22363

<!-- 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/23997)
<!-- Reviewable:end -->
bors-servo added a commit to servo/servo that referenced this pull request Aug 22, 2019
@bors-servo
Auto merge of #23997 - collares:audiobuffersourcenode, r=ferjm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5832781 
Implement missing functionality of AudioBufferSourceNode

<!-- Please describe your changes on the following line: -->
This is a tiny PR to support [Media #293](servo/media#293). Last time I ran WPT there was only one failing AudioBufferSourceNode test (which I will debug later this week). Once I rebased, however, I started getting unrelated servo-media build failures:

```
error[E0599]: no method named `shutdown_audio_context` found for type `std::sync::Arc<servo_media::ServoMedia>` in the current scope
   --> components/script/dom/baseaudiocontext.rs:566:14

error[E0599]: no method named `shutdown_player` found for type `std::sync::Arc<servo_media::ServoMedia>` in the current scope
    --> components/script/dom/htmlmediaelement.rs:1866:18
```

I wanted to get this PR up so servo/media#293 can be tested. When the unrelated build failures are fixed, I will run `./mach update-wpt` and update this PR.

---
<!-- 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` only reports unrelated errors (broken servo/media master?)
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #22363

<!-- 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/23997)
<!-- Reviewable:end -->
bors-servo added a commit to servo/servo that referenced this pull request Aug 23, 2019
@bors-servo
Auto merge of #23997 - collares:audiobuffersourcenode, r=ferjm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
cacd4ed 
Implement missing functionality of AudioBufferSourceNode

<!-- Please describe your changes on the following line: -->
This is a tiny PR to support [Media #293](servo/media#293). Last time I ran WPT there was only one failing AudioBufferSourceNode test (which I will debug later this week). Once I rebased, however, I started getting unrelated servo-media build failures:

```
error[E0599]: no method named `shutdown_audio_context` found for type `std::sync::Arc<servo_media::ServoMedia>` in the current scope
   --> components/script/dom/baseaudiocontext.rs:566:14

error[E0599]: no method named `shutdown_player` found for type `std::sync::Arc<servo_media::ServoMedia>` in the current scope
    --> components/script/dom/htmlmediaelement.rs:1866:18
```

I wanted to get this PR up so servo/media#293 can be tested. When the unrelated build failures are fixed, I will run `./mach update-wpt` and update this PR.

---
<!-- 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` only reports unrelated errors (broken servo/media master?)
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #22363

<!-- 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/23997)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ferjm ferjm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@collares @ferjm @bors-servo
You can’t perform that action at this time.