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 b9898a869d9c3e42e0e206cce3d8230a93d8aee9..e15050d949b88be88d7db080ba9f9d008888699e 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -559,6 +559,24 @@ RenderWidgetHostViewAndroid::GetRenderWidgetHost() const {
 }
 
 void RenderWidgetHostViewAndroid::WasResized() {
+  // If we're entering a fullscreen transition, show black until the transition
+  // is completed.
+  if (content_view_core_ &&
+      view_.GetPhysicalBackingSize() != prev_physical_backing_size_) {

[Khushal, 2017/05/26 21:46:18]

This should compare the physical backing size with the size of the current
frame. You can call the function that is dealing with state transitions here.

Btw, I noticed that we don't update current_surface_size_ to empty on evicting a
frame. That should be fixed.

[steimel, 2017/06/06 03:07:54]

Done.

+    bool is_fullscreen = content_view_core_->GetWebContents()->IsFullscreen();
+    if (is_fullscreen || was_fullscreen_) {
+      fullscreen_transitioning_ = true;

[Khushal, 2017/05/26 21:46:18]

I think I follow this better now. So for the case where you are transitioning
out of fullscreen, you can have a case where the fullscreen state on the browser
and renderer frame match, but the sizes still mismatch. And we want that to also
be black?

I would suggest adding an enum to make the states clearer.

enum class FullscreenState {
  // The browser is not fullscreen and the renderer frame is consistent with
this state.
  kNotFullscreen,
  // The browser has transitioned to fullscreen state but the renderer frame has
not transitioned, i.e., either
  // the renderer frame is not fullscreen or the frame size does not match the
expected size.
  kEnteringFullscreen,
  // The browser is in fullscreen state and the renderer frame is consistent
with expected state.
  kFullscreen,
  // The browser has exited fullscreen state but the renderer frame has not
transitioned.
  kExitingFullscreen,
}

And instead of using WasResized, plumb the correct fullscreen state from
WebContentsImpl here. Just add it RWHVA and cast the current RWHV in
WebContentsImpl to propagate it.

You can keep the logic for updating and validating the state transitions in one
function, have it evict the current frame if necessary, and call it from all
places where you expect the state to change.

[steimel, 2017/06/06 03:07:53]

The logic has been revamped (albeit without the addition of the enum you
suggested). My hope is that it's been simplified enough to not need it, but let
me know if you still think the enum would make a measurable difference in
readability.

+
+      // TODO update this comment. Immediately queue a black frame to hide jank while we wait to receive a
+      // "good" frame via SubmitCompositorFrame.
+      TRACE_EVENT_BEGIN0("frame_eviction", "RenderWidgetHostViewAndroid::WasResized Eviction");
+      EvictDelegatedFrame();
+      TRACE_EVENT_END0("frame_eviction", "RenderWidgetHostViewAndroid::WasResized Eviction");
+      UpdateBackgroundColor(SK_ColorBLACK);
+    }
+    was_fullscreen_ = is_fullscreen;
+    prev_physical_backing_size_ = view_.GetPhysicalBackingSize();
+  }
   host_->WasResized();
 }
 
@@ -1181,6 +1199,35 @@ void RenderWidgetHostViewAndroid::DidCreateNewRendererCompositorFrameSink(
   surface_returned_resources_.clear();
 }
 
+bool RenderWidgetHostViewAndroid::CheckForFullscreenTransitionJank(
+    const cc::CompositorFrameMetadata& metadata) {
+  // Sometimes we'll receive a new frame before WasResized initializes a
+  // fullscreen transition. In that case, we'll block everything until
+  // WasResized is called.
+  if (content_view_core_ &&
+      content_view_core_->GetWebContents()->IsFullscreen() != was_fullscreen_) {
+    return true;
+  }
+
+  // We only check for fullscreen jank during a fullscreen transition.
+  if (fullscreen_transitioning_) {
+    bool mismatched_fullscreen_states =
+        content_view_core_ &&
+        (content_view_core_->GetWebContents()->IsFullscreen() !=
+         metadata.is_fullscreen);
+    bool mismatched_layer_sizes =
+        view_.GetPhysicalBackingSize() != metadata.device_viewport_size;

[Khushal, 2017/05/26 21:46:18]

I think this should be the same as |current_surface_size_|. Can you verify?

[steimel, 2017/06/06 03:07:54]

Done.

+    if (mismatched_fullscreen_states || mismatched_layer_sizes) {
+      return true;
+    } else {
+      // Done with current fullscreen transition.
+      fullscreen_transitioning_ = false;
+    }
+  }
+
+  return false;
+}
+
 void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
     const cc::LocalSurfaceId& local_surface_id,
     cc::CompositorFrame frame) {
@@ -1189,6 +1236,8 @@ void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
     return;
   }
 
+  bool has_jank = CheckForFullscreenTransitionJank(frame.metadata);
+
   last_scroll_offset_ = frame.metadata.root_scroll_offset;
   DCHECK(!frame.render_pass_list.empty());
 
@@ -1225,6 +1274,13 @@ 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);
+
+  if (has_jank) {
+    TRACE_EVENT_BEGIN0("frame_eviction", "RenderWidgetHostViewAndroid::SubmitCompositorFrame Eviction");
+    EvictDelegatedFrame();
+    TRACE_EVENT_END0("frame_eviction", "RenderWidgetHostViewAndroid::SubmitCompositorFrame Eviction");
+    UpdateBackgroundColor(SK_ColorBLACK);
+  }
 }
 
 void RenderWidgetHostViewAndroid::DestroyDelegatedContent() {