Make WebAudio capable of multi-channel output. The number of output channels will be the max number of channels of the destination device.

Does that mean you intend to limit the number of decoded audio channels based on what the output device support, or will this only affect playback? There shouldn't be a max number of channels with webaudio as far as the JS developer is concerned. We currently limit to 8 channels, and this has been mentioned to me several times as a critical limitation of firefox.

This affects the output for WebAudio. Changes will occur in MSG so playback behavior will not change. Cubeb does not allow more than 8 channels so we should limit the max number of channel on that.

(In reply to Jean-Yves Avenard [:jya] from comment #1) > Does that mean you intend to limit the number of decoded audio channels > based on what the output device support, or will this only affect playback? We audio supports 32 channels inside the processing graph. When it's being pushed to the system via cubeb, it's (for now) stereo, but this bug is about changing that so that if you're processing 8 channels and have 7.1 hardware, it works (for now it's downmixed to stereo). > There shouldn't be a max number of channels with webaudio as far as the JS > developer is concerned. Implementations MUST support at least 32 channels inside the graph [0], but can support more. There is the need to be limit, but it can be high. > We currently limit to 8 channels, and this has been mentioned to me several times as a critical limitation of firefox. HTMLMediaElement should support more than 8 channels for sure. The Web Audio API implementation in Firefox as it is today already supports processing up to 32 channels. [0]: https://webaudio.github.io/web-audio-api/#widl-BaseAudioContext-createBuffer-AudioBuffer-unsigned-long-numberOfChannels-unsigned-long-length-float-sampleRate

As it is today, none of the audio decoder used for decoding the comppressed stream, allow more than 8 channels input. https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaInfo.h#40 AudioInfo::IsValid checks the following: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaInfo.h#368 bool IsValid() const override { return mChannels > 0 && mChannels <= MAX_AUDIO_CHANNELS && mRate > 0 && mRate <= MAX_RATE; } So any file with a metadata indicating more than 8 channels will cause a decoding error. Additionally, when it comes to reordering the channel layout , ready for the output. The AudioConfig object can only process up to 8 channels. https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaInfo.h#604 these problems will be tackled in bug 1378077

I upload for feedback before continue with the review. Please note the followings: 1. Max number of channels in AudioContext is up to 32 but on MSG is up to 8. This is because graph driver/cubeb accept up to 8. 2. Undefined layout in cubeb file (just OSX here). Do we need to apply that change in cubeb before land this one?

Comment on attachment 8883576 [details] [diff] [review] multichannel-webaudio Review of attachment 8883576 [details] [diff] [review]: ----------------------------------------------------------------- 1. 32 for a regular AudioContext is good. We will need to bump the channel count limit soon, people need more channels. 2. We want to have cubeb accept any number of output channel, only limited by what the driver supports. In particular, we want to have SoundFlower 64ch. device working for testing. undefined layout should be legal and mean that it's not one of the classic mappings (2.0, 2.1, 5.1, etc.). I think we need to land the cubeb changes before yes. Other than that, looks good! ::: dom/media/AudioBufferUtils.h @@ +38,5 @@ > mSamples(0), > + mSampleWriteOffset(1), > + mChannels(aChannels) > + > + {MOZ_ASSERT(aChannels);} New lines please. @@ +164,5 @@ > return framesToWrite; > } > private: > /* The spilled data. */ > + T* mBuffer; unique ptr or auto ptr or something. ::: dom/media/GraphDriver.cpp @@ +561,5 @@ > } > > AudioCallbackDriver::AudioCallbackDriver(MediaStreamGraphImpl* aGraphImpl) > : GraphDriver(aGraphImpl) > + , mChannelCount(mGraphImpl->AudioChannelCount()) I'd prefer we would put "output" somewhere in those names, and to have it be symmetrical with input. ::: dom/media/GraphDriver.h @@ +469,5 @@ > /* Start the cubeb stream */ > bool StartStream(); > friend class AsyncCubebTask; > bool Init(); > /* MediaStreamGraphs are always down/up mixed to stereo for now. */ Fix the comment here. ::: dom/media/MediaStreamGraphImpl.h @@ +453,5 @@ > > // Always stereo for now. > + uint32_t AudioChannelCount() const { > + return std::min<uint32_t>(8, CubebUtils::MaxNumberOfChannels()); > + } Why 8 here ? cubeb supports more than that. ::: dom/media/gtest/TestAudioBuffers.cpp @@ +13,5 @@ > > TEST(AudioBuffers, Test) > { > + mozilla::AudioCallbackBufferWrapper<float> mBuffer(CHANNELS); > + mozilla::SpillBuffer<float, 128> b(CHANNELS); You might want to make a for loop and test from 1 to 8 channels or something.

(In reply to Paul Adenot (:padenot) from comment #6) > Why 8 here ? cubeb supports more than that. Cubeb does not allow to request more than 8 channels: http://searchfox.org/mozilla-central/source/media/libcubeb/src/cubeb.c#66

Comment on attachment 8883576 [details] [diff] [review] multichannel-webaudio Review of attachment 8883576 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/AudioBufferUtils.h @@ +106,5 @@ > uint32_t mSamples; > /* The position at which new samples should be written. We want to return to > * the audio callback iff this is equal to mSamples. */ > uint32_t mSampleWriteOffset; > + uint32_t mChannels; should be const @@ +144,3 @@ > // If we didn't empty the spill buffer for some reason, shift the remaining data down > if (mPosition > 0) { > + PodMove(mBuffer, mBuffer + FramesToSamples(mChannels, framesToWrite), should have release assert that FramesToSamples(mChannels, framesToWrite) + mPosition is always < BLOCK_SIZE * mChannels @@ +157,3 @@ > mPosition)); > > + PodCopy(mBuffer + mPosition, aInput, FramesToSamples(mChannels, same here: should have release assert that FramesToSamples(mChannels, framesToWrite) + mPosition is always < BLOCK_SIZE * mChannels @@ +164,5 @@ > return framesToWrite; > } > private: > /* The spilled data. */ > + T* mBuffer; +1, use UniquePtr @@ +169,4 @@ > /* The current write position, in samples, in the buffer when filling, or the > * amount of buffer filled when emptying. */ > uint32_t mPosition; > + uint32_t mChannels; should be const ::: dom/media/MediaStreamGraphImpl.h @@ +451,5 @@ > mStreamOrderDirty = true; > } > > // Always stereo for now. > + uint32_t AudioChannelCount() const { { on a new line per coding style (and also preserve consistency with this code

(In reply to Jean-Yves Avenard [:jya] from comment #8) > should have release assert that FramesToSamples(mChannels, framesToWrite) + > mPosition is always < BLOCK_SIZE * mChannels I guess you mean less or equal not just less

(In reply to Alex Chronopoulos [:achronop] from comment #9) > (In reply to Jean-Yves Avenard [:jya] from comment #8) > > > should have release assert that FramesToSamples(mChannels, framesToWrite) + > > mPosition is always < BLOCK_SIZE * mChannels > > I guess you mean less or equal not just less no, because indexed are from 0. So if size is 1, you can't have an index greater than 0

When mPosition is 0 framesToWrite can be equal to BLOCK_SIZE thus we need less or equal. I made a run with just less and hit the assert.

(In reply to Alex Chronopoulos [:achronop] from comment #11) > When mPosition is 0 framesToWrite can be equal to BLOCK_SIZE thus we need > less or equal. I made a run with just less and hit the assert. That code is placed in if (mPosition > 0) { .. } block. so how could you hit that assert? But yeah, my bad. I was thinking about mPosition always being < BLOCK_SIZE * mChannels, except if framesToWrite is 0.

(In reply to Jean-Yves Avenard [:jya] from comment #12) > That code is placed in if (mPosition > 0) { .. } block. so how could you hit > that assert? I was referring to the same assert in SpillBuffer::Fill method.

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review167344 ::: dom/media/AudioBufferUtils.h:140 (Diff revision 1) > * the number of frames written. */ > - uint32_t Empty(AudioCallbackBufferWrapper<T, CHANNELS>& aBuffer) { > + uint32_t Empty(AudioCallbackBufferWrapper<T>& aBuffer) { > uint32_t framesToWrite = std::min(aBuffer.Available(), > - SamplesToFrames(CHANNELS, mPosition)); > + SamplesToFrames(mChannels, mPosition)); > > - aBuffer.WriteFrames(mBuffer, framesToWrite); > + aBuffer.WriteFrames(mBuffer.get(), framesToWrite); Is the `.get()` needed here? ::: dom/media/MediaStreamGraphImpl.h:454 (Diff revision 1) > void SetStreamOrderDirty() > { > mStreamOrderDirty = true; > } > > // Always stereo for now. Adjust the comment here. ::: dom/media/VideoUtils.cpp:147 (Diff revision 1) > } > return buffered; > } > > +void DownmixToStereo(const mozilla::AudioDataValue* aBuffer, > + const uint32_t inChannels, This should be aInChannels. ::: dom/media/VideoUtils.cpp:162 (Diff revision 1) > +#else > + int sample = 0; > +#endif > + for (uint32_t ch = channels; ch < inChannels; ++ch) { > + // The sample of the buffer would be interleaved. > + sample += aBuffer[fIdx*channels] / inChannels; It's unclear what we want to do here. It's a bit arbitrary to lower the energy of all the channels but front left and right, and to sum them. Why not doing a normal summation ? Here, the surround right will bleed in the front left, for example, which will be confusing for the AEC I suppose. Have a look here: http://searchfox.org/mozilla-central/source/dom/media/AudioConverter.cpp#163, it implements what you want to do. I don't know if it's easy to use, but you can use the same matrix. ::: dom/media/gtest/TestAudioBuffers.cpp:21 (Diff revision 1) > - float fromCallback[SAMPLES]; > - float other[SAMPLES]; > + mozilla::AudioCallbackBufferWrapper<float> mBuffer(channels); > + mozilla::SpillBuffer<float, 128> b(channels); > +#if defined(_WIN32) || defined(WIN32) > + /* Variable length array is not allowed in windows compiler > + * (C2131: expression did not evaluate to a constant) */ > + float * fromCallback = new float[samples]; Use some RAII construct, you're leaking, here. ::: dom/media/webaudio/AudioContext.cpp:614 (Diff revision 1) > > uint32_t > AudioContext::MaxChannelCount() const > { > - return mIsOffline ? mNumberOfChannels : CubebUtils::MaxNumberOfChannels(); > + return std::min<uint32_t>(WebAudioUtils::MaxChannelCount, > + mIsOffline ? mNumberOfChannels : CubebUtils::MaxNumberOfChannels()); What happens if the audio count changes during the lifetime of an `AudioContext` ? This is not something that is specified on the web platform for now, there is no answer. Just make sure it does not crash or something. ::: dom/media/webrtc/AudioOutputObserver.h:53 (Diff revision 1) > uint32_t mChunkSize; > > // chunking to 10ms support > FarEndAudioChunk *mSaved; // can't be nsAutoPtr since we need to use free(), not delete > uint32_t mSamplesSaved; > + AudioDataValue mDownmixBuffer[480 * 2];// big enough to hold 10ms in max rate [MAX_SAMPLING_FREQ * MAX_CHANNELS / 100] Why not putting the computation in the brackets instead of putting number in the brackets and a comment with the computation ? ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:106 (Diff revision 1) > AudioOutputObserver::InsertFarEnd(const AudioDataValue *aBuffer, uint32_t aFrames, bool aOverran, > int aFreq, int aChannels) > { > + // Prepare for downmix if needed > + int channels = aChannels; > + AudioDataValue * buffer = const_cast<AudioDataValue*>(aBuffer); Unclear if it's better to `const_cast` here or to remove the `const` in the signature.

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review167376 ::: dom/media/VideoUtils.cpp:146 (Diff revision 1) > startOffset = aStream->GetNextCachedData(endOffset); > } > return buffered; > } > > +void DownmixToStereo(const mozilla::AudioDataValue* aBuffer, This doesn't belong to VideoUtils; it has nothing to do with video. To dowmix to stereo; please use the AudioConverter class. That and this downmix function doesn't preserve volumes. ::: dom/media/VideoUtils.cpp:170 (Diff revision 1) > + aOutBuffer[fIdx + 1] = aBuffer[fIdx + 1] + sample; > + } > +} > + > void DownmixStereoToMono(mozilla::AudioDataValue* aBuffer, > uint32_t aFrames) Ditto here, not sure how this ended here. ::: dom/media/gtest/TestAudioBuffers.cpp:21 (Diff revision 1) > - float fromCallback[SAMPLES]; > - float other[SAMPLES]; > + mozilla::AudioCallbackBufferWrapper<float> mBuffer(channels); > + mozilla::SpillBuffer<float, 128> b(channels); > +#if defined(_WIN32) || defined(WIN32) > + /* Variable length array is not allowed in windows compiler > + * (C2131: expression did not evaluate to a constant) */ > + float * fromCallback = new float[samples]; float* , please look after coding style ::: dom/media/gtest/TestAudioBuffers.cpp:39 (Diff revision 1) > mBuffer.SetBuffer(fromCallback, FRAMES); > > // Fill the SpillBuffer with data. > ASSERT_TRUE(b.Fill(other, 15) == 15); > ASSERT_TRUE(b.Fill(other, 17) == 17); > - for (uint32_t i = 0; i < 32 * CHANNELS; i++) { > + for (uint32_t i = 0; i < 32 * channels; i++) { why 32 * here? is 32 * channels < samples? should add an assert here. ::: dom/media/gtest/TestAudioBuffers.cpp:50 (Diff revision 1) > > // Check available return something reasonnable > ASSERT_TRUE(mBuffer.Available() == FRAMES - 32); > > // Fill the buffer with the rest of the data > - mBuffer.WriteFrames(other + 32 * CHANNELS, FRAMES - 32); > + mBuffer.WriteFrames(other + 32 * channels, FRAMES - 32); same here. need to static assert that FRAMES > 32 and that 32*channels < samples ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:106 (Diff revision 1) > AudioOutputObserver::InsertFarEnd(const AudioDataValue *aBuffer, uint32_t aFrames, bool aOverran, > int aFreq, int aChannels) > { > + // Prepare for downmix if needed > + int channels = aChannels; > + AudioDataValue * buffer = const_cast<AudioDataValue*>(aBuffer); AudioDataValue* ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:108 (Diff revision 1) > { > + // Prepare for downmix if needed > + int channels = aChannels; > + AudioDataValue * buffer = const_cast<AudioDataValue*>(aBuffer); > + if (aChannels > MAX_CHANNELS) { > + channels = MAX_CHANNELS; the output would be wrong is channels > MAX_CHANNELS ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:159 (Diff revision 1) > } > > - int16_t *dest = &(mSaved->mData[mSamplesSaved * aChannels]); > - ConvertAudioSamples(aBuffer, dest, to_copy * aChannels); > + if (aChannels > MAX_CHANNELS) { > + DownmixToStereo(aBuffer, aChannels, to_copy, buffer); > + } > + int16_t *dest = &(mSaved->mData[mSamplesSaved * channels]); int16_t*

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review167344 > Is the `.get()` needed here? Yes > What happens if the audio count changes during the lifetime of an `AudioContext` ? > > This is not something that is specified on the web platform for now, there is no answer. > > Just make sure it does not crash or something. When the output changes the cubeb stream is re-initialized internally, in cubeb, with the same channel count. When the input channel count changes a new driver is instanciated with the max available output channel count for the current device. Let me know if that's suficient. > Why not putting the computation in the brackets instead of putting number in the brackets and a comment with the computation ? This is because MAX_SAMPLING_FREQ and MAX_CHANNELS are macros defined in cpp files. I do not want to declare macros in the header. If you do not like it I could create a global buffer in the cpp file but this seems worse to me.

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review167376 > why 32 * here? > > is 32 * channels < samples? > > should add an assert here. My patch does not affect the logic of the test. 32 is because buffered is filled with 15 and then with 17 frames. > same here. > need to static assert that FRAMES > 32 and that 32*channels < samples above > the output would be wrong is channels > MAX_CHANNELS We try to support that case as well by downmixing.

Comment on attachment 8892484 [details] Bug 1378070 - Implement multichannel WebAudio. https://reviewboard.mozilla.org/r/163456/#review169298 ::: dom/media/AudioConverter.h:200 (Diff revision 1) > } > return frames; > } > > + template <typename Value> > + size_t Process(Value* aOutBuffer, const Value* aInBuffer, size_t aFrames) you need to add a mechanism to bound the write, as you can't know for sure the amount of data you're about to write. and that's just an open door for security issues. Either pass a new argument, similar to ByteWriter, that would work with T type. or the length of aInBuffer is to be provided, and you need to add the mechanisms that will ensure we'll never write too much data. Personally, I think having a new constructor that takes a destination buffer and its size would be preferable. but if you think its too much work, that's okay. ::: dom/media/AudioConverter.h:207 (Diff revision 1) > + MOZ_DIAGNOSTIC_ASSERT(mIn.Format() == mOut.Format()); > + size_t frames = ProcessInternal(aOutBuffer, aInBuffer, aFrames); > + if (mIn.Rate() == mOut.Rate()) { > + return frames; > + } > + if (frames) { this doesnt seem right, while would you drain the resampler when we have samples to process? ::: dom/media/MediaStreamGraphImpl.h (Diff revision 1) > void SetStreamOrderDirty() > { > mStreamOrderDirty = true; > } > > - // Always stereo for now. out of scope for this change. ::: dom/media/VideoUtils.h (Diff revision 1) > // Downmix Stereo audio samples to Mono. > // Input are the buffer contains stereo data and the number of frames. > void DownmixStereoToMono(mozilla::AudioDataValue* aBuffer, > uint32_t aFrames); > > -void DownmixToStereo(const mozilla::AudioDataValue* aBuffer, same here. please split this patch in two. One adding modification of AudioConverter to work in specified buffer, and the other making use of it, replacing DownmixToStereo

Comment on attachment 8892484 [details] Bug 1378070 - Implement multichannel WebAudio. https://reviewboard.mozilla.org/r/163456/#review169298 > you need to add a mechanism to bound the write, as you can't know for sure the amount of data you're about to write. and that's just an open door for security issues. > > Either pass a new argument, similar to ByteWriter, that would work with T type. > > or the length of aInBuffer is to be provided, and you need to add the mechanisms that will ensure we'll never write too much data. > > Personally, I think having a new constructor that takes a destination buffer and its size would be preferable. but if you think its too much work, that's okay. As we discussed in IRC I changed the new Process() method to accept an AlignedBuffer which ensures safe write.

Comment on attachment 8892484 [details] Bug 1378070 - Implement multichannel WebAudio. https://reviewboard.mozilla.org/r/163456/#review169966 ::: dom/media/AudioConverter.h:211 (Diff revisions 1 - 2) > + size_t frames = ProcessInternal(aOutBuffer.Data(), aInBuffer, aFrames); > if (mIn.Rate() == mOut.Rate()) { > return frames; > } > - if (frames) { > - frames = DrainResampler(aOutBuffer); > + if (!frames) { > + return DrainResampler(aOutBuffer.Data()); you need to do like before, store frames, so that you can adjust the length of the AlignedBuffer. That way the AligneBuffer always have the length of what it contains. ::: dom/media/AudioConverter.h:213 (Diff revisions 1 - 2) > - if (frames) { > - frames = DrainResampler(aOutBuffer); > + if (!frames) { > + return DrainResampler(aOutBuffer.Data()); > - } else { > - frames = ResampleAudio(aOutBuffer, aInBuffer, aFrames); > } > - return frames; > + return ResampleAudio(aOutBuffer.Data(), aInBuffer, aFrames); bonus points if you adjust the method: AudioDataBuffer<Format, Value> Process(AudioDataBuffer<Format, Value>&& aBuffer) so that it uses this one. Otherwise it's all duplicated code. ::: dom/media/gtest/TestAudioBuffers.cpp (Diff revision 1) > mozilla::AudioCallbackBufferWrapper<float> mBuffer(channels); > mozilla::SpillBuffer<float, 128> b(channels); > -#if defined(_WIN32) || defined(WIN32) > - /* Variable length array is not allowed in windows compiler > + std::vector<float> fromCallback(samples, 0.0); > + std::vector<float> other(samples, 1.0); > - * (C2131: expression did not evaluate to a constant) */ > - float * fromCallback = new float[samples]; float* please store this in UniquePtr, you're leaking here. ::: dom/media/gtest/TestAudioBuffers.cpp:35 (Diff revision 1) > } > > // Empty it in the AudioCallbackBufferWrapper > ASSERT_TRUE(b.Empty(mBuffer) == 32); > > - // Check available return something reasonnable > + // Check available return something reasonable reasonable the previous spelling was correct. ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp (Diff revision 1) > AudioOutputObserver::InsertFarEnd(const AudioDataValue *aBuffer, uint32_t aFrames, bool aOverran, > int aFreq, int aChannels) > { > // Prepare for downmix if needed > int channels = aChannels; > - AudioDataValue * buffer = const_cast<AudioDataValue*>(aBuffer); AudioDataValue* ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp (Diff revision 1) > - DownmixToStereo(aBuffer, aChannels, to_copy, buffer); > + DownmixToStereo(aBuffer, aChannels, to_copy, mDownmixBuffer); > + ConvertAudioSamples(mDownmixBuffer, dest, to_copy * channels); > + } else { > + ConvertAudioSamples(aBuffer, dest, to_copy * channels); > } > - int16_t *dest = &(mSaved->mData[mSamplesSaved * channels]); int16_t*

Comment on attachment 8893453 [details] Bug 1378070 - Apply review comments. https://reviewboard.mozilla.org/r/164544/#review169968 ::: commit-message-e67b1:1 (Diff revision 1) > +Bug 1378070 - Apply review comments. r?padenot,jya please have a proper commit message. Merge this in P1 and P2, and have a P3 making use of the new AudioConverter methods.

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review170500 ::: dom/media/AudioConverter.h:198 (Diff revision 3) > > + template <typename Value> > + size_t Process(AlignedBuffer<Value>& aOutBuffer, const Value* aInBuffer, size_t aFrames) > + { > + MOZ_DIAGNOSTIC_ASSERT(mIn.Format() == mOut.Format()); > + if (!aOutBuffer.SetLength(FramesOutToSamples(aFrames))) { The danger with this, is that this is dependent on having the AlignedAudioBjffer Setlength never changing the data structure when called with aa lower value. Otherwise the conversion in place will be broken as the destination length is modified before we've read it.

Comment on attachment 8892484 [details] Bug 1378070 - Implement multichannel WebAudio. https://reviewboard.mozilla.org/r/163456/#review171092

(In reply to Alex Chronopoulos [:achronop] from comment #7) > (In reply to Paul Adenot (:padenot) from comment #6) > > > Why 8 here ? cubeb supports more than that. > > Cubeb does not allow to request more than 8 channels: > http://searchfox.org/mozilla-central/source/media/libcubeb/src/cubeb.c#66 That is something that we need to change at some point.

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review178068 None of those changes are necessary. It already does what it's supposed to do. For safety, just assert that the buffer is null only if aFrames is also 0. ::: dom/media/AudioConverter.h:135 (Diff revisions 3 - 4) > { > MOZ_DIAGNOSTIC_ASSERT(mIn.Format() == mOut.Format() && mIn.Format() == Format); > AudioDataBuffer<Format, Value> buffer = Move(aBuffer); > if (CanWorkInPlace()) { > AlignedBuffer<Value> temp = buffer.Forget(); > - Process(temp, temp.Data(), SamplesInToFrames(buffer.Length())); > + Process(temp, temp.Data(), SamplesInToFrames(temp.Length())); good spotting.. damn ::: dom/media/AudioConverter.h:198 (Diff revisions 3 - 4) > > template <typename Value> > size_t Process(AlignedBuffer<Value>& aOutBuffer, const Value* aInBuffer, size_t aFrames) > { > MOZ_DIAGNOSTIC_ASSERT(mIn.Format() == mOut.Format()); > + size_t frames = 0; actually none of this is necessary of aInBuffer is null, then aFrames must be 0. so you instead assert that aFrames && aInBuffer || !aFrames ::: dom/media/AudioConverter.h:200 (Diff revisions 3 - 4) > size_t Process(AlignedBuffer<Value>& aOutBuffer, const Value* aInBuffer, size_t aFrames) > { > MOZ_DIAGNOSTIC_ASSERT(mIn.Format() == mOut.Format()); > + size_t frames = 0; > + MOZ_ALWAYS_TRUE(aOutBuffer.SetLength(0)); > + if (aInBuffer) { unecessary as aFrames is 0, and as such ProcessInternal will immediately return ::: dom/media/AudioConverter.h:213 (Diff revisions 3 - 4) > + // Check if resampling is needed > if (mIn.Rate() == mOut.Rate()) { > return frames; > } > + // Prepare output in cases of drain and up-sampling > + if ((!frames || mOut.Rate() > mIn.Rate()) && unecessary. this is taken care of after because !frames will be true ::: dom/media/AudioConverter.h:214 (Diff revisions 3 - 4) > if (mIn.Rate() == mOut.Rate()) { > return frames; > } > + // Prepare output in cases of drain and up-sampling > + if ((!frames || mOut.Rate() > mIn.Rate()) && > + !aOutBuffer.SetLength(FramesOutToSamples(ResampleRecipientFrames(frames)))) { you call SetLength twice unecessarily in this case here.

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review178068 > unecessary. > > this is taken care of after because !frames will be true See bellow. > you call SetLength twice unecessarily in this case here. I am not sure I get the last two comments. When !frame we need to call SetLength() to make sure there is ebough room for the drain in the output. If 'frames!=0 && upsampling' we need again to call SetLength() to make sure there is ebough room for the extra frames in the output. Indeed I do SetLength twice but it is in perpuse. Or I am missing something. P.S. I push the latest version with that comment in case it helps. P.S. Before exit from an error case I call SetLength(0) to the output. Let me know if we want that.

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review178068 > I am not sure I get the last two comments. When !frame we need to call SetLength() to make sure there is ebough room for the drain in the output. If 'frames!=0 && upsampling' we need again to call SetLength() to make sure there is ebough room for the extra frames in the output. Indeed I do SetLength twice but it is in perpuse. Or I am missing something. > > P.S. I push the latest version with that comment in case it helps. > P.S. Before exit from an error case I call SetLength(0) to the output. Let me know if we want that. When you call will a null buffer, you call process with aFrames = 0. if aFrames == 0, then ProcessInternal is a no-op (that could be made even more explicit by immediately returning 0 there) and it returns 0. so frames = 0. There's already a test later that check if frames is 0, which then drain the resampler and will set the lenght. With your change now, if aFrames is 0, you no longer drain the resampler. That's wrong. All I'm saying is that none of the changes from 3 to 4 (with the exception of Process(temp, temp.Data(), SamplesInToFrames(temp.Length()));) is necessary. And you could do without. The current tests you added look messy, and serve no purpose as the generic case already covers it.

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review178572

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review178068 > When you call will a null buffer, you call process with aFrames = 0. > > if aFrames == 0, then ProcessInternal is a no-op (that could be made even more explicit by immediately returning 0 there) and it returns 0. so frames = 0. > > There's already a test later that check if frames is 0, which then drain the resampler and will set the lenght. > > With your change now, if aFrames is 0, you no longer drain the resampler. That's wrong. > > All I'm saying is that none of the changes from 3 to 4 (with the exception of Process(temp, temp.Data(), SamplesInToFrames(temp.Length()));) is necessary. And you could do without. > > The current tests you added look messy, and serve no purpose as the generic case already covers it. If I do not take the changes between 3-4 I have the following try error. https://treeherder.mozilla.org/#/jobs?repo=try&author=achronop@gmail.com&selectedJob=124883888 By making the ProcessInternal more explicit in the way that you mention I could avoid that error (and will look better imo). But again, I need to SetLength() before drainning or upsampling in order to make sure that there is enough space in the output. You do a similar thing here: http://searchfox.org/mozilla-central/source/dom/media/AudioConverter.h#166

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review178582 my bad sorry for that. not sure what I was thinking...

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review178068 > If I do not take the changes between 3-4 I have the following try error. > https://treeherder.mozilla.org/#/jobs?repo=try&author=achronop@gmail.com&selectedJob=124883888 > By making the ProcessInternal more explicit in the way that you mention I could avoid that error (and will look better imo). > > But again, I need to SetLength() before drainning or upsampling in order to make sure that there is enough space in the output. You do a similar thing here: > http://searchfox.org/mozilla-central/source/dom/media/AudioConverter.h#166 Then I would prefer setting the length requires for receiving the frames draines, right before the call to DrainResampler

Comment on attachment 8890835 [details] Bug 1378070 - Update AudioConverter to use destination buffer. https://reviewboard.mozilla.org/r/162054/#review178618 ::: dom/media/AudioConverter.h:198 (Diff revisions 4 - 6) > > template <typename Value> > size_t Process(AlignedBuffer<Value>& aOutBuffer, const Value* aInBuffer, size_t aFrames) > { > MOZ_DIAGNOSTIC_ASSERT(mIn.Format() == mOut.Format()); > - size_t frames = 0; > + MOZ_ASSERT(aFrames && aInBuffer || !aFrames); missing parenthesis here, that would give you a compilation warning

Pushed by achronop@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1502253e5b92 Update AudioConverter to use destination buffer. r=jya https://hg.mozilla.org/integration/autoland/rev/f276eea5bee3 Implement multichannel WebAudio. r=padenot

https://hg.mozilla.org/mozilla-central/rev/1502253e5b92 https://hg.mozilla.org/mozilla-central/rev/f276eea5bee3

There are lots of places where we could mention this update, but I'm not sure what is for the best. For the moment, I've just added a note to the relevant rel notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/57#APIs What documentation would you like to see about multi-channel web audio? In the MDN docs, we've kind of just assumed this works, and not really said anything about it specifically.

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #59) > There are lots of places where we could mention this update, but I'm not > sure what is for the best. For the moment, I've just added a note to the > relevant rel notes: > > https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/57#APIs > > What documentation would you like to see about multi-channel web audio? In > the MDN docs, we've kind of just assumed this works, and not really said > anything about it specifically. I think that's fine, thanks.