DelayNode repeats last signal multiple times, does no delay

VERIFIED FIXED in Firefox 25

Status

()

Core
Web Audio
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: patrick.borgeat, Assigned: karlt)

Tracking

25 Branch
mozilla26
x86
All
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 verified, firefox26 fixed)

Details

(Whiteboard: [blocking-webaudio+])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 771655 [details]
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)

Updated

5 years ago
Attachment #771655 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

5 years ago
Assignee: nobody → karlt
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
(Assignee)

Comment 1

5 years ago
Created attachment 772516 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
Created attachment 772520 [details] [diff] [review]
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

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

Comment 4

5 years ago
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

5 years ago
(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

5 years ago
Can you please bring this up on public-audio?

Comment 7

4 years ago
What's the status of these patches?
Whiteboard: [blocking-webaudio+]
(Assignee)

Comment 8

4 years ago
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.
Attachment #772516 - Flags: review?(ehsan)
(Assignee)

Comment 9

4 years ago
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.
Attachment #772520 - Flags: review?(ehsan)

Comment 10

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

4 years ago
(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?
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Comment 13

4 years ago
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.
Attachment #772520 - Attachment description: Zero delay buffer when there is no input, to avoid echos → Record silence in the delay buffer when there is no input, to avoid echos

Comment 14

4 years ago
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.
Attachment #772516 - Flags: review?(ehsan) → review+

Comment 15

4 years ago
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!
Attachment #772520 - Flags: review?(ehsan) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 797696 [details] [diff] [review]
add more documentation and make length optional when createExpectedBuffers is present

https://tbpl.mozilla.org/?tree=Try&rev=d4226ba4443c
Attachment #797696 - Flags: review?(ehsan)

Comment 17

4 years ago
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.
Attachment #797696 - Flags: review?(ehsan) → review+
(Assignee)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
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
Flags: in-testsuite+
(Assignee)

Comment 20

4 years ago
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
QA Contact: manuela.muntean
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
status-firefox25: --- → fixed
status-firefox26: --- → fixed
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.
status-firefox25: fixed → verified

Updated

4 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.