[go: nahoru, domu]

[M123] Revert "Improve AudioRendererMixer sharing across RenderFrames."

This reverts commit a6068ebab3469d9d7793381987b1168434afaabe.

Reason for revert: Causing too many unforeseen consequences, see
327884890, 327489297

Original change's description:
> Improve AudioRendererMixer sharing across RenderFrames.
>
> It's common for a page to have a lot of YouTube embeds. With the
> current code, none of those embeds share a mixer and thus can
> trigger the 50 physical stream limit quickly.
>
> AFAICT, there's no good reason to prevent sharing of these streams
> across render frames when the default device is used. They are
> output only and an uncompromised renderer can't directly observe
> any effects (readback for WebAudio and MediaStreams happens
> asynchronously below the shared output).
>
> Often the default device is already being shared at the OS level,
> so any demultiplexing we do in the renderer seems pointless.
>
> Looking through the history we've always limited the sharing to
> per RenderFrameID, but it was only out of an abundance of caution
> rather than a particular concern.
>
> For non-default devices there's good reasons we shouldn't share:
> permissions are required per-origin to access the non-default
> device.
>
> R=olka, tguilbert
>
> Bug: b/314766691
> Change-Id: I7ee6865eb675962aff2aeedcd7290666b38c91e5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5185891
> Commit-Queue: Olga Sharonova <olka@chromium.org>
> Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
> Reviewed-by: Olga Sharonova <olka@chromium.org>
> Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1245750}

(cherry picked from commit 5f20e0e777c3820b92b85fa941a888fe0a50750d)

Bug: b/314766691, b/327489297, b/327884890
Change-Id: Idb20538593c6303900ebc6062501b424974cbb55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5354149
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1269893}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5356349
Cr-Commit-Position: refs/branch-heads/6312@{#526}
Cr-Branched-From: 6711dcdae48edaf98cbc6964f90fac85b7d9986e-refs/heads/main@{#1262506}
diff --git a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.h b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.h
index d7e5ebc..5b236704 100644
--- a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.h
+++ b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.h
@@ -123,6 +123,8 @@
   // mixers where only irrelevant keys mismatch.
   struct MixerKeyCompare {
     bool operator()(const MixerKey& a, const MixerKey& b) const {
+      if (a.source_frame_token != b.source_frame_token)
+        return a.source_frame_token < b.source_frame_token;
       if (a.params.channels() != b.params.channels())
         return a.params.channels() < b.params.channels();
 
@@ -141,13 +143,6 @@
       if (a.params.effects() != b.params.effects())
         return a.params.effects() < b.params.effects();
 
-      // Don't check the `source_frame_token` when the default device is used
-      // since that will prevent sharing of the output device across render
-      // frames. A common scenario is a lot of YouTube embeds which would each
-      // end up with their own output device if we check the frame token here.
-      //
-      // Output sink behavior can't be directly observed by the page, so there's
-      // no harm in sharing the sink across origins for the default device.
       if (media::AudioDeviceDescription::IsDefaultDevice(a.device_id) &&
           media::AudioDeviceDescription::IsDefaultDevice(b.device_id)) {
         // Both device IDs represent the same default device => do not compare
@@ -155,12 +150,6 @@
         return false;
       }
 
-      // Since a non-default device is being used, we must check the frame
-      // token to ensure authorization has been completed for the frame.
-      if (a.source_frame_token != b.source_frame_token) {
-        return a.source_frame_token < b.source_frame_token;
-      }
-
       return a.device_id < b.device_id;
     }
   };
diff --git a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager_test.cc b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager_test.cc
index 3c6a324..1015555 100644
--- a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager_test.cc
+++ b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager_test.cc
@@ -270,45 +270,29 @@
       kFrameToken, base::UnguessableToken(), kDefaultDeviceId,
       AudioLatency::Type::kPlayback, params, &callback);
   EXPECT_EQ(0u, mixer_count());
-  ASSERT_EQ(mock_sink_, nullptr);  // Sink is consumed by CreateInputHelper.
-
-  // Despite being from another frame, this input uses the default device, so
-  // should share the previously created mixer.
   media::FakeAudioRenderCallback another_callback(1, kSampleRate);
+
+  EXPECT_FALSE(!!mock_sink_);
+  mock_sink_ = CreateNormalSink();
+  EXPECT_CALL(*mock_sink_, Start()).Times(1);
   auto another_input = CreateInputHelper(
       kAnotherFrameToken, base::UnguessableToken(), kDefaultDeviceId,
       AudioLatency::Type::kPlayback, params, &another_callback);
   EXPECT_EQ(0u, mixer_count());
 
-  // Since this input uses a non-default device id it should not share the
-  // previous mixer.
-  media::FakeAudioRenderCallback another_callback2(1, kSampleRate);
-  mock_sink_ = CreateNormalSink(kAnotherDeviceId);
-  EXPECT_CALL(*mock_sink_, Start()).Times(1);
-  auto another_input2 = CreateInputHelper(
-      kAnotherFrameToken, base::UnguessableToken(), kAnotherDeviceId,
-      AudioLatency::Type::kPlayback, params, &another_callback2);
-  EXPECT_EQ(0u, mixer_count());
-  ASSERT_EQ(mock_sink_, nullptr);  // Sink is consumed by CreateInputHelper.
-
   // Implicitly test that AudioRendererMixerInput was provided with the expected
   // callbacks needed to acquire an AudioRendererMixer and return it.
   input->Start();
   EXPECT_EQ(1u, mixer_count());
   another_input->Start();
-  EXPECT_EQ(1u, mixer_count());
-  another_input2->Start();
   EXPECT_EQ(2u, mixer_count());
 
   // Destroying the inputs should destroy the mixers.
   input->Stop();
   input = nullptr;
-  EXPECT_EQ(2u, mixer_count());
+  EXPECT_EQ(1u, mixer_count());
   another_input->Stop();
   another_input = nullptr;
-  EXPECT_EQ(1u, mixer_count());
-  another_input2->Stop();
-  another_input2 = nullptr;
   EXPECT_EQ(0u, mixer_count());
 }