Issue metadata
Sign in to add a comment
|
Flash on low-end devices when Chrome comes to foreground |
||||||||||||||||||||||||||||||||
Issue descriptionThis bug is forked from b/69965212, see there for video and initial description. In issue 725303 we implemented a change to destroy GL contexts after Chrome is backgrounded for 5 seconds. Turns out that causes "flash" when Chrome is foregrounded, because the first UI draw operation doesn't capture the GL content, only omnibox + background, so the transition is "Snapshot of a page taken by Android" -> "omnibox + backgound" -> "omnibox + content". I'm not sure exactly why that happens, but the connection to destroying GL contexts is clear. See the attached trace (with some extra trace events), where I backgrounded / foregrounded Chrome 5 times: * First time I foregrounded Chrome (~6.5s) there was no flash. GpuCommandBufferStub::Initialize() didn't call gl::init::CreateGLContext() (i.e. GL context was not recreated), GLContextEGL::MakeCurrent() took 0.1ms and GpuChannelHost::Send() blocked UI thread for just 17ms. * Second time (~10.2s) there was a flash. GpuChannelHost::Send() blocked UI thread for 80ms, because GpuCommandBufferStub::Initialize() recreated GL context (gl::init::CreateGLContext() was called), and GLContextEGL::MakeCurrent() took 51ms (BindGLApi() took 29ms and InitializeDynamicBindings() took 22ms). The context was destroyed by GpuChannelManager::OnApplicationBackgrounded(), which was called at 9.377s mark. What's interesting is that the call to OnApplicationBackgrounded() was scheduled when I backgrounded Chrome for the first time (see "GpuChannelManager::OnApplicationStateChange->OnApplicationBackgrounded" event at 4.377s). * Third and fourth times were like the second time - there were flashes, GpuChannelHost::Send() blocked UI thread for ~70s, GL context was destroyed by OnApplicationBackgrounded() scheduled by backgrounding Chrome one time before. * Before backgrounding Chrome for the fifth time I waited for 5 seconds for OnApplicationBackgrounded() scheduled by the 4th backgounding to run. It was called at 20.173s mark, but didn't destroy the GL context because app was in the foreground. Then I backgrounded Chrome at 23.831s mark (ChromeTabbedActivity.onPause), and foregrounded it at 25.943s mark (ChromeTabbedActivity.onResume). As expected there was no flash, GL context was not recreated, and GpuChannelHost::Send() took only 18ms. Having said that I still don't know exactly why recreating GL context causes a flash. The trace includes view hierarchy / IWindowSession events from crrev.com/c/775182, and I can see that where there a flash there are two ViewHierarchy:draw events before GpuChannelHost::Send(). Not sure if that matters though. Last thing is that flashes are visually worse when GPU raster is enabled. There seems to be an extra frame with white (page's background?) background before GL content appears. I see two action items here: 1. Make sure scheduled OnApplicationBackgrounded() call is invalided once we're in the foreground. I.e. we need to make sure that only OnApplicationBackgrounded() call scheduled by the last backgrounding destroys GL contexts. 2. We need to understand exactly why the flash happens, and try to avoid it.
,
Dec 5 2017
I'll look into this. Khushal mentioned that we may be failing to load our renderer snapshot off disk in a timely fashion. It's interesting that this seems OK the first time you background though...
,
Dec 5 2017
Yes, the first time is fine because GL context is there. Second time should be fine too if you wait >5s in the foreground.
,
Dec 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/101fc851400b64b7e29217d253a237708de49517 commit 101fc851400b64b7e29217d253a237708de49517 Author: Eric Karl <ericrk@chromium.org> Date: Wed Dec 20 02:59:06 2017 Refactor CompositorLock management into its own class Currently, management of outstanding compositor locks and timeouts is managed on ui::Compositor. We want to use this functionality on content::CompositorImpl as well, so this refactor lets us share code. TBR=sky@chromium.org for purely mechanical change (rename) in ash/rotator/screen_rotation_animator.cc Bug: 792120 Change-Id: I13665c1d9eec6a1822ab86baabfdf1bf66db5178 Reviewed-on: https://chromium-review.googlesource.com/834882 Commit-Queue: Eric Karl <ericrk@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#525240} [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/ash/rotator/screen_rotation_animator.cc [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/content/browser/renderer_host/compositor_resize_lock_unittest.cc [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/ui/compositor/BUILD.gn [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/ui/compositor/compositor.cc [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/ui/compositor/compositor.h [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/ui/compositor/compositor_lock.cc [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/ui/compositor/compositor_lock.h [add] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/ui/compositor/compositor_lock_unittest.cc [modify] https://crrev.com/101fc851400b64b7e29217d253a237708de49517/ui/compositor/compositor_unittest.cc [delete] https://crrev.com/8d67775413dd2c00d7ecd4957160ad99503a1547/ui/compositor/test/fake_compositor_lock.cc [delete] https://crrev.com/8d67775413dd2c00d7ecd4957160ad99503a1547/ui/compositor/test/fake_compositor_lock.h
,
Jan 5 2018
Hey Eric, what's the status of this?
,
Jan 5 2018
,
Jan 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c1ced2b2f072d9f2281b04a88f793602dc23a3d4 commit c1ced2b2f072d9f2281b04a88f793602dc23a3d4 Author: Eric Karl <ericrk@chromium.org> Date: Fri Jan 05 21:54:22 2018 Improve timing of placeholder image on Chrome resume Currently, when Chrome is foregrounded, Android OS shows a placeholder image of the last screen rendered. It does this until Chrome fires the surfaceRedrawAsync callback. Chrome currently does this as soon as it produces its first frame after foregrounding. Unfortunately, Chrome doesn't wait for renderer content before producing a frame, so the first frame may include a blank renderer. This is especially common on low-end devices which produce frames more slowly. In these cases, we see the placeholder, then a blank renderer, then a populated renderer, which produces a flickering effect. This CL ports the ui/compositor's compositor lock to the android CompositorImpl. We take this lock when a renderer first connects to the browser, and release it when the renderer produces a frame. This prevents the browser from producing a frame until it has renderer content (or a failsafe timeout has been reached). Note that the VrCompositor used to provide similar functionality by calling DeferComimits direclty. This code wasn't working as intended and after discussion with the VR team has been removed. Bug: 792120 Change-Id: I011428bfb75e7455aae00426258ea05daf89fb38 Reviewed-on: https://chromium-review.googlesource.com/832959 Commit-Queue: Eric Karl <ericrk@chromium.org> Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Commit-Position: refs/heads/master@{#527398} [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/chrome/browser/android/vr_shell/vr_compositor.cc [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/chrome/browser/android/vr_shell/vr_compositor.h [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/content/browser/BUILD.gn [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/content/browser/android/overscroll_controller_android_unittest.cc [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/content/browser/renderer_host/compositor_impl_android.h [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/content/public/browser/android/compositor.h [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/ui/android/BUILD.gn [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/ui/android/DEPS [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/ui/android/delegated_frame_host_android.cc [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/ui/android/delegated_frame_host_android.h [modify] https://crrev.com/c1ced2b2f072d9f2281b04a88f793602dc23a3d4/ui/android/window_android_compositor.h
,
Jan 5 2018
Re #5: The fix that just landed should address this issue. We likely want to merge this to M64 if possible, as the flashing is pretty annoying. When merging back we may want to consider restricting to low-end, as that's the only place where the issue is seen with any frequency - this should reduce risk associated with the merge.
,
Jan 5 2018
This bugfix is quite critical for memory constrained devices. Can we please incorporate it into M64 as it is quite a jarring user experience.
,
Jan 5 2018
Per comment #9, we'd like to merge to M64. The fix requires two patches: the patch from #4, which is a bit large, but is a no-op refactor (and has been in Canary for a few weeks), and the patch from #7, which is much smaller but implements the actual logic change.
,
Jan 5 2018
This bug requires manual review: DEPS changes referenced in bugdroid comments. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 6 2018
Per #9, bumping to P1.
,
Jan 8 2018
How critical is this for M64? Is this a regression in M64?
,
Jan 8 2018
I think the change that regressed this landed in M-61, but this only affects low-end devices so it wasn't immediately caught. This is high priority fix for Android Go.
,
Jan 8 2018
Per further discussion with mariakhomenko@, I think that rather than try to fix this for M64, which requires the larger change seen above, we should just temporarily disable the optimization which introduced this. This optimization is nice, but not worth this regression. We can then re-enable the optimization (with the above fix) for M65. Disabling the optimization is a trivial change, and well tested, as the optimization is never enabled on non-low-end Android.
,
Jan 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c0a0cf3967bf4481fbc18e5012219cf5da2edddb commit c0a0cf3967bf4481fbc18e5012219cf5da2edddb Author: Eric Karl <ericrk@chromium.org> Date: Mon Jan 08 20:54:39 2018 Temporarily disable context-clear on background for low-end Android This optimization was added in M61, but was causing flicker on app resume. We'll re-enable it once a fix for that issue has landed. Bug: 792120 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ief43b80527877ae6d245d328d9ed3c3b3bd88ebb Reviewed-on: https://chromium-review.googlesource.com/854740 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/heads/master@{#527748} [modify] https://crrev.com/c0a0cf3967bf4481fbc18e5012219cf5da2edddb/gpu/ipc/service/gpu_channel_manager.cc
,
Jan 8 2018
The original fix (Comment #7) has led to some issues, so reverting that for now. This has the benefit of letting us test the small fix in #16 on its own. cmasso@, our plan is now that we'd like to merge the spot fix from #16 after it's been confirmed in Canary. This spot fix is very small and scoped, and removes a low-end Android memory optimization that was causing the bug in question. The behavior after this change is well tested as it's the same behavior taken by all non-low-end devices. Let me know how this sounds.
,
Jan 8 2018
+cmasso@ for the question in #17 (context in #13/14/15).
,
Jan 9 2018
Merge approved for the change in #16. Please make sure to verify it in tonight's canary before merging it.
,
Jan 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63 commit 2cb58045a4eb375eb1f36f7f2cc9f5c584233d63 Author: Eric Karl <ericrk@chromium.org> Date: Tue Jan 09 01:12:39 2018 Revert "Improve timing of placeholder image on Chrome resume" This reverts commit c1ced2b2f072d9f2281b04a88f793602dc23a3d4. Reason for revert: Caused various bugs incl crbug.com/799976. Will fix and re-land. Original change's description: > Improve timing of placeholder image on Chrome resume > > Currently, when Chrome is foregrounded, Android OS shows a placeholder > image of the last screen rendered. It does this until Chrome fires the > surfaceRedrawAsync callback. Chrome currently does this as soon as it > produces its first frame after foregrounding. > > Unfortunately, Chrome doesn't wait for renderer content before > producing a frame, so the first frame may include a blank renderer. > This is especially common on low-end devices which produce frames > more slowly. In these cases, we see the placeholder, then a blank > renderer, then a populated renderer, which produces a flickering > effect. > > This CL ports the ui/compositor's compositor lock to the android > CompositorImpl. We take this lock when a renderer first connects to the > browser, and release it when the renderer produces a frame. This > prevents the browser from producing a frame until it has renderer > content (or a failsafe timeout has been reached). > > Note that the VrCompositor used to provide similar functionality by > calling DeferComimits direclty. This code wasn't working as intended > and after discussion with the VR team has been removed. > > Bug: 792120 > Change-Id: I011428bfb75e7455aae00426258ea05daf89fb38 > Reviewed-on: https://chromium-review.googlesource.com/832959 > Commit-Queue: Eric Karl <ericrk@chromium.org> > Reviewed-by: Michael Thiessen <mthiesse@chromium.org> > Reviewed-by: ccameron <ccameron@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Cr-Commit-Position: refs/heads/master@{#527398} TBR=ccameron@chromium.org,mthiesse@chromium.org,bajones@chromium.org,piman@chromium.org,ericrk@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 792120 Change-Id: I124af4f34e3280abbf2344e2932d5b3fd39098bc Reviewed-on: https://chromium-review.googlesource.com/855063 Reviewed-by: Eric Karl <ericrk@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/heads/master@{#527844} [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/chrome/browser/android/vr_shell/vr_compositor.cc [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/chrome/browser/android/vr_shell/vr_compositor.h [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/content/browser/BUILD.gn [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/content/browser/android/overscroll_controller_android_unittest.cc [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/content/browser/renderer_host/compositor_impl_android.h [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/content/public/browser/android/compositor.h [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/ui/android/BUILD.gn [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/ui/android/DEPS [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/ui/android/delegated_frame_host_android.cc [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/ui/android/delegated_frame_host_android.h [modify] https://crrev.com/2cb58045a4eb375eb1f36f7f2cc9f5c584233d63/ui/android/window_android_compositor.h
,
Jan 9 2018
,
Jan 9 2018
,
Jan 9 2018
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bb8fffc54ddb84de9f7ca77ac38a89be1e74d9ad commit bb8fffc54ddb84de9f7ca77ac38a89be1e74d9ad Author: Eric Karl <ericrk@chromium.org> Date: Wed Jan 10 19:29:51 2018 Temporarily disable context-clear on background for low-end Android This optimization was added in M61, but was causing flicker on app resume. We'll re-enable it once a fix for that issue has landed. TBR=ericrk@chromium.org (cherry picked from commit c0a0cf3967bf4481fbc18e5012219cf5da2edddb) Bug: 792120 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ief43b80527877ae6d245d328d9ed3c3b3bd88ebb Reviewed-on: https://chromium-review.googlesource.com/854740 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#527748} Reviewed-on: https://chromium-review.googlesource.com/860862 Reviewed-by: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#480} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/bb8fffc54ddb84de9f7ca77ac38a89be1e74d9ad/gpu/ipc/service/gpu_channel_manager.cc
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/465a525298a32c85cadc184a793f5d7bf7c54fab commit 465a525298a32c85cadc184a793f5d7bf7c54fab Author: Eric Karl <ericrk@chromium.org> Date: Wed Jan 10 21:16:27 2018 Reland "Improve timing of placeholder image on Chrome resume"" Improve timing of placeholder image on Chrome resume Currently, when Chrome is foregrounded, Android OS shows a placeholder image of the last screen rendered. It does this until Chrome fires the surfaceRedrawAsync callback. Chrome currently does this as soon as it produces its first frame after foregrounding. Unfortunately, Chrome doesn't wait for renderer content before producing a frame, so the first frame may include a blank renderer. This is especially common on low-end devices which produce frames more slowly. In these cases, we see the placeholder, then a blank renderer, then a populated renderer, which produces a flickering effect. This CL ports the ui/compositor's compositor lock to the android CompositorImpl. If Chrome is producing the first frame since becoming visible, we take this lock when a renderer first connects to the browser, and release it when the renderer produces a frame. This prevents the browser from producing a frame until it has renderer content (or a failsafe timeout has been reached). Note that the VrCompositor used to provide similar functionality by calling DeferComimits direclty. This code wasn't working as intended and after discussion with the VR team has been removed. TBR=ccameron@chromium.org,mthiesse@chromium.org,bajones@chromium.org Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Bug: 792120 , 800743 Change-Id: I69b2c0d7da9c5e132f02a6ae951be8846889607c Reviewed-on: https://chromium-review.googlesource.com/857836 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Khushal <khushalsagar@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/heads/master@{#528425} [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/chrome/browser/android/vr_shell/vr_compositor.cc [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/chrome/browser/android/vr_shell/vr_compositor.h [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/content/browser/BUILD.gn [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/content/browser/android/overscroll_controller_android_unittest.cc [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/content/browser/renderer_host/compositor_impl_android.h [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/content/public/browser/android/compositor.h [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/gpu/ipc/service/gpu_channel_manager.cc [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/ui/android/BUILD.gn [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/ui/android/DEPS [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/ui/android/delegated_frame_host_android.cc [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/ui/android/delegated_frame_host_android.h [modify] https://crrev.com/465a525298a32c85cadc184a793f5d7bf7c54fab/ui/android/window_android_compositor.h
,
Jan 10 2018
The temporary fix has been merged to M64 and the long-term fix has landed. closing.
,
Mar 1 2018
Hmm, I can still see this issue on Canary 66.0.3356.0 with Maps PWA: https://photos.app.goo.gl/3n4w5OD45ZX0SXw02
,
Mar 1 2018
It's interesting that the issue doesn't repro on the stable Chrome (64.0.3282.137).
,
Mar 1 2018
Taking a look
,
Mar 1 2018
It appears that the long-term fix in #25 didn't fully address the problem. Is it possible to merge the same short-term fix from #24 to M65. This fix was already merged to M64 and is very safe.
,
Mar 1 2018
This bug requires manual review: We are only 4 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 1 2018
Merge approved upon verification of the fix in canary
,
Mar 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9e460609c08e26f7107e4897e5f1ed85f2a42f3 commit f9e460609c08e26f7107e4897e5f1ed85f2a42f3 Author: Eric Karl <ericrk@chromium.org> Date: Fri Mar 02 00:47:41 2018 [Reland] Temporarily disable context-clear on background for low-end Android This optimization was added in M61, but was causing flicker on app resume. We'll re-enable it once a fix for that issue has landed. TBR=piman Bug: 792120 Change-Id: I7d0ddc96b56a2e99f39ef160f175b79ec466531b Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/854740 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#527748} Reviewed-on: https://chromium-review.googlesource.com/944193 Reviewed-by: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/heads/master@{#540384} [modify] https://crrev.com/f9e460609c08e26f7107e4897e5f1ed85f2a42f3/gpu/ipc/service/gpu_channel_manager.cc
,
Mar 2 2018
mariakhomenko@ - wanted to get your opinion on this. I'll be merging the short-term fix to M65 tomorrow (want to verify in Canary, although I've verified locally and in M64, so not too concerned), but it's not certain that there will be a respin. We can likely follow up next tues/wed. once it's clearer whether a respin is needed for other reasons, but wanted to ask now to keep this on our radar and in case there were timing constraints I'm unaware of. To summarize the current state, a follow-up patch I landed in M65 made my fix become flaky and fail quite frequently. I unfortunately missed these flakes when I landed it. So overall, if the short-term fix misses 65, we will see the flash-on-resume very often.
,
Mar 2 2018
I am not aware of any specific timing constraints. However, I think it's important to pick up this fix into M-65 stable. Estelle, what do you think the chances are of doing a respin?
,
Mar 5 2018
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/236b296e25c8076788573dd5050d887b3269a553 commit 236b296e25c8076788573dd5050d887b3269a553 Author: Eric Karl <ericrk@chromium.org> Date: Mon Mar 05 18:25:16 2018 [Reland] Temporarily disable context-clear on background for low-end Android This optimization was added in M61, but was causing flicker on app resume. We'll re-enable it once a fix for that issue has landed. TBR=ericrk@chromium.org, piman (cherry picked from commit f9e460609c08e26f7107e4897e5f1ed85f2a42f3) Bug: 792120 Change-Id: I7d0ddc96b56a2e99f39ef160f175b79ec466531b Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/854740 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#527748} Reviewed-on: https://chromium-review.googlesource.com/944193 Reviewed-by: Eric Karl <ericrk@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#540384} Reviewed-on: https://chromium-review.googlesource.com/949447 Cr-Commit-Position: refs/branch-heads/3325@{#660} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/236b296e25c8076788573dd5050d887b3269a553/gpu/ipc/service/gpu_channel_manager.cc
,
Mar 5 2018
Verified and merged to M65. cmasso@, ping for Maria's question in #35. Thanks!
,
Mar 6 2018
The NextAction date has arrived: 2018-03-06
,
Mar 7 2018
,
Mar 7 2018
Since this is now in M65, we will take it if there is any respin. We are currently at 10% and planning to reach 20% today since there is no critical issue to warrant a respin.
,
Mar 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f22e7bc51b3fd43045d53fea22a8b2ba33460e29 commit f22e7bc51b3fd43045d53fea22a8b2ba33460e29 Author: Eric Karl <ericrk@chromium.org> Date: Thu Mar 08 00:52:23 2018 Add UnitTests for compositor lock behavior of DelegatedFrameHostAndroid DelegatedFrameHostAndroid was updated to take a compositor lock when it attaches to the compositor. This patch adds unit tests verifying that this behavior works as expected. Bug: 792120 Change-Id: I0eb783e15663df1f1f9efce01249946aec7678a1 Reviewed-on: https://chromium-review.googlesource.com/952662 Commit-Queue: Eric Karl <ericrk@chromium.org> Reviewed-by: Khushal <khushalsagar@chromium.org> Cr-Commit-Position: refs/heads/master@{#541658} [modify] https://crrev.com/f22e7bc51b3fd43045d53fea22a8b2ba33460e29/ui/android/BUILD.gn [add] https://crrev.com/f22e7bc51b3fd43045d53fea22a8b2ba33460e29/ui/android/delegated_frame_host_android_unittest.cc
,
Mar 12 2018
Realized that the CL in #33 (merged to 65) just missed 66, requesting merge there as well.
,
Mar 12 2018
This bug requires manual review: DEPS changes referenced in bugdroid comments. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 12 2018
,
Mar 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ea8877503213cc4a8500f31f8ad2b7f4f898d5b6 commit ea8877503213cc4a8500f31f8ad2b7f4f898d5b6 Author: Eric Karl <ericrk@chromium.org> Date: Tue Mar 13 00:27:17 2018 [Reland] Temporarily disable context-clear on background for low-end Android This optimization was added in M61, but was causing flicker on app resume. We'll re-enable it once a fix for that issue has landed. TBR=ericrk@chromium.org, piman (cherry picked from commit f9e460609c08e26f7107e4897e5f1ed85f2a42f3) Bug: 792120 Change-Id: I7d0ddc96b56a2e99f39ef160f175b79ec466531b Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/854740 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#527748} Reviewed-on: https://chromium-review.googlesource.com/944193 Reviewed-by: Eric Karl <ericrk@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#540384} Reviewed-on: https://chromium-review.googlesource.com/959437 Cr-Commit-Position: refs/branch-heads/3359@{#183} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/ea8877503213cc4a8500f31f8ad2b7f4f898d5b6/gpu/ipc/service/gpu_channel_manager.cc
,
Mar 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/73cfd3fcb696344b7ecc66519c3a702f0ef6e127 commit 73cfd3fcb696344b7ecc66519c3a702f0ef6e127 Author: Eric Karl <ericrk@chromium.org> Date: Mon Mar 26 22:49:45 2018 Trigger GPU context dropping on low-end from Browser proc We drop GL contexts on low-end devices when we go to the background. This is currently initiated by the GPU process, which kills all GPU channels, eventually causing context-loss in the various clients. While the current approach works, it makes it difficult to avoid rendering artifacts on app resume. As context-loss is lazily identified, it's possible for the browser and renderer to be notified at very different times. This can lead to races where the browser will attempt to draw with invalid renderer resources. To avoid these issues, this CL causes the context-dropping to be triggered from the browser itself. This allows the browser to drop renderer resources at the same time, greatly reducing (but unfortunately not eliminating) the possibility of a race. If we still see these issues with any frequency, further follow-up work, leveraging Surface Synchronization, will likely be able to fully eliminate these cases. Bug: 792120 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I1c0f2c6000fc9c1f4171952b09484101a220654c Reviewed-on: https://chromium-review.googlesource.com/959605 Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: enne <enne@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> Reviewed-by: Khushal <khushalsagar@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/heads/master@{#545879} [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/components/viz/client/frame_eviction_manager.cc [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/components/viz/client/frame_eviction_manager.h [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/components/viz/host/server_gpu_memory_buffer_manager_unittest.cc [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/components/viz/service/gl/gpu_service_impl.cc [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/components/viz/service/gl/gpu_service_impl.h [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/content/browser/gpu/gpu_ipc_browsertests.cc [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/content/browser/renderer_host/compositor_impl_android.h [add] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/content/browser/renderer_host/compositor_impl_android_browsertest.cc [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/content/test/BUILD.gn [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/content/test/DEPS [add] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/content/test/gpu_browsertest_helpers.cc [add] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/content/test/gpu_browsertest_helpers.h [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/gpu/ipc/service/gpu_channel_manager.cc [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/gpu/ipc/service/gpu_channel_manager.h [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/gpu/ipc/service/gpu_channel_manager_unittest.cc [modify] https://crrev.com/73cfd3fcb696344b7ecc66519c3a702f0ef6e127/services/viz/privileged/interfaces/gl/gpu_service.mojom
,
Mar 28 2018
Given that this issue is keeps popping up, I'd appreciate it if test could run some additional verification of the fix. candrada@, can you help find someone to help? If everything looks good, we should close this out. Thanks!
,
Mar 29 2018
askatte@, please do some testing on Android Go.
,
Mar 29 2018
We had tried to repro this flash issue earlier too (to verify the fix in #37). So far we haven't seen any flashing when bringing Chrome to foreground on Android Go 512 Mb or 1GB devices on older M65 builds of Chrome. We will watch out for such bugs during our testing on Android Go devices.
,
Apr 16 2018
|
|||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||||||||||
Comment 1 by dskiba@chromium.org
, Dec 5 2017