New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 618987 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Grey/black patch is seen during orientation changes

Project Member Reported by ajit...@samsung.com, Jun 10 2016

Issue description

Version: Chrome 51.0.2704.81
OS: Android 5.1.1 Lollipop
Device: Samsung Galaxy Note4 SM-N910F

What steps will reproduce the problem?
(1) Fetch www.naver.com
(2) Change orientation to Landscape by rotating.
(3) Observe a grey patch on right half of screen
(4) Change orientation again
(5) Observe a grey patch on bottom half of screen

What is the expected output?
Grey patch should not be seen. It should be clean content

What do you see instead?
Grey/black patch is seen during orientation changes.
 
orientation.mp4
5.1 MB View Download
Components: Blink>Paint
Cc: aska...@chromium.org
askatte@ - can you try to repro?  Thanks.
I don't see a black patch, but I do see a grey patch during orientation change on other devices too. And AFAIK, it has always been like that.


I see a black patch sometimes during orientation change on Note 4 / 4.4.4. I don't see it on a Nexus device.
Components: UI>Browser>Mobile
We chose to re-orient the content as fast as possible, then fill in the rest. This gives feedback to the user that the orientation change has been processed, regardless of how long we might take to redraw the screen. The alternative, delaying any redraw until we have a complete frame at the new orientation, would leave a user without a clear immediate signal that the orientation change has been recognized.

I think this is WontFix, but I'll let the mobile UI folks comment.
Cc: siev...@chromium.org
In the video, the grey fill looks better to me than the black one.  I agree  with schenney@ that we will attempt to fill content instead of waiting for a relayout from the renderer and a new frame.

I'm more surprised that there is a different color used in the orientation change from portrait to landscape (black there) and then back (grey there).  I would have expected they'd be the same.

aelias@/sievers@, is that fill logic all done in the browser compositor?  What are your thoughts on the differences between the two rotations?

Comment 7 by aelias@chromium.org, Jul 26 2016

In the video, there's a gray fill for both rotations (gray is the page background color), but it's preceded by a black fill for the rotation to landscape (so there are two stages of placeholder fill).

Given the reported lack of consistent reproducibility in #4, I'd guess it's some kind of race.  We have quite a number of subsystems that fall back to gutter fill so I won't hazard a guess about what is racing with what.
Cc: khushals...@chromium.org
+kushal also since we were talking about simplifying/revisiting the gutter/solidcolor logic.

When I looked at it briefly recently, I wasn't even convinced there exists code nowadays that handles this. And there are around two places (at least) that might draw solid color:

compositor_view.cc has a SolidColorLayer that it might make drawable based on ShouldShowBackground():

bool StaticTabSceneLayer::ShouldShowBackground() {
  scoped_refptr<cc::Layer> root = layer_->RootLayer();
  return root && root->bounds() != layer_->bounds();
}

The other one is the SolidColorLayer in ContentViewCore, but that doesn't handle this size check I think (but only the mere presence of a valid child/frame from the renderer).

We should do this:

- CompositorView should only draw background if there is no content layer attached. But then again, maybe that case should actually never happen instead.

- ContentViewCore's SolidColorLayer can just draw and set its bounds according to the current frame (UpdateFrameInfo or so). For that it would compare the size of the frame that just arrived to the container size.

And ContentViewCore *should* have the right notion of the background color, while I'm not sure about StaticTabSceneLayer::GetBackgroundColor().


Components: -Blink>Paint
Status: Available (was: Untriaged)
Removing Blink label since it's probably browser-side.
A third place is CC's default gutter quad fill logic AppendQuadsToFillScreen() in layer_tree_host_impl.cc.
And we never call LayerTreeHost::set_background_color(), so that one would explain black.
(not saying that black is not correct for the background of the UI.. it probably is reasonable)
@sievers - As per you, showing grey patch (background color of the page) is reasonable, but we need to prevent black patch.

Or

grey patch itself is an issue, which we can't fix right now due to difficulty in fix it.

Could you pls clarify on above statements.

FYI when I change StaticTabSceneLayer::ShouldShowBackground() as below,
bool StaticTabSceneLayer::ShouldShowBackground() {
  scoped_refptr<cc::Layer> root = layer_->RootLayer();
  return root != nullptr;
}

Black patch is not observed, but grey patch (background color) is still there for some duration.
Owner: khushals...@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 15 by bugdroid1@chromium.org, Aug 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a693aa733d2a91627e174d62988c7ef591d963f2

commit a693aa733d2a91627e174d62988c7ef591d963f2
Author: khushalsagar <khushalsagar@google.com>
Date: Tue Aug 16 22:18:06 2016

content: Move Surfaces related code out of RWHVA.

Move logic for managing cc::Surfaces for CompositorFrames and building
Surface Layers to DelegatedFrameHostAndroid from
RenderWidgetHostViewAndroid. This patch moves the logic for SurfaceLayers
and for drawing the background when there is no content from the
renderer.

BUG= 624666 ,  618987 

Review-Url: https://codereview.chromium.org/2133873004
Cr-Commit-Position: refs/heads/master@{#412356}

[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/ui/android/BUILD.gn
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/ui/android/DEPS
[add] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/ui/android/delegated_frame_host_android.cc
[add] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/ui/android/delegated_frame_host_android.h

Status: Fixed (was: Assigned)
This explicitly handles background fill when we have a resize from orientation change so it should fix this bug.
Status: Assigned (was: Fixed)
This issue is still reproducible.

54.0.2837.2 / Samsung Note 4 / 4.4.4

Demo @ http://go/chrome-androidlogs1/6/618987 
Can you try with this revision, https://chromium.googlesource.com/chromium/src/+/4c584d37669a3b2a4e0b80130cb61bad93b0e32e. There was a fix I had to make to the initial logic.
Status: Fixed (was: Assigned)
Should be fixed now.

Sign in to add a comment