890528 - DelayNode repeats last signal multiple times, does no delay

Status: VERIFIED FIXED

(Core :: Web Audio, defect)

Description

[patrick.borgeat]

Attachment: Simple Reproducer

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)

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: delay buffer must be greater than max delay frames to avoid reading what has just been written

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

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Record silence in the delay buffer when there is no input, to avoid echos

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.

Comment 3

[(no longer active)]

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

Comment 4

[Karl Tomlinson (:karlt)]

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.

Comment 5

[(no longer active)]

(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...

Comment 6

[(no longer active)]

Can you please bring this up on public-audio?

Comment 7

[(no longer active)]

What's the status of these patches?

Comment 8

[Karl Tomlinson (:karlt)]

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 9

[Karl Tomlinson (:karlt)]

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 10

[(no longer active)]

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.

Comment 11

[(no longer active)]

(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?

Comment 12

[Karl Tomlinson (:karlt)]

(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 13

[Karl Tomlinson (:karlt)]

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 14

[(no longer active)]

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 15

[(no longer active)]

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 16

[Karl Tomlinson (:karlt)]

Attachment: add more documentation and make length optional when createExpectedBuffers is present

https://tbpl.mozilla.org/?tree=Try&rev=d4226ba4443c

Comment 17

[(no longer active)]

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.

Comment 18

[Karl Tomlinson (:karlt)]

(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.

Comment 19

[Karl Tomlinson (:karlt)]

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

Comment 20

[Karl Tomlinson (:karlt)]

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

Comment 21

[Ed Morley [:emorley]]

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

Comment 22

[Manuela Muntean [Away]]

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.

Comment 23

[Paul Adenot (:padenot)]

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

Comment 24

[Manuela Muntean [Away]]

(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!

Comment 25

[Manuela Muntean [Away]]

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?

Comment 26

[Paul Adenot (:padenot)]

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

Comment 27

[Paul Adenot (:padenot)]

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

Comment 28

[Manuela Muntean [Away]]

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.