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.

[mlamouri (slow - plz ping), 2017/06/05 12:57:44]

We should probably have a content or cc Feature in order to turn this on/off and
have this behind an experiment.

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

Good idea. I don't know much about hiding features behind flags but maybe we can
discuss offline

+  if (content_view_core_ &&
+      view_.GetPhysicalBackingSize() != prev_physical_backing_size_) {
+    bool is_fullscreen = content_view_core_->GetWebContents()->IsFullscreen();
+    if (is_fullscreen || was_fullscreen_) {
+      fullscreen_transitioning_ = true;
+
+      // 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);
+    }
+    // TODO change logic to not require these variables.
+    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) {
+  // TODO simplify this logic.
+  if (content_view_core_ &&
+      content_view_core_->GetWebContents()->IsFullscreen() != was_fullscreen_) {
+    was_fullscreen_ = !was_fullscreen_;

[mlamouri (slow - plz ping), 2017/06/05 12:57:44]

Here and below, it is fairly confusing that a method called `CheckFor...` ends
up.

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

I'm assuming you mean "ends up modifying state". I didn't really know how to
name this to correctly imply what it was doing without accidentally implying
that it doesn't change state (outside of the fact that it's not const).
Fortunately, in my cleanup of the logic this function doesn't modify state
anymore, so it's const and hopefully clearer :).

+    fullscreen_transitioning_ = true;
+    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;
+    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);
+  }

[mlamouri (slow - plz ping), 2017/06/05 12:57:44]

For my culture, why do we have to repeat the logic here?

Also, would it make sense to have this in a different method as the logic is
fairly the same as above?

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

I have fixed this in my cleanup. It was separate here to make it easier to trace
separately from the other frame eviction events while I was debugging an issue.

 }
 
 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