WebAudio / web-audio-api

The Web Audio API v1.0, developed by the W3C Audio WG
https://webaudio.github.io/web-audio-api/
Other
1.05k stars 167 forks source link

(DeZippering): De-zippering is not defined #76

Closed olivierthereaux closed 9 years ago

olivierthereaux commented 11 years ago

Originally reported on W3C Bugzilla ISSUE-17339 Tue, 05 Jun 2012 11:32:29 GMT Reported by Philip Jägenstedt Assigned to

Audio-ISSUE-46 (DeZippering): De-zippering is not defined [Web Audio API]

http://www.w3.org/2011/audio/track/issues/46

Raised by: Philip Jägenstedt On product: Web Audio API

https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#AudioGainNode says:

"""The implementation must make gain changes to the audio stream smoothly, without introducing noticeable clicks or glitches. This process is called "de-zippering"."""

It needs to be defined in detail what this means.

chrislo commented 10 years ago

De-zippering is the process of smoothly approaching a target value instead of jumping to it directly. The intention is to prevent audio "glitches" which might occur if the value is changed suddenly.

Here's a demo that shows the effect of de-zippering.

De-zippering is implemented in webkit when the Boolean smooth flag is set to true in the C++ code that associates an AudioParam with a value of a given node in that nodes implementation.

If a developer explicitly sets values (using setValueAtTime for example) then de-zippering is not performed. If instead values are set through the setters on an instance of a node, for example

gain_node = (new AudioContext).createGainNode() gain_node.gain.value = target_value``

then de-zippering is performed.

The equation for calculating the new value as the internal time, t advances in the webkit implementation is

`new_value = (target_value - old_value) * smoothing_constant``

Where new_value is the value at t_n and old_value is the value at time t_(n-1).

As the new value approaches the target value exponentially in theory it will never reach the target value, to cope with this the new_value "snaps" to the target value when it is within a threshold

if abs(new_value - target_value) < snap_threshold new_value = target_value

Webkit has default values of smoothing_constant and snap_threshold of 0.05 and 0.001 respectively.

olivierthereaux commented 10 years ago

Documenting the resolution from the teleconference on December 19, 2013: http://www.w3.org/2013/12/19-audio-minutes.html#item02

RESOLUTION: smoothing by default on all audioparam when setting .value, essentially an alias to settargetvalueattime, time constants TBD

joeberkovitz commented 10 years ago

Note the interaction with questions of observability of changes in AudioParam values #318 and interaction with automation #128.

joeberkovitz commented 9 years ago

TPAC re-resolution: we are removing de-zippering from the API altogether to avoid hiding reality from devs and making many arbitrary decisions on their behalf. Use setTargetAtTime(), if that's what you want.

joeberkovitz commented 9 years ago

Should add non-normative material explaining the need to use setTargetAtTime if de-zippered behavior is desired.

cwilso commented 9 years ago

I note, by the way, that removing dezippering is going to be really nasty-sound to almost any usage of setting delayTime - unless the audio stream is constant, any delayTime.value= setting is going to cause a discontinuous jump.

cwilso commented 9 years ago

Actually, upon close reflection - I'm thinking removing dezippering is a really bad idea. Many common chains are going to cause discontinuous jumps if we remove dezippering.

joeberkovitz commented 9 years ago

Chris -- can you share any further thoughts on what's changed your mind? I think the WG understood in the discussion at TPAC that discontinuous jumps would be the norm much of the time, but (at least at the time) the sense was that it was bad to require an explicit API call to force an immediate transition in any AudioParam value. There were some use cases put forth in which de-zippering was bad instead of good -- although of course, the fact that this would be a behavioral change in the API may tip the balance regardless of the merits of one behavior vs. the other. We'll put this on the next telcon agenda but it would be great to surface some thinking about it in advance.

Also, can you tell us what you have in mind as a way to selectively disable dezippering? Is it just setValueAtTime(value, context.currentTime)? Or do you think there should be a new boolean attribute on the AudioParam that gates dezippering somehow?

cwilso commented 9 years ago

Sorry for the shorthand- this was sparked by our discussion last week of the "smoothing" comment (issue #79). After thinking about it a little harder and discussing with @hoch, I realized that ANY parameter modification that caused changes to temporal effects, not just gain controls - delayTime being the prime example - would be virtually guaranteed to make a disjoint jump in amplitude (causing nasty clicks). If we stick with the current plan of removing dezippering, we need a big bright warning sticker that .value should not be set unless it's disconnected or has silence (a time-invariant DC signal) running through it, or it WILL cause clicking and glitches (even for relatively small delay changes).

The only parameters where the effects of dezippering might be undesirable are, upon reflection, the set where the param value affects the future of the value, but not the instantaneous value at the point the change is made - frequency and detune on Oscillator, playbackRate and detune on BufferSource. (in effect, those params affect the rate of change in the future, but they don't mess with the current value.)

Any other changes - gain, the set of parameters on DynamicsCompressor, pan on StereoPanner - smoothing those changes over even a short period of time (<1 block) would be wildly better. I've asked Hongchan to take a closer look at how changes in the k-rate BiquadFilter work today, because it's not instantaneously apparent to me how the subsequent blocks line up there anyway.

With that in mind, I've converted from "we should remove dezippering" to "we should not remove dezippering". We should either document how to avoid it in the phase-changing params, or we should make dezippering an attribute of the AudioParam (in with case, yes, we'd need to put a boolean on the AudioParam creation for AudioWorkers).

cwilso commented 9 years ago

Duh. Biquad is a second-order filter, so does not affect instantaneous value directly.

I still think we should not remove dezippering, and should just put in a note on frequency and detune about using setValueAtTime. I did a little playing around with changing instantaneous value in basic cases from my input effects test, and the clicking/aliasing noise is pretty bad.

karlt commented 9 years ago

Chris Wilson writes:

I still think we should not remove dezippering, and should just put in a note on frequency and detune about using setValueAtTime. I did a little playing around with changing instantaneous value in basic cases from my input effects test, and the clicking/aliasing noise is pretty bad.

Is that changing the instantaneous value of frequency or gain?

As you indicate in #issuecomment-66212455, I would expect effects of discontinuous changes to depend on the nature of the AudioParam. For a gain, clicking would be expected, but the effect of a instantaneous change in frequency would be much less noticeable.

Is there sufficient value in keeping so-called "dezippering" on frequency and detune?

The delayTime parameter is a clear example that a single dezippering method does not work for all AudioParam changes. See the links in [1].

[1] http://lists.w3.org/Archives/Public/public-audio/2013OctDec/0335.html

cwilso commented 9 years ago

Is that changing the instantaneous value of frequency or gain?

Both, and delayTime.

Yes, as from my above note - parameters that affect the future movement of the value, but not the instantaneous value, would likely be at least as well served by not de-zippering. But I don't think the problem is particularly bad.

I think your example is highly synthetic - changing a delay value from 0 to 10 seconds is a huge change, and unlikely to occur in the real world without muting (or, as you did in the last example, crossfading), since you're guaranteed to have issues. For smaller changes, however - a few milliseconds of change between values, say - I believe dezippering will be a good experience.

I'm on the fence about dezippering on frequency and detune .value sets. If we don't want it, it's a lot more work (in the spec and in implementations, and in audio worker params) to represent the switch. I'd probably just leave it in, frankly, and just detail what's going on.

karlt commented 9 years ago

Chris Wilson writes:

I think your example is highly synthetic - changing a delay value from 0 to 10 seconds is a huge change, and unlikely to occur in the real world without muting (or, as you did in the last example, crossfading), since you're guaranteed to have issues. For smaller changes, however - a few milliseconds of change between values, say - I believe dezippering will be a good experience.

For very small changes in delay, the Doppler effect may be acceptable.

I don't think it is a good experience at 20ms, and the tolerable size of changes gets smaller with higher frequencies.

If we want implementations to dezipper the effects of delayTime changes, then I think they should be allowed or required to cross-fade.

cwilso commented 9 years ago

I disagree. I want implementations to dezipper delayTime changes for those basic small-delta cases (I'm tweaking a delayTime slider, e.g.), and I think crossfading is a very complex change to the delay node (and not one guaranteed to have a better audio experience).

If an implementation wants a great experience, it can build a crossfader, or a dezipper that is delta-dependent, or whatever else it wants.

joeberkovitz commented 9 years ago

A further note here: we discussed a per-AudioParam boolean attribute gating dezippering (both for native nodes and for AudioWorkers).

But given that the appropriate time constant for dezippering may vary from one parameter to another, would it be better to change the boolean to a number, specifying the time constant in effect for assignments to that parameter's value (i.e. the number that would yield the same result if passed as the time argument to setTargetAtTime?)? If this number is zero, that's equivalent to disabling de-zippering.

I think this would give developers more control over how de-zippering works.

cwilso commented 9 years ago

It would have to be explicit that a zero is "disable dezippering" - a timeConstant of zero in setTargetAtTime causes a divide-by-zero.

On Thu, Dec 11, 2014 at 9:59 AM, Joe Berkovitz notifications@github.com wrote:

A further note here: we discussed a per-AudioParam boolean attribute gating dezippering (both for native nodes and for AudioWorkers).

But given that the appropriate time constant for dezippering may vary from one parameter to another, would it be better to change the boolean to a number, specifying the time constant in effect for assignments to that parameter's value (i.e. the number that would yield the same result if passed as the time argument to setTargetAtTime?)? If this number is zero, that's equivalent to disabling de-zippering.

I think this would give developers more control over how de-zippering works.

— Reply to this email directly or view it on GitHub https://github.com/WebAudio/web-audio-api/issues/76#issuecomment-66660388 .

hoch commented 9 years ago

When the control over the time constant is needed, I believe that AudioParam methods should be used, not de-zippering. If we introduce more complexity into de-zippering, we are moving away its original purpose.

rtoy commented 9 years ago

I agree with Hongchan: Use AudioParam if you want fine control.

joeberkovitz commented 9 years ago

I wasn't suggesting that people use this "default time constant" as a substitute for setTargetAtTime() -- rather, that this would be a way to provide different built-in dezippering time constants without relying on a set of invisible magic values hardcoded into the implementation.

But perhaps it's going to be too tempting for developers to use this attribute instead of setTargetAtTime.

cwilso commented 9 years ago

I think we should just define the dezippering that happens, and tell people to use setValueAtTime for the couple of cases (frequency and detune) that dezippering isn't helpful for.

Barring that, we should define one dezippering factor, set it as a Boolean attribute on AudioParam, and define which AudioParams it's on and off for (like how we define a-rate vs k-rate). I'll have to expose it for AudioWorkers too.

On Fri, Dec 12, 2014 at 8:38 AM, Joe Berkovitz notifications@github.com wrote:

I wasn't suggesting that people use this "default time constant" as a substitute for setTargetAtTime() -- rather, that this would be a way to provide different built-in dezippering time constants without relying on a set of invisible magic values hardcoded into the implementation.

But perhaps it's going to be too tempting for developers to use this attribute instead of setTargetAtTime.

— Reply to this email directly or view it on GitHub https://github.com/WebAudio/web-audio-api/issues/76#issuecomment-66797684 .

rtoy commented 9 years ago

The dezippering algorithm used in blink and webkit (as given by @chrislo in https://github.com/WebAudio/web-audio-api/issues/76#issuecomment-27954865) has some issues.

The smoothing quantum is fixed (as is the snap threshold). The formula is applied for each rendering quantum. That means for contexts at high sample rates, the parameter approaches the final value in much less time than in a context with a slower sampling rate.

This is undesirable; the parameters should take the same amount of time to reach the same value, not the same number of rendering quanta. The smoothing constant needs to be adjusted for the sample rate. Let's say .05 is the time constant at 44.1 kHz and adjust it appropriately for other sample rates.

Also, the parameter can have very large or very small values, but the snap threshold is the same value, independent of the parameter. This means that for parameter values less than .001, we always snap them to the final value. That means no smoothing at all. I can't think of any parameters now where the difference between .001 and 0 is significant, but it's not impossible where such changes would be audible for some (current or future) parameter.

joeberkovitz commented 9 years ago

@rtoy I agree with you, but the comment you are citing is sort of a red herring.

The resolved approach is here: http://www.w3.org/2013/12/19-audio-minutes.html#item02 and states that dezippering should be "essentially an alias to setTargetAtTime()" which implies some time constant that is independent of sample rate. We did not agree that there would be a snap threshold and I don't know if there is any good rationale for the conditional snapping; I haven't heard one advanced.

If we adopt the resolved approach then the current blink/webkit impl. will not be compliant with the spec (both because sample rate affects it, and also because of the snap threshold).

rtoy commented 9 years ago

On Tue, May 5, 2015 at 12:59 PM, Joe Berkovitz notifications@github.com wrote:

@rtoy https://github.com/rtoy I agree with you, but the comment you are citing is sort of a red herring.

The resolved approach is here: http://www.w3.org/2013/12/19-audio-minutes.html#item02 and states that dezippering should be "essentially an alias to setTargetAtTime()" which implies some time constant that is independent of sample

​Ah, I had forgotten about that. This is acceptable to me as long as we note that k-rate parameter smoothing isn't exactly like setTargetAtTime because the parameter is held constant.​

rate. We did not agree that there would be a snap threshold and I don't know if there is any good rationale for the conditional snapping; I haven't heard one advanced.

​My best guess is that it allows the implementation to skip the smoothing if the current value equals the target value. In this day and age, I don't think that optimization really matters anymore.​

If we adopt the resolved approach then the current blink/webkit impl. will not be compliant with the spec (both because sample rate affects it, and also because of the snap threshold).

​I'm ok with that. I don't think the sample rate affects were too big since most people are probably running 44.1 or 48 kHz. The snap threshold probably​ isn't needed because it's probably not audible for any parameter we have.

— Reply to this email directly or view it on GitHub https://github.com/WebAudio/web-audio-api/issues/76#issuecomment-99203396 .

Ray

joeberkovitz commented 9 years ago

After much painful review and dissection of various cases, we've reached the conclusion (for a second time, so help us) that dezippering requires special-casing and ad hoc distinctions that favor one use case over another.

We're going to remove the piece of the spec that describes dezippering, and instead be very clear that automation methods on AudioParam must be used to achieve smooth transitions in use cases where this is desirable.

joeberkovitz commented 9 years ago

Add informative note to spec that the recommended technique for de-zippering is to place successive calls to setTargetAtTime(context.currentTime, value, timeConstant). The spec will be clarified to communicate that the starting value for setTargetAtTime() is the current computed value at the start time, not the value of .value.

joeberkovitz commented 9 years ago

@padenot You already have a branch 76-remove-dezippering for this but no PR -- can you create the PR for your branch please? (I created my own completely separate resolution of this issue bc I didn't realize you'd done that)

padenot commented 9 years ago

@joeberkovitz, it's been opened since the 4th of November last year.

joeberkovitz commented 9 years ago

@padenot any reason not to merge it at this point?

padenot commented 9 years ago

No reason, I'll do that now.

sidewayss commented 6 years ago

FYI - I just received the deprecation notice for this change in the Chrome Dev Tools Console M63. As of M64, January 2018, GainNode.gain.value is no longer valid.

As someone who is arriving here fresh from the deprecation notice, here is my translation/consolidation of the information I have gleaned from the various links presented to me. Hopefully this will save some time/energy for other newbies. 1) De-Zippering is another word for smooth fades between gain values. It's useful in digital audio because sudden changes might cause pops or clicks. 2) The W3C decided that browsers should no longer provide default de-zippering for javascript, and developers must henceforth specify a smooth fade via AudioParam.setTargetAtTime or a sudden change of value (not smooth) via setValueAtTime. 3) There is also setValueCurveAtTime, which creates more sophisticated fades, based on an array of values. 4) It seems that setValueAtTime is generally less desirable due to the possibility for clicks and pops.

Finally: This seems to be exclusive to GainNodes, though I can see how it might apply to other types of web audio nodes. Can someone on this thread clarify that point? The discussion here so far is ambiguous.

cwilso commented 6 years ago

You are essentially correct. The only important thing is that this isn't just about GainNodes - in fact, if it were only about gain, leaving the smoothing in would probably be best.

  1. De-zippering is another word for smoothing between subsequent values in an AudioParam (it's not just gain values, but any AudioParam - in fact, the worst problems with built-in zippering were with smoothing of frequency values, which caused portamento-like "pitch glide" effects). De-zippering is just avoiding the "stair-stepping" between values set only occasionally, by using the audio param scheduler to make changes at audio rate.
  2. Yes, if you want a sudden change of value (or don't really care), use setValueAtTime. If you want to smooth values, setTargetAtTime is probably easiest to use.
  3. setValueCurveAtTime probably isn't the best replacement - it can be used, but it's a lot more complex, and probably not necessary.
  4. setValueAtTime won't give you pops and clicks in many scenarios - for example, in setting frequency values on an oscillator, it's probably the right thing to do. For gain, yes, you probably want to use setTargetAtTime - for delayTime on a delay node, you definitely want to use setTargetAtTime, because unsmoothed values WILL cause pops and clicks.

For most changes you can probably just use something like setTargetAtTime( value, 0.01 ) and it will smooth well enough.

sidewayss commented 6 years ago

Thanks for the detailed explanations. But is this Chrome M64 deprecation and subsequent removal of a feature only for GainNode.gain.value? Or is this not a Chrome-specific thread? I'm trying to clarify the current implementation(s) too. Seems like I should change all my .value settings to use setXxxAtTime. Does this apply to other properties than .value? I get the impression that your list of scenarios is incomplete.

Personally, I'm not doing much with web audio beyond simple mixing - no f/x. I'm going to assume that your "For most changes" suggestion covers all my needs. But I'm curious if there is any kind of general rule as to when/where this applies, versus a list of disconnected circumstances. I saw some detail in one of your comments earlier in this thread, but I'm not up on all the math involved.

cwilso commented 6 years ago

IIRC, Mozilla never implemented de-zippering. Chrome (and all other Webkit- and Blink-derived browsers) did de-zippering EVERYWHERE (i.e. on all AudioParams). The deprecation in Chrome will make it so that all AudioParams will not automatically smooth, and thus be compatible with Firefox.

The problem is that some parameters should NOT be smoothed automatically - an OscillatorNode's frequency.value, for example, as I stated before - but some arguably should be smoothed (e.g. delayTime on a DelayNode - where almost any change would result in pops and clicks as it would cause a discontinuous jump in the waveform). The Working Group looked through this, and decided it was better not to apply "magic" here; that is, to just put it in the hands of developers. There's no "general rule", per se - just have to think through how smoothing would affect things, and if it's necessary to avoid discontinuous jumps in the output.

Realistically, if all you're doing is something like offering a slider that is linked to a GainNode's .value, you're probably not going to hear any zippering anyway. But it's safer, for Gain, to use a setTargetAtTime to avoid any possibility.

cwilso commented 6 years ago

Oops, meant to also say - if you were going to hear clicks, you'd already be hearing them in Firefox, since they never implemented de-zippering.

sidewayss commented 6 years ago

Thanks again for the detailed info. The delay time example now makes perfect sense to me, no math required. This is yet another time when I'm glad my web audio manipulation is simple so far. It's a lot of moving parts when you get into the full feature set, a lot of details to think through.

joeberkovitz commented 6 years ago

@sidewayss GainNode.gain.value is valid, it's just that it does not do dezippering any more. (This applies to all value setters for all AudioParams, not just GainNode.) The deprecation message is not clearly worded.

When called as a setter, the spec states that value is identical to setValueAtTime(value, context.currentTime). See: https://webaudio.github.io/web-audio-api/#dom-audioparam-value

rtoy commented 6 years ago

As the author of the message, it says GainNode because you used a GainNode. If you had used a different node, it would mention the node. We thought it would be better to specify the node that was affected rather than be generic and leaving it up to you to figure out which of the zillion nodes in the graph were affected by this.

sidewayss commented 6 years ago

OK, I'm going to modify my code regardless, if for no other reason than to avoid extraneous, brightly colored console messages. Yes, the deprecation message is muddily worded and the links/sub-links, including this thread, don't provide clear info. That's the entire reason I posted originally on this thread, to both receive and provide clarity. Thanks for providing some additional clarity. @joeberkovitz and @rtoy.

Yes, I figured that the deprecation message might be using my code to customize the message, but that was part of the reason I needed further clarity, especially after reading all the info in the trail of links from that message, including this issue thread. This probably isn't the best place to clarify all this info, but it's better than nowhere, and at least I personally am now properly informed :-)

rtoy commented 6 years ago

Thanks for reading the deprecation messages and acting on it before the deadline!

Yeah, I should fix the message (unlikely now since M64 is coming soon) or update the link targets to have better info.

sidewayss commented 6 years ago

@rtoy - I could consolidate the relevant text from this thread into a few paragraphs if you would like to post that someplace and link to it in the deprecation message. I am a programmer who doesn't mind doing documentation, especially if I learn something in the process, as I have here. I am always glad to save someone else the trouble I have gone through to figure out something new. Otherwise you might link to the numbered comments by myself and @cwilso in this thread, which have some consolidated info, and below which follow these additional comments and clarifications.

sidewayss commented 6 years ago

...and won't there be a post deprecation "this has already changed" message? Or will there not be one because of the fact that AudioParam.value still works, it just doesn't de-zipper? That would be the worst! To have older, production code, then only stumble on this problem after M64 via clicks and pops or other glitches, because it's still legal, and the deprecation period has passed. I have not been away from this code in Chrome for more than a few weeks, so this deprecation message only appeared in M63, AFAICT. Not much of a window of opportunity for deprecation.

rtoy commented 6 years ago

We can certainly postpone the removal. The main reason for the relatively short time frame is that Firefox never implemented de-zippering.

But there's also another side-effect of this change. The value setter becomes exactly the same as setValueAtTime. This means that if already had an automation running, you may get errors because you can't call that in the middle of a setValueCurveAtTime automation. Internal stats show that this doesn't really ever happen, fortunately.

sidewayss commented 6 years ago

I agree that it's a limited audience, a limited codebase that is affected by this. The glitch is: This is not a full-on feature deprecation, where the old feature becomes illegal. It's a sub-feature deprecation within the feature. So post-deprecation looks identical to pre-deprecation-notice to the programmer, unless there are audio glitches. I'm the lowest person on this thread's totem pole in terms of any decision, but if you're asking for my input: I would not delay the actual deprecation, but I would keep the deprecation message for a while, so those programmers have more time to catch it and understand the implications prior to user complaints about audio glitches. Just change the words in the M64 message to reflect that the change has already occurred.

rtoy commented 6 years ago

This is no longer about the spec. So if there are additional comments, please file a bug at https://crbug.com/new and we can continue the discussion there. That being said, the message links to https://www.chromestatus.com/features/5287995770929152, and I can update that to provide a bit more detail.

As far as the spec is concerned, this is a done deal and we're not going to change this. Chrome was just slow in removing the dezippering.