Grey/black patch is seen during orientation changes |
||||||||||
Issue descriptionVersion: 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.
,
Jul 25 2016
askatte@ - can you try to repro? Thanks.
,
Jul 25 2016
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.
,
Jul 25 2016
I see a black patch sometimes during orientation change on Note 4 / 4.4.4. I don't see it on a Nexus device.
,
Jul 26 2016
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.
,
Jul 26 2016
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?
,
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.
,
Jul 28 2016
+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().
,
Jul 28 2016
Removing Blink label since it's probably browser-side.
,
Jul 28 2016
A third place is CC's default gutter quad fill logic AppendQuadsToFillScreen() in layer_tree_host_impl.cc.
,
Jul 28 2016
And we never call LayerTreeHost::set_background_color(), so that one would explain black.
,
Jul 28 2016
(not saying that black is not correct for the background of the UI.. it probably is reasonable)
,
Aug 1 2016
@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.
,
Aug 8 2016
,
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
,
Aug 16 2016
This explicitly handles background fill when we have a resize from orientation change so it should fix this bug.
,
Aug 23 2016
This issue is still reproducible. 54.0.2837.2 / Samsung Note 4 / 4.4.4 Demo @ http://go/chrome-androidlogs1/6/618987
,
Aug 23 2016
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.
,
Oct 31 2017
Should be fixed now. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by cbiesin...@chromium.org
, Jul 25 2016