1480661 - avoid reading channel count of most recent delay buffer input when reading samples at max delay

Status: RESOLVED FIXED

(Core :: Web Audio, enhancement, P3)

Description

[Karl Tomlinson (:karlt)]

When maxDelay in DelayBuffer::Read() is mMaxDelayTicks and these are an integer multiple of block size, the +1 for determining oldestChunk for channel count calculation is enough to wrap around the buffer and find the most recent chunk.
The most recent chunk may have a different channel count to that of the oldest chunk, which is from where output samples are obtained.

This is causing some unexpected behavior in tests I'm writing for bug 1474222.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: bug 1480661 avoid reading channel count of most recent delay buffer input when reading samples at max delay

When maxDelay was mMaxDelayTicks and these were an integer multiple of block
size, the +1 for determining oldestChunk was enough to wrap around the buffer
and find the most recent chunk, which may have a different channel count to
that of the oldest.

Review commit: https://reviewboard.mozilla.org/r/261466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261466/

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: bug 1480661 add test for DelayNode channelCount

Review commit: https://reviewboard.mozilla.org/r/261468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261468/

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: bug 1480661 move DelayNode channelCount test to wpt

Review commit: https://reviewboard.mozilla.org/r/261470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261470/

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: bug 1480661 remove unused mCurrentDelay

Review commit: https://reviewboard.mozilla.org/r/261472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261472/

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: bug 1480661 use single precision for operating on delay values

These are derived from single precision AudioParam values
and so there is little to gain from using double.

double has the disadvantage that values intended to be integer
frame counts are less likely to be integer. e.g.

(rr) print ((float)(128.0/48000.0))*48000.f
$91 = 128
(rr) print ((float)(128.0/48000.0))*48000.0
$92 = 127.99999862909317
(rr) print ((float)(128.0/44100.0))*44100.f
$93 = 128
(rr) print ((float)(128.0/44100.0))*44100.0
$94 = 127.99999765120447
(rr) print ((float)(768.0/48000.0))*48000.f
$95 = 768.000061
(rr) print ((float)(768.0/48000.0))*48000.0
$96 = 768.0000364780426
(rr) print ((float)(768.0/44100.0))*44100.f
$97 = 768
(rr) print ((float)(768.0/44100.0))*44100.0
$98 = 768.00002697855234

Review commit: https://reviewboard.mozilla.org/r/261474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261474/

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8997743 [details]
bug 1480661 move DelayNode channelCount test to wpt

https://reviewboard.mozilla.org/r/261470/#review268474

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8997743 [details]
bug 1480661 move DelayNode channelCount test to wpt

https://reviewboard.mozilla.org/r/261470/#review268476

I think this test is consistent with the intention of the consensus in
https://github.com/WebAudio/web-audio-api/issues/25
but the wording that ended up in the spec is not so clear IMO,
and so I'm not sure whether this should go into wpt yet or not.
What do you think, Paul?

Comment 8

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> I think this test is consistent with the intention of the consensus in
> https://github.com/WebAudio/web-audio-api/issues/25
> but the wording that ended up in the spec is not so clear IMO,
> and so I'm not sure whether this should go into wpt yet or not.
> What do you think, Paul?

I'd tend to agree with you.

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8997741 [details]
bug 1480661 avoid reading channel count of most recent delay buffer input when reading samples at max delay

https://reviewboard.mozilla.org/r/261466/#review269352

My reviews were blocked during my PTO but this looks finished, so I'm having a look.

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8997742 [details]
bug 1480661 add test for DelayNode channelCount

https://reviewboard.mozilla.org/r/261468/#review269354

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8997743 [details]
bug 1480661 move DelayNode channelCount test to wpt

https://reviewboard.mozilla.org/r/261470/#review269356

To me, this is what was intended in issue #25. However, I agree that the current text is weird. I've filed https://github.com/WebAudio/web-audio-api/issues/1718 to clarify this.

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8997744 [details]
bug 1480661 remove unused mCurrentDelay

https://reviewboard.mozilla.org/r/261472/#review269362

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8997745 [details]
bug 1480661 use single precision for operating on delay values

https://reviewboard.mozilla.org/r/261474/#review269364

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d168f623e879
avoid reading channel count of most recent delay buffer input when reading samples at max delay r=padenot
https://hg.mozilla.org/integration/autoland/rev/8d9e3375e91d
add test for DelayNode channelCount r=padenot
https://hg.mozilla.org/integration/autoland/rev/3802f485af6f
move DelayNode channelCount test to wpt r=padenot
https://hg.mozilla.org/integration/autoland/rev/a1f7a37f6390
remove unused mCurrentDelay r=padenot
https://hg.mozilla.org/integration/autoland/rev/b3f01b9ce2bf
use single precision for operating on delay values r=padenot

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12464 for changes under testing/web-platform/tests

Comment 16

[Karl Tomlinson (:karlt)]

Thank you for jumping on this Paul, and for filing the spec issue.

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 18

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d168f623e879
https://hg.mozilla.org/mozilla-central/rev/8d9e3375e91d
https://hg.mozilla.org/mozilla-central/rev/3802f485af6f
https://hg.mozilla.org/mozilla-central/rev/a1f7a37f6390
https://hg.mozilla.org/mozilla-central/rev/b3f01b9ce2bf

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 20

[Jorg K (CEST = GMT+2)]

On MSVS C++ this gives a compiler warning, and turning warnings into errors, this doesn't compile:
https://hg.mozilla.org/mozilla-central/rev/b3f01b9ce2bf#l4.12

-const double MaxDelayTimeSeconds = 0.002;
+const float MaxDelayTimeSeconds = 0.002;

8:46.23 c:/mozilla-source/comm-central/dom/media/webaudio/blink/HRTFPanner.cpp(40): error C2220: warning treated as error - no 'object' file generated
8:46.23 c:/mozilla-source/comm-central/dom/media/webaudio/blink/HRTFPanner.cpp(40): warning C4305: 'initializing': truncation from 'double' to 'float'