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

Issue 757569 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression
Proj-XR
Proj-XR-VR



Sign in to add a comment

blue screen for ~ 1 second or more when entering WebVR presentation

Project Member Reported by dbbrooks@chromium.org, Aug 21 2017

Issue description

Chrome Version: 61.0.3163.57
OS: Android N
Device: Pixel XL
VrCore: 1.8.163477258

What steps will reproduce the problem?
(1) go to https://webvr.info/samples/
(2) Select any webVR presentation
(3) Press enter VR button <---- Notice a blue screen for a brief moment here. (Didn't use to appear)
(4) Follow DON flow to start presentation. <--- Notice blue screen right after syncing controller before presentation starts.
(5) Exit presentation
(5) Immediately after exiting, re-enter presentation (which skips the DON flow) <--- Notice a longer blue screen here (~ 2 seconds) before presentation starts.


What is the expected result? There didn't used to be a blue screen.

What happens instead? Blue screen is part of the flow now when entering and exiting presentations. 

 
Description: Show this description
Cc: mthiesse@chromium.org
Components: -Internals>VR UI>Browser>VR
Labels: M-62 Pri-2 Type-Bug-Regression
Status: Available (was: Untriaged)

Comment 3 by bshe@chromium.org, Aug 22 2017

Labels: -Pri-2 Pri-1
I am seeing it too on a version which I synced this morning. This seem a p1 issue.
Owner: cjgrant@chromium.org
Status: Started (was: Available)
I'll take this and start by figuring out when it was introduced.
I don't see the issue after syncing later in the day.  Still working to repro this.
bisect result:

[68156e68656b00d9c963c5b30463a27b9627d14d] Enable surface references by default.

Excerpts from CL description:

    This CL switches from using surface sequences to surface references for
    surface lifetime management. This entails adding temporary references
    and using the surface reference graph starting at the top-level root
    surface and not adding surface sequences everywhere. 
    ...

     


Cc: -mthiesse@chromium.org
Owner: mthiesse@chromium.org
We know when this issue started.  Michael is currently the best equipped to investigate why, as time allows.
Cc: kylec...@chromium.org
cc kylechar, is this expected behavior from your change? Do blue flashes indicate we're doing something wrong?
It sounds like a viz::Surface is getting deleted and whatever is behind the SurfaceDrawQuad (maybe nothing) is showing instead of the viz::Surface contents. I changed how we track the lifetime of viz::Surfaces in that CL, basically I disabled using SurfaceSequences.

I don't know much about WebVR unfortunately. A couple questions:
1. Where do you generate / submit CompositorFrames for the WebVR content?
2. Were you explicitly adding SurfaceSequences anywhere?
3. Were you registering frame sink heirarchy anywhere?

If you can point me at the code related to this, I'll take a look!
I'm not seeing any blue flashes when entering VR, only when exiting VR the content window shows solid blue (and then only maybe 1/5 times). So something appears to be going wrong when switching between the VrCompositor and the Android CompositorView...
Cc: -kylec...@chromium.org mthiesse@chromium.org
Owner: kylec...@chromium.org
kylechar is looking into this. I couldn't figure out what was going wrong :P
I'm not seeing this on Canary, can anybody repro this on Canary/Dev? If not, then we can probable lower the priority and drop the M-62 target.
I've spent some time digging into this. There is a race, related to viz::Surface lifetime, when switching from WebVR mode back to the browser mode. The race is solvable, although it's maybe the symptom of a bigger problem.

I added a bunch of log statements to debug what was happening on 6P when visiting https://webvr.info/samples/03-vr-presentation.html, here is a quick summary:

1. Initially in browser mode with WebVR sample website loaded. The renderer is submitting CompositorFrames with dsf=3.5 and SurfaceId S1. The browser embeds S1. 
2. Tap the "Enter VR" button and enter WebVR mode.
3. The renderer stops submitting CompositorFrames here. WebVR mode draws without submitting CompositorFrames.
4. The renderer gets an IPC to resize with dsf=1.9326.
5. Tap the X button and switch back to browser mode.
6. CompositorImpl reinitializes itself.
7. The renderer gets a signal to start submitting CompositorFrames again.
8. The browser embeds the last renderer surface which is SurfaceId S1.
9. The renderer submits a CompositorFrame with dsf=1.9326 now. This CompositorFrames has SurfaceId S2. 
10. The browser embeds SurfaceId S2 with the wrong dsf.
11. The renderer gets an IPC to resize with dsf=3.5.
12. The renderer submits a CompositorFrame with dsf=3.5 now. These CompositorFrames have SurfaceId S3.
13. The browser embeds SurfaceId S3 with the right dsf.

There is a race around steps 7-9 where the renderer can submit a CompositorFrame before the browser embeds S1. If the timing is right, browser still embeds S1 but it has been deleted and the background color shows through.

The render probably shouldn't submit SurfaceId S2 at all. It's not the correct dsf, so it won't look right, and it delays showing the right contents on screen. Since the dsf never changes (as far as CompositorFrames are concerned), the renderer will continue submitting SurfaceId S1 when exiting from WebVR mode.
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Should this be flagged as needing a merge, Kyle?
verified does not repro in 63.0.3214.0
Also verified with 63.0.3215.0
Project Member

Comment 18 by bugdroid1@chromium.org, Sep 18 2017

Labels: merge-merged-3202
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

Status: Fixed (was: Started)
Sorry about the delay in getting this merged. Should be fixed in M62 and I'll make sure that there is no missing surfaces with WebVR when I turn surface references back on for Android.

Sign in to add a comment