[Media, Video] Enable the video track for a new renderer.

media/blink/webmediaplayer_impl.cc

Index: media/blink/webmediaplayer_impl.cc
diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
index a73d688ffb666568451ea1889d549ee59786de9f..c44ada4b28883a4085ef020e6e0df6d22797948b 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -200,6 +200,8 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
                      base::Unretained(this)),
           base::Bind(&WebMediaPlayerImpl::OnPipelineSeeked, AsWeakPtr()),
           base::Bind(&WebMediaPlayerImpl::OnPipelineSuspended, AsWeakPtr()),
+          base::Bind(&WebMediaPlayerImpl::OnBeforePipelineResume, AsWeakPtr()),
+          base::Bind(&WebMediaPlayerImpl::OnPipelineResumed, AsWeakPtr()),
           base::Bind(&WebMediaPlayerImpl::OnError, AsWeakPtr())),
       load_type_(LoadTypeURL),
       opaque_(false),
@@ -474,6 +476,7 @@ void WebMediaPlayerImpl::pause() {
   DCHECK(watch_time_reporter_);
   watch_time_reporter_->OnPaused();
   media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::PAUSE));
+
   UpdatePlayState();
 }
 
@@ -671,9 +674,7 @@ void WebMediaPlayerImpl::selectedVideoTrackChanged(
 
   std::ostringstream logstr;
   std::vector<MediaTrack::Id> selectedVideoMediaTrackId;
-  bool canAddVideoTrack =
-      !IsBackgroundVideoTrackOptimizationEnabled() || !IsHidden();
-  if (selectedTrackId && canAddVideoTrack) {
+  if (selectedTrackId && !video_track_disabled_) {
     selectedVideoMediaTrackId.push_back(selectedTrackId->utf8().data());
     logstr << selectedVideoMediaTrackId[0];
   }
@@ -1064,6 +1065,7 @@ void WebMediaPlayerImpl::OnCdmAttached(bool success) {
 void WebMediaPlayerImpl::OnPipelineSeeked(bool time_updated) {
   seeking_ = false;
   seek_time_ = base::TimeDelta();
+
   if (paused_) {
 #if defined(OS_ANDROID)  // WMPI_CAST
     if (isRemote()) {
@@ -1084,6 +1086,10 @@ void WebMediaPlayerImpl::OnPipelineSeeked(bool time_updated) {
   // Reset underflow count upon seek; this prevents looping videos and user
   // actions from artificially inflating the underflow count.
   underflow_count_ = 0;
+
+  // Apply optimizations for the hidden videos if hidden.
+  if (IsHidden())
+    OnHidden();

[sandersd (OOO until July 31), 2017/01/10 21:34:37]

OnHidden() does a bit more than what is requested, please factor out the
relevant part.

[whywhat, 2017/01/11 19:41:30]

I think that's the point of it, no? We initialize the player as if it's shown
despite it's being hidden so when it starts and we're hidden we want all of it -
update play state, schedule idle pause and so on. I think it shouldn't change
the existing behavior with the flag being off since the media is playing at this
point.

[sandersd (OOO until July 31), 2017/01/11 21:54:51]

The target state for this code is that OnHidden() calls UpdatePlayState() and
does nothing else; I don't want to have to think about every possible transition
that WMPI can go through.

If there is something that is behaving incorrectly with regards to that, it is a
bug.

Looking at what's already in there:
  - WatchTimeReporter is constructed in the correct state.
  - Selected video track is this CL.
  - ScheduleIdlePauseTimer() is probably buggy. I'm not fully up to date on
autoplay cases so I'm not sure what behavior we want, but this should move into
UpdatePlayState() eventually.

 }
 
 void WebMediaPlayerImpl::OnPipelineSuspended() {
@@ -1111,6 +1117,26 @@ void WebMediaPlayerImpl::OnPipelineSuspended() {
   }
 }
 
+void WebMediaPlayerImpl::OnBeforePipelineResume() {
+  // Enable video track if we disabled it in the background - this way the new
+  // renderer will attach its callbacks to the video stream properly.
+  // TODO(avayvod): Remove this when disabling and enabling video tracks in
+  // non-playing state works correctly. See https://crbug.com/678374.
+  EnableVideoTrackIfNeeded();
+  is_pipeline_resuming_ = true;
+}
+
+void WebMediaPlayerImpl::OnPipelineResumed() {
+  is_pipeline_resuming_ = false;
+
+  if (IsHidden()) {
+    DisableVideoTrackIfNeeded();
+    return;

[sandersd (OOO until July 31), 2017/01/10 21:34:37]

Nit: This may be a case where early return is more confusing than an else
branch.

(I'd go further and say this should be part of UpdatePlayState() but since the
plan is to rip it all out I won't recommend that.)

[whywhat, 2017/01/11 19:41:30]

Done. For some reason, I expect lint to complain though.

+  }
+
+  EnableVideoTrackIfNeeded();
+}
+
 void WebMediaPlayerImpl::OnDemuxerOpened() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   client_->mediaSourceOpened(
@@ -1353,18 +1379,16 @@ void WebMediaPlayerImpl::OnHidden() {
   if (watch_time_reporter_)
     watch_time_reporter_->OnHidden();
 
-  if (!IsStreaming() && IsBackgroundVideoTrackOptimizationEnabled()) {
-    if (ShouldPauseWhenHidden()) {
-      // OnPause() will set |paused_when_hidden_| to false and call
-      // UpdatePlayState(), so set the flag to true after and then return.
-      OnPause();
-      paused_when_hidden_ = true;
-      return;
-    }
-
-    selectedVideoTrackChanged(nullptr);
+  if (!paused_when_hidden_ && ShouldPauseWhenHidden()) {

[sandersd (OOO until July 31), 2017/01/10 21:34:37]

As a matter of style, I prefer idempotency wherever possible. Since in this case
running this code twice will actually result in disabling the video track, it
would be better to make the |!paused_when_hidden_| condition one level deeper.

Relatedly, ShouldDisableVideoWhenHidden() should probably return false if
ShouldPauseWhenHidden() is true.

[whywhat, 2017/01/11 19:41:30]

I think it won't because ShouldPauseWhenHidden and ShouldDisableWhenHidden are
mutually exclusive via hasAudio().

Done though.
That's already true.

+    // OnPause() will set |paused_when_hidden_| to false and call
+    // UpdatePlayState(), so set the flag to true after and then return.
+    OnPause();
+    paused_when_hidden_ = true;
+    return;
   }
 
+  DisableVideoTrackIfNeeded();
+
   UpdatePlayState();
 
   // Schedule suspended playing media to be paused if the user doesn't come back
@@ -1382,19 +1406,14 @@ void WebMediaPlayerImpl::OnShown() {
       base::Bind(&VideoFrameCompositor::SetForegroundTime,
                  base::Unretained(compositor_), base::TimeTicks::Now()));
 
-  if (!IsStreaming() && IsBackgroundVideoTrackOptimizationEnabled()) {
-    if (paused_when_hidden_) {
-      paused_when_hidden_ = false;
-      OnPlay();  // Calls UpdatePlayState() so return afterwards.
-      return;
-    }
-
-    if (client_->hasSelectedVideoTrack()) {
-      WebMediaPlayer::TrackId trackId = client_->getSelectedVideoTrackId();
-      selectedVideoTrackChanged(&trackId);
-    }
+  if (paused_when_hidden_) {

[sandersd (OOO until July 31), 2017/01/10 21:34:37]

By the same reasoning, this should go right before UpdatePlayState() so that all
the other state changes are idempotent. (And |must_suspend_| is important to
update in any case.)

[whywhat, 2017/01/11 19:41:30]

Moved the state update above the new logic.

I don't want to check if the video "should be paused/disabled" here though since
it may change since it has been paused/disabled and we must unpause/enable it
anyway. (not sure if you meant that or not).

We could have a hidden state enum instead of two bools to enforce exclusiveness
of being either paused or without video track but not both.

+    paused_when_hidden_ = false;
+    OnPlay();  // Calls UpdatePlayState() so return afterwards.
+    return;
   }
 
+  EnableVideoTrackIfNeeded();
+
   must_suspend_ = false;
   background_pause_timer_.Stop();
 
@@ -2080,12 +2099,53 @@ void WebMediaPlayerImpl::ActivateViewportIntersectionMonitoring(bool activate) {
 }
 
 bool WebMediaPlayerImpl::ShouldPauseWhenHidden() const {
+  DCHECK(IsHidden());
+// Don't pause videos being Cast (Android only) or if the background video
+// optimizations are off (desktop only).
 #if defined(OS_ANDROID)  // WMPI_CAST
   if (isRemote())
     return false;
-#endif  // defined(OS_ANDROID)  // WMPI_CAST
+#else   // defined(OS_ANDROID)
+  if (!IsBackgroundVideoTrackOptimizationEnabled())
+    return false;
+#endif  // defined(OS_ANDROID)
 
   return hasVideo() && !hasAudio();

[sandersd (OOO until July 31), 2017/01/10 21:34:37]

Probably worth a comment somewhere: these conditions imply that
ShouldPauseWHenHidden() and ShouldDisableVideoWhenHidden() return false before
Start() is called.

They don't quite imply that they return false until Start() has finished, but
|seeking_| does.

[whywhat, 2017/01/11 19:41:30]

Should I make an even stronger comment that they will be false until the demuxer
is initialized and OnMetadata is called? Or is it the same thing essentially?
|seeking_| implies that tracks won't be disabled in the methods below, but these
methods can return true already if OnMetadata was called, right?

Added comments in the header.
Didn't find anything I could easily DCHECK.

[sandersd (OOO until July 31), 2017/01/11 21:54:51]

OnMetadata is called after demuxer initialization, seeking completes after
decoder initialization. Exactly what to document depends on what is relevant to
track changes: I assumed it was decoder, but that may be stricter than
necessary.

 }
 
+bool WebMediaPlayerImpl::ShouldDisableVideoWhenHidden() const {
+  DCHECK(IsHidden());
+  return IsBackgroundVideoTrackOptimizationEnabled() && hasVideo() &&
+         hasAudio() && !IsStreaming();
+}
+
+void WebMediaPlayerImpl::EnableVideoTrackIfNeeded() {
+  DCHECK(!IsHidden());
+
+  // Don't change video track while the pipeline is resuming or seeking.
+  if (is_pipeline_resuming_ || seeking_)
+    return;
+
+  if (video_track_disabled_) {
+    video_track_disabled_ = false;
+    if (client_->hasSelectedVideoTrack()) {
+      WebMediaPlayer::TrackId trackId = client_->getSelectedVideoTrackId();
+      selectedVideoTrackChanged(&trackId);
+    }
+  }
+}
+
+void WebMediaPlayerImpl::DisableVideoTrackIfNeeded() {
+  DCHECK(IsHidden());
+
+  // Don't change video track while the pipeline is resuming or seeking.
+  if (is_pipeline_resuming_ || seeking_)
+    return;
+
+  if (!video_track_disabled_ && ShouldDisableVideoWhenHidden()) {
+    video_track_disabled_ = true;
+    selectedVideoTrackChanged(nullptr);
+  }
+}
+
 }  // namespace media