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

StereoPannerNode backend #243

Merged
merged 1 commit into from Apr 30, 2019

Conversation

Projects
None yet
4 participants
@PurpleHairEngineer
Copy link
Contributor

commented Apr 27, 2019

Implemented the backend for StereoPannerNode in the media crate by creating a new node implementation using PannerNode as a guide according to the specification.

@PurpleHairEngineer PurpleHairEngineer changed the title Implemented StereoPannerNode backend StereoPannerNode backend Apr 27, 2019

@PurpleHairEngineer PurpleHairEngineer referenced this pull request Apr 27, 2019

Merged

StereoPannerNode DOM #23281

0 of 5 tasks complete
@Manishearth
Copy link
Member

left a comment

I don't think this compiles yet?

This will also need an example, something similar to params.rs except you use a StereoPannerNode instead of a GainNode, and you only need to do a SetValue plus some linear ramps on the pan param.

(with headphones you should hear the sound moving around)

Show resolved Hide resolved audio/stereo_panner_node.rs Outdated
Show resolved Hide resolved audio/stereo_panner_node.rs Outdated
Show resolved Hide resolved audio/stereo_panner_node.rs Outdated
Show resolved Hide resolved audio/stereo_panner_node.rs Outdated
Show resolved Hide resolved audio/stereo_panner_node.rs Outdated
@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@Manishearth Okay, I updated stereo_panner_node.rs to reflect the changes you suggested. I will make another commit once I have made the example.

@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

@Manishearth Okay, I added an example. As I said to the dev-servo email list, I am still having trouble running any examples on my computer, so I am not able to test my example. But it seems to compile using "cargo build". Please let me know if I need to change anything else.

Show resolved Hide resolved audio/stereo_panner_node.rs Outdated
Show resolved Hide resolved audio/stereo_panner_node.rs Outdated
@Manishearth
Copy link
Member

left a comment

Your code still doesn't build, please ensure that cargo build passes.

Currently you don't have a module entry for your file, once you add that to lib.rs then Rust will find your code and be able to compile it.

Show resolved Hide resolved audio/stereo_panner_node.rs Outdated
@Manishearth
Copy link
Member

left a comment

This still doesn't build, please ensure it builds first.

It's fine if you're still having trouble running it, but you shouldn't have any rust-side errors.

Show resolved Hide resolved audio/stereo_panner.rs Outdated
Show resolved Hide resolved examples/stereo_panner.rs Outdated
Show resolved Hide resolved audio/stereo_panner.rs Outdated
Show resolved Hide resolved audio/stereo_panner.rs Outdated
Show resolved Hide resolved audio/stereo_panner.rs
Show resolved Hide resolved audio/stereo_panner.rs Outdated
Show resolved Hide resolved audio/stereo_panner.rs Outdated
Show resolved Hide resolved audio/stereo_panner.rs Outdated
Show resolved Hide resolved audio/stereo_panner.rs Outdated
Show resolved Hide resolved audio/stereo_panner.rs
Show resolved Hide resolved audio/stereo_panner.rs Outdated
@Manishearth
Copy link
Member

left a comment

Still doesn't build, commented with some obvious errors but I suspect there are more.

Show resolved Hide resolved audio/stereo_panner.rs
Show resolved Hide resolved audio/stereo_panner.rs Outdated
@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@Manishearth Thank you. I've just been committing incrementally as I've been fixing the problems you mention. I'm sorry if you get a notification every time. It seems to build now so please take a look.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@Manishearth
Copy link
Member

left a comment

Make sure you address the warnings as well.

Show resolved Hide resolved audio/stereo_panner.rs Outdated
Show resolved Hide resolved audio/node.rs
Show resolved Hide resolved audio/stereo_panner.rs Outdated
@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@Manishearth I made the changes you suggested, but it looks like all the current warnings are either because an import was never used or because a variable does not need to be mutable. It looks like a lot of the warnings are outside the scope of the files I've changed.

@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@Manishearth It seems to build successfully now, and all the warnings are for files/parts unmodified by the committed code. Could you please take another look?

@Manishearth
Copy link
Member

left a comment

This works, thanks! Please run rustfmt on the files you added, and squash your commits. Let me know when you're done!

Show resolved Hide resolved examples/stereo_panner.rs
@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@Manishearth I am trying to squash my commits based on the instructions that were given in #242. However, when I run git rebase -i HEAD~15, I see a lot of additional commits from before I started committing (47 commands total). I am not sure how to handle this (what to do with them in the text editor).

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

It's because you merged master into this PR instead of rebasing over master. No big deal, I'll fix it.

@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@Manishearth Sorry! So I should squash them all? Or just do nothing? I ran rustfmt on the modified files like you asked.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Actually it's just the first merge commit, remove everything before "Implemented the backend for StereoPannerNode" in the rebase dialog before using fixup on everything after that commit

@PurpleHairEngineer PurpleHairEngineer force-pushed the josephhutch:StereoPannerNodeMedia branch from bdf592d to f76cde0 Apr 30, 2019

@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@Manishearth Okay, I did that. It stopped midway after I ran the fixup command and told me I had a merge conflict in Cargo.toml, which I thought I resolved. It seems to have squashed the commits, but now it says there are conflicts that must be resolved. I'm not sure how to fix this, since the Resolve Conflicts button is grayed out?

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Can you tick the "allow collaborators to push to this pull request" checkbox on the side of this PR? I've got it rebased.

@PurpleHairEngineer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@Manishearth The only checkbox I see is "Allow edits from maintainers", and it was already checked, but I just unchecked it and checked it again.

@Manishearth Manishearth force-pushed the josephhutch:StereoPannerNodeMedia branch from f76cde0 to 6ba5507 Apr 30, 2019

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Ah, i had the fork wrong. Fixed.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@bors-servo r+

Thanks!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

📌 Commit 6ba5507 has been approved by Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

⌛️ Testing commit 6ba5507 with merge 38a1f23...

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

Auto merge of #243 - josephhutch:StereoPannerNodeMedia, r=Manishearth
StereoPannerNode backend

Implemented the backend for StereoPannerNode in the media crate by creating a new node implementation using PannerNode as a guide according to the specification.
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

☀️ Test successful - checks-travis
Approved by: Manishearth
Pushing 38a1f23 to master...

@bors-servo bors-servo merged commit 6ba5507 into servo:master Apr 30, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

bors-servo added a commit to servo/servo that referenced this pull request Apr 30, 2019

Auto merge of #23281 - josephhutch:StereoPannerNode, r=<try>
StereoPannerNode DOM

<!-- Please describe your changes on the following line: -->

This is the DOM implementation for StereoPannerNode. Backend implemented in servo/media#243.

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

<!-- Either: -->
- [ ] 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/23281)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Apr 30, 2019

Auto merge of #23281 - josephhutch:StereoPannerNode, r=Manishearth
StereoPannerNode DOM

<!-- Please describe your changes on the following line: -->

This is the DOM implementation for StereoPannerNode. Backend implemented in servo/media#243.

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

<!-- Either: -->
- [ ] 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/23281)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Apr 30, 2019

Auto merge of #23281 - josephhutch:StereoPannerNode, r=Manishearth
StereoPannerNode DOM

<!-- Please describe your changes on the following line: -->

This is the DOM implementation for StereoPannerNode. Backend implemented in servo/media#243.

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

<!-- Either: -->
- [ ] 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/23281)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request May 1, 2019

Auto merge of #23281 - josephhutch:StereoPannerNode, r=Manishearth
StereoPannerNode DOM

<!-- Please describe your changes on the following line: -->

This is the DOM implementation for StereoPannerNode. Backend implemented in servo/media#243.

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

<!-- Either: -->
- [ ] 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/23281)
<!-- Reviewable:end -->
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.