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

What should happen in case of cycles containing a delay node is under-defined #75

Closed
olivierthereaux opened this issue Sep 11, 2013 · 16 comments
Assignees
Labels
Needs Edits Decision has been made, the issue can be fixed. https://speced.github.io/spec-maintenance/about/
Milestone

Comments

@olivierthereaux
Copy link
Contributor

Originally reported on W3C Bugzilla ISSUE-23037 Wed, 21 Aug 2013 17:05:52 GMT
Reported by paul@paul.cx
Assigned to

Consider the following code:

var ctx = new AudioContext();
var source = ctx.createBufferSource();
// some_loaded_buffer is a valid AudioBuffer
source.buffer = some_loaded_buffer;

var gain = ctx.createGain();
var delay = ctx.createDelay();
// defaults to zero anyways, just there to be explicit
delay.delayTime = 0.0;

source.connect(gain);
gain.connect(delay);
delay.connect(ctx.destination);
// cycle
delay.connect(gain);

source.start(0);

The spec does not describe the case where the |delayTime| parameter of the |delay| node is zero. In fact, problems would arise if the |delayTime| is lower than |128/ctx.sampleRate|.

While it is fairly easy to detect something like |delay.delayTime.value = 0.0| where |delay| is in a cycle (and we could throw), there are be some cases where we can't easily predict the value of the AudioParam.

I propose that in such hard-to-detect cases, the subgraph containing the cycle is treated as a silent input, no error being thrown. It could also be of interest in such case to warn the author in the eventual error console of the UA.

@olivierthereaux
Copy link
Contributor Author

Original comment by Robert O'Callahan (Mozilla) on W3C Bugzilla. Wed, 21 Aug 2013 22:17:00 GMT

Having to recheck for cycles every time we sample the delay AudioParams would be pretty bad I think.

So I think we should clamp the DelayNode's delay to a minimum of 128 cycles at all times. Then cycle-checking just needs to ensure there's at least one DelayNode in every cycle and we're fine.

@olivierthereaux
Copy link
Contributor Author

Original comment by bjornm on W3C Bugzilla. Thu, 22 Aug 2013 07:58:49 GMT

Regarding clamping: Delay times below |128/ctx.sampleRate| are very useful, e.g. in chorus effects, flangers and when adjusting group delay time in parallel signal paths.

@olivierthereaux
Copy link
Contributor Author

Original comment by Robert O'Callahan (Mozilla) on W3C Bugzilla. Thu, 22 Aug 2013 10:42:09 GMT

We actually resolved about a year ago to clamp to a minimum delay:
http://lists.w3.org/Archives/Public/public-audio/2012JulSep/0768.html
Making the minimum delay be one block was really the only viable option.

@olivierthereaux
Copy link
Contributor Author

Original comment by paul@paul.cx on W3C Bugzilla. Thu, 22 Aug 2013 11:05:08 GMT

Right.

For what it's worth, in case of loop without at least a one block delay, Pd refuses to starts the processing ("error: DSP loop detected"), and Max mutes the subgraph containing the cycle, but does not log an error.

Reading the 2012-09-12 minutes, we still have to agree on whether we want to clamp in ms or block. I'd say that one block is better.

Either way, we could warn the author in the console (maybe only the first time it occurs), that his value has been clamped.

@olivierthereaux
Copy link
Contributor Author

Original comment by Chris Wilson on W3C Bugzilla. Thu, 22 Aug 2013 16:46:44 GMT

I do want to ensure we're talking about clamping ONLY in the case of a cycle, not without a cycle (where short delays are useful for phase effects).

@olivierthereaux
Copy link
Contributor Author

Original comment by paul@paul.cx on W3C Bugzilla. Thu, 22 Aug 2013 17:00:53 GMT

Yes, we are talking about clamping only for DelayNodes that are part of a cycle.

Are we fine with having a clamping value of one block? (128 frames, 128 / sampleRate seconds) ?

@olivierthereaux
Copy link
Contributor Author

Original comment by Chris Wilson on W3C Bugzilla. Thu, 22 Aug 2013 17:08:08 GMT

I think clamping to a minimum of one sample block (128 samples) is fine.

@olivierthereaux
Copy link
Contributor Author

Original comment by Jussi Kalliokoski on W3C Bugzilla. Mon, 26 Aug 2013 21:14:28 GMT

(In reply to comment #2)

Regarding clamping: Delay times below |128/ctx.sampleRate| are very useful,
e.g. in chorus effects, flangers and when adjusting group delay time in
parallel signal paths.

I agree with this statement. Short delays are also useful for wind / pipe synthesis, etc. A lot of the graph based synthesis software seem to allow this (e.g. NI Reaktor), I wonder how the implementation is done... I think it would even be fine to fall back to per-sample processing if the delay goes under the block size. It's nicer to pay a performance price for features than to not have them at all. In cases of delay node + gain node (i.e. finite response) feedback loops you could usually even optimize it away into vector operations. This would have the additional benefit of not exposing implementation details, i.e. the block size.

@olivierthereaux
Copy link
Contributor Author

Original comment by Robert O'Callahan (Mozilla) on W3C Bugzilla. Mon, 26 Aug 2013 21:22:34 GMT

Clamping to a minimum of one block is only going to happen when the DelayNode is part of a cycle. Hopefully your use-cases for low delays don't require DelayNodes in cycles.

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Wed, 28 Aug 2013 18:09:57 GMT

I also support clamping down to a minimum of 128 samples in the looping case.

@olivierthereaux
Copy link
Contributor Author

Original comment by Jussi Kalliokoski on W3C Bugzilla. Sat, 31 Aug 2013 17:21:05 GMT

(In reply to comment #9)

Clamping to a minimum of one block is only going to happen when the
DelayNode is part of a cycle. Hopefully your use-cases for low delays don't
require DelayNodes in cycles.

Unfortunately they do; One common way of achieving wind synthesis is to have a noise and DC source which is then fed to a short feedback loop. There you get feedback resonation, and the length of the delay essentially becomes the base wave length of the note you want to play (similar to how sound moves in a pipe). In practice it's not quite as simple as this of course, there's a lot of filter-work to be done afterwards, but this phase is crucial for the process. If you clamp the delay time down to 128 samples, at the sample rate of 48kHz this gives you the hard limit of 48000Hz/128=375Hz which doesn't really suffice for any wind instrument.

@olivierthereaux
Copy link
Contributor Author

Original comment by Robert O'Callahan (Mozilla) on W3C Bugzilla. Sat, 31 Aug 2013 23:54:07 GMT

OK. Unfortunately that's just too bad. You'll need to write your simulator in JS.

@olivierthereaux
Copy link
Contributor Author

Original comment by Jussi Kalliokoski on W3C Bugzilla. Sun, 01 Sep 2013 12:08:03 GMT

(In reply to comment #12)

OK. Unfortunately that's just too bad. You'll need to write your simulator
in JS.

Hahah, yeah, I expected as much. I'll just have to wait until for worker processing (and that I make a new audio processing library) before finishing my work on it. Kinda sad how even the simplest parts of Web Audio API, such as delay, fail to be usable in some cases. There probably won't be a backwards compatible way of supporting this use case in the future either, aside from adding another delay node type with different semantics.

@olivierthereaux
Copy link
Contributor Author

Original comment by paul@paul.cx on W3C Bugzilla. Fri, 06 Sep 2013 14:36:25 GMT

*** Bug 17326 has been marked as a duplicate of this bug. ***

@cwilso cwilso added Editorial/Documentation Needs Edits Decision has been made, the issue can be fixed. https://speced.github.io/spec-maintenance/about/ and removed Clarification (Requires change) labels Oct 22, 2014
@cwilso
Copy link
Contributor

cwilso commented Oct 22, 2014

This is interoperably implemented, I believe - we just need to specify that the minimum delay is clamped to a processing block in the case where there is a cycle.

@padenot padenot self-assigned this Oct 24, 2014
@cwilso cwilso modified the milestone: Web Audio Last Call 1 Oct 29, 2014
@padenot
Copy link
Member

padenot commented Sep 10, 2015

This has already been written in:

If DelayNode is part of a cycle, then the value of the delayTime attribute is clamped to a minimum of 128
frames (one block).

Closing.

@padenot padenot closed this as completed Sep 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Edits Decision has been made, the issue can be fixed. https://speced.github.io/spec-maintenance/about/
Projects
None yet
Development

No branches or pull requests

5 participants