[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..25daa905e12599276a0282134ef480830f460b53 100644
--- a/remoting/protocol/audio_pump.cc
+++ b/remoting/protocol/audio_pump.cc
@@ -4,19 +4,93 @@
 
 #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();

[joedow, 2017/05/26 16:01:50]

Could an empty packet be passed in here (with the result being 0 / 0 / 0)? 
would it make sense for some DCHECKs here?

[Hzj_jie, 2017/05/26 20:08:03]

AudioPump uses an AudioSource interface. So assuming the input looks dangerous
to me.

+}
+
+std::unique_ptr<media::AudioBus> ToAudioBus(

[joedow, 2017/05/26 16:01:50]

nit: Rename to 'PacketToAudioBus' ?

[Hzj_jie, 2017/05/26 20:08:03]

Done.

+    const remoting::AudioPacket& packet) {
+  using namespace media;

[joedow, 2017/05/26 16:01:50]

The inline using directives are a bit repetitive.  If you need them, please move
them out of the method so all of the methods in the file can use them. 
Repeating them in each method hurts readability IMO and isn't consistent with
the rest of the codebase.

[Hzj_jie, 2017/05/26 20:08:03]

Done.

+  const int frame_count = CalculateFrameCount(packet);
+  if (frame_count <= 0) {

[joedow, 2017/05/26 16:01:50]

nit: 'frame_count < 1' is simpler

[Hzj_jie, 2017/05/26 20:08:03]

Done.

+    return nullptr;
+  }
+  if (static_cast<int>(packet.data(0).size()) >=
+      AudioBus::CalculateMemorySize(packet.channels(), frame_count)) {
+    // AudioBus::BuildChannelData() should receive const float* instead of
+    // float*, meanwhile WrapMemory() function.

[joedow, 2017/05/26 16:01:50]

Can you fix this sentence fragment.  The first bit sounds like a TODO and the
second doesn't complete the thought.

[Hzj_jie, 2017/05/26 20:08:03]

Done.

+    return media::AudioBus::WrapMemory(
+        packet.channels(),
+        frame_count,
+        reinterpret_cast<int16_t*>(const_cast<char*>(packet.data(0).data())));
+  }
+
+  std::unique_ptr<AudioBus> result =
+      AudioBus::Create(packet.channels(), frame_count);
+  result->FromInterleaved<SignedInt16SampleTypeTraits>(
+      reinterpret_cast<const int16_t*>(packet.data(0).data()), frame_count);
+  return result;
+}
+
+std::unique_ptr<remoting::AudioPacket> FromAudioBus(
+    const media::AudioBus& packet) {
+  using namespace media;
+  using remoting::AudioPacket;
+  std::unique_ptr<AudioPacket> result = base::MakeUnique<AudioPacket>();
+  result->add_data()->resize(
+      AudioBus::CalculateMemorySize(packet.channels(), packet.frames()) /
+      sizeof(float) * sizeof(int16_t));
+  packet.ToInterleaved<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.
+  using namespace media;
+  using remoting::AudioPacket;
+  switch (packet.channels()) {
+    case AudioPacket::CHANNELS_INVALID:
+      return CHANNEL_LAYOUT_UNSUPPORTED;
+    case AudioPacket::CHANNELS_MONO:
+      return CHANNEL_LAYOUT_MONO;
+    case AudioPacket::CHANNELS_STEREO:
+      return CHANNEL_LAYOUT_STEREO;
+    case AudioPacket::CHANNELS_5_1:
+      return CHANNEL_LAYOUT_5_1;
+    case AudioPacket::CHANNELS_6_1:
+      return CHANNEL_LAYOUT_6_1;
+    case AudioPacket::CHANNELS_7_1:
+      return CHANNEL_LAYOUT_7_1;
+  }
+  NOTREACHED() << "Invalid AudioPacket::Channels";
+  return CHANNEL_LAYOUT_UNSUPPORTED;
+}
+
+}  // namespace
+
 namespace remoting {
 namespace protocol {
 
@@ -36,6 +110,9 @@ class AudioPump::Core {
   void OnPacketSent(int size);
 
  private:
+  std::unique_ptr<remoting::AudioPacket> Downmix(
+    std::unique_ptr<remoting::AudioPacket> packet);

[joedow, 2017/05/26 16:01:50]

You don't need the remote prefix here since it is inside the remoting namespace
already.

[Hzj_jie, 2017/05/26 20:08:03]

Done.

+
   void EncodeAudioPacket(std::unique_ptr<AudioPacket> packet);
 
   base::ThreadChecker thread_checker_;
@@ -53,6 +130,9 @@ class AudioPump::Core {
   // yet.
   int bytes_pending_;
 
+  std::unique_ptr<media::ChannelMixer> mixer_ = nullptr;

[joedow, 2017/05/26 16:01:50]

No need to assign nullptr here since unique_ptr is initialized to nullptr in its
c'tor.

[Hzj_jie, 2017/05/26 20:08:03]

Done.

+  media::ChannelLayout last_input_layout_ = media::CHANNEL_LAYOUT_NONE;
+
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
@@ -101,6 +181,10 @@ void AudioPump::Core::EncodeAudioPacket(std::unique_ptr<AudioPacket> packet) {
   if (!enabled_ || bytes_pending_ > max_buffered_bytes)
     return;
 
+  if (packet->channels() > 2) {
+    packet = Downmix(std::move(packet));
+  }
+
   std::unique_ptr<AudioPacket> encoded_packet =
       audio_encoder_->Encode(std::move(packet));
 
@@ -116,6 +200,45 @@ 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);
+  using namespace media;
+  if (packet->data_size() != 1 ||
+      packet->data(0).empty() ||
+      packet->bytes_per_sample() != AudioPacket::BYTES_PER_SAMPLE_2) {
+    return packet;
+  }
+  const ChannelLayout input_layout = RetrieveLayout(*packet);
+  if (input_layout == CHANNEL_LAYOUT_UNSUPPORTED ||
+      input_layout == CHANNEL_LAYOUT_MONO ||
+      input_layout == CHANNEL_LAYOUT_STEREO) {
+    return packet;
+  }
+
+  if (!mixer_ || last_input_layout_ != input_layout) {
+    last_input_layout_ = input_layout;
+    mixer_ = base::MakeUnique<ChannelMixer>(
+        input_layout, CHANNEL_LAYOUT_STEREO);
+  }
+
+  std::unique_ptr<AudioBus> input = ToAudioBus(*packet);
+  if (!input) {
+    return packet;
+  }
+  std::unique_ptr<AudioBus> output = AudioBus::Create(2, input->frames());

[joedow, 2017/05/26 16:01:50]

Can you replace the magic '2' with AudioPacket::CHANNELS_STEREO?

[Hzj_jie, 2017/05/26 20:08:03]

Done.

+  mixer_->Transform(input.get(), output.get());
+
+  std::unique_ptr<AudioPacket> result = FromAudioBus(*output);
+  DCHECK(result);

[joedow, 2017/05/26 16:01:50]

I don't think this DCHECK is needed here as the next line derefs the pointer
since the * is deref'd on the next line.

[Hzj_jie, 2017/05/26 20:08:03]

Done.

+  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);
+  return result;
+}
+
 AudioPump::AudioPump(
     scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner,
     std::unique_ptr<AudioSource> audio_source,