search

servo / media

Mozilla Public License 2.0
82 stars 53 forks source link

Wrong ParamRate for BufferSourceNode messages #338

Open khodzha opened 4 years ago

khodzha commented 4 years ago

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 commented 4 years ago

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

Manishearth commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

I tried doing something like this: https://github.com/khodzha/media/commit/108d7c6335e0de1c18f1cfafb971a22ef6a7e152#diff-c073ca9541b87e1d6a2a9d87c644a81cR146

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

Manishearth commented 4 years ago

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

khodzha commented 4 years ago

We actually sample these as k-rate anyway

you mean now ARate is implemented as KRate?

Manishearth commented 4 years ago

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

khodzha commented 4 years ago

https://github.com/servo/media/pull/340

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