Refactor and unit test CastAudioOutputStream

CAOS was refactored to simplify the logic for the service connector,
multithreading, and unit tests.

The CAOS::Backend object was renamed to CAOS::CmaWrapper, and much of
the implementation was moved out into CAOS to simplify the object.

The service connector is created in CastAudioManager and passed to
CAOS's audio thread. Instead of passing this then to the media thread
as done previously, we only utilize the connector in the audio thread.
The connector can be easily overriden for tests.

CAOS runs on two threads. It is constructed and controlled from
CastAudioManager in the audio thread. All CMA construction and control
happens in the media thread in the CAOS::CmaWrapper object.

The thread usage in the unit tests has been significantly simplified.
Instead of using RunLoops and multiple threads, we use a single thread
retrieved from a ScopedTaskEnvironment in CAM, CAOS:audio, and CAOS:media.
This means that everything in the unit tests are running on a single
thread, and the tests are far less flaky.

In order to allow everything to run on a single thread, the usages of
WaitableEvent were removed. WaitableEvent was previously used to allow
the synchronous behavior of the audio thread to work with the
asynchronous thread hopping. To compensate for this, certain task
postings between threads can be cached or cancelled, and errors are
returned late.

The following unit tests were added to verify the above behavior and
fill in untested implementation:
- CastAudioManagerTest
  .HasValidOutputStreamParameters
  .CanMakeStream
  .CanMakeMixerStream
- CastAudioOutputStreamTest
  .CloseWithoutStart
  .CloseWithoutStop
  .CloseCancelsOpen
  .CloseCancelsStart
  .CloseCancelsStop
  .StartImmediatelyAfterOpen
  .SetVolumeImmediatelyAfterOpen
  .StopCancelsStart
  .StopDoesNotCancelSetVolume
  .PushFrameAfterStop
  .PushFrameAfterClose
  .MultiroomInfo
  .SessionId

Bug: b/111993375
Test: cast_media_unittests, udon build
Change-Id: Ib7dce8df8a36a2311e13d6a0062b7e56d4928995
Reviewed-on: https://chromium-review.googlesource.com/1159824
Commit-Queue: Aidan Wolter <awolter@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Reviewed-by: Kenneth MacKay <kmackay@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586096}

chromecast/media/audio/cast_audio_manager.cc

@@ -8,6 +8,8 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
 #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"
@@ -37,21 +39,26 @@ CastAudioManager::CastAudioManager(
     ::media::AudioLogFactory* audio_log_factory,
     base::RepeatingCallback<CmaBackendFactory*()> backend_factory_getter,
     scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> backend_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> media_task_runner,
     service_manager::Connector* connector,
     bool use_mixer)
     : AudioManagerBase(std::move(audio_thread), audio_log_factory),
       backend_factory_getter_(std::move(backend_factory_getter)),
       browser_task_runner_(std::move(browser_task_runner)),
-      backend_task_runner_(std::move(backend_task_runner)),
-      browser_connector_(connector) {
+      media_task_runner_(std::move(media_task_runner)),
+      browser_connector_(connector),
+      weak_factory_(this) {
+  DCHECK(browser_task_runner_->BelongsToCurrentThread());
   DCHECK(backend_factory_getter_);
   DCHECK(browser_connector_);
+  weak_this_ = weak_factory_.GetWeakPtr();
   if (use_mixer)
     mixer_ = std::make_unique<CastAudioMixer>(this);
 }
 
-CastAudioManager::~CastAudioManager() = default;
+CastAudioManager::~CastAudioManager() {
+  DCHECK(browser_task_runner_->BelongsToCurrentThread());
+}
 
 bool CastAudioManager::HasAudioOutputDevices() {
   return true;
@@ -96,10 +103,10 @@ void CastAudioManager::ReleaseOutputStream(::media::AudioOutputStream* stream) {
   }
 }
 
-CmaBackendFactory* CastAudioManager::backend_factory() {
-  if (!backend_factory_)
-    backend_factory_ = backend_factory_getter_.Run();
-  return backend_factory_;
+CmaBackendFactory* CastAudioManager::cma_backend_factory() {
+  if (!cma_backend_factory_)
+    cma_backend_factory_ = backend_factory_getter_.Run();
+  return cma_backend_factory_;
 }
 
 ::media::AudioOutputStream* CastAudioManager::MakeLinearOutputStream(
@@ -111,8 +118,7 @@ CmaBackendFactory* CastAudioManager::backend_factory() {
   if (mixer_)
     return mixer_->MakeStream(params);
   else
-    return new CastAudioOutputStream(params, browser_task_runner_,
-                                     browser_connector_, this);
+    return new CastAudioOutputStream(this, GetConnector(), params);
 }
 
 ::media::AudioOutputStream* CastAudioManager::MakeLowLatencyOutputStream(
@@ -125,8 +131,7 @@ CmaBackendFactory* CastAudioManager::backend_factory() {
   if (mixer_)
     return mixer_->MakeStream(params);
   else
-    return new CastAudioOutputStream(params, browser_task_runner_,
-                                     browser_connector_, this);
+    return new CastAudioOutputStream(this, GetConnector(), params);
 }
 
 ::media::AudioInputStream* CastAudioManager::MakeLinearInputStream(
@@ -175,10 +180,31 @@ CmaBackendFactory* CastAudioManager::backend_factory() {
 
   // Keep a reference to this stream for proper behavior on
   // CastAudioManager::ReleaseOutputStream.
-  mixer_output_stream_.reset(new CastAudioOutputStream(
-      params, browser_task_runner_, browser_connector_, this));
+  mixer_output_stream_.reset(
+      new CastAudioOutputStream(this, GetConnector(), params));
   return mixer_output_stream_.get();
 }
 
+void CastAudioManager::SetConnectorForTesting(
+    std::unique_ptr<service_manager::Connector> connector) {
+  connector_ = std::move(connector);
+}
+
+service_manager::Connector* CastAudioManager::GetConnector() {
+  if (!connector_) {
+    service_manager::mojom::ConnectorRequest request;
+    connector_ = service_manager::Connector::Create(&request);
+    browser_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&CastAudioManager::BindConnectorRequest,
+                                  weak_this_, std::move(request)));
+  }
+  return connector_.get();
+}
+
+void CastAudioManager::BindConnectorRequest(
+    service_manager::mojom::ConnectorRequest request) {
+  browser_connector_->BindConnectorRequest(std::move(request));
+}
+
 }  // namespace media
 }  // namespace chromecast

chromecast/media/audio/cast_audio_manager.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "media/audio/audio_manager_base.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -27,7 +28,7 @@ class CastAudioManager : public ::media::AudioManagerBase {
       ::media::AudioLogFactory* audio_log_factory,
       base::RepeatingCallback<CmaBackendFactory*()> backend_factory_getter,
       scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner,
-      scoped_refptr<base::SingleThreadTaskRunner> backend_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> media_task_runner,
       service_manager::Connector* connector,
       bool use_mixer);
   ~CastAudioManager() override;
@@ -42,11 +43,14 @@ class CastAudioManager : public ::media::AudioManagerBase {
   const char* GetName() override;
   void ReleaseOutputStream(::media::AudioOutputStream* stream) override;
 
-  CmaBackendFactory* backend_factory();
-  base::SingleThreadTaskRunner* backend_task_runner() {
-    return backend_task_runner_.get();
+  CmaBackendFactory* cma_backend_factory();
+  base::SingleThreadTaskRunner* media_task_runner() {
+    return media_task_runner_.get();
   }
 
+  void SetConnectorForTesting(
+      std::unique_ptr<service_manager::Connector> connector);
+
  protected:
   // AudioManagerBase implementation.
   ::media::AudioOutputStream* MakeLinearOutputStream(
@@ -74,15 +78,21 @@ class CastAudioManager : public ::media::AudioManagerBase {
 
  private:
   friend class CastAudioMixer;
+  service_manager::Connector* GetConnector();
+  void BindConnectorRequest(service_manager::mojom::ConnectorRequest request);
 
   base::RepeatingCallback<CmaBackendFactory*()> backend_factory_getter_;
-  CmaBackendFactory* backend_factory_ = nullptr;
+  CmaBackendFactory* cma_backend_factory_ = nullptr;
   scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner_;
-  scoped_refptr<base::SingleThreadTaskRunner> backend_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> media_task_runner_;
   service_manager::Connector* const browser_connector_;
   std::unique_ptr<::media::AudioOutputStream> mixer_output_stream_;
   std::unique_ptr<CastAudioMixer> mixer_;
+  std::unique_ptr<service_manager::Connector> connector_;
 
+  // Weak pointers must be dereferenced on the |browser_task_runner|.
+  base::WeakPtr<CastAudioManager> weak_this_;
+  base::WeakPtrFactory<CastAudioManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CastAudioManager);
 };

chromecast/media/audio/cast_audio_manager_alsa.cc

@@ -35,14 +35,14 @@ CastAudioManagerAlsa::CastAudioManagerAlsa(
     ::media::AudioLogFactory* audio_log_factory,
     base::RepeatingCallback<CmaBackendFactory*()> backend_factory_getter,
     scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> backend_task_runner,
+    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),
                        browser_task_runner,
-                       backend_task_runner,
+                       media_task_runner,
                        connector,
                        use_mixer),
       wrapper_(new ::media::AlsaWrapper()) {}

chromecast/media/audio/cast_audio_manager_alsa.h

@@ -27,7 +27,7 @@ class CastAudioManagerAlsa : public CastAudioManager {
       ::media::AudioLogFactory* audio_log_factory,
       base::RepeatingCallback<CmaBackendFactory*()> backend_factory_getter,
       scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner,
-      scoped_refptr<base::SingleThreadTaskRunner> backend_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> media_task_runner,
       service_manager::Connector* connector,
       bool use_mixer);
   ~CastAudioManagerAlsa() override;

chromecast/media/audio/cast_audio_mixer_unittest.cc

@@ -23,6 +23,15 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+
+std::unique_ptr<service_manager::Connector> CreateConnector() {
+  service_manager::mojom::ConnectorRequest request;
+  return service_manager::Connector::Create(&request);
+}
+
+}  // namespace
+
 namespace chromecast {
 namespace media {
 namespace {
@@ -34,11 +43,6 @@ using testing::Return;
 using testing::SaveArg;
 using testing::StrictMock;
 
-std::unique_ptr<service_manager::Connector> CreateConnector() {
-  service_manager::mojom::ConnectorRequest request;
-  return service_manager::Connector::Create(&request);
-}
-
 // Utility functions
 ::media::AudioParameters GetAudioParams() {
   return ::media::AudioParameters(
@@ -97,14 +101,16 @@ class MockMediaAudioOutputStream : public ::media::AudioOutputStream {
 
 class MockCastAudioManager : public CastAudioManager {
  public:
-  explicit MockCastAudioManager(service_manager::Connector* connector)
+  explicit MockCastAudioManager(
+      service_manager::Connector* connector,
+      scoped_refptr<base::SingleThreadTaskRunner> media_task_runner)
       : CastAudioManager(
             std::make_unique<::media::TestAudioThread>(),
             nullptr,
             base::BindRepeating(&MockCastAudioManager::GetCmaBackendFactory,
                                 base::Unretained(this)),
-            nullptr,
-            nullptr,
+            media_task_runner,
+            media_task_runner,
             connector,
             true /* use_mixer */) {
     ON_CALL(*this, ReleaseOutputStream(_))
@@ -136,7 +142,8 @@ class CastAudioMixerTest : public ::testing::Test {
 
  protected:
   void SetUp() override {
-    mock_manager_.reset(new StrictMock<MockCastAudioManager>(connector_.get()));
+    mock_manager_.reset(new StrictMock<MockCastAudioManager>(
+        connector_.get(), scoped_task_environment_.GetMainThreadTaskRunner()));
     mock_mixer_stream_.reset(new StrictMock<MockMediaAudioOutputStream>());
 
     ON_CALL(*mock_manager_, MakeMixerOutputStream(_))

chromecast/media/audio/cast_audio_output_stream.h

@@ -8,25 +8,82 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
+#include "base/threading/thread_checker.h"
+#include "base/timer/timer.h"
+#include "chromecast/base/task_runner_impl.h"
+#include "chromecast/common/mojom/multiroom.mojom.h"
+#include "chromecast/media/cma/backend/cma_backend.h"
+#include "chromecast/media/cma/base/decoder_buffer_adapter.h"
 #include "media/audio/audio_io.h"
 #include "media/base/audio_parameters.h"
+#include "media/base/audio_timestamp_helper.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 namespace chromecast {
 namespace media {
 
+enum AudioOutputState {
+  kClosed = 0,
+  kOpened = 1,
+  kStarted = 2,
+  kPendingClose = 3,
+};
+
 class CastAudioManager;
 
+// Chromecast implementation of AudioOutputStream that forwards to CMA backend.
+//
+// This class lives inside two threads:
+// 1. Audio thread
+//    |CastAudioOutputStream|
+//    Where the object gets construction from AudioManager.
+//    How the object gets controlled from AudioManager.
+// 2. Media thread
+//    |CastAudioOutputStream::CmaWrapper|
+//    All CMA logic lives in this thread.
+//
+// The interface between AudioManager and AudioOutputStream is synchronous, so
+// in order to allow asynchronous thread hops, we:
+// * Maintain the current state independently in each thread.
+// * Cache function calls like Start() and SetVolume() as bound callbacks.
+//
+// The individual thread states should nearly always be the same. The only time
+// they are expected to be different is when the audio thread has executed a
+// task and posted to the media thread, but the media thread has not executed
+// yet.
+//
+//  Audio Thread |CAOS|                         Media Thread |CmaWrapper|
+//  ¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯                         ¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯
+//  *[ Closed ]                                 *[ Closed ]
+//      |                                           |
+//      |                                           |
+//      v                                           v
+//   [   Opened    ]    --post open-->           [   Opened   ]
+//      |     |  ^                                  |     |
+//      |     |  |                                  |     |
+//      |     | Stop()                              |     |
+//      |     v  |                                  |     v
+//      |  [ Started ]  --post start-->             |  [ Started ]
+//      |     |                                     |     |
+//      |     |                                     |     |
+//      v     v                                     v     v
+// **[ Pending Close ]  --post close-->        **[ Pending Close ]
+//      |                                           |
+//      |                                           |
+//   ( waits for closure )         <--post closure--'
+//      |
+//      v
+//   ( released)
+//
+// *  Initial states.
+// ** Final states.
 class CastAudioOutputStream : public ::media::AudioOutputStream {
  public:
-  CastAudioOutputStream(
-      const ::media::AudioParameters& audio_params,
-      scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner,
-      // If the |browser_connector| is nullptr, then it will be fetched from the
-      // BrowserThread before usage.
-      service_manager::Connector* browser_connector,
-      CastAudioManager* audio_manager);
+  CastAudioOutputStream(CastAudioManager* audio_manager,
+                        service_manager::Connector* connector,
+                        const ::media::AudioParameters& audio_params);
   ~CastAudioOutputStream() override;
 
   // ::media::AudioOutputStream implementation.
@@ -37,22 +94,31 @@ class CastAudioOutputStream : public ::media::AudioOutputStream {
   void SetVolume(double volume) override;
   void GetVolume(double* volume) override;
 
-  void BindConnectorRequest(
-      service_manager::mojom::ConnectorRequest connector_request);
-
  private:
-  class Backend;
-  void BindConnectorRequestOnBrowserTaskRunner(
-      service_manager::mojom::ConnectorRequest connector_request);
+  class CmaWrapper;
+
+  void FinishClose();
+  void OnGetMultiroomInfo(const std::string& application_session_id,
+                          chromecast::mojom::MultiroomInfoPtr multiroom_info);
+  void InitializeCmaBackend(const std::string& application_session_id,
+                            chromecast::mojom::MultiroomInfoPtr multiroom_info);
 
-  const ::media::AudioParameters audio_params_;
-  scoped_refptr<base::SingleThreadTaskRunner> browser_task_runner_;
-  service_manager::Connector* browser_connector_ = nullptr;
-  CastAudioManager* const audio_manager_;
   double volume_;
+  AudioOutputState audio_thread_state_;
+  CastAudioManager* const audio_manager_;
+  service_manager::Connector* connector_;
+  const ::media::AudioParameters audio_params_;
+  chromecast::mojom::MultiroomManagerPtr multiroom_manager_;
+  std::unique_ptr<CmaWrapper> cma_wrapper_;
+
+  // Hold bindings to Start and SetVolume if they were called before Open
+  // completed. After initialization has finished, these bindings will be
+  // called.
+  base::OnceCallback<void()> pending_start_;
+  base::OnceCallback<void()> pending_volume_;
 
-  // Only valid when the stream is open.
-  std::unique_ptr<Backend> backend_;
+  THREAD_CHECKER(audio_thread_checker_);
+  base::WeakPtrFactory<CastAudioOutputStream> audio_weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CastAudioOutputStream);
 };

chromecast/media/cma/test/mock_multiroom_manager.h

@@ -27,18 +27,30 @@ class MockMultiroomManager : public mojom::MultiroomManager {
   void GetMultiroomInfo(const std::string& session_id,
                         GetMultiroomInfoCallback callback) override;
 
+  void SetMultiroomInfo(chromecast::mojom::MultiroomInfo info);
+  const std::string GetLastSessionId() { return last_session_id_; }
+
  private:
   mojo::Binding<mojom::MultiroomManager> binding_;
+  chromecast::mojom::MultiroomInfo info_;
+  std::string last_session_id_;
 };
 
-inline MockMultiroomManager::MockMultiroomManager() : binding_(this) {}
+inline MockMultiroomManager::MockMultiroomManager()
+    : binding_(this), last_session_id_("default_session_id") {}
 
 inline MockMultiroomManager::~MockMultiroomManager() = default;
 
 inline void MockMultiroomManager::GetMultiroomInfo(
     const std::string& session_id,
     GetMultiroomInfoCallback callback) {
-  std::move(callback).Run(chromecast::mojom::MultiroomInfo::New());
+  last_session_id_ = session_id;
+  std::move(callback).Run(info_.Clone());
+}
+
+inline void MockMultiroomManager::SetMultiroomInfo(
+    chromecast::mojom::MultiroomInfo info) {
+  info_ = std::move(info);
 }
 
 }  // namespace media