[Chromoting] Implement down mixing in AudioPump

remoting/protocol/audio_pump.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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 {
 
 // Limit the data stored in the pending send buffers to 250ms.
 const int kMaxBufferedIntervalMs = 250;
 
 class AudioPump::Core {
  public:
   Core(base::WeakPtr<AudioPump> pump,
        std::unique_ptr<AudioSource> audio_source,
        std::unique_ptr<AudioEncoder> audio_encoder);
   ~Core();
 
   void Start();
   void Pause(bool pause);
 
   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_;
 
   base::WeakPtr<AudioPump> pump_;
 
   scoped_refptr<base::SingleThreadTaskRunner> pump_task_runner_;
 
   std::unique_ptr<AudioSource> audio_source_;
   std::unique_ptr<AudioEncoder> audio_encoder_;
 
   bool enabled_;
 
   // Number of bytes in the queue that have been encoded but haven't been sent
   // 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);
 };
 
 AudioPump::Core::Core(base::WeakPtr<AudioPump> pump,
                       std::unique_ptr<AudioSource> audio_source,
                       std::unique_ptr<AudioEncoder> audio_encoder)
     : pump_(pump),
       pump_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       audio_source_(std::move(audio_source)),
       audio_encoder_(std::move(audio_encoder)),
@@ skipping 28 matching lines @@
 
 void AudioPump::Core::EncodeAudioPacket(std::unique_ptr<AudioPacket> packet) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(packet);
 
   int max_buffered_bytes =
       audio_encoder_->GetBitrate() * kMaxBufferedIntervalMs / 1000 / 8;
   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));
 
   // The audio encoder returns a null audio packet if there's no audio to send.
   if (!encoded_packet)
     return;
 
   int packet_size = encoded_packet->ByteSize();
   bytes_pending_ += packet_size;
 
   pump_task_runner_->PostTask(
       FROM_HERE, base::Bind(&AudioPump::SendAudioPacket, pump_,
                             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,
     std::unique_ptr<AudioEncoder> audio_encoder,
     AudioStub* audio_stub)
     : audio_task_runner_(audio_task_runner),
       audio_stub_(audio_stub),
       weak_factory_(this) {
   DCHECK(audio_stub_);
 
@@ skipping 28 matching lines @@
 }
 
 void AudioPump::OnPacketSent(int size) {
   audio_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&Core::OnPacketSent, base::Unretained(core_.get()), size));
 }
 
 }  // namespace protocol
 }  // namespace remoting