Hide fullscreen rotation jank

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index 2da5ad962dfed375abda9c138e1ef5a92d183fbd..8435766cbf5a1abdfb4e8ae94569a1c3ca0f11b8 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -71,6 +71,7 @@
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host_iterator.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "gpu/command_buffer/client/gles2_implementation.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
@@ -565,6 +566,12 @@ void RenderWidgetHostViewAndroid::WasResized() {
   host_->WasResized();
 }
 
+void RenderWidgetHostViewAndroid::OnFullscreenStateChanged(
+    bool entered_fullscreen) {
+  web_contents_is_fullscreen_ = entered_fullscreen;
+  UpdateFullscreenState(false);
+}
+
 void RenderWidgetHostViewAndroid::SetSize(const gfx::Size& size) {
   // Ignore the given size as only the Java code has the power to
   // resize the view on Android.
@@ -1184,6 +1191,64 @@ void RenderWidgetHostViewAndroid::DidCreateNewRendererCompositorFrameSink(
   surface_returned_resources_.clear();
 }
 
+void RenderWidgetHostViewAndroid::UpdateFullscreenState(
+    bool physical_backing_resized) {
+  if (!base::FeatureList::IsEnabled(features::kHideFullscreenTransitionJank))
+    return;
+
+  bool mismatched_fullscreen_states =
+      web_contents_is_fullscreen_ != current_frame_is_fullscreen_;
+  bool mismatched_sizes =
+      view_.GetPhysicalBackingSize() != current_surface_size_;
+
+  if (physical_backing_resized)
+    fullscreen_transition_awaiting_resize_ = false;
+
+  switch (fullscreen_state_) {
+    // From kNotFullscreen, we transition to kEnteringFullscreen whenever the
+    // fullscreen state changes.
+    case FullscreenState::kNotFullscreen:
+      if (mismatched_fullscreen_states) {
+        DCHECK(web_contents_is_fullscreen_);
+        DCHECK(!physical_backing_resized);
+        fullscreen_state_ = FullscreenState::kEnteringFullscreen;
+        fullscreen_transition_awaiting_resize_ = true;
+      }
+      break;
+    // From kFullscreen, we transition to kExitingFullscreen on a fullscreen
+    // state change, or to kFullscreenRotation if we get a new size.
+    case FullscreenState::kFullscreen:
+      if (mismatched_fullscreen_states) {
+        DCHECK(!web_contents_is_fullscreen_);
+        DCHECK(!physical_backing_resized);
+        fullscreen_state_ = FullscreenState::kExitingFullscreen;
+        fullscreen_transition_awaiting_resize_ = true;
+      }
+      if (mismatched_sizes)

[aelias_OOO_until_Jul13, 2017/06/16 22:46:40]

"else".  I don't think ExitingFullscreen being clobbered by FullscreenRotation
is ever desired.

[steimel, 2017/06/16 23:24:12]

Good point. done.

+        fullscreen_state_ = FullscreenState::kFullscreenRotation;
+      break;
+    // From any transition state, once we receive a "good" frame we transition
+    // to the proper "good" state.
+    case FullscreenState::kExitingFullscreen:
+    case FullscreenState::kEnteringFullscreen:
+    case FullscreenState::kFullscreenRotation:
+      if (!mismatched_fullscreen_states && !mismatched_sizes &&
+          !fullscreen_transition_awaiting_resize_) {
+        fullscreen_state_ = web_contents_is_fullscreen_
+                                ? FullscreenState::kFullscreen
+                                : FullscreenState::kNotFullscreen;
+      }
+      break;
+  }
+
+  if (fullscreen_state_ == FullscreenState::kEnteringFullscreen ||
+      fullscreen_state_ == FullscreenState::kExitingFullscreen ||
+      fullscreen_state_ == FullscreenState::kFullscreenRotation) {
+    EvictDelegatedFrame();

[aelias_OOO_until_Jul13, 2017/06/16 22:46:40]

This only needs to happen in SubmitCompositorFrame(), right?  Please move this
out of here and at the end of SubmitCompositorFrame(), explicitly do "if
(IsInFullscreenTransition()) EvictDelegatedFrame();".

[steimel, 2017/06/16 23:24:12]

No, this needs to happen regardless of where UpdateFullscreenState is called,
since we don't want to wait until the next frame to start showing a black screen

[aelias_OOO_until_Jul13, 2017/06/17 01:18:34]

I'd still rather it be broken out and called in multiple places because this
meaty substantive result of the method is easy to miss when mixed in with all
the other logic, and I think it makes more sense for UpdateFullscreenState to
just passively update the state machine.  Also, is it needed to call it on
physical backing size change?

[Khushal, 2017/06/19 18:46:34]

The physical backing size change is the main trigger for handling rotations.

[steimel, 2017/06/22 01:32:11]

Done.

+    UpdateBackgroundColor(SK_ColorBLACK);

[aelias_OOO_until_Jul13, 2017/06/16 22:46:41]

Please change the main UpdateBackgroundColor call in OnFrameMetadataUpdated to
take fullscreen state into account, instead of calling it a second time here.

[steimel, 2017/06/16 23:24:12]

If we're not coming from a SubmitCompositorFrame call, we won't have an
OnFrameMetadataUpdated call AFAIK, and as before, we'll want this to happen
before we receive the next frame.

[aelias_OOO_until_Jul13, 2017/06/17 01:18:34]

Is transparency equally good as black for your purposes?  is_transparent is used
exclusively for fullscreen anyway.  I might suggest setting that argument to
true if the fullscreen state is in transition.

Alternatively, if SkColor_BLACK is better than SkColor_TRANSPARENT during the
transition, then always set both to black.  Either way I'd rather not have two
different background color paths both to solve fullscreen transition cases.

[liberato (no reviews please), 2017/06/19 13:26:37]

aside: i'm decoupling uses of is_transparent_background / SkColor_TRANSPARENT
from fullscreen as part of switching away from ContentVideoView.  once this CL
lands, i think that black will be preferable anyway.

[Khushal, 2017/06/19 18:46:34]

Transparency is only used when the video is being rendered to a seperate
SurfaceView. This doesn't capture the cases where a non-video element is
fullscreen-ed that doesn't go through this path, which I think is used by pages
for custom video controls.
We can not always set the color to black right now since in the separate
SurfaceView case, transparency is needed for the browser compositor to draw
transparent quads to make only the controls visible. May be liberato@'s work
will change that?

[aelias_OOO_until_Jul13, 2017/06/19 19:01:32]

I'd be interested in hearing whether or not we see any flickers if we try
transparency instead of black in the cases meant to be fixed by this patch.  If
we don't, maybe we could always use transparency for video use cases.

[liberato (no reviews please), 2017/06/19 20:22:29]

re "transparency is needed... controls visible".  my work won't change how that
part works.

however, i don't believe that's related to the layer tree background, either. 
the LT background just helps to make a nice orientation transition, else the
re-oriented (by android) SurfaceView clips against the black gutter until blink
does a relayout.

the part that lets the controls show over the video is this: creating a
ContentVideoView starts a  chain of events that causes CompostorSurfaceManager
to notify android not to ignore the alpha channel in the compositor's output. 
it's shortly before CompositorView fiddles with has_transparent_background, if i
remember.

either way, the browser compositor will draw a transparent quad that exactly
covers the SurfaceView through which one can see the video (if android doesn't
ignore alpha!).  that's done by the overlay processer, i think, in the browser
compositor.

the work i'm involved in decouples "using overlay" from "using fullscreen".  it
makes the transparent gutter trick somewhat less appealing, since i'd like to
use it for custom video controls too.

i'm not sure that we need the transparent gutter thing at all once this CL
lands.  we might just be able to go back to a black gutter, all the way from
blink to here.  at least, it looks pretty good in my local testing on slower
hardware with this CL applied.

+  }
+}
+
 void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
     const cc::LocalSurfaceId& local_surface_id,
     cc::CompositorFrame frame) {
@@ -1197,6 +1262,7 @@ void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
 
   cc::RenderPass* root_pass = frame.render_pass_list.back().get();
   current_surface_size_ = root_pass->output_rect.size();
+  current_frame_is_fullscreen_ = frame.metadata.is_fullscreen;

[aelias_OOO_until_Jul13, 2017/06/16 22:46:40]

Please move this to OnFrameMetadataUpdated.

[steimel, 2017/06/16 23:24:12]

Done.

   bool is_transparent = root_pass->has_transparent_background;
 
   cc::CompositorFrameMetadata metadata = frame.metadata.Clone();
@@ -1228,6 +1294,8 @@ void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
   // As the metadata update may trigger view invalidation, always call it after
   // any potential compositor scheduling.
   OnFrameMetadataUpdated(std::move(metadata), is_transparent);
+
+  UpdateFullscreenState(false);

[aelias_OOO_until_Jul13, 2017/06/16 22:46:40]

Please move this call to OnFrameMetadataUpdated.  In this spot, do "if
(IsInFullscreenTransition()) EvictDelegatedFrame();" instead.

[steimel, 2017/06/16 23:24:12]

Done with first part. Second part discussed above

 }
 
 void RenderWidgetHostViewAndroid::DestroyDelegatedContent() {
@@ -1243,6 +1311,7 @@ void RenderWidgetHostViewAndroid::DestroyDelegatedContent() {
 
   frame_evictor_->DiscardedFrame();
   delegated_frame_host_->DestroyDelegatedContent();
+  current_surface_size_.SetSize(0, 0);
 }
 
 void RenderWidgetHostViewAndroid::OnDidNotProduceFrame(
@@ -1930,6 +1999,12 @@ SkColor RenderWidgetHostViewAndroid::GetCachedBackgroundColor() const {
   return cached_background_color_;
 }
 
+bool RenderWidgetHostViewAndroid::CanShowThumbnailPlaceholder() const {

[aelias_OOO_until_Jul13, 2017/06/16 22:46:41]

I'd prefer this to be called IsInFullscreenTransition().

[steimel, 2017/06/16 23:24:12]

It's a bit tricky, since yes, CanShowThumbnailPlaceholder ==
IsInFullscreenTransition. This is used specifically by the WebContentsAndroid to
ask whether or not it can show the thumbnail layer. If we rename this to
IsInFullscreenTransition, then it's the WebContentsAndroid "deciding" that we
can't show the thumbnail because we're in a fullscreen transition. If we leave
the name as is, then it's the RWHVA "deciding" that we can't show the thumbnail
because we're in a fullscreen transition, and the WebContentsAndroid just gets
the end result.

I'm fine with either name, but I figured I'd let you know why first before you
decide.

[steimel, 2017/06/20 21:28:35]

This is method getting removed in the next patch

+  return !(fullscreen_state_ == FullscreenState::kEnteringFullscreen ||
+           fullscreen_state_ == FullscreenState::kExitingFullscreen ||
+           fullscreen_state_ == FullscreenState::kFullscreenRotation);
+}
+
 void RenderWidgetHostViewAndroid::SetIsInVR(bool is_in_vr) {
   is_in_vr_ = is_in_vr;
 }
@@ -2059,6 +2134,9 @@ void RenderWidgetHostViewAndroid::OnGestureEvent(
 }
 
 void RenderWidgetHostViewAndroid::OnPhysicalBackingSizeChanged() {
+  // If we're entering a fullscreen transition, show black until the transition
+  // is completed.
+  UpdateFullscreenState(true);

[aelias_OOO_until_Jul13, 2017/06/16 22:46:40]

Simpler to remove "physical_backing_resized" argument and instead do
"fullscreen_transition_awaiting_resize_ = false; UpdateFullscreenState();

[steimel, 2017/06/16 23:24:12]

Done.

   WasResized();
 }