901633 - Improve internal support for stereo audio in the webrtc media pipeline

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P5)

Description

[Randell Jesup [:jesup] (needinfo me)]

Explicitly for the part outside of the webrtc.org/GIPS code.

Comment 1

[Paul Adenot (:padenot)]

Attachment: patch for webrtc-landing to turn it into a test case

This applies on pc_test.html from the WebRTC landing repo.

It makes it so that the sender sends an stereo sine wave that smoothly oscillates between hard-panning to the left and right.

Currently, we only send mono so it oscillates between silence and non-silence.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Part 1 - Implement a generic audio packetizer. r=

Comment 3

[Paul Adenot (:padenot)]

Attachment: Part 2 - Make AudioChannelFormat and AudioSegment more generic. r=

* * *
tofold

Comment 4

[Paul Adenot (:padenot)]

Attachment: Part 3 - Fix TrackEncoder to use the new AudioChunk methods. r=

Comment 5

[Paul Adenot (:padenot)]

Attachment: Part 4 - Update AudioNodeStream to use new chunk methods. r=

Comment 6

[Paul Adenot (:padenot)]

Attachment: Part 5 - Make MediaPipeline downmix and properly convert audio for webrtc.org code. r=

This means converting to int16, interleaving, and down-mixing to stereo (or
keeping it to mono if it's already mono of course).

Comment 7

[Paul Adenot (:padenot)]

Attachment: Part 6 - Update DelayBuffer to use the new AudioChunk methods

Comment 8

[Paul Adenot (:padenot)]

Attachment: Part 7 - Update AudioNodeExternalInputStream to use the new AudioChunk methods

Comment 9

[Paul Adenot (:padenot)]

Attachment: Part 8 - Use our new generic packetizer in the MediaPipeline so that we can packetize stereo easily. r=

Comment 10

[Paul Adenot (:padenot)]

Attachment: Part 9 - Make the necessary changes to VoEExternalMediaImpl::ExternalRecordingInsertData so that it the number of channels is forwarded down the webrtc.org code. r=

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8642473 [details] [diff] [review]
Part 1 - Implement a generic audio packetizer. r=

We have too much ad-hoc packetizers in the tree. Here is one more that is better (and tested).

The plan is to change the MediaPipeline one and the AEC feed back one to it.

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8642477 [details] [diff] [review]
Part 5 - Make MediaPipeline downmix and properly convert audio for webrtc.org code. r=

This is using the new templated conversion functions in part 2. We still need to have a switch here, because we don't know what the MSG is feeding us at this point: it can be Web Audio (floats), or directly a mic (int).

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8642486 [details] [diff] [review]
Part 9 - Make the necessary changes to VoEExternalMediaImpl::ExternalRecordingInsertData so that it the number of channels is forwarded down the webrtc.org code. r=

That's the patch where I don't know if we want to upstream it.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8642473 [details] [diff] [review]
Part 1 - Implement a generic audio packetizer. r=

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

::: dom/media/AudioPacketizer.h
@@ +42,5 @@
> +       "The packet size and the number of channel should be strictly positive");
> +  }
> +  void Input(InputType* aFrames, uint32_t aFrameCount)
> +  {
> +    uint32_t inputSamples = aFrameCount * mChannels;

I suppose in theory we should use size_t for all these, or something along those lines - but uint32_t is *way* bigger than could ever be needed

@@ +113,5 @@
> +  uint32_t PacketsAvailable() {
> +    return AvailableSamples() / mChannels / mPacketSize;
> +  }
> +
> +  bool Empty() {

Should some of these methods be const?

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8642482 [details] [diff] [review]
Part 8 - Use our new generic packetizer in the MediaPipeline so that we can packetize stereo easily. r=

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +974,5 @@
>  
>    MOZ_ASSERT(!(rate%100)); // rate should be a multiple of 100
>  
> +  // Check if the rate or the number of channels has changed since the last time
> +  // we came through I realize it may be overkill to check if the rate has

"through."

Comment 16

[Paul Adenot (:padenot)]

Attachment: Part 1 - Implement a generic audio packetizer. r=

This new version fixes a ridiculous bug (I forgot to add an offset to a pointer
in a PodCopy call) that was somehow not caught by the tests (and adds a test
that would have caught it off course).

Comment 17

[Paul Adenot (:padenot)]

Attachment: Part 2 - Make AudioChannelFormat and AudioSegment more generic. r=

This is basically templating more things in AudioSegment.h and
AudioChannelFormat.h so we can use them for float or integers (WebRTC uses
integers and needed those).

Comment 18

[Paul Adenot (:padenot)]

Attachment: Part 3 - Fix TrackEncoder to use the new AudioChunk methods. r=

I don't think we have much people working on this this days, so you get to
review. It's rather simple though.

Comment 19

[Paul Adenot (:padenot)]

Attachment: Part 4 - Update AudioNodeStream to use new chunk methods. r=

This is making more things typed instead of using void*.

Comment 20

[Paul Adenot (:padenot)]

Attachment: Part 5 - Make MediaPipeline downmix and properly convert audio for webrtc.org code. r=

This means converting to int16, interleaving, and down-mixing to stereo (or
keeping it to mono if it's already mono of course).

Already r+ed by jesup, carrying forward.

Comment 21

[Paul Adenot (:padenot)]

Attachment: Part 6 - Update DelayBuffer to use the new AudioChunk methods

Again, making some things more typed now that we don't use void* everywhere.

Comment 22

[Paul Adenot (:padenot)]

Attachment: Part 7 - Update AudioNodeExternalInputStream to use the new AudioChunk methods

Same thing for AudioNodeExternalInputStream.

Comment 23

[Paul Adenot (:padenot)]

Attachment: Part 8 - Use our new generic packetizer in the MediaPipeline so that we can packetize stereo easily. r=

This was already r+ed by jesup, carrying forward.

Comment 24

[Paul Adenot (:padenot)]

Attachment: Part 9 - Make the necessary changes to VoEExternalMediaImpl::ExternalRecordingInsertData so that it the number of channels is forwarded down the webrtc.org code. r=

Already reviewed by jesup, carrying forward.

Comment 25

[Paul Adenot (:padenot)]

Attachment: Part 10 - Change the receiving side of the MediaPipeline so that it can detect and handle stereo. r=

This is the receiving side. This patch makes it possible to receive stereo, and
removes a malloc call as well.

Comment 26

[Paul Adenot (:padenot)]

Attachment: Part 11 - Add an API in webrtc.org's output mixer to get the output channel count. r=

This is supporting code for the previous patch so that we can get two channels
if needed: the webrtc.org code does not down mix anymore if it receives stereo.

Comment 27

[Paul Adenot (:padenot)]

Attachment: Part 12 - Add a function to deinterleave and convert an audio buffer. r=

This adds a generic function to convert and deinterleave audio we get from
webrtc.org code so that we can feed it to the MSG.

Comment 28

[Paul Adenot (:padenot)]

Attachment: Part 13 - Teach the resampler at the input of the MSG to dynamically change its channel count if needed. r=

When the audio comes from a PeerConnection, we don't know how many channels the
audio will have, and it can change anyways, so we need to teach the input
resampler to change its channel count dynamically.

Comment 29

[Paul Adenot (:padenot)]

Attachment: Part 14 - Add testing for our audio processing functions. r=

Now we can unit tests our new generic AudioSegment.h and AudioChannelFormat.h
methods, here are some C++ tests. Also this includes trivial fixes so that the
tests for the AudioPacketizer work on Windows and with static analysis builds.

Comment 30

[Paul Adenot (:padenot)]

Attachment: Part 15 - Remove an allocation on the sending side, out of the packetizer. r=

This removes an alloocation on the sending side of the PeerConnection, for
audio, by taking advantage of the fact that webrtc.org synchronously does a
copy later down the stack when it feeds data to its own resampler.

Comment 31

[Paul Adenot (:padenot)]

Attachment: Part 16 - Remove another allocation in the sending side. r=

This removes the other allocation in the common case on the sending side so we
don't malloc anymore on the critical path.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8653441 [details] [diff] [review]
Part 2 - Make AudioChannelFormat and AudioSegment more generic. r=

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

::: dom/media/AudioSegment.cpp
@@ +184,2 @@
>  
> +    switch(c.mBufferFormat) {

space before (

Comment 33

[Karl Tomlinson (:karlt)]

Comment on attachment 8653441 [details] [diff] [review]
Part 2 - Make AudioChannelFormat and AudioSegment more generic. r=

>+  nsTArray<const T*> ChannelData()
>+  {
>+    nsTArray<const T*> channels;
>+    channels.SetLength(mChannelData.Length());
>+    for (uint32_t channel = 0; channel < mChannelData.Length(); channel++) {
>+      channels[channel] = reinterpret_cast<const T*>(mChannelData[channel]);
>+    }
>+    return channels;
>+  }

This is adding at least one allocation/free pair, possibly more depending on
how much RVO can do.

I assume it is possible to return type const nsTArray<const T*>& and get that
from *reinterpret_cast<nsTArray<const T*>*>(&mChannelData).

The API provides the opportunity to add

  MOZ_ASSERT(AudioSampleTypeToFormat<T>::Format == mBufferFormat);

Comment 34

[Karl Tomlinson (:karlt)]

Comment on attachment 8653448 [details] [diff] [review]
Part 6 - Update DelayBuffer to use the new AudioChunk methods

>+  mUpmixChannels = mChunks[aNewReadChunk].ChannelData<float>();

I want to avoid the new allocation here.  This is fine if that can be resolved.

Bonus points for removing static_cast<const float*>() from ReadChannels().
(I assume that is the advantage of templating AudioChannelsUpMix because the
function is the same regardless of the template parameter.)

Comment 35

[Karl Tomlinson (:karlt)]

Comment on attachment 8653449 [details] [diff] [review]
Part 7 - Update AudioNodeExternalInputStream to use the new AudioChunk methods

>-CopyChunkToBlock(const AudioChunk& aInput, AudioChunk *aBlock,
>+CopyChunkToBlock(const nsTArray<const T*>& aInput, uint32_t aDuration, float aVolume, AudioChunk *aBlock,

I think the API is tidier as is with the const AudioChunk& aInput rather than the caller providing the three associated parameters.  This method copies chunk to block so having only the output be a chunk would confuse things.

Instead call with the template parameter.  e.g. CopyChunkToBlock<float>

>-      AudioChannelsUpMix(&channels, blockChannels, nullptr);
>+      AudioChannelsUpMix(&channels, blockChannels, SilentChannel::ZeroChannel<T>());

Passing a buffer of zeros instead of null defeats the null channel
optimization below.

>+        nsTArray<const float*> channelData = ci->ChannelData<float>();

Unnecessary memory allocation, as indicated previously.
The API may be fine though if the reinterpret_cast array reference works.

Comment 36

[Paul Adenot (:padenot)]

Attachment: Part 5 - Make MediaPipeline downmix and properly convert audio for webrtc.org code. r=

This means converting to int16, interleaving, and down-mixing to stereo (or
keeping it to mono if it's already mono of course).

Comment 37

[Paul Adenot (:padenot)]

Attachment: Part 2 - Make AudioChannelFormat and AudioSegment more generic. r=

Yeah this reinterpret_cast trick works fine.

Comment 38

[Paul Adenot (:padenot)]

Attachment: Part 6 - Update DelayBuffer to use the new AudioChunk methods

This is similar, but ChannelData() has been changed, and the static_cast has
been removed.

Comment 39

[Paul Adenot (:padenot)]

Attachment: Part 7 - Update AudioNodeExternalInputStream to use the new AudioChunk methods

This is indeed better.

Comment 40

[Karl Tomlinson (:karlt)]

Comment on attachment 8654087 [details] [diff] [review]
Part 6 - Update DelayBuffer to use the new AudioChunk methods

Thanks!

Comment 41

[Karl Tomlinson (:karlt)]

Comment on attachment 8654089 [details] [diff] [review]
Part 7 - Update AudioNodeExternalInputStream to use the new AudioChunk methods

>-  if (aInput.IsNull()) {

>+  const nsTArray<const T*>& inputChannels = aInput.ChannelData<T>();
>+  if (!inputChannels[0]) {
>     channels.SetLength(blockChannels);
>     PodZero(channels.Elements(), blockChannels);
>   } else {
>-    channels.SetLength(aInput.ChannelCount());
>-    PodCopy(channels.Elements(), aInput.mChannelData.Elements(), channels.Length());
>+    channels.SetLength(inputChannels.Length());
>+    PodCopy(channels.Elements(), inputChannels.Elements(), channels.Length());

Leave the aInput.IsNull() test because the existing code is fine and
inputChannels[0] can read beyond the end of the array.

Instead move the inputChannels declaration into the else block.

>+    switch(ci->mBufferFormat) {

space before (

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8653439 [details] [diff] [review]
Part 1 - Implement a generic audio packetizer. r=

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

::: dom/media/AudioPacketizer.h
@@ +40,5 @@
> +  {
> +     MOZ_ASSERT(aPacketSize > 0 && aChannels > 0,
> +       "The packet size and the number of channel should be strictly positive");
> +  }
> +  void Input(InputType* aFrames, uint32_t aFrameCount)

add blank line (nit)

@@ +91,5 @@
> +  {
> +    uint32_t samplesNeeded = mPacketSize * mChannels;
> +    OutputType* out = new OutputType[samplesNeeded];
> +
> +    // Under-run. Pad the end of the buffer with silence.

Is there any need to (optionally) log underruns?  (it shouldn't be the default)

@@ +95,5 @@
> +    // Under-run. Pad the end of the buffer with silence.
> +    if (AvailableSamples() < samplesNeeded) {
> +      uint32_t zeros = samplesNeeded - AvailableSamples();
> +      PodZero(out + AvailableSamples(), zeros);
> +      samplesNeeded -= zeros;

Good, this puts the zeros after the real data

@@ +114,5 @@
> +
> +    return out;
> +  }
> +
> +  uint32_t PacketsAvailable() {

A number of these methods should be const

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8653442 [details] [diff] [review]
Part 3 - Fix TrackEncoder to use the new AudioChunk methods. r=

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

::: dom/media/encoder/TrackEncoder.cpp
@@ +149,5 @@
> +      InterleaveTrackData(array, aDuration, aOutputChannels, aOutput, aChunk.mVolume);
> +      break;
> +   }
> +   case AUDIO_FORMAT_SILENCE: {
> +      MOZ_ASSERT(false, "To implement.");

File a bug?

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8654083 [details] [diff] [review]
Part 5 - Make MediaPipeline downmix and properly convert audio for webrtc.org code. r=

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

Easy fix

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ -953,2 @@
>  
> -  if (enabled_ && chunk.mBuffer) {

You lost the bit that zeros the buffers if enabled_ is false

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8653456 [details] [diff] [review]
Part 10 - Change the receiving side of the MediaPipeline so that it can detect and handle stereo. r=

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +1361,2 @@
>      AudioSegment segment;
> +    // We derive the number of channel of the stream from the number of samples

channels

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8653463 [details] [diff] [review]
Part 13 - Teach the resampler at the input of the MSG to dynamically change its channel count if needed. r=

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

::: dom/media/MediaStreamGraph.cpp
@@ +2482,5 @@
>    AudioSegment* segment = static_cast<AudioSegment*>(aSegment);
>    int channels = segment->ChannelCount();
>  
> +  // If this segment is just silence, we delay instanciating the resampler. We
> +  // also need too recreate the resampler if the channel count changes.

too -> to

Comment 47

[Paul Adenot (:padenot)]

Attachment: Part 10 - Change the receiving side of the MediaPipeline so that it can detect and handle stereo. r=

Yes, good catch !

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Paul Adenot (:padenot) from comment #47)
> Created attachment 8654762 [details] [diff] [review]
> Part 10 - Change the receiving side of the MediaPipeline so that it can
> detect and handle stereo. r=
> 
> Yes, good catch !

Are you sure you didn't mean to update part 5, which I r-'d?  part 10 I r+'d with a single grammar error

Comment 49

[Paul Adenot (:padenot)]

Attachment: Part 5 - Make MediaPipeline downmix and properly convert audio for webrtc.org code

Ah yes indeed.

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b18471f2ff7
https://hg.mozilla.org/integration/mozilla-inbound/rev/19dec0df775c
https://hg.mozilla.org/integration/mozilla-inbound/rev/afd376183231
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e896277675f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b026e7f47823
https://hg.mozilla.org/integration/mozilla-inbound/rev/10869420ec36
https://hg.mozilla.org/integration/mozilla-inbound/rev/f13da3f6b494
https://hg.mozilla.org/integration/mozilla-inbound/rev/4547ae1e73ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/142737374b1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7cbf699f687
https://hg.mozilla.org/integration/mozilla-inbound/rev/54499fc0c9c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac76eb834c13
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be6a307bd1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9603ea5b1bfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/90293f4ef514
https://hg.mozilla.org/integration/mozilla-inbound/rev/0021aceac846

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b18471f2ff7
https://hg.mozilla.org/mozilla-central/rev/19dec0df775c
https://hg.mozilla.org/mozilla-central/rev/afd376183231
https://hg.mozilla.org/mozilla-central/rev/2e896277675f
https://hg.mozilla.org/mozilla-central/rev/b026e7f47823
https://hg.mozilla.org/mozilla-central/rev/10869420ec36
https://hg.mozilla.org/mozilla-central/rev/f13da3f6b494
https://hg.mozilla.org/mozilla-central/rev/4547ae1e73ee
https://hg.mozilla.org/mozilla-central/rev/142737374b1c
https://hg.mozilla.org/mozilla-central/rev/f7cbf699f687
https://hg.mozilla.org/mozilla-central/rev/54499fc0c9c6
https://hg.mozilla.org/mozilla-central/rev/ac76eb834c13
https://hg.mozilla.org/mozilla-central/rev/4be6a307bd1b
https://hg.mozilla.org/mozilla-central/rev/9603ea5b1bfe
https://hg.mozilla.org/mozilla-central/rev/90293f4ef514
https://hg.mozilla.org/mozilla-central/rev/0021aceac846