Fix CastAudioOutputStream and related unit tests

1. Fix the following problem: When Stopped,
CastAudioOutputStream would crash stopping the
|io_thread_|, which is also unecessary.

2. Make the unit tests of CastAudioOutputStream
and CastAudioManager only test CMABackend implementation
of CastAudioOutputStream because they are not ready to
test the newer MixerService implementation.

Bug: internal b/29571387
Test:
1. Play a lasting sin wave audio on two devices, to make
sure the AudioOutputStream plays fine whether or not MixerService
is used.
2. Run cast_media_unittests on estelle and all the tests passed.
3. Run internal CQ and the CQ including cast_media_unittests passed.

Change-Id: I380991235344348af56293ae214fb8c0e2be80b0
Reviewed-on: https://chromium-review.googlesource.com/c/1294190
Commit-Queue: Guohui Deng <guohuideng@chromium.org>
Reviewed-by: Kenneth MacKay <kmackay@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601818}

chromecast/media/audio/cast_audio_manager.cc

@@ -13,6 +13,7 @@
 #include "chromecast/media/audio/cast_audio_mixer.h"
 #include "chromecast/media/audio/cast_audio_output_stream.h"
 #include "chromecast/media/cma/backend/cma_backend_factory.h"
+#include "chromecast/public/cast_media_shlib.h"
 #include "chromecast/public/media/media_pipeline_backend.h"
 
 namespace {
@@ -42,11 +43,30 @@ CastAudioManager::CastAudioManager(
     scoped_refptr<base::SingleThreadTaskRunner> media_task_runner,
     service_manager::Connector* connector,
     bool use_mixer)
+    : CastAudioManager(std::move(audio_thread),
+                       audio_log_factory,
+                       std::move(backend_factory_getter),
+                       std::move(browser_task_runner),
+                       std::move(media_task_runner),
+                       connector,
+                       use_mixer,
+                       false) {}
+
+CastAudioManager::CastAudioManager(
+    std::unique_ptr<::media::AudioThread> audio_thread,
+    ::media::AudioLogFactory* audio_log_factory,
+    base::RepeatingCallback<CmaBackendFactory*()> backend_factory_getter,
+    scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> media_task_runner,
+    service_manager::Connector* connector,
+    bool use_mixer,
+    bool force_use_cma_backend_for_output)
     : AudioManagerBase(std::move(audio_thread), audio_log_factory),
       backend_factory_getter_(std::move(backend_factory_getter)),
       browser_task_runner_(std::move(browser_task_runner)),
       media_task_runner_(std::move(media_task_runner)),
       browser_connector_(connector),
+      force_use_cma_backend_for_output_(force_use_cma_backend_for_output),
       weak_factory_(this) {
   DCHECK(browser_task_runner_->BelongsToCurrentThread());
   DCHECK(backend_factory_getter_);
@@ -115,10 +135,13 @@ CmaBackendFactory* CastAudioManager::cma_backend_factory() {
   DCHECK_EQ(::media::AudioParameters::AUDIO_PCM_LINEAR, params.format());
 
   // If |mixer_| exists, return a mixing stream.
-  if (mixer_)
+  if (mixer_) {
     return mixer_->MakeStream(params);
-  else
-    return new CastAudioOutputStream(this, GetConnector(), params);
+  } else {
+    return new CastAudioOutputStream(
+        this, GetConnector(), params,
+        GetMixerServiceConnectionFactoryForOutputStream(params));
+  }
 }
 
 ::media::AudioOutputStream* CastAudioManager::MakeLowLatencyOutputStream(
@@ -128,10 +151,13 @@ CmaBackendFactory* CastAudioManager::cma_backend_factory() {
   DCHECK_EQ(::media::AudioParameters::AUDIO_PCM_LOW_LATENCY, params.format());
 
   // If |mixer_| exists, return a mixing stream.
-  if (mixer_)
+  if (mixer_) {
     return mixer_->MakeStream(params);
-  else
-    return new CastAudioOutputStream(this, GetConnector(), params);
+  } else {
+    return new CastAudioOutputStream(
+        this, GetConnector(), params,
+        GetMixerServiceConnectionFactoryForOutputStream(params));
+  }
 }
 
 ::media::AudioInputStream* CastAudioManager::MakeLinearInputStream(
@@ -180,8 +206,9 @@ CmaBackendFactory* CastAudioManager::cma_backend_factory() {
 
   // Keep a reference to this stream for proper behavior on
   // CastAudioManager::ReleaseOutputStream.
-  mixer_output_stream_.reset(
-      new CastAudioOutputStream(this, GetConnector(), params));
+  mixer_output_stream_.reset(new CastAudioOutputStream(
+      this, GetConnector(), params,
+      GetMixerServiceConnectionFactoryForOutputStream(params)));
   return mixer_output_stream_.get();
 }
 
@@ -206,5 +233,19 @@ void CastAudioManager::BindConnectorRequest(
   browser_connector_->BindConnectorRequest(std::move(request));
 }
 
+MixerServiceConnectionFactory*
+CastAudioManager::GetMixerServiceConnectionFactoryForOutputStream(
+    const ::media::AudioParameters& params) {
+  bool use_cma_backend =
+      (params.effects() & ::media::AudioParameters::MULTIZONE) ||
+      !CastMediaShlib::AddDirectAudioSource ||
+      force_use_cma_backend_for_output_;
+
+  if (use_cma_backend)
+    return nullptr;
+  else
+    return &mixer_service_connection_factory_;
+}
+
 }  // namespace media
-}  // namespace chromecast
+}  // namespace chromecast
\ No newline at end of file

chromecast/media/audio/cast_audio_manager.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
+#include "chromecast/media/audio/mixer_service/mixer_service_connection_factory.h"
 #include "media/audio/audio_manager_base.h"
 #include "services/service_manager/public/cpp/connector.h"
 
@@ -19,6 +20,8 @@ namespace chromecast {
 namespace media {
 
 class CastAudioMixer;
+class CastAudioManagerTest;
+class CastAudioOutputStreamTest;
 class CmaBackendFactory;
 
 class CastAudioManager : public ::media::AudioManagerBase {
@@ -78,9 +81,27 @@ class CastAudioManager : public ::media::AudioManagerBase {
 
  private:
   friend class CastAudioMixer;
+  friend class CastAudioManagerTest;
+  friend class CastAudioOutputStreamTest;
   service_manager::Connector* GetConnector();
   void BindConnectorRequest(service_manager::mojom::ConnectorRequest request);
 
+  CastAudioManager(
+      std::unique_ptr<::media::AudioThread> audio_thread,
+      ::media::AudioLogFactory* audio_log_factory,
+      base::RepeatingCallback<CmaBackendFactory*()> backend_factory_getter,
+      scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> media_task_runner,
+      service_manager::Connector* connector,
+      bool use_mixer,
+      bool force_use_cma_backend_for_output);
+
+  // Return nullptr if it is not appropriate to use MixerServiceConnection
+  // for output stream audio playback.
+  MixerServiceConnectionFactory*
+  GetMixerServiceConnectionFactoryForOutputStream(
+      const ::media::AudioParameters& params);
+
   base::RepeatingCallback<CmaBackendFactory*()> backend_factory_getter_;
   CmaBackendFactory* cma_backend_factory_ = nullptr;
   scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner_;
@@ -90,6 +111,15 @@ class CastAudioManager : public ::media::AudioManagerBase {
   std::unique_ptr<CastAudioMixer> mixer_;
   std::unique_ptr<service_manager::Connector> connector_;
 
+  // Let unit test force the CastOutputStream to uses
+  // CmaBackend implementation.
+  // TODO(b/117980762):: After refactoring CastOutputStream, so
+  // that the CastOutputStream has a unified output API, regardless
+  // of the platform condition, then the unit test would be able to test
+  // CastOutputStream properly.
+  bool force_use_cma_backend_for_output_;
+  MixerServiceConnectionFactory mixer_service_connection_factory_;
+
   // Weak pointers must be dereferenced on the |browser_task_runner|.
   base::WeakPtr<CastAudioManager> weak_this_;
   base::WeakPtrFactory<CastAudioManager> weak_factory_;

chromecast/media/audio/cast_audio_manager_unittest.cc

@@ -62,7 +62,6 @@ int OnMoreData(base::TimeDelta delay,
 
 namespace chromecast {
 namespace media {
-namespace {
 
 class CastAudioManagerTest : public testing::Test {
  public:
@@ -111,13 +110,14 @@ class CastAudioManagerTest : public testing::Test {
     }
 
     mock_backend_factory_ = std::make_unique<MockCmaBackendFactory>();
-    audio_manager_ = std::make_unique<CastAudioManager>(
+    audio_manager_ = base::WrapUnique(new CastAudioManager(
         std::make_unique<::media::TestAudioThread>(), &fake_audio_log_factory_,
         base::BindRepeating(&CastAudioManagerTest::GetCmaBackendFactory,
                             base::Unretained(this)),
         scoped_task_environment_.GetMainThreadTaskRunner(),
         scoped_task_environment_.GetMainThreadTaskRunner(), connector_.get(),
-        use_mixer);
+        use_mixer, true /* force_use_cma_backend_for_output*/
+        ));
     // A few AudioManager implementations post initialization tasks to
     // audio thread. Flush the thread to ensure that |audio_manager_| is
     // initialized and ready to use before returning from this function.
@@ -226,6 +226,5 @@ TEST_F(CastAudioManagerTest, CanMakeMixerStream) {
   stream->Close();
 }
 
-}  // namespace
 }  // namespace media
 }  // namespace chromecast

chromecast/media/audio/cast_audio_output_stream.cc

@@ -334,7 +334,9 @@ void CastAudioOutputStream::CmaWrapper::OnDecoderError() {
 class CastAudioOutputStream::MixerServiceWrapper
     : public chromecast::media::MixerServiceConnection::Delegate {
  public:
-  MixerServiceWrapper(const ::media::AudioParameters& audio_params);
+  MixerServiceWrapper(
+      const ::media::AudioParameters& audio_params,
+      MixerServiceConnectionFactory* mixer_service_connection_factory);
   ~MixerServiceWrapper() override = default;
 
   void Start(AudioSourceCallback* source_callback);
@@ -356,6 +358,7 @@ class CastAudioOutputStream::MixerServiceWrapper
   void OnEosPlayed() override { NOTREACHED(); }
 
   const ::media::AudioParameters audio_params_;
+  MixerServiceConnectionFactory* mixer_service_connection_factory_;
   std::unique_ptr<::media::AudioBus> audio_bus_;
   AudioSourceCallback* source_callback_;
   std::unique_ptr<media::MixerServiceConnection> mixer_connection_;
@@ -371,11 +374,14 @@ class CastAudioOutputStream::MixerServiceWrapper
 };
 
 CastAudioOutputStream::MixerServiceWrapper::MixerServiceWrapper(
-    const ::media::AudioParameters& audio_params)
+    const ::media::AudioParameters& audio_params,
+    MixerServiceConnectionFactory* mixer_service_connection_factory)
     : audio_params_(audio_params),
+      mixer_service_connection_factory_(mixer_service_connection_factory),
       source_callback_(nullptr),
       volume_(1.0f),
       io_thread_("CastAudioOutputStream IO") {
+  DCHECK(mixer_service_connection_factory_);
   DETACH_FROM_THREAD(io_thread_checker_);
 
   base::Thread::Options options;
@@ -414,7 +420,8 @@ void CastAudioOutputStream::MixerServiceWrapper::Start(
 
   source_callback_ = source_callback;
   mixer_connection_ =
-      std::make_unique<media::MixerServiceConnection>(this, params);
+      mixer_service_connection_factory_->CreateMixerServiceConnection(this,
+                                                                      params);
   mixer_connection_->Connect();
   mixer_connection_->SetVolumeMultiplier(volume_);
 }
@@ -430,7 +437,6 @@ void CastAudioOutputStream::MixerServiceWrapper::Close(
     base::OnceClosure closure) {
   DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
   Stop();
-  io_thread_.Stop();
   std::move(closure).Run();
 }
 
@@ -483,12 +489,14 @@ void CastAudioOutputStream::MixerServiceWrapper::OnConnectionError() {
 CastAudioOutputStream::CastAudioOutputStream(
     CastAudioManager* audio_manager,
     service_manager::Connector* connector,
-    const ::media::AudioParameters& audio_params)
+    const ::media::AudioParameters& audio_params,
+    MixerServiceConnectionFactory* mixer_service_connection_factory)
     : volume_(1.0),
       audio_thread_state_(kClosed),
       audio_manager_(audio_manager),
       connector_(connector),
       audio_params_(audio_params),
+      mixer_service_connection_factory_(mixer_service_connection_factory),
       audio_weak_factory_(this) {
   DCHECK(audio_manager_);
   DCHECK(connector_);
@@ -659,21 +667,22 @@ void CastAudioOutputStream::OnGetMultiroomInfo(
   if (audio_thread_state_ == kPendingClose)
     return;
 
-  // If direct audio is not available, MixerService would use CMA backend.
-  // So no need to use MixerService which adds extra latency in this case.
-  bool use_cma_backend =
-      (audio_params_.effects() & ::media::AudioParameters::MULTIZONE) ||
-      !CastMediaShlib::AddDirectAudioSource;
-
-  if (use_cma_backend) {
+  if (!mixer_service_connection_factory_) {
     cma_wrapper_ = std::make_unique<CmaWrapper>(
         audio_manager_->GetTaskRunner(), audio_params_,
         audio_manager_->cma_backend_factory());
     POST_TO_CMA_WRAPPER(Initialize, application_session_id,
                         std::move(multiroom_info));
   } else {
-    mixer_service_wrapper_ =
-        std::make_unique<MixerServiceWrapper>(audio_params_);
+    // If direct audio is not available, valid
+    // |mixer_service_connection_factory_|
+    // shouldn't has been passed in so the CastAudioOutputStream would use
+    // CmaBackend.
+    DCHECK(!(audio_params_.effects() & ::media::AudioParameters::MULTIZONE) &&
+           CastMediaShlib::AddDirectAudioSource);
+
+    mixer_service_wrapper_ = std::make_unique<MixerServiceWrapper>(
+        audio_params_, mixer_service_connection_factory_);
   }
 
   if (pending_start_)

chromecast/media/audio/cast_audio_output_stream.h

@@ -32,11 +32,14 @@ enum AudioOutputState {
 };
 
 class CastAudioManager;
+class MixerServiceConnectionFactory;
 
 // Chromecast implementation of AudioOutputStream.
-// This class forwards to MixerService if Direct Audio is available for
-// a lower latency audio playback (using MixerServiceWrapper), otherwise
-// it forwards to CMA backend (using CmaWrapper).
+// This class forwards to MixerService if valid
+// |mixer_service_connection_factory| is passed in on the construction call,
+// for a lower latency audio playback (using MixerServiceWrapper). Otherwise,
+// when a nullptr is passed in as |mixer_service_connection_factory| it forwards
+// to CMA backend (using CmaWrapper).
 //
 // In either case, involved components live on two threads:
 // 1. Audio thread
@@ -92,11 +95,18 @@ class CastAudioManager;
 // applied to MixerServiceConnection after the MixerServiceConnection is
 // established on the Start() call.
 
+// TODO(b/117980762): CastAudioOutputStream should be refactored
+// to be more unit test friendly. And the unit tests can be improved.
+
 class CastAudioOutputStream : public ::media::AudioOutputStream {
  public:
-  CastAudioOutputStream(CastAudioManager* audio_manager,
-                        service_manager::Connector* connector,
-                        const ::media::AudioParameters& audio_params);
+  // When nullptr is passed as |mixer_service_connection_factory|, CmaWrapper
+  // will be used for audio playback.
+  CastAudioOutputStream(
+      CastAudioManager* audio_manager,
+      service_manager::Connector* connector,
+      const ::media::AudioParameters& audio_params,
+      MixerServiceConnectionFactory* mixer_service_connection_factory);
   ~CastAudioOutputStream() override;
 
   // ::media::AudioOutputStream implementation.
@@ -122,6 +132,7 @@ class CastAudioOutputStream : public ::media::AudioOutputStream {
   CastAudioManager* const audio_manager_;
   service_manager::Connector* connector_;
   const ::media::AudioParameters audio_params_;
+  MixerServiceConnectionFactory* mixer_service_connection_factory_;
   chromecast::mojom::MultiroomManagerPtr multiroom_manager_;
   std::unique_ptr<CmaWrapper> cma_wrapper_;
   std::unique_ptr<MixerServiceWrapper> mixer_service_wrapper_;

chromecast/media/audio/cast_audio_output_stream_unittest.cc

@@ -63,6 +63,8 @@ int OnMoreData(base::TimeDelta /* delay */,
   return dest->frames();
 }
 
+}  // namespace
+
 class NotifyPushBufferCompleteTask : public chromecast::TaskRunner::Task {
  public:
   explicit NotifyPushBufferCompleteTask(CmaBackend::Decoder::Delegate* delegate)
@@ -270,13 +272,13 @@ class CastAudioOutputStreamTest : public ::testing::Test {
     }
 
     mock_backend_factory_ = std::make_unique<MockCmaBackendFactory>();
-    audio_manager_ = std::make_unique<CastAudioManager>(
+    audio_manager_ = base::WrapUnique(new CastAudioManager(
         std::make_unique<::media::TestAudioThread>(), nullptr,
         base::BindRepeating(&CastAudioOutputStreamTest::GetCmaBackendFactory,
                             base::Unretained(this)),
         scoped_task_environment_.GetMainThreadTaskRunner(),
         scoped_task_environment_.GetMainThreadTaskRunner(), connector_.get(),
-        use_mixer);
+        use_mixer, true /* force_use_cma_backend_for_output*/));
     audio_manager_->SetConnectorForTesting(std::move(connector_));
 
     // A few AudioManager implementations post initialization tasks to
@@ -863,6 +865,5 @@ TEST_F(CastAudioOutputStreamTest, SessionId) {
   stream->Close();
 }
 
-}  // namespace
 }  // namespace media
 }  // namespace chromecast

chromecast/media/audio/mixer_service/BUILD.gn

@@ -40,6 +40,8 @@ cast_source_set("connection") {
   sources = [
     "mixer_service_connection.cc",
     "mixer_service_connection.h",
+    "mixer_service_connection_factory.cc",
+    "mixer_service_connection_factory.h",
   ]
 
   deps = [

chromecast/media/audio/mixer_service/mixer_service_connection_factory.cc

+// Copyright 2018 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 "chromecast/media/audio/mixer_service/mixer_service_connection_factory.h"
+
+namespace chromecast {
+namespace media {
+
+std::unique_ptr<MixerServiceConnection>
+MixerServiceConnectionFactory::CreateMixerServiceConnection(
+    MixerServiceConnection::Delegate* delegate,
+    const mixer_service::MixerStreamParams& params) {
+  return std::make_unique<media::MixerServiceConnection>(delegate, params);
+}
+
+}  // namespace media
+}  // namespace chromecast

chromecast/media/audio/mixer_service/mixer_service_connection_factory.h

+// Copyright 2018 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.
+
+#ifndef CHROMECAST_MEDIA_AUDIO_MIXER_SERVICE_MIXER_SERVICE_CONNECTION_FACTORY_H_
+#define CHROMECAST_MEDIA_AUDIO_MIXER_SERVICE_MIXER_SERVICE_CONNECTION_FACTORY_H_
+
+#include <memory>
+
+#include "chromecast/media/audio/mixer_service/mixer_service_connection.h"
+
+namespace chromecast {
+namespace media {
+
+class MixerServiceConnectionFactory {
+ public:
+  MixerServiceConnectionFactory() = default;
+  ~MixerServiceConnectionFactory() = default;
+
+  std::unique_ptr<MixerServiceConnection> CreateMixerServiceConnection(
+      MixerServiceConnection::Delegate* delegate,
+      const mixer_service::MixerStreamParams& params);
+
+  DISALLOW_COPY_AND_ASSIGN(MixerServiceConnectionFactory);
+};
+
+}  // namespace media
+}  // namespace chromecast
+
+#endif  // CHROMECAST_MEDIA_AUDIO_MIXER_SERVICE_MIXER_SERVICE_CONNECTION_FACTORY_H_
\ No newline at end of file