Merge M72: "Release sink when AudioSourceProvider::SetClient() is called."

There's currently no way to resume these sinks from html/js so there's
no point in restoring the sink state after disconnection. Specifically
you can't call MediaElementAudioSourceNode.disconnect() and expect the
underlying media element to be able to ever work again. See this demo:

http://storage.googleapis.com/dalecurtis/webaudio-test.html

Once AudioContext.createMediaElementSource(element) has been called on
an element, that element can never be used standalone again. It must
always be connected to the AudioContext to be used.

In fact it's actually causing unnecessary work when the elements are
destructed since removing the client respins the sink and mixer, etc.

BUG=905506,910951
TEST=passes cq.
R=​hongchan

Change-Id: I5ff7fc532545075d62859a30f96d17c83bff9d21
Reviewed-on: https://chromium-review.googlesource.com/c/1359092
Commit-Queue: Dale Curtis <[email protected]>
Reviewed-by: Hongchan Choi <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#613346}(cherry picked from commit 391671c)
Reviewed-on: https://chromium-review.googlesource.com/c/1362296
Reviewed-by: Dale Curtis <[email protected]>
Cr-Commit-Position: refs/branch-heads/3626@{#50}
Cr-Branched-From: d897fb1-refs/heads/master@{#612437}

Author: dalecurtis

media/blink/webaudiosourceprovider_impl.cc

@@ -130,9 +130,14 @@ void WebAudioSourceProviderImpl::SetClient(
   base::AutoLock auto_lock(sink_lock_);
   if (client) {
     // Detach the audio renderer from normal playback.
-    if (sink_)
+    if (sink_) {
       sink_->Stop();
 
+      // It's not possible to resume an element after disconnection, so just
+      // drop the sink entirely for now.
+      sink_ = nullptr;
+    }
+
     // The client will now take control by calling provideInput() periodically.
     client_ = client;
 
@@ -148,15 +153,9 @@ void WebAudioSourceProviderImpl::SetClient(
     return;
   }
 
-  // Restore normal playback.
+  // Drop client, but normal playback can't be restored. This is okay, the only
+  // way to disconnect a client is internally at time of destruction.
   client_ = nullptr;
-  if (sink_) {
-    sink_->SetVolume(volume_);
-    if (state_ >= kStarted)
-      sink_->Start();
-    if (state_ >= kPlaying)
-      sink_->Play();
-  }
 }
 
 void WebAudioSourceProviderImpl::ProvideInput(
@@ -198,7 +197,8 @@ void WebAudioSourceProviderImpl::Initialize(const AudioParameters& params,
 
   tee_filter_->Initialize(renderer, params.channels(), params.sample_rate());
 
-  sink_->Initialize(params, tee_filter_.get());
+  if (sink_)
+    sink_->Initialize(params, tee_filter_.get());
 
   if (set_format_cb_)
     std::move(set_format_cb_).Run();
@@ -209,30 +209,30 @@ void WebAudioSourceProviderImpl::Start() {
   DCHECK(tee_filter_);
   DCHECK_EQ(state_, kStopped);
   state_ = kStarted;
-  if (!client_)
+  if (!client_ && sink_)
     sink_->Start();
 }
 
 void WebAudioSourceProviderImpl::Stop() {
   base::AutoLock auto_lock(sink_lock_);
   state_ = kStopped;
-  if (!client_)
+  if (!client_ && sink_)
     sink_->Stop();
 }
 
 void WebAudioSourceProviderImpl::Play() {
   base::AutoLock auto_lock(sink_lock_);
   DCHECK_EQ(state_, kStarted);
   state_ = kPlaying;
-  if (!client_)
+  if (!client_ && sink_)
     sink_->Play();
 }
 
 void WebAudioSourceProviderImpl::Pause() {
   base::AutoLock auto_lock(sink_lock_);
   DCHECK(state_ == kPlaying || state_ == kStarted);
   state_ = kStarted;
-  if (!client_)
+  if (!client_ && sink_)
     sink_->Pause();
 }
 
@@ -257,10 +257,12 @@ void WebAudioSourceProviderImpl::GetOutputDeviceInfoAsync(
     return;
   }
 
+  // Just return empty hardware parameters. When a |client_| is attached, the
+  // underlying audio renderer will prefer the media parameters. See
+  // IsOptimizedForHardwareParameters() for more details.
   base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(std::move(info_cb),
-                     OutputDeviceInfo(OUTPUT_DEVICE_STATUS_ERROR_NOT_FOUND)));
+      FROM_HERE, base::BindOnce(std::move(info_cb),
+                                OutputDeviceInfo(OUTPUT_DEVICE_STATUS_OK)));
 }
 
 bool WebAudioSourceProviderImpl::IsOptimizedForHardwareParameters() {

media/blink/webaudiosourceprovider_impl_unittest.cc

@@ -114,19 +114,17 @@ class WebAudioSourceProviderImplTest
 };
 
 TEST_F(WebAudioSourceProviderImplTest, SetClientBeforeInitialize) {
-  // setClient() with a NULL client should do nothing if no client is set.
-  wasp_impl_->SetClient(NULL);
+  // setClient() with a nullptr client should do nothing if no client is set.
+  wasp_impl_->SetClient(nullptr);
 
   // If |mock_sink_| is not null, it should be stopped during setClient(this).
   if (mock_sink_)
-    EXPECT_CALL(*mock_sink_.get(), Stop()).Times(2);
+    EXPECT_CALL(*mock_sink_.get(), Stop());
 
   wasp_impl_->SetClient(this);
   base::RunLoop().RunUntilIdle();
 
-  if (mock_sink_)
-    EXPECT_CALL(*mock_sink_.get(), SetVolume(1)).Times(1);
-  wasp_impl_->SetClient(NULL);
+  wasp_impl_->SetClient(nullptr);
   base::RunLoop().RunUntilIdle();
 
   wasp_impl_->SetClient(this);
@@ -155,34 +153,10 @@ TEST_F(WebAudioSourceProviderImplTest, SinkMethods) {
   SetClient(this);
   CallAllSinkMethodsAndVerify(false);
 
-  // Removing the client should cause WASP to revert to the underlying sink.
-  EXPECT_CALL(*mock_sink_, SetVolume(kTestVolume));
-  SetClient(NULL);
-  CallAllSinkMethodsAndVerify(true);
-}
-
-// Verify underlying sink state is restored after client removal.
-TEST_F(WebAudioSourceProviderImplTest, SinkStateRestored) {
-  wasp_impl_->Initialize(params_, &fake_callback_);
-
-  // Verify state set before the client is set propagates back afterward.
-  EXPECT_CALL(*mock_sink_, Start());
-  wasp_impl_->Start();
-  SetClient(this);
-
-  EXPECT_CALL(*mock_sink_, SetVolume(1.0));
-  EXPECT_CALL(*mock_sink_, Start());
-  SetClient(NULL);
-
-  // Verify state set while the client was attached propagates back afterward.
-  SetClient(this);
-  wasp_impl_->Play();
-  wasp_impl_->SetVolume(kTestVolume);
-
-  EXPECT_CALL(*mock_sink_, SetVolume(kTestVolume));
-  EXPECT_CALL(*mock_sink_, Start());
-  EXPECT_CALL(*mock_sink_, Play());
-  SetClient(NULL);
+  // Removing the client should cause WASP to revert to the underlying sink;
+  // this shouldn't crash, but shouldn't do anything either.
+  SetClient(nullptr);
+  CallAllSinkMethodsAndVerify(false);
 }
 
 // Test the AudioRendererSink state machine and its effects on provideInput().