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..c40b0e9e27d3b9e9b8864d6bef19fae5f230e510 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();
+}
+
 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,58 @@ void RenderWidgetHostViewAndroid::DidCreateNewRendererCompositorFrameSink(
   surface_returned_resources_.clear();
 }
 
+void RenderWidgetHostViewAndroid::UpdateFullscreenState() {
+  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_;
+
+  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_);
+        fullscreen_state_ = FullscreenState::kEnteringFullscreen;
+        fullscreen_transition_awaiting_resize_ = true;

[boliu, 2017/06/20 20:54:44]

this seems way too brittle. how do you know enter/exit fullscreen will always
come before the resize?

[aelias_OOO_until_Jul13, 2017/06/20 21:15:14]

Agreed, I gave a lot of feedback/suggestions about this above.

[Khushal, 2017/06/20 21:26:35]

You're right, this is a brittle assumption. We need a better way to detect when
the fullscreen transition on the browser actually completes. Having android
resize the surface is async and we currently notify the renderer that the
transition is complete before that happens and we start getting frames with the
element fullscreen-ed using the old surface size which causes videos to jump
around during these transitions.

So either notifying the renderer needs to wait on all state transitions on the
browser to finish or we need an API that the code can call here to know what the
fullscreen size is supposed to be and not mark the transition complete until
both the browser and renderer converge to that size (which is what I think
aelias@ was suggesting).

[Khushal, 2017/06/20 21:44:03]

How do you feel about an API via ViewAndroidDelegate to know what fullscreen
size to expect and using that here instead?

[boliu, 2017/06/20 22:07:51]

You can get the display size directly through the ui::display api. No need to go
through ViewAndroidDelegate.

But I'm more concerned about if that's actually the fullscreen size. This is
android.. any kind of weirdness is possible. Also in webview, the fullscreen
size is controlled directly by the app, and apps have no obligations to actually
make it fullscreen (although I guess this code is only really relevant to chrome
I guess). Also not sure how this plays with picture in picture, mlamouri@ can
comment on that.

We probably can order these events though. The full screen signal goes from
renderer to browser, then browser requests fullscreen, which leads to a
corresponding resize. So if we set this pending fullscreen state before we
request fullscreen, then the resize is guaranteed to come after. It's just this
code is definitely not written that way. Although I suppose the problem then
might be the reverse, that an empty frame is swapped before the fullscreen
transition starts?

[Khushal, 2017/06/20 23:25:05]

WebView was what I was most unsure about. Is a resize guaranteed there?

For Chrome whichever of these 2 you think will be a better assumption to make in
this code would be fine.
Sure, the pending state can be set before the transition is initiated (in
WebContentsImpl). The reason it works currently is because the resize always
comes asynchronously from android. Though WebContentsImpl uses
IsFullscreenForTab after requesting the transition to know if it was granted. Is
there a case where that request could fail?

The empty frame doesn't get committed/swapped until the next frame is requested
by the Display. That behavior won't change.

[boliu, 2017/06/20 23:53:51]

Nope. (It's more likely the app doesn't implement fullscreen, in which case the
request to go fullscreen will just be ignored). But webview doesn't have a
browser compositor, and EvictDelegatedFrame here does nothing on webview, so
it's kind of moot anyway.
Hmm?
Webview: yes. Chrome: dunno?
Wait, I don't get that part. Display as in cc::Display, right? Chrome is has a
push model, frames are pushed from rendere to display. Do you mean display can
stops sending begin frames? Does that actually happen during fullscreen
transition?

[Khushal, 2017/06/21 00:08:25]

Oh right. WebView doesn't get any frames at all here. It has its own frame sink.
I meant between assuming that a resize will follow, or assuming the screen size
should match the expected fullscreen size.
I meant that the frame update will happen on a different task, whether you evict
the frame before initiating the transition or after. The frame eviction will
have to go through a browser compositor commit since it will need a new
CompositorFrame which does not embed the Surface for the renderer's frame.

[boliu, 2017/06/21 00:32:36]

I'd prefer neither. But resize is probably safer of the two. I think on older
versions of android, there was no way to actually query the display size, and
ui::display just returns the size minus the borders which presumably would
disappear in fullscreen.

+      }
+      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_);
+        fullscreen_state_ = FullscreenState::kExitingFullscreen;
+        fullscreen_transition_awaiting_resize_ = true;
+      } else if (mismatched_sizes) {
+        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 &&

[boliu, 2017/06/20 20:54:44]

also, how do you know entering/existing fullscreen will cause a resize? it's
possible in theory that the webcontents stay the same size (no window borders
etc).

for rotation, in the pathological case case, what if you have a square phone?

I don't think there's any guarantee that the fullscreen state converges either.

overall, I think this code is too brittle, it's adding assumptions/dependencies
between a whole bunch of largely independent signals, with no tests. no one is
going to understand this in a year or two, and it'll probably just get removed

[aelias_OOO_until_Jul13, 2017/06/20 21:15:14]

I'm comfortable assuming that fullscreen and rotation transitions will always
cause resize.  These concerns are too theoretical -- let's not overconstrain the
solution space unnecessarily, because this problem is difficult enough as it is.
 There have been thousands of models of Android devices and all of them to the
best of my knowledge have a mandatory notification shade and are not perfectly
square, and we probably have a fair amount of code implicitly relying on these
properties already.

[Khushal, 2017/06/20 21:26:35]

Yup, need a more explicit way to handle the resize.
The primary cases where the rotation transition looks bad is where there is a
mismatch in portrait vs landscape sizes and the video suddenly jumps from a
corner on the screen to fullscreen. In the case where these sizes match, not
entering the transition should look fine IMO.
What would be the case where the fullscreen states don't converge?

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

A lot of good points here, though regarding the square phone case: this will
(correctly) not trigger since we won't need to block any frames (since they'll
already be the right size)

[boliu, 2017/06/20 21:33:49]

Sure square phone is not realistic. But I'm not sure about notification shade
thing. How many of the existing assumptions would completely break rendering if
it was wrong?

On 2017/06/20 21:26:35, Khushal wrote:
Glanced at the cc change. I don't think setting fullscreen actually guarantees a
frame will be produced. It's all side effects of other code..

[Khushal, 2017/06/20 21:44:03]

The place where that bit is being set doesn't require a commit because its done
where the renderer is granted fullscreen so the bit will come whenever the first
fullscreen frame is produced. Its similar to the background color change.

[boliu, 2017/06/20 22:07:51]

Err... that makes me think background color is wrong too, not the other way
around.

Btw, if you want something to go through renderer compositor, but not
necessarily trigger swap or any additional work that's otherwise not needed, you
want SwapPromise. Looks like this might be a good fit (assuming if you don't
take the transparent background advice)

[Khushal, 2017/06/20 23:25:05]

Hmmm, may be it should be asking for a commit...
I wanted this state to be bundled with the frame so we can use it to decide
whether to use a frame on the browser or not so the metadata was the best place
to put it. SwapPromise wouldn't give that.

The reason why the transparent bg wouldn't work was because blink uses that only
for the path where a separate SurfaceView is used for videos and we wanted a
signal to capture all fullscreen cases. But since the fullscreen size is
different on Chrome, do we just want to use that to decide that the renderer
frame is or is not fullscreen and evict frames if there is a mismatch?

+          !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();

[boliu, 2017/06/20 20:54:44]

need a comment why this is desirable

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

Done.

+    UpdateBackgroundColor(SK_ColorBLACK);
+  }
+}
+
 void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
     const cc::LocalSurfaceId& local_surface_id,
     cc::CompositorFrame frame) {
@@ -1243,6 +1302,7 @@ void RenderWidgetHostViewAndroid::DestroyDelegatedContent() {
 
   frame_evictor_->DiscardedFrame();
   delegated_frame_host_->DestroyDelegatedContent();
+  current_surface_size_.SetSize(0, 0);
 }
 
 void RenderWidgetHostViewAndroid::OnDidNotProduceFrame(
@@ -1455,6 +1515,7 @@ void RenderWidgetHostViewAndroid::OnFrameMetadataUpdated(
     touch_selection_controller_->OnViewportChanged(viewport_rect);
   }
 
+  current_frame_is_fullscreen_ = frame_metadata.is_fullscreen;
   UpdateBackgroundColor(is_transparent ? SK_ColorTRANSPARENT
                                        : frame_metadata.root_background_color);
 
@@ -1488,6 +1549,8 @@ void RenderWidgetHostViewAndroid::OnFrameMetadataUpdated(
       frame_metadata.top_controls_height *
           frame_metadata.top_controls_shown_ratio,
       top_shown_pix, top_changed, is_mobile_optimized);
+
+  UpdateFullscreenState();
 }
 
 void RenderWidgetHostViewAndroid::ShowInternal() {
@@ -1930,6 +1993,12 @@ SkColor RenderWidgetHostViewAndroid::GetCachedBackgroundColor() const {
   return cached_background_color_;
 }
 
+bool RenderWidgetHostViewAndroid::CanShowThumbnailPlaceholder() const {
+  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 +2128,8 @@ void RenderWidgetHostViewAndroid::OnGestureEvent(
 }
 
 void RenderWidgetHostViewAndroid::OnPhysicalBackingSizeChanged() {
+  fullscreen_transition_awaiting_resize_ = false;
+  UpdateFullscreenState();
   WasResized();
 }