[Chromoting] Implement down mixing in AudioPump

remoting/protocol/audio_pump.cc

Index: remoting/protocol/audio_pump.cc
diff --git a/remoting/protocol/audio_pump.cc b/remoting/protocol/audio_pump.cc
index 66eff6ebff4d58946b7746163d2b9a1c32c2ef20..609ec51e2e7d60ee12fe91998ef0193619369ea4 100644
--- a/remoting/protocol/audio_pump.cc
+++ b/remoting/protocol/audio_pump.cc
@@ -4,19 +4,96 @@
 
 #include "remoting/protocol/audio_pump.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "media/base/audio_bus.h"
+#include "media/base/audio_sample_types.h"
+#include "media/base/channel_layout.h"
+#include "media/base/channel_mixer.h"
 #include "remoting/codec/audio_encoder.h"
 #include "remoting/proto/audio.pb.h"
 #include "remoting/protocol/audio_source.h"
 #include "remoting/protocol/audio_stub.h"
 
+namespace {
+
+int CalculateFrameCount(const remoting::AudioPacket& packet) {
+  return packet.data(0).size() / packet.channels() / packet.bytes_per_sample();
+}
+
+std::unique_ptr<media::AudioBus> AudioPacketToAudioBus(
+    const remoting::AudioPacket& packet) {
+  const int frame_count = CalculateFrameCount(packet);
+  if (frame_count < 1) {

[Sergey Ulanov, 2017/06/02 18:34:36]

packet.data(0).is_empty()

[Sergey Ulanov, 2017/06/02 18:34:36]

Why would we get an empty packet here? Can it be replaced with a DCHECK?

[Hzj_jie, 2017/06/02 22:11:25]

(frame_count == 0) != (packet.data(0).empty())
Considering a crappy AudioCapturer returns two channels frames without enough
data: data(0).size() < 1 * packet.channels() * packet.bytes_per_sample().

[Hzj_jie, 2017/06/02 22:11:25]

It's not an empty packet, but a defeated packet.

[Sergey Ulanov, 2017/06/05 18:03:19]

Then this should be a DCHECK. We own AudioCapturer and if there is any scenario
in which any of the capturers returns fractional invalid frames it should be
fixed in the capturer instead of working around it here. If you are concerned
about partial frames returned by the capturer, then this should be a broader
DCHECK, e.g.:
  DCHECK(packet.data(0).size() % (packet.channels() * packet.bytes_per_sample())
== 0);

If the problem is caused by system API it should be handled at the capturer
layer, not here.

[Hzj_jie, 2017/06/06 03:21:42]

I still doubt this decision, especially the DCHECK() takes no effect on users'
machines, and may cause unexpected behavior.

[Sergey Ulanov, 2017/06/07 06:24:59]

Do you have an examples when that would make a difference?

[Hzj_jie, 2017/06/07 20:27:18]

IMO, changing the following or similar pattern:
========
DCHECK(p.IsInValidState());
p.DoSomething();
========
into
========
DCHECK(p.IsInValidState());
if (!p.IsInValidState()) { return; }
p.DoSomething();
========
or
========
CHECK(p.IsInValidState());
p.DoSomething();
========
would be safer.

Considering if DoSomething() is an operation to modify important users' data:
working in invalid state may break the entire data set, using DCHECK() here
won't actually help on users' environments.

[Sergey Ulanov, 2017/06/07 21:16:29]

See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
. It addressed both of your suggestions. CHECK() may be appropriate when
violation of the value would result in security vulnerability. I don't think
this is one of these cases.
There are million things that can go wrong. This CHECK() can't guarantee that we
are in a valid state and that DoSomething() will work correctly. 

+    return nullptr;
+  }
+  if (static_cast<int>(packet.data(0).size()) >=
+      media::AudioBus::CalculateMemorySize(packet.channels(), frame_count)) {
+    // TODO(zijiehe): AudioBus::BuildChannelData() and WrapMemory() should
+    // receive "const float*" instead of "float*". We can remove the const_cast
+    // afterwards.
+    return media::AudioBus::WrapMemory(

[Sergey Ulanov, 2017/06/02 18:34:36]

Does this work correctly
remoting::AudioPacket always uses 16-bit signed integers, while
AudioBus::WrapMemory() expects 32-bit FP (i.e. float) samples.

[Hzj_jie, 2017/06/02 22:11:25]

I just realize this condition won't meet anyway. I will remove it.

+        packet.channels(),
+        frame_count,
+        reinterpret_cast<int16_t*>(const_cast<char*>(packet.data(0).data())));

[Sergey Ulanov, 2017/06/02 18:34:37]

You can use string_as_array() from base/stl_util.h instead of the const_cast
const_cast<char*>

[Hzj_jie, 2017/06/02 22:11:25]

Done.

+  }
+
+  std::unique_ptr<media::AudioBus> result =
+      media::AudioBus::Create(packet.channels(), frame_count);
+  result->FromInterleaved<media::SignedInt16SampleTypeTraits>(
+      reinterpret_cast<const int16_t*>(packet.data(0).data()), frame_count);
+  return result;
+}
+
+std::unique_ptr<remoting::AudioPacket> AudioBusToAudioPacket(
+    const media::AudioBus& packet) {
+  std::unique_ptr<remoting::AudioPacket> result =
+      base::MakeUnique<remoting::AudioPacket>();
+  result->add_data()->resize(
+      media::AudioBus::CalculateMemorySize(packet.channels(), packet.frames()) /
+      sizeof(float) * sizeof(int16_t));

[Sergey Ulanov, 2017/06/02 18:34:36]

I don't think we want to use AudioBus::CalculateMemorySize() here. This code is
correct only with the assumption that AudioBus uses float internally, and there
is no reason we need to depend on it here. "packet.channels() * packet.frames()
* AudioPacket::BYTES_PER_SAMPLE_2" would be simpler and clearer

[Hzj_jie, 2017/06/02 22:11:25]

Done.

+  packet.ToInterleaved<media::SignedInt16SampleTypeTraits>(
+      packet.frames(),
+      reinterpret_cast<int16_t*>(&(result->mutable_data(0)->at(0))));
+  return result;
+}
+
+media::ChannelLayout RetrieveLayout(const remoting::AudioPacket& packet) {
+  // This switch should match AudioPacket::Channels enum in audio.proto.
+  switch (packet.channels()) {
+    case remoting::AudioPacket::CHANNELS_INVALID:
+      return media::CHANNEL_LAYOUT_UNSUPPORTED;
+    case remoting::AudioPacket::CHANNELS_MONO:
+      return media::CHANNEL_LAYOUT_MONO;
+    case remoting::AudioPacket::CHANNELS_STEREO:
+      return media::CHANNEL_LAYOUT_STEREO;
+    case remoting::AudioPacket::CHANNELS_SURROUND:
+      return media::CHANNEL_LAYOUT_SURROUND;
+    case remoting::AudioPacket::CHANNELS_QUAD:
+      return media::CHANNEL_LAYOUT_QUAD;

[Sergey Ulanov, 2017/06/02 18:34:36]

I don't think that downmixing all 4-channel layouts as quadraphonic is
acceptable. In case the system is using 3.1 layout we would have center channel
mixed to the left and LFE mixed to the right, which may severely affect stereo
imaging (voice in the left ear, special effects in the right).

My understanding from your previous comment was that you were suggesting to
treat all 4-speaker layouts as surround (i.e.  L / R / C / BC). That would mix
the two additional channels to the left and the right channels equally, which is
acceptable even if the system uses quad layout.

[Hzj_jie, 2017/06/02 22:11:25]

Done.

+    case remoting::AudioPacket::CHANNELS_4_1:
+      return media::CHANNEL_LAYOUT_4_1;
+    case remoting::AudioPacket::CHANNELS_5_1:
+      return media::CHANNEL_LAYOUT_5_1;
+    case remoting::AudioPacket::CHANNELS_6_1:
+      return media::CHANNEL_LAYOUT_6_1;
+    case remoting::AudioPacket::CHANNELS_7_1:
+      return media::CHANNEL_LAYOUT_7_1;
+  }
+  NOTREACHED() << "Invalid AudioPacket::Channels";
+  return media::CHANNEL_LAYOUT_UNSUPPORTED;
+}
+
+}  // namespace
+
 namespace remoting {
 namespace protocol {
 
@@ -36,6 +113,8 @@ class AudioPump::Core {
   void OnPacketSent(int size);
 
  private:
+  std::unique_ptr<AudioPacket> Downmix(std::unique_ptr<AudioPacket> packet);
+
   void EncodeAudioPacket(std::unique_ptr<AudioPacket> packet);
 
   base::ThreadChecker thread_checker_;
@@ -53,6 +132,9 @@ class AudioPump::Core {
   // yet.
   int bytes_pending_;
 
+  std::unique_ptr<media::ChannelMixer> mixer_;
+  media::ChannelLayout last_input_layout_ = media::CHANNEL_LAYOUT_NONE;
+
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
@@ -98,8 +180,13 @@ void AudioPump::Core::EncodeAudioPacket(std::unique_ptr<AudioPacket> packet) {
 
   int max_buffered_bytes =
       audio_encoder_->GetBitrate() * kMaxBufferedIntervalMs / 1000 / 8;
-  if (!enabled_ || bytes_pending_ > max_buffered_bytes)
+  if (!enabled_ || bytes_pending_ > max_buffered_bytes) {
     return;
+  }
+
+  if (packet->channels() > AudioPacket::CHANNELS_STEREO) {
+    packet = Downmix(std::move(packet));
+  }
 
   std::unique_ptr<AudioPacket> encoded_packet =
       audio_encoder_->Encode(std::move(packet));
@@ -116,6 +203,44 @@ void AudioPump::Core::EncodeAudioPacket(std::unique_ptr<AudioPacket> packet) {
                             base::Passed(&encoded_packet), packet_size));
 }
 
+std::unique_ptr<AudioPacket> AudioPump::Core::Downmix(
+    std::unique_ptr<AudioPacket> packet) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(packet);
+  if (packet->data_size() != 1 ||
+      packet->data(0).empty() ||
+      packet->bytes_per_sample() != AudioPacket::BYTES_PER_SAMPLE_2) {

[Sergey Ulanov, 2017/06/02 18:34:36]

I think all these checks can be DCHECKs

[Hzj_jie, 2017/06/02 22:11:25]

I have removed packet->data(0).empty() check, it will be covered by
AudioPacketToAudioBus() anyway.
IMO, dchecking return value of an interface is not the right thing to do as I
have replied in Joe's comment. p.s. The DCHECK(packet) in line 209 does not help
in production: if by any reason an AudioCapturer returns nullptr on users'
environment but cannot be produced on our test environment, the following logic
will crash the service anyway.

+    return packet;
+  }
+  const media::ChannelLayout input_layout = RetrieveLayout(*packet);
+  if (input_layout == media::CHANNEL_LAYOUT_UNSUPPORTED ||

[Sergey Ulanov, 2017/06/02 18:34:37]

I think in case the layout is unsupported it doesn't make sense to continue with
the packet. Encoder won't be able to handle it. Return nullptr in that case?

[Hzj_jie, 2017/06/02 22:11:25]

Done. But IMO, unsupported by media::ChannelMixer does not need to mean it
cannot be supported by Encoder: Encoder does not use media::CHANNEL_LAYOUT at
all.

+      input_layout == media::CHANNEL_LAYOUT_MONO ||
+      input_layout == media::CHANNEL_LAYOUT_STEREO) {
+    return packet;
+  }
+
+  if (!mixer_ || last_input_layout_ != input_layout) {

[Sergey Ulanov, 2017/06/02 18:34:36]

It's better to move this code above mono/stereo check. Otherwise
last_input_layout_ won't be set to MONO/STEREO, which is confusing. 

[Hzj_jie, 2017/06/02 22:11:25]

If the input_layout is mono or stereo, we won't need to initialize |mixer_|.
Maybe I should change |last_input_layout_| into |mixer_input_layout_|.

+    last_input_layout_ = input_layout;
+    mixer_ = base::MakeUnique<media::ChannelMixer>(
+        input_layout, media::CHANNEL_LAYOUT_STEREO);
+  }
+
+  std::unique_ptr<media::AudioBus> input = AudioPacketToAudioBus(*packet);
+  if (!input) {
+    return packet;

[Sergey Ulanov, 2017/06/02 18:34:36]

Why would AudioPacketToAudioBus() fail? If it does fail, do we want pass the
packet to the encoder anyway?

[Hzj_jie, 2017/06/02 22:11:25]

A corrupted data without a frame.

+  }
+  std::unique_ptr<media::AudioBus> output =
+      media::AudioBus::Create(AudioPacket::CHANNELS_STEREO, input->frames());
+  mixer_->Transform(input.get(), output.get());
+
+  std::unique_ptr<AudioPacket> result = AudioBusToAudioPacket(*output);
+  result->set_encoding(AudioPacket::ENCODING_RAW);
+  result->set_sampling_rate(packet->sampling_rate());
+  result->set_bytes_per_sample(packet->bytes_per_sample());
+  result->set_channels(AudioPacket::CHANNELS_STEREO);

[Sergey Ulanov, 2017/06/02 18:34:37]

I think it would be best to set all these properties in AudioBusToAudioPacket().
Otherwise it looks wrong that AudioBusToAudioPacket() copies data, but doesn't
set other properties.

[Hzj_jie, 2017/06/02 22:11:24]

There is no sampling rate in AudioBus, so I leave set_sampling_rate() still in
this function.

+  return result;
+}
+
 AudioPump::AudioPump(
     scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner,
     std::unique_ptr<AudioSource> audio_source,