User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/536.30.1 (KHTML, like Gecko) Version/6.0.5 Safari/536.30.1 Steps to reproduce: While checking out DelayNodes in Firefox Nightly I built a very simple Audio Chain, containing just a simple Delay, a Filter und a BufferSourceNode (playing some noise). To Reproduce: Open HTML file, click on the headline. Actual results: Filtered Noise burst sounds without delay, and then is repeated multiple times. Expected results: One filtered noise burst per click, delayed by one second. (As it does in Chrome 27)

This fixes the lack of initial delay. Patch applies on top of patches for bug 865241.

Perhaps DelayNode could have a fast path for when there is no input and nothing remaining in the buffer. I don't know whether or not the filter is continuing to supply input. I haven't looked at why the playedBackAllLeftOvers/Clear() logic doesn't save us.

The only thing needed here is the first patch, right? If yes can you please move the second one to a separate bug? Thanks!

No, the first patch only fixes the lack of initial delay. The second patch fixes the repeating. We can still do this in separate bugs.

(In reply to comment #4) > No, the first patch only fixes the lack of initial delay. > The second patch fixes the repeating. > We can still do this in separate bugs. Oh OK, nevermind then... That being said, it's not clear to me whether patch 2 is correct or not... Fixing bug 857610 will make me a bit more confident on what the right thing to do there would be, but it's not clear how we should fix that bug either...

Can you please bring this up on public-audio?

What's the status of these patches?

Comment on attachment 772516 [details] [diff] [review] delay buffer must be greater than max delay frames to avoid reading what has just been written This is good to go, apart from adding a test to our suite. The other patch, in comment 2, is consistent with what I think we should do for multiple channels, as described in http://lists.w3.org/Archives/Public/public-audio/2013JulSep/0569.html , and we'll need it to handle maxDelayTime not falling on a block boundary, but I'll look into the things mentioned in comment 2 to see whether they are working as intended.

Comment on attachment 772520 [details] [diff] [review] Record silence in the delay buffer when there is no input, to avoid echos The playedBackAllLeftOvers/Clear() logic mentioned in comment 2 doesn't happen because of the AllInputsFinished() test (bug 910174), but we shouldn't rely on that to reset the delay buffer because we want sensible results when an input is disconnected and another connected.

Comment on attachment 772516 [details] [diff] [review] delay buffer must be greater than max delay frames to avoid reading what has just been written Review of attachment 772516 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, can you please explain what problem this patch is solving? It's not clear to me at all.

(In reply to Karl Tomlinson (:karlt) from comment #9) > Comment on attachment 772520 [details] [diff] [review] > Zero delay buffer when there is no input, to avoid echos > > The playedBackAllLeftOvers/Clear() logic mentioned in comment 2 doesn't > happen because of the AllInputsFinished() test (bug 910174), but we > shouldn't rely on that to reset the delay buffer because we want sensible > results when an input is disconnected and another connected. And what is that desired behavior? Also, see bug 910174 comment 2. I think we should fix the root cause for that. Also, note that this code is shared between DelayNode and HRTFPanner, have you tested the implications of this patch on both?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10) > Comment on attachment 772516 [details] [diff] [review] > delay buffer must be greater than max delay frames to avoid reading what has > just been written > > Hmm, can you please explain what problem this patch is solving? It's not > clear to me at all. The way the delay is implemented is by writing input to the delay buffer sequentially, and reading output from a position determined by the delay. I'll use 's' to count the current sample number, 'N' for the number of samples that the buffer can hold, and 'd' for the current delay in samples. The position in the buffer is determined using modular arithmetic with modulus N. An input sample s is written to position s % N in the buffer. An output sample s is read from position (s - d) % N in the buffer (ignoring interpolation when the delay is not a multiple of the sample interval). The buffer must be initialized to zero when created to produce silence instead of reading samples that have not been written. The input sample is written before the output sample is read to handle a delay of 0. Reading before writing would be equivalent to a delay of N. If d can be equal to N as currently in this testcase, then the read and write positions are the same because s ≡ s - N (mod N) The read comes from the current input sample instead of from the overwritten sample recorded d = N samples previous. Setting the buffer length to one more than the maximum delay means we always have d < N, avoiding this issue.

Comment on attachment 772520 [details] [diff] [review] Record silence in the delay buffer when there is no input, to avoid echos I've updated the commit message. (In reply to :Ehsan Akhgari (needinfo? me!) from comment #11) > (In reply to Karl Tomlinson (:karlt) from comment #9) > > The playedBackAllLeftOvers/Clear() logic mentioned in comment 2 doesn't > > happen because of the AllInputsFinished() test (bug 910174), but we > > shouldn't rely on that to reset the delay buffer because we want sensible > > results when an input is disconnected and another connected. > > And what is that desired behavior? When there is silence on the input, we want to record silence in the buffer, so that silence will be played out later, instead of samples recorded a multiple of N samples previous. > Also, see bug 910174 comment 2. I think > we should fix the root cause for that. We should fix bug 910174, but that won't save us when we try to read a sample at a record time after input started producing null, and we will still do that at least for part of one block in the case when the delay is not aligned with block sizes. When we fix bug 857610, we can record sample sets of silence as having zero channels, and up-mix zero channels to multi-channel silence when necessary on output, but right now it is easier to record multi-channel silence in the buffer than to record the change in channel count. Silence is silence whatever the channel count. > Also, note that this code is shared between DelayNode and HRTFPanner, Yes, I know. > have you tested the implications of this patch on both? Yes.

Comment on attachment 772516 [details] [diff] [review] delay buffer must be greater than max delay frames to avoid reading what has just been written Review of attachment 772516 [details] [diff] [review]: ----------------------------------------------------------------- You're right! Please add a short comment about this before landing.

Comment on attachment 772520 [details] [diff] [review] Record silence in the delay buffer when there is no input, to avoid echos Review of attachment 772520 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the explanation!

Comment on attachment 797696 [details] [diff] [review] add more documentation and make length optional when createExpectedBuffers is present Review of attachment 797696 [details] [diff] [review]: ----------------------------------------------------------------- Why is this needed? You don't seem to be using it in any test. But if you have other tests which you haven't submitted for review which need this, then r=me.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17) > Why is this needed? webaudio.js needs the particular lengths specified here because it uses ScriptProcessorNode. I found that out when trying to write a test with an unsupported length. Making the length property optional in some situations saves the test from having to specify the length twice, once in length and once when creating the expected buffer. > You don't seem to be using it in any test. But if you > have other tests which you haven't submitted for review which need this, > then r=me. The test for this bug is in https://hg.mozilla.org/try/rev/caf32612cd86 It could specify the length twice, but there is not much to gain from that. For better or worse, new tests don't usually require review, but have a look if you'd like to check whether there is something I've missed. I'm going to remove the commented out gain node code. It is not needed to reproduce (part of) the repeat bug because the delay length is not a multiple of the block size. Modifications to existing tests always benefit from author review, of course.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed23ecc80370 https://hg.mozilla.org/integration/mozilla-inbound/rev/132f26bd5f49 https://hg.mozilla.org/integration/mozilla-inbound/rev/b4c42f7b488f https://hg.mozilla.org/integration/mozilla-inbound/rev/b30f83dcf6e2

Touched up the test and corrected logic in webaudio.js change. - if (gTest.length && !gTest.createExpectedBuffers) { + if (gTest.length && gTest.createExpectedBuffers) { is(expectedFrames, gTest.length, "Correct number of expected frames"); } https://hg.mozilla.org/integration/mozilla-inbound/rev/64df96108142 https://hg.mozilla.org/integration/mozilla-inbound/rev/007e590f7e96

https://hg.mozilla.org/mozilla-central/rev/ed23ecc80370 https://hg.mozilla.org/mozilla-central/rev/132f26bd5f49 https://hg.mozilla.org/mozilla-central/rev/b4c42f7b488f https://hg.mozilla.org/mozilla-central/rev/b30f83dcf6e2 https://hg.mozilla.org/mozilla-central/rev/64df96108142 https://hg.mozilla.org/mozilla-central/rev/007e590f7e96

While testing for the pre-beta sign-off of this feature, I tried to verify this bug with the latest Aurora (build ID: 20130901004005) on a Windows 8 32bit machine, and I can still reproduce the issue from comment 0: filtered Noise burst sounds without delay, and then is repeated multiple times.

Well, this has just landed on mozilla-central, it is not yet on Aurora.

(In reply to Paul Adenot (:padenot) from comment #23) > Well, this has just landed on mozilla-central, it is not yet on Aurora. You are right Paul! I got carried away! I'm sorry. Thanks!

With the latest Nightly (build ID: 20130902030220) on a Win 8 32bit machine, I still hear some differences opposed to Chrome: - with Chrome, I can hear one filtered noise burst per click - with Nightly, I can hear 11 filtered noises burst per click, delayed by one second (with a pause of 1 second between them) Is this the expected behavior?

This code is not in current nightly yet. Probably tomorrow.

Uplifted to Aurora, with a=webaudio https://hg.mozilla.org/releases/mozilla-aurora/rev/2058e9859873 https://hg.mozilla.org/releases/mozilla-aurora/rev/9e9f4ff8bd5c https://hg.mozilla.org/releases/mozilla-aurora/rev/917e9180f309 https://hg.mozilla.org/releases/mozilla-aurora/rev/cffe4c4ccdf2 https://hg.mozilla.org/releases/mozilla-aurora/rev/3483bf5de405 https://hg.mozilla.org/releases/mozilla-aurora/rev/aeac941ac517

Verified as fixed with the latest Aurora (build ID: 20130903004001) on Ubuntu 12.10 32bit and Win 8 32bit. Chrome and Aurora offer the same user experience.