Skip to content

Fix AudioParam behaviour #424

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

Closed
wants to merge 9 commits into from
Closed

Fix AudioParam behaviour #424

wants to merge 9 commits into from

Conversation

b-ma
Copy link
Collaborator

@b-ma b-ma commented Jan 4, 2024

Hey, some new stuff regarding AudioParam

  • Fix AudioParam argument parsing
  • Fix AutomationRate set behaviour on control thread
  • ~Fix return value for raw setters (i.e. set_value and set_automation_rate)~~
  • Fix return value for raw set_value setter
  • Some refactoring

Copy link
Owner

@orottier orottier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes.
I will probably pick this PR apart in the separate issues

}

/// Update the current value of the automation rate of the AudioParam
///
/// # Panics
///
/// Some nodes have automation rate constraints and may panic when updating the value.
pub fn set_automation_rate(&self, value: AutomationRate) {
pub fn set_automation_rate(&mut self, value: AutomationRate) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think marking these methods as &mut self will work in practise, because they are return as immutable refs from the AudioNodes:
pub fn gain(&self) -> &AudioParam

This could of course be changed to pub fn gain(&mut self) -> &mut AudioParam but that is a rather big change where we should assess pros and cons.

@@ -272,9 +274,11 @@ pub(crate) struct AudioParamRaw {
default_value: f32, // immutable
min_value: f32, // immutable
max_value: f32, // immutable
automation_rate: AutomationRate,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including the non-Arc field here breaks the Clone implementation (which we need for the node bindings).
Calling set_automation_rate on a cloned AudioParam will not update the automation rate of the original AudioParam

@b-ma
Copy link
Collaborator Author

b-ma commented Jan 4, 2024

Hum right, I didn't think about these points indeed (and sorry I indeed forked from the previous PR branch rather than from main)

For the automation rate problem, maybe the more straightforward solution is then just to update it through AudioParamShared when calling the method on control side rather than when the message is handled on render side, as in any case the renderer only uses it own automation_rate copy?

I let you cherry pick the changes, feel free to close the PR when needed

@orottier
Copy link
Owner

orottier commented Jan 4, 2024

Continued in #425 #422 and new issue #426

@orottier orottier closed this Jan 4, 2024
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 this pull request may close these issues.

2 participants