Handle channel count changes in delay node

RESOLVED FIXED in mozilla30

Status

()

defect
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: karlt)

Tracking

(Blocks 1 bug)

Trunk
mozilla30
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [blocking-webaudio-])

Attachments

(5 attachments, 3 obsolete attachments)

Follow-up from bug 853721.
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
(Assignee)

Comment 2

6 years ago
I've written a post for public-audio, but I'm waiting for <87ob8p0zvi.fsf@karlt.net>, send just now, to appear in the archives, so I can reference that.
Whiteboard: [blocking-webaudio-]
(Assignee)

Updated

6 years ago
Blocks: 922639
(Assignee)

Updated

6 years ago
Blocks: 924718
Priority: -- → P2
(Assignee)

Updated

5 years ago
Blocks: 932400
(Assignee)

Updated

5 years ago
Assignee: nobody → karlt
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 976927
(Assignee)

Comment 4

5 years ago
I'd like to separate writing to and reading from the delay buffer for bug 932400, and the names for Read() and Write() methods make more sense if this is called "buffer" rather than "processor".
Attachment #8381958 - Flags: review?(paul)
(Assignee)

Comment 5

5 years ago
A subsequent patch will write the whole inputBus AudioChunk to the DelayBuffer in one call, which would make this loop seem more silly.
Attachment #8381963 - Flags: review?(paul)
(Assignee)

Comment 6

5 years ago
(Assignee)

Comment 7

5 years ago
Simplifying a little before a rewrite of DelayBuffer.
Attachment #8381967 - Flags: review?(paul)
(Assignee)

Comment 8

5 years ago
The basic idea is to write out the signal that came in with the same number of channels it had as when it came in.  Things get a bit more complicated when one output block may be derived from more than one input block, each having different numbers of channels.  When this happens, the input blocks with fewer channels are upsampled, so as not to lose (or distort) any signal in the block with more channels.

Handling channel count changes required a rewrite of the core code, so this version includes the separate read and write for bug 932400.

HRTFPanner no longer uses exponential decay (with time constant 20ms) for
delay changes, but a smoother linear transition during cross-fade time (~45s).
Attachment #8381982 - Flags: review?(paul)
Comment on attachment 8381958 [details] [diff] [review]
rename DelayProcessor to DelayBuffer

Review of attachment 8381958 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.
Attachment #8381958 - Flags: review?(paul) → review+
(Assignee)

Comment 10

5 years ago
Working around bug 976927, adding some necessary includes provided only by unified_sources, and a stray initialization on a member declaration.

https://tbpl.mozilla.org/?tree=Try&rev=37c07c746c27
and some other platforms on the previous version:
https://tbpl.mozilla.org/?tree=Try&rev=aabd32aa9d0d
Attachment #8381982 - Attachment is obsolete: true
Attachment #8381982 - Flags: review?(paul)
(Assignee)

Updated

5 years ago
Attachment #8382710 - Flags: review?(paul)
Attachment #8381967 - Flags: review?(paul) → review+
Attachment #8381963 - Flags: review?(paul) → review+
(Assignee)

Comment 11

5 years ago
Comment on attachment 8382710 [details] [diff] [review]
part 4 v1.1: handle DelayNode channel count changes and separate buffer read and write

Will fix a couple of bugs here.
Attachment #8382710 - Flags: review?(paul)
(Assignee)

Comment 12

5 years ago
Handle null buffer on allocation failure and change the public API a little to
simplify and clarify differences in Read methods.
Add NextBlock call in HRTF panner.
Invalidate upmix cache when channel count changes, but read chunk stays the
same.

Try runs with new test cases for this bug and the HRTF panner bug in previous patch.
https://tbpl.mozilla.org/?tree=Try&rev=0a89e4f5fc80
https://tbpl.mozilla.org/?tree=Try&rev=6862e80dd11c
Attachment #8382710 - Attachment is obsolete: true
Attachment #8383490 - Flags: review?(paul)
(Assignee)

Updated

5 years ago
Attachment #8383490 - Flags: review?(paul)
(Assignee)

Comment 13

5 years ago
Fixed more bugs:

mVolume was being used uninitialized when default initialized null chunks were
read from the delay buffer.  This was multiplied by zero, and so usually
didn't cause problems, but sometimes it could be multiplied by inf or NaN
producing NaNs in the output.

The OOM path in ReadChannel() for pre-allocated AudioChunks, in the previous
patch, was bad.
Attachment #8383490 - Attachment is obsolete: true
Attachment #8383574 - Flags: review?(paul)
Comment on attachment 8383574 [details] [diff] [review]
part 4 v1.3: handle DelayNode channel count changes and separate buffer read and write

Review of attachment 8383574 [details] [diff] [review]:
-----------------------------------------------------------------

s/the input blocks with fewer channels are upsampled/the input blocks with fewer channels are upmixed/, in the commit message.
Comment on attachment 8383574 [details] [diff] [review]
part 4 v1.3: handle DelayNode channel count changes and separate buffer read and write

Review of attachment 8383574 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/DelayBuffer.cpp
@@ +114,5 @@
> +       channel < readChannelsEnd; ++channel) {
> +    PodZero(outputChannels[channel], WEBAUDIO_BLOCK_SIZE);
> +  }
> +
> +  for (unsigned i = 0; i < WEBAUDIO_BLOCK_SIZE; ++i) {

It's kind of sad that we have the two loops in this order, but the alternative might not be worth it, this is complicated enough as it is.
Attachment #8383574 - Flags: review?(paul) → review+
(Assignee)

Updated

5 years ago
No longer blocks: 922639
Duplicate of this bug: 922639
You need to log in before you can comment on or make changes to this bug.