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

Update AudioParam automation rate #88

Closed
orottier opened this issue Jan 10, 2022 · 5 comments · Fixed by #169
Closed

Update AudioParam automation rate #88

orottier opened this issue Jan 10, 2022 · 5 comments · Fixed by #169
Assignees

Comments

@orottier
Copy link
Owner

https://www.w3.org/TR/webaudio/#dom-audioparam-automationrate

The automation rate, barring some constraints, can be changed by the control thread.

  • change AudioParam automation_rate to something like an AtomicBool
  • add set_automation_rate to impl AudioParam
  • how to deal with constraints? We can panic for now, but where do we encode this logic?
  • fetch automation_rate in the Renderer and use it accordingly
  • reconsider signature of AudioParamValues.get(..). It now returns a slice of full render quantum length always. But the spec says it can be length 1 too (for k-rate and for constant pieces) [1]. For type safety we can also make it return an enum.

[1] https://www.w3.org/TR/webaudio/#audioworkletprocess-callback-parameters

@orottier orottier added this to the Version 1.0 milestone Jan 10, 2022
@orottier
Copy link
Owner Author

Marking it a prerequisite for v1.0, but only because of the possible breaking API change in the last bullet

@b-ma
Copy link
Collaborator

b-ma commented Jan 11, 2022

My two cents:

  • add set_automation_rate to impl AudioParam
  • how to deal with constraints? We can panic for now, but where do we encode this logic?

Maybe a new field in AudioParamOptions that would optionally contain a coercion Fn would be quite clean? e.g.:

// node.rs
let param_options = {
   // ...
  constraint: Some(|rate: AutomationRate| -> AutomationRate { ... })
}

// param.rs
set_automation_rate(rate: AutomationRate) {
  if self.constraint.is_some() {
     rate = self.constraint.unwrap()(rate);
  }

  self.automation_rate = rate;
}

if you think it's a good approach, I can try to propose something.

  • reconsider signature of AudioParamValues.get(..). It now returns a slice of full render quantum length always. But the spec says it can be length 1 too (for k-rate and for constant pieces) [1]. For type safety we can also make it return an enum.

This question is more tricky somehow and unfolds other ones I have the impression.

The spec does not even say that the length is related to a-rate or k-rate (so I'm really not sure about the enum type in this case):

if no automation is scheduled during this render quantum, the array MAY have length 1 with the array element being the constant value of the AudioParam for the render quantum.

At first sight, it kind of sounds to me more like: "Chrome people (or whoever) did that and they don't want to change their implementation, so let's consider it's valid because it doesn't hurt" :)
But, at contrary, it also seems like this is the only way (maybe I missed something here...) to know if a param is a-rate or k-rate (or "constant during this block") in the AudioWorklet::process.

My impression here is that we could also be defensive for v1.0: deliberately avoid the extensibility problem (be it called AudioWorklet or "implement your own node") and mark all that as pub(crate) until we are pretty sure that it's the best possible solution (...I still have the impression that even in the AudioWorklet the audio param guts are not that much exposed)

In any case, let me know what you think I can also have a look on that point, I'm pretty confident the unit tests will allow us to see quite fast if some unexpected problem reveals itself. Beyond the fact we decide or not to keep this len() == 1 solution, I think trying to implement it could actually help to clean the automation code.

@orottier
Copy link
Owner Author

Thanks for the comments!

Regarding the constraints, your solution is fine but maybe it is a bit over-engineered. Reading the spec, I think we can get away with a simple option force_k_rate_processing: bool (defaulting to false). Agree?

I'm looking at the current signature of the audio param values:
pub fn get(&self, index: &AudioParamId) -> &[f32]
This is maybe already fine, as it allows for both the len=1 and len=QUANTUM_SIZE cases. The comment needs to be updated though

- For both A & K-rate params, it will provide a slice of length crate::RENDER_QUANTUM_SIZE
+ In the current implementation, the slice will always have a length of crate::RENDER_QUANTUM_SIZE,
+ but one should not rely on this. A slice of length 1 may be returned in the future if all values in the
+ current quantum are identical (e.g. for k-rate processing)

Then we can don't need to make the final decision before release v1.0
In any case, if you want to go ahead and simplify the value calculations for k-rate processing, go ahead!

@b-ma
Copy link
Collaborator

b-ma commented Jan 12, 2022

Regarding the constraints, your solution is fine but maybe it is a bit over-engineered. Reading the spec, I think we can get away with a simple option force_k_rate_processing: bool (defaulting to false). Agree?

Yep, you are right, I didn't check the spec and I was thinking there was more special cases. I'm not completely sure the flag will be enough for panner with HRTF (but my solution neither... :), but HRFT will be a tricky thing anyway.

I'm looking at the current signature of the audio param values: (...)

Yep, that looks like a good tradeoff

In any case, if you want to go ahead and simplify the value calculations for k-rate processing, go ahead!

Yes I think it worth the case to at least try to see what we get there, I'd really like the advance on the Oscillator sub-quantum scheduling first but I will check that after (assigning myself to the issue as it is somehow related to #20)

@b-ma b-ma self-assigned this Jan 12, 2022
@orottier orottier removed this from the Version 1.0 milestone Jan 22, 2022
@orottier
Copy link
Owner Author

Regarding the discussion at #98 I'm removing the V1 milestone here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants