Hide fullscreen rotation jank

content/browser/renderer_host/render_widget_host_view_android.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_view_android.h"
 
 #include <android/bitmap.h>
 
 #include <utility>
 
@@ skipping 541 matching lines @@
     RenderWidgetHostView* reference_host_view) {
   NOTIMPLEMENTED();
 }
 
 RenderWidgetHost*
 RenderWidgetHostViewAndroid::GetRenderWidgetHost() const {
   return host_;
 }
 
 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();
 }
 
 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.
   default_bounds_ = gfx::Rect(default_bounds_.origin(), size);
 }
 
 void RenderWidgetHostViewAndroid::SetBounds(const gfx::Rect& rect) {
@@ skipping 602 matching lines @@
                                     bad_message::RWH_BAD_FRAME_SINK_REQUEST);
     return;
   }
   delegated_frame_host_->CompositorFrameSinkChanged();
   renderer_compositor_frame_sink_ = renderer_compositor_frame_sink;
   // Accumulated resources belong to the old RendererCompositorFrameSink and
   // should not be returned.
   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) {
   if (!delegated_frame_host_) {
     DCHECK(!using_browser_compositor_);
     return;
   }
 
+  bool has_jank = CheckForFullscreenTransitionJank(frame.metadata);
+
   last_scroll_offset_ = frame.metadata.root_scroll_offset;
   DCHECK(!frame.render_pass_list.empty());
 
   cc::RenderPass* root_pass = frame.render_pass_list.back().get();
   current_surface_size_ = root_pass->output_rect.size();
   bool is_transparent = root_pass->has_transparent_background;
 
   cc::CompositorFrameMetadata metadata = frame.metadata.Clone();
 
   bool has_content = !current_surface_size_.IsEmpty();
@@ skipping 16 matching lines @@
     frame_evictor_->SwappedFrame(!host_->is_hidden());
     AcknowledgeBeginFrame(ack);
   }
 
   if (host_->is_hidden())
     RunAckCallbacks();
 
   // 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() {
   DCHECK(!delegated_frame_host_ ||
          delegated_frame_host_->HasDelegatedContent() ==
              frame_evictor_->HasFrame());
 
   if (!delegated_frame_host_)
     return;
 
@@ skipping 1000 matching lines @@
 
   ui::WindowAndroidCompositor* compositor = window_android->GetCompositor();
   if (!compositor)
     return;
 
   overscroll_controller_ = base::MakeUnique<OverscrollControllerAndroid>(
       overscroll_refresh_handler, compositor, view_.GetDipScale());
 }
 
 }  // namespace content