Increase in missing surface with surface references on Android |
|||||||
Issue descriptionThere is still an increase in the number of missing surfaces after enabling surface references on Android and fixing other bugs. Platforms other than Android didn't see much change. These are numbers comparing stable (using surface sequences) vs canary (using surface references) for 2017/09/10: Android Stable = 0.0002 missing surfaces per million aggregations Android Canary = 2.5706 missing surfaces per million aggregations A missing surface will mean the content can't be drawn and whatever is in the background will show through. The number of missing surfaces is still fairly small, 2.5 for every million aggregations, but the it's a big increase from basically 0 before. The surface sequence code is still there and we can switch it on with the --disable-surface-references flag. Alternatively, I can merge a commit to M62 branch that makes surface sequences the default on Android. I have not been able to figure out how to reproduce the issue. I think it might be related to how CompositorImpl destroys LayerTreeFrameSink when it's hidden. When the LayerTreeFrameSink is destroyed, the CompositorFrameSinkSupport and Surface for the browser is destroyed. When the browser Surface is destroyed, the only reference to the renderer Surface is destroyed with it. The renderer Surface is *not* destroyed, because we keep the newest surface for a CompositorFrameSinkSupport. The renderer SurfaceLayer, owned by DelegatedFrameHostAndroid, still points to the renderer Surface. When CompositorImpl is shown again, a new LayerTreeFrameSink is created along with CompositorFrameSinkSupport and Surface for the browser process. The SurfaceLayer is added to CompositorImpls LayerTreeHost and the browser Surface will embed the renderer Surface again. Once the browser submits a CompositorFrame, it adds a new surface reference to the renderer Surface and the reference graph is in a good state again. I thought there might be a race where the renderer could submit a CompositorFrame with a new size, deleting the old Surface and creating a new Surface, before the new browser CompositorFrameSinkSupport submits a CompositorFrame and creates a Surface. I think this would only be possible if there was gap between layer tree commit and submitting the CompositorFrame for the browser. Looking at SingleThreadProxy it appears that commit and submit happen in the same callstack though. If I'm going to disable surface references before the beta it needs to happen ASAP. Otherwise, we could leave surface references enabled on the beta and see if any bug reports come in that reproduce the problem. Any thoughts?
,
Sep 13 2017
I suggest disabling on beta (either on Android or all platforms, depending on what makes the most sense) while we look for / confirm the root cause.
,
Sep 13 2017
I will disable surface references on M62 Android. The cut point for the first beta release has already passed, so it will happen in the second beta release.
,
Sep 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ca224d466239318c18a99fcebe64b6937bbd51aa commit ca224d466239318c18a99fcebe64b6937bbd51aa Author: kylechar <kylechar@chromium.org> Date: Thu Sep 14 01:22:10 2017 Switch default back to surface sequences on Android. This CL switches Android to use surface sequences instead of surface references by default. This is due to an increase in the number of missing surfaces. The --enable-surface-references flag turns on surface references if necessary. This CL is temporary and will be reverted after it is verified on canary and merged to M62. Bug: 764460 , 757569 Change-Id: Ic0782dd09fbdc2b7064ee3b1d2d4287ab5c73005 Reviewed-on: https://chromium-review.googlesource.com/665697 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: kylechar <kylechar@chromium.org> Cr-Commit-Position: refs/heads/master@{#501828} [modify] https://crrev.com/ca224d466239318c18a99fcebe64b6937bbd51aa/content/browser/renderer_host/compositor_impl_android.cc
,
Sep 18 2017
I would like to merge this to M62. Turning on surface sequences regressed the number of missing surfaces on Android. See bug description for more details. The CL flips a flag that turns on surface sequences instead of surface references. The surface sequence code is being used in M61 and wasn't modified, so this change should be safe.
,
Sep 18 2017
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 18 2017
Merge approved for M62 branch 3202, please merge ASAP to make this week's beta cut. Un-CC'ing myself, re-add me if you have further questions.
,
Sep 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/822008d69631063a5580041123f5ed07273cf2bb commit 822008d69631063a5580041123f5ed07273cf2bb Author: kylechar <kylechar@chromium.org> Date: Mon Sep 18 20:57:17 2017 Switch default back to surface sequences on Android. This CL switches Android to use surface sequences instead of surface references by default. This is due to an increase in the number of missing surfaces. The --enable-surface-references flag turns on surface references if necessary. Bug: 764460 , 757569 Change-Id: Ic0782dd09fbdc2b7064ee3b1d2d4287ab5c73005 Reviewed-on: https://chromium-review.googlesource.com/665697 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: kylechar <kylechar@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#501828}(cherry picked from commit ca224d466239318c18a99fcebe64b6937bbd51aa) Reviewed-on: https://chromium-review.googlesource.com/671823 Reviewed-by: kylechar <kylechar@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#304} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/822008d69631063a5580041123f5ed07273cf2bb/content/browser/renderer_host/compositor_impl_android.cc
,
Oct 12 2017
This was fixed by turning off surface references on Android.
,
Nov 8 2017
Migrating from Internals>Viz to Internals>Services>Viz. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by kylec...@chromium.org
, Sep 12 2017