Closed Bug 1474222 Opened 6 years ago Closed 6 years ago

ConvolverNode output should sometimes be mono

Categories

(Core :: Web Audio, defect, P3)

Product:
Core
Component:
Web Audio
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(7 files)

bug 1474222 uninline ConvolverNodeEngine::ProcessBlock()
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1474222 keep memory allocated for convolver volume premultiplication until no longer required
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1474222 change ConvolverNode output to mono for single channel convolution
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1474222 consider allocation size instead of used channel count for re-using AudioBlockBuffer
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1474222 test ConvolverNode up-mixing
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1474222 move convolver up-mixing test to wpt
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1474222 test ConvolverNode channelInterpretation changes
59 bytes, text/x-review-board-request
padenot
: review+
Details
https://webaudio.github.io/web-audio-api/#Convolution-channel-configurations "Single channel convolution operates on a mono audio input, using a mono impulse response, and generating a mono output." Detected by webaudio/the-audio-api/the-convolvernode-interface/convolver-response-1-chan.html: [convolver-response-1-chan.html] [X 1: Channel 1: Expected 0 for all values but found 1280 unexpected values: \n\tIndex\tActual\n\t[0\]\t-1.4901161193847656e-7\n\t[1\]\t-8.940696716308594e-8\n\t[2\]\t0.3311062455177307\n\t[3\]\t0.6248594522476196\n\t...and 1276 more errors.]
Priority: -- → P3
See Also: → 1474470
Depends on: 1475158
Depends on: 1474470
See Also: 1474470 →
Blocks: 1476231
Comment on attachment 8992567 [details] bug 1474222 uninline ConvolverNodeEngine::ProcessBlock() https://reviewboard.mozilla.org/r/257426/#review264702
Attachment #8992567 - Flags: review?(padenot) → review+
Comment on attachment 8992568 [details] bug 1474222 keep memory allocated for convolver volume premultiplication until no longer required https://reviewboard.mozilla.org/r/257428/#review264704
Attachment #8992568 - Flags: review?(padenot) → review+
Comment on attachment 8992569 [details] bug 1474222 change ConvolverNode output to mono for single channel convolution https://reviewboard.mozilla.org/r/257430/#review264722 ::: dom/media/webaudio/ConvolverNode.cpp:53 (Diff revision 1) > + // convolver is used for mono processing. When stereo input arrives after > + // mono input, the right convolver has no history and so would not up-mix > + // the tail still in the left convolver. If channelInterpretation is > + // "speakers" when the input changes from mono to stereo, then the > + // difference of input channel signals is convolved and added to the left > + // channel to produce the right output. This up-mixes the output in the Maybe change the language here a bit, this can be read in a couple of ways: - "to produce the right output" -> "to produce the correct output" - "to produce the right output" -> "to produce the right channel of the output" ::: dom/media/webaudio/ConvolverNode.cpp:55 (Diff revision 1) > + // the tail still in the left convolver. If channelInterpretation is > + // "speakers" when the input changes from mono to stereo, then the > + // difference of input channel signals is convolved and added to the left > + // channel to produce the right output. This up-mixes the output in the > + // same way as a mono input would be up-mixed when a stereo input is > + // added. Add a comment about "If channelInterpretation is "discrete" ... ", since it's a case we're handling in the processing. ::: dom/media/webaudio/ConvolverNode.cpp:236 (Diff revision 1) > + ChannelInterpretation channelInterpretation = > + aStream->GetChannelInterpretation(); > + if (inputChannelCount == 2) { > + if (mRemainingRightOutput <= 0) { > + // Will start the second convolver. Choose to convolve the right > + // channel directly if the is no left tail to up-mix or up-mixing s/if the/if there/ ::: dom/media/webaudio/ConvolverNode.cpp:258 (Diff revision 1) > + if (channelInterpretation == ChannelInterpretation::Discrete && > + mRightConvolverMode == RightConvolverMode::Difference) { > + // channelInterpretation has changed since the second convolver was > + // added. Up-mixing input would produce a silent right channel, but > + // the second channel needs to be a difference for the > + // RightConvolverMode. Invert the first channel for difference. I don't understand the comment here. ::: dom/media/webaudio/ConvolverNode.cpp:260 (Diff revision 1) > + // channelInterpretation has changed since the second convolver was > + // added. Up-mixing input would produce a silent right channel, but > + // the second channel needs to be a difference for the > + // RightConvolverMode. Invert the first channel for difference. > + float* right = mReverbInput.ChannelFloatsForWrite(1); > + AudioBlockInPlaceScale(right, -1.0f); We're using phase cancelation here, right? ::: dom/media/webaudio/ConvolverNode.cpp:260 (Diff revision 1) > + // channelInterpretation has changed since the second convolver was > + // added. Up-mixing input would produce a silent right channel, but > + // the second channel needs to be a difference for the > + // RightConvolverMode. Invert the first channel for difference. > + float* right = mReverbInput.ChannelFloatsForWrite(1); > + AudioBlockInPlaceScale(right, -1.0f); We're using phase cancellation here, so that we convolve 2 channels but output only one? I don't think I understand this bit, sorry.
Attachment #8992569 - Flags: review?(padenot)
Comment on attachment 8992569 [details] bug 1474222 change ConvolverNode output to mono for single channel convolution https://reviewboard.mozilla.org/r/257430/#review264760 Is there some code or a test that would show how this is supposed to work, the expected output for different scenarios? I find this a bit hard to reason about.
No longer blocks: 1476231
Depends on: 1480661
Comment on attachment 8992569 [details] bug 1474222 change ConvolverNode output to mono for single channel convolution https://reviewboard.mozilla.org/r/257430/#review264760 I'll submit some tests, which caught a couple of significant oversights. https://bugzilla.mozilla.org/attachment.cgi?id=8990853 is also a testcase for the common scenario. I've gone over the comments and attempted to make them explain better. I think it is best to think of what the second convolver processes and the kind of up-mixing as orthogonal procedures.
Comment on attachment 8992569 [details] bug 1474222 change ConvolverNode output to mono for single channel convolution https://reviewboard.mozilla.org/r/257430/#review264722 > Maybe change the language here a bit, this can be read in a couple of ways: > - "to produce the right output" -> "to produce the correct output" > - "to produce the right output" -> "to produce the right channel of the output" Good point thanks. There were a few occurrances of that. > Add a comment about "If channelInterpretation is "discrete" ... ", since it's a case we're handling in the processing. I'd prefer not to add in the complexity of the different up-mixing for different channelInterpretations here. RightConvolverMode determines whether the second convolver processes r or r - l. That is orthogonal to the kind of up-mixing required for channelInterpretation, except that the channelInterpretation at the time of starting the right convolver affects the RightConvolverMode. I've re-worded this somewhat to clarify. > s/if the/if there/ Thanks. > I don't understand the comment here. It didn't help that the code was not correct, which was discovered by a test I added. I've dropped the sentence about "invert", because the fact that the second convolver is actually processing -l is largely irrelevant. I've made the other sentences more precise. > We're using phase cancellation here, so that we convolve 2 channels but output only one? I don't think I understand this bit, sorry. Sometimes we need to convolve 2 channels when there is only one input. I've re-worked comments and fixed the code. > We're using phase cancelation here, right? I don't know what phase cancellation is, but I don't think that is the technique here.
Comment on attachment 8998390 [details] bug 1474222 consider allocation size instead of used channel count for re-using AudioBlockBuffer https://reviewboard.mozilla.org/r/261620/#review268934 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/webaudio/AudioBlock.cpp:90 (Diff revision 1) > { > return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf); > } > > private: > - AudioBlockBuffer() {} > + AudioBlockBuffer(uint32_t aChannelsAllocated) Error: Bad implicit conversion constructor for 'AudioBlockBuffer' [clang-tidy: mozilla-implicit-constructor] AudioBlockBuffer(uint32_t aChannelsAllocated) ^
Comment on attachment 8992569 [details] bug 1474222 change ConvolverNode output to mono for single channel convolution https://reviewboard.mozilla.org/r/257430/#review269424 ::: dom/media/webaudio/ConvolverNode.cpp:297 (Diff revision 3) > + AddScaledLeftToRight(&mReverbInput, -1.0f); > + } > + } else if (mRemainingRightHistory > 0) { > + // There is one channel of input, but a second convolver also > + // requires input. Up-mix appropriately for the second convolver. > + if ((mRightConvolverMode == RightConvolverMode::Difference) ^ Weird to have ^ on booleans, but it works. Maybe it's best to spell it out in code, though, this whole block is complex enough as it is.
Attachment #8992569 - Flags: review?(padenot) → review+
Comment on attachment 8998390 [details] bug 1474222 consider allocation size instead of used channel count for re-using AudioBlockBuffer https://reviewboard.mozilla.org/r/261620/#review269426
Attachment #8998390 - Flags: review?(padenot) → review+
Comment on attachment 8998391 [details] bug 1474222 test ConvolverNode up-mixing https://reviewboard.mozilla.org/r/261622/#review269428 This works if we change the spec as you proposed in web-audio-api issue #25 and now in #1718. Couldn't this be tested with `AudioBufferSourceNode` with specific channel counts, a gain node to mix everything, and calling start/stop at the right time ? I found the use of the delay line unintuitive here. ::: dom/media/webaudio/test/test_convolver-upmixing-1-channel-response.html:108 (Diff revision 2) > + leftSignal.start(); > + rightSignal.start(); > + monoSignal.connect(testConvolver); > + monoSignal.connect(referenceMonoConvolver); > + leftSignal.connect(stereoMerger, 0, 0); > + rightSignal.connect(stereoMerger, 0, 1); The spec (unlike Gecko) does not offer the guarantee that the `start` and `connect` calls are going to be resolved together. It's probably best to connect first and start later. ::: dom/media/webaudio/test/test_convolver-upmixing-1-channel-response.html:142 (Diff revision 2) > +promise_test(() => test_linear_upmixing("discrete", MONO_FRAMES), > + "discrete"); > +// Gecko uses a separate path for "speakers" up-mixing when the convolver's > +// first input is stereo, so test that separately. > +promise_test(() => test_linear_upmixing("speakers", 0), > + "speakers, initially stereo"); Have you planned to open an issue in the spec so that the behaviour to adopt when changing channel count and interpretation dynamically change during processing? I can do it otherwise. The way you do it intuitively makes sense, but it's probably an edge case that needs to be specced. Taking this further, this applies to all node that can produce a tail as well, I think.
Attachment #8998391 - Flags: review?(padenot) → review+
Comment on attachment 8998392 [details] bug 1474222 move convolver up-mixing test to wpt https://reviewboard.mozilla.org/r/261624/#review269432
Attachment #8998392 - Flags: review?(padenot) → review+
Comment on attachment 8998393 [details] bug 1474222 test ConvolverNode channelInterpretation changes https://reviewboard.mozilla.org/r/261626/#review269434
Attachment #8998393 - Flags: review?(padenot) → review+
Comment on attachment 8992569 [details] bug 1474222 change ConvolverNode output to mono for single channel convolution https://reviewboard.mozilla.org/r/257430/#review269424 > Weird to have ^ on booleans, but it works. > > Maybe it's best to spell it out in code, though, this whole block is complex enough as it is. I've added an assert that spells out the two situations that are involved here. I kinda like how the xor indicates this covers opposite corners of a 2D matrix. Knowing that is necessary to understand that the else overs the other corners. With the assert, this is spelled out in every case.
Comment on attachment 8998391 [details] bug 1474222 test ConvolverNode up-mixing https://reviewboard.mozilla.org/r/261622/#review269428 For AudioBufferSourceNode "The number of channels of the output always equals the number of channels of the AudioBuffer assigned to the buffer attribute, or is one channel of silence if buffer is null" and so start and stop would have no effect. Changing that is covered by https://github.com/WebAudio/web-audio-api/issues/462 and may happen as part of fixing https://github.com/WebAudio/web-audio-api/issues/1471 but we don't have a resolution on either of these yet. I think there will be better ways to do this in a future version of the API, but this was the best I could come up with right now. Once we have a solution for https://github.com/WebAudio/web-audio-api/issues/21 we'll have a way to easily make any controlled changes during offline rendering. > The spec (unlike Gecko) does not offer the guarantee that the `start` and `connect` calls are going to be resolved together. It's probably best to connect first and start later. Both of these calls are before startRendering() and so they will be resolved before any rendering. But following the same convention for offline rendering as is often used for realtime rendering might provide advantages should this code be copied, and so I've moved the start calls to after the connect calls.
Comment on attachment 8998392 [details] bug 1474222 move convolver up-mixing test to wpt https://reviewboard.mozilla.org/r/261624/#review269564 ::: testing/web-platform/tests/webaudio/the-audio-api/the-convolvernode-interface/convolver-upmixing-1-channel-response.html:102 (Diff revision 3) > + monoSignal.start(); > + leftSignal.start(); > + rightSignal.start(); > monoSignal.connect(testConvolver); > monoSignal.connect(referenceMonoConvolver); > leftSignal.connect(stereoMerger, 0, 0); > rightSignal.connect(stereoMerger, 0, 1); > - monoSignal.start(); > - leftSignal.start(); > - rightSignal.start(); That's creepy that histedit reverted that change when reordering the fix to apply it before the copy. I'll fix that in the next revision.
Comment on attachment 8998391 [details] bug 1474222 test ConvolverNode up-mixing https://reviewboard.mozilla.org/r/261622/#review269428 > Have you planned to open an issue in the spec so that the behaviour to adopt when changing channel count and interpretation dynamically change during processing? I can do it otherwise. > > The way you do it intuitively makes sense, but it's probably an edge case that needs to be specced. > > Taking this further, this applies to all node that can produce a tail as well, I think. I've now filed https://github.com/WebAudio/web-audio-api/issues/1719
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7574547bff84 uninline ConvolverNodeEngine::ProcessBlock() r=padenot https://hg.mozilla.org/integration/autoland/rev/67d29891df61 keep memory allocated for convolver volume premultiplication until no longer required r=padenot https://hg.mozilla.org/integration/autoland/rev/e6fc5ab5dd8c consider allocation size instead of used channel count for re-using AudioBlockBuffer r=padenot https://hg.mozilla.org/integration/autoland/rev/3ac736416349 change ConvolverNode output to mono for single channel convolution r=padenot https://hg.mozilla.org/integration/autoland/rev/a10cd240dcaf test ConvolverNode up-mixing r=padenot https://hg.mozilla.org/integration/autoland/rev/7e3fde45f417 move convolver up-mixing test to wpt r=padenot https://hg.mozilla.org/integration/autoland/rev/5a18b9c9a5a6 test ConvolverNode channelInterpretation changes r=padenot
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12520 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Blocks: 1444508
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: