-
Notifications
You must be signed in to change notification settings - Fork 20
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
WIP: Support for automated parameters #16
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
Any thoughts on this, @developit? |
Hey @martinholters! Sorry about the wait there - I read this but then forgot to come back to the tab. I like the approach and I think this is the right way to move forward. However, I have to wonder - do you think this would be worth pulling out into its own library as an Cheers! |
@@ -25,14 +25,180 @@ if (typeof AudioWorkletNode !== 'function') { | |||
const outputChannels = options && options.outputChannelCount ? options.outputChannelCount[0] : 2; | |||
const scriptProcessor = context.createScriptProcessor(undefined, 2, outputChannels); | |||
|
|||
const automationEvents = new Map(); | |||
|
|||
function SetValue (value, startTime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like these classes could get pulled out of AudioWorkletNode
so they're shared?
I'm not sure I fully understand, so let me check we're on the same page. It might make sense to pull this into a library which provides a way to construct an |
You described what I meant perfectly, and agreed regarding it not really needing to be a polyfill. Shouldn't block merging based on that goal though, I'll re-review (sorry for the delay!) |
Hi @martinholters, I just came across this thread. I recently created a very similar abstraction for my standardized-audio-context package. The little library is called automation-events. It maintains a list of automation events and provides a method to get the value of the hypothetical AudioParam at a certain point in time. Maybe that comes in handy for what you're planning to do. |
This is a proof-of-concept implementation picking up the idea from #15 (comment). It works like a charm for my limited use case (
exponentialRampToValueAtTime
andcancelAndHoldAtTime
only). I've tried to implement the whole spec (except for throwing on error conditions for now), and the complexity is somewhat alarming. I also haven't done much testing, so this is likely still rather buggy. Before investing more time to finish this, I'd appreciate some feedback:Fixes #15.