Don't allow MultiBuffer size increases until kReadyStateHaveEnough.

Since we started allowing play() to occur before the future data state,
we can now run into cases where MultiBuffer ends up increasing its
buffer sizes before renderer initialization starts. It will not satisfy
reads until the new larger buffer sizes are fulfilled, which causes
playback startup delays.

There's no reason to do this since we can just increase the size after
playback starts without penalty. So prevent size increases before
kReadyStateHaveEnough and invoke them only after reaching that state.

BUG=965684
TEST=new unittest.
R=sandersd

Change-Id: I0a7a37528f8c814731a34b6bcc4a6c9a3f9ca5d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1637527
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664952}

media/blink/multibuffer_data_source.cc

@@ -299,8 +299,7 @@ UrlData::CorsMode MultibufferDataSource::cors_mode() const {
 
 void MultibufferDataSource::MediaPlaybackRateChanged(double playback_rate) {
   DCHECK(render_task_runner_->BelongsToCurrentThread());
-
-  if (playback_rate < 0.0)
+  if (playback_rate < 0 || playback_rate == playback_rate_)
     return;
 
   playback_rate_ = playback_rate;
@@ -310,6 +309,9 @@ void MultibufferDataSource::MediaPlaybackRateChanged(double playback_rate) {
 
 void MultibufferDataSource::MediaIsPlaying() {
   DCHECK(render_task_runner_->BelongsToCurrentThread());
+  if (media_has_played_)
+    return;
+
   media_has_played_ = true;
   cancel_on_defer_ = false;
   // Once we start playing, we need preloading.

media/blink/webmediaplayer_impl.cc

@@ -478,7 +478,6 @@ WebMediaPlayer::LoadTiming WebMediaPlayerImpl::Load(
     LoadType load_type,
     const blink::WebMediaPlayerSource& source,
     CorsMode cors_mode) {
-  DVLOG(1) << __func__;
   // Only URL or MSE blob URL is supported.
   DCHECK(source.IsURL());
   blink::WebURL url = source.GetAsURL();
@@ -759,9 +758,6 @@ void WebMediaPlayerImpl::Play() {
   pipeline_controller_->SetPlaybackRate(playback_rate_);
   background_pause_timer_.Stop();
 
-  if (mb_data_source_)
-    mb_data_source_->MediaIsPlaying();
-
   if (observer_)
     observer_->OnPlaying();
 
@@ -779,6 +775,8 @@ void WebMediaPlayerImpl::Play() {
     video_decode_stats_reporter_->OnPlaying();
 
   media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::PLAY));
+
+  MaybeUpdateBufferSizesForPlayback();
   UpdatePlayState();
 }
 
@@ -894,11 +892,10 @@ void WebMediaPlayerImpl::SetRate(double rate) {
   }
 
   playback_rate_ = rate;
-  if (!paused_) {
+  if (!paused_)
     pipeline_controller_->SetPlaybackRate(rate);
-    if (mb_data_source_)
-      mb_data_source_->MediaPlaybackRateChanged(rate);
-  }
+
+  MaybeUpdateBufferSizesForPlayback();
 }
 
 void WebMediaPlayerImpl::SetVolume(double volume) {
@@ -2037,8 +2034,11 @@ void WebMediaPlayerImpl::OnBufferingStateChangeInternal(
     SetReadyState(CanPlayThrough() ? WebMediaPlayer::kReadyStateHaveEnoughData
                                    : WebMediaPlayer::kReadyStateHaveFutureData);
 
-    // Let the DataSource know we have enough data. It may use this information
-    // to release unused network connections.
+    // Let the DataSource know we have enough data -- this is the only function
+    // during which we advance to (or past) the kReadyStateHaveEnoughData state.
+    // It may use this information to update buffer sizes or release unused
+    // network connections.
+    MaybeUpdateBufferSizesForPlayback();
     if (mb_data_source_ && !client_->CouldPlayIfEnoughData()) {
       // For LazyLoad this will be handled during OnPipelineSuspended().
       if (for_suspended_start && did_lazy_load_)
@@ -3559,4 +3559,16 @@ void WebMediaPlayerImpl::MaybeSetContainerName() {
 #endif
 }
 
+void WebMediaPlayerImpl::MaybeUpdateBufferSizesForPlayback() {
+  // Don't increase the MultiBufferDataSource buffer size until we've reached
+  // kReadyStateHaveEnoughData. Otherwise we will unnecessarily slow down
+  // playback startup -- it can instead be done for free after playback starts.
+  if (!mb_data_source_ || highest_ready_state_ < kReadyStateHaveEnoughData)
+    return;
+
+  mb_data_source_->MediaPlaybackRateChanged(playback_rate_);
+  if (!paused_)
+    mb_data_source_->MediaIsPlaying();
+}
+
 }  // namespace media

media/blink/webmediaplayer_impl.h

@@ -580,6 +580,11 @@ class MEDIA_BLINK_EXPORT WebMediaPlayerImpl
 
   void SendBytesReceivedUpdate();
 
+  // Notifies |mb_data_source_| of playback and rate changes which may increase
+  // the amount of data the DataSource buffers. Does nothing prior to reaching
+  // kReadyStateHaveEnoughData for the first time.
+  void MaybeUpdateBufferSizesForPlayback();
+
   blink::WebLocalFrame* const frame_;
 
   // The playback state last reported to |delegate_|, to avoid setting duplicate

media/blink/webmediaplayer_impl_unittest.cc

@@ -581,6 +581,10 @@ class WebMediaPlayerImplTest : public testing::Test {
     return wmpi_->mb_data_source_->cancel_on_defer_for_testing();
   }
 
+  bool IsDataSourceMarkedAsPlaying() const {
+    return wmpi_->mb_data_source_->media_has_played();
+  }
+
   enum class LoadType { kFullyBuffered, kStreaming };
   void Load(std::string data_file,
             LoadType load_type = LoadType::kFullyBuffered) {
@@ -857,6 +861,32 @@ TEST_F(WebMediaPlayerImplTest, LoadPreloadMetadataSuspend) {
   EXPECT_EQ(reported_memory_ - data_source_size, 0);
 }
 
+// Verify that Play() before kReadyStateHaveEnough doesn't increase buffer size.
+TEST_F(WebMediaPlayerImplTest, NoBufferSizeIncreaseUntilHaveEnough) {
+  InitializeWebMediaPlayerImpl();
+  EXPECT_CALL(client_, CouldPlayIfEnoughData()).WillRepeatedly(Return(true));
+  wmpi_->SetPreload(blink::WebMediaPlayer::kPreloadAuto);
+  LoadAndWaitForReadyState(kAudioOnlyTestFile,
+                           blink::WebMediaPlayer::kReadyStateHaveMetadata);
+  testing::Mock::VerifyAndClearExpectations(&client_);
+  EXPECT_CALL(client_, ReadyStateChanged()).Times(AnyNumber());
+  wmpi_->Play();
+  EXPECT_FALSE(IsDataSourceMarkedAsPlaying());
+
+  while (wmpi_->GetReadyState() <
+         blink::WebMediaPlayer::kReadyStateHaveEnoughData) {
+    // Clear the mock so it doesn't have a stale QuitClosure.
+    testing::Mock::VerifyAndClearExpectations(&client_);
+
+    base::RunLoop loop;
+    EXPECT_CALL(client_, ReadyStateChanged())
+        .WillRepeatedly(RunClosure(loop.QuitClosure()));
+    loop.Run();
+  }
+
+  EXPECT_TRUE(IsDataSourceMarkedAsPlaying());
+}
+
 // Verify that preload=metadata suspend works properly for streaming sources.
 TEST_F(WebMediaPlayerImplTest, LoadPreloadMetadataSuspendNoStreaming) {
   InitializeWebMediaPlayerImpl();