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

Wrong ParamRate for BufferSourceNode messages #338

Open
khodzha opened this issue May 5, 2020 · 8 comments
Open

Wrong ParamRate for BufferSourceNode messages #338

khodzha opened this issue May 5, 2020 · 8 comments

Comments

@khodzha
Copy link
Contributor

khodzha commented May 5, 2020

The spec says that playbackRate and detune of BufferSourceNode must be k-rate.
But right now they are a-rate.

Also spec says it must be impossible to change them from k-rate to a-rate.

I suggest to put ParamType into Param struct so its possible to enforce these constraints directly in Param

@jdm
Copy link
Member

jdm commented May 5, 2020

Using types to avoid this problem sounds like a wise solution.

@jdm jdm changed the title Wrong ParamRate for Wrong ParamRate for BufferSourceNode messages May 5, 2020
@Manishearth
Copy link
Member

Types can't fix this, the automationrate is mutable. It's some specific parameters where it's not.

(In general: most knobs in WebAudio are tweakable in general and have constraints in specific cases)

@Manishearth
Copy link
Member

Manishearth commented May 5, 2020

We actually sample these as k-rate anyway. We should still fix the rate here, but that's not actually going to affect anything; the real change is in libscript to introduce parameter constraints.

@khodzha
Copy link
Contributor Author

khodzha commented May 5, 2020

I tried doing something like this:
khodzha@108d7c6#diff-c073ca9541b87e1d6a2a9d87c644a81cR146

but idk how to go from AudioNodeEngine to PanningNode to query panning_model

@Manishearth
Copy link
Member

Right, those checks belong in servo's script crate, at best we can have some debug assertions here IMO

@khodzha
Copy link
Contributor Author

khodzha commented May 5, 2020

We actually sample these as k-rate anyway

you mean now ARate is implemented as KRate?

@Manishearth
Copy link
Member

@khodzha hm? I mean that we sample them at the beginning of the block, not once per tick/frame.

@khodzha
Copy link
Contributor Author

khodzha commented May 5, 2020

#340

I've exposed node types via audio context, then they can be used in SetAutomationRate to do these checks

bors-servo added a commit to servo/servo that referenced this issue May 24, 2020
…<try>

WIP AutomationRate constraints

[webaudio spec](https://webaudio.github.io/web-audio-api/#AudioParam-attributes) specifies that some nodes have constraints on their params automation rates.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes (partially) fix servo/media#338

- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___
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

No branches or pull requests

3 participants