Support for both 44.1 kHz and 48 kHz on the client.

remoting/client/plugin/pepper_audio_player.cc

 // Copyright (c) 2012 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/client/plugin/pepper_audio_player.h"
 
 #include <algorithm>
 
 #include "base/stl_util.h"
-#include "remoting/proto/audio.pb.h"
 
 namespace {
 // Constants used to create an audio configuration resource.
 // The sample count we will request from the browser.
 const uint32_t kSampleFrameCount = 4096u;
 // The number of channels in the audio stream (only supporting stereo audio
 // for now).
 const uint32_t kChannels = 2u;
 const int kSampleSizeBytes = 2;
 }  // namespace
 
 namespace remoting {
 
 bool PepperAudioPlayer::IsRunning() const {
   return running_;
 }
 
 PepperAudioPlayer::PepperAudioPlayer(pp::Instance* instance)
-      : samples_per_frame_(kSampleFrameCount),
-        bytes_consumed_(0),
-        running_(false) {
+    : instance_(instance),
+      sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID),
+      samples_per_frame_(kSampleFrameCount),
+      bytes_consumed_(0),
+      running_(false) {
+}
+
+PepperAudioPlayer::~PepperAudioPlayer() {
+  if (running_ && !audio_.is_null())
+    DoStop();

[Sergey Ulanov, 2012/07/23 22:01:47]

Do we really need to call this explicitly? Doesn't it stop implicitly if we just
destroy |audio_|.

[kxing, 2012/07/23 22:58:47]

Done.

+}
+
+bool PepperAudioPlayer::Start() {
+  running_ = true;

[Sergey Ulanov, 2012/07/23 22:01:47]

Do we want to do that even when we fail to start?

[kxing, 2012/07/23 22:58:47]

Done.

+  if (!audio_.is_null())
+    return DoStart();

[Sergey Ulanov, 2012/07/23 22:01:47]

That's going to fail if audio_ is null. I wonder why we need Start() method at
all? Can we start playback implicitly when the first audio packet is received?

[kxing, 2012/07/23 22:58:47]

Done.

+  return true;
+}
+
+bool PepperAudioPlayer::DoStart() {

[Sergey Ulanov, 2012/07/23 22:01:47]

Doesn't seem like a good name given that it called not only from Start(). Maybe
InitializePlayback()? or StartPlayback()?

[kxing, 2012/07/23 22:58:47]

Done.

+  bool success = audio_.StartPlayback();
+  if (!success)
+    LOG(ERROR) << "Failed to start Pepper audio player";
+  return success;
+}
+
+void PepperAudioPlayer::DoStop() {
+  bool success = audio_.StopPlayback();
+  if (!success)
+    LOG(ERROR) << "Failed to stop Pepper audio player";
+}
+
+// static
+PP_AudioSampleRate PepperAudioPlayer::ConvertToPepperSampleRate(

[Sergey Ulanov, 2012/07/23 22:01:47]

Doesn't need to be a class member. Just make it a static function in anonymous
namespace.

[kxing, 2012/07/23 22:58:47]

Done.

+    AudioPacket::SamplingRate sampling_rate) {
+  switch (sampling_rate) {
+    case AudioPacket::SAMPLING_RATE_44100:
+      return PP_AUDIOSAMPLERATE_44100;
+    case AudioPacket::SAMPLING_RATE_48000:
+      return PP_AUDIOSAMPLERATE_48000;
+    default:
+      NOTREACHED();
+  }
+  return PP_AUDIOSAMPLERATE_NONE;
+}
+
+void PepperAudioPlayer::ResetAudioPlayer(
+    AudioPacket::SamplingRate sampling_rate) {
+  sampling_rate_ = sampling_rate;
+  PP_AudioSampleRate sample_rate =
+      ConvertToPepperSampleRate(sampling_rate);
+
   // Ask the browser/device for an appropriate sample frame count size.
   samples_per_frame_ =
-      pp::AudioConfig::RecommendSampleFrameCount(instance,
-                                                 PP_AUDIOSAMPLERATE_44100,
+      pp::AudioConfig::RecommendSampleFrameCount(instance_,
+                                                 sample_rate,
                                                  kSampleFrameCount);
 
   // Create an audio configuration resource.
-  pp::AudioConfig audio_config = pp::AudioConfig(instance,
-                                                 PP_AUDIOSAMPLERATE_44100,
+  pp::AudioConfig audio_config = pp::AudioConfig(instance_,
+                                                 sample_rate,
                                                  samples_per_frame_);
 
   // Create an audio resource.
-  audio_ = pp::Audio(instance, audio_config, PepperAudioPlayerCallback, this);
-}
-
-PepperAudioPlayer::~PepperAudioPlayer() { }
-
-bool PepperAudioPlayer::Start() {
-  running_ = audio_.StartPlayback();
-  return running_;
+  audio_ = pp::Audio(instance_, audio_config, PepperAudioPlayerCallback, this);
 }
 
 void PepperAudioPlayer::ProcessAudioPacket(scoped_ptr<AudioPacket> packet) {
   // TODO(kxing): Limit the size of the queue so that latency doesn't grow
   // too large.
   if (packet->data().size() % (kChannels * kSampleSizeBytes) != 0) {
     LOG(WARNING) << "Received corrupted packet.";
     return;
   }
   base::AutoLock auto_lock(lock_);
 
+  // Start the Pepper audio player if this is the first packet.
+  if (sampling_rate_ != packet->sampling_rate()) {
+    if (running_ && !audio_.is_null())
+      DoStop();

[Sergey Ulanov, 2012/07/23 22:01:47]

ResetAudioPlayer() creates new pp::Audio object anyway, so I don't think you
need explicit stop here.

[kxing, 2012/07/23 22:58:47]

Done.

+    ResetAudioPlayer(packet->sampling_rate());
+    if (running_)
+      DoStart();
+  }
+
   queued_packets_.push_back(packet.release());
 }
 
 // static
 void PepperAudioPlayer::PepperAudioPlayerCallback(void* samples,
                                                   uint32_t buffer_size,
                                                   void* data) {
   PepperAudioPlayer* audio_player = static_cast<PepperAudioPlayer*>(data);
   audio_player->FillWithSamples(samples, buffer_size);
 }
@@ skipping 31 matching lines @@
         bytes_needed - bytes_extracted);
     memcpy(next_sample, packet_data.data() + bytes_consumed_, bytes_to_copy);
 
     next_sample += bytes_to_copy;
     bytes_consumed_ += bytes_to_copy;
     bytes_extracted += bytes_to_copy;
   }
 }
 
 }  // namespace remoting