Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754493002:
Expose VSync pausing through WindowAndroid and pause VSync during webVR presentation (Closed)
DescriptionDisables the standard compositing path while in WebVR Presentation by pausing VSync. This prevents rAFs and other unnecessary compositor work from happening.
BUG=698923
Review-Url: https://codereview.chromium.org/2754493002
Cr-Commit-Position: refs/heads/master@{#457552}
Committed: https://chromium.googlesource.com/chromium/src/+/b10e886bab7da85506656b4bb83e707cc2170209
Patch Set 1 #Patch Set 2 : Switch to pausing BeginFrameSource #Patch Set 3 : Fix behaviour #
Total comments: 3
Patch Set 4 : Move implementation to WindowAndroid + Add test #Patch Set 5 : Address missed comment #
Total comments: 20
Patch Set 6 : Address comments #Patch Set 7 : remove log #
Total comments: 2
Patch Set 8 : Address comments #Patch Set 9 : Missed uploading WindowAndroid changes. #Patch Set 10 : Fix 'unused' member #Messages
Total messages: 37 (10 generated)
mthiesse@chromium.org changed reviewers: + boliu@chromium.org
PTAL boliu, WebVR presentation bypasses the compositor and draws over the entire display, so we don't want it to be active at all. I've tested this on a few webVR sites and it seems to work as expected, but let me know if there's a better approach, or any hidden gotchas here.
+cc klaus
On 2017/03/14 18:28:07, mthiesse wrote: > PTAL boliu, > > WebVR presentation bypasses the compositor and draws over the entire display, so > we don't want it to be active at all. > > I've tested this on a few webVR sites and it seems to work as expected, but let > me know if there's a better approach, or any hidden gotchas here. Just stopping vsyncs can lead to deadlocks. The whole cc::BeginFrameSource chain already has this "BeginFramePaused" concept that will unblock any such deadlocks. It's probably not hooked up all the way to WindowAndroid though, but I guess it should.
That's perfect, thanks! PTAL
On 2017/03/14 21:23:40, mthiesse wrote: > That's perfect, thanks! > > PTAL FYI, when I combine this with my proposed fix for the input processing issue in https://codereview.chromium.org/2748293002 , it seems that it's still doing the normal compositing flow every frame with your patch included. Unless I'm doing something wrong, I think we may need something more radical.
On 2017/03/15 01:28:33, klausw (use chromium instead) wrote: > On 2017/03/14 21:23:40, mthiesse wrote: > > That's perfect, thanks! > > > > PTAL > > FYI, when I combine this with my proposed fix for the input processing issue in > https://codereview.chromium.org/2748293002 , it seems that it's still doing the > normal compositing flow every frame with your patch included. Unless I'm doing > something wrong, I think we may need something more radical. Thanks Klaus, it appears that just telling the beginFrameObserver that it's paused doesn't do much in the way of actually pausing it :P This was being masked by the issue you mentioned where the compositor already failed to run properly. Fixed. Bo, now that we have a setVSyncPaused method in WindowAndroid, would you prefer the logic for actually pausing the vsync requests to move there as well, or should it stay in VrWindowAndroid because only VR cares about it?
I think this should just be a feature of WindowAndroid, and not spread across some random subclass. Then probably should have a test for this, probably in content. Maybe check in js that requestAnimationFrame no longer fire within 1s or something like that.. For correctness should early out in OnVSync, in addition to stop requesting vsync updates, in case there are any trailing requests. https://codereview.chromium.org/2754493002/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/2754493002/diff/40001/ui/android/window_andro... ui/android/window_android.cc:63: obs->OnBeginFrameSourcePausedChanged(false); this is no longer correct
https://codereview.chromium.org/2754493002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java (right): https://codereview.chromium.org/2754493002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java:81: @CalledByNative fwiw, @CalledByNative is used for generating c++ jni headers, meaning it's totally useless here
Description was changed from ========== Disable standard compositing path while in WebVR Presentation. This stops the vsync loop while presenting in webVR, preventing rAF callbacks and unnecessary compositor work. BUG=698923 ========== to ========== Disables the standard compositing path while in WebVR Presentation by pausing the vsync loop. This prevents rAFs and other unnecessary compositor work from happening. BUG=698923 ==========
Moved implementation to WindowAndroid.java, fixed some corner cases, and added a test. https://codereview.chromium.org/2754493002/diff/40001/ui/android/window_andro... File ui/android/window_android.cc (right): https://codereview.chromium.org/2754493002/diff/40001/ui/android/window_andro... ui/android/window_android.cc:63: obs->OnBeginFrameSourcePausedChanged(false); On 2017/03/15 17:13:42, boliu wrote: > this is no longer correct Done.
https://codereview.chromium.org/2754493002/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java (right): https://codereview.chromium.org/2754493002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java:21: public class VSyncPausedTest extends ContentShellTestBase { I think a bunch of content tests have been converted to junit4, so use ContentShellActivityTestRule rather than inheriting from ContentShellTestBase. Should be able to just look at some other test using ContentShellActivityTestRule and copy from that. https://codereview.chromium.org/2754493002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java:44: private void fullyLoadUrl(final String url) throws Throwable { not used https://codereview.chromium.org/2754493002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java:72: } catch (TimeoutException e) { add a comment timeout is expected https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:112: private boolean mPendingVSyncRequest; hmm.. reading over this, I think it makes more sense to have all this logic in native in BFS? java can just purely pass through for the set paused call. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:506: protected void requestVSyncUpdate() { keep private https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:571: if (mVSyncPaused) nativeSetVSyncPaused(mNativeWindowAndroid, true); hmm, this doesn't look correct to me, why is vsync initially paused? what's unpausing it then? https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:740: * Pauses/Unpauses the VSync loop. A paused VSync loop will stop the compositor for this window, nit: it's not a loop (in my head at least).. so drop the "loop" part https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:744: mVSyncPaused = paused; generally, these things have an early out when mVSyncPaused and paused are equal
https://codereview.chromium.org/2754493002/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java (right): https://codereview.chromium.org/2754493002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java:21: public class VSyncPausedTest extends ContentShellTestBase { On 2017/03/15 22:08:11, boliu wrote: > I think a bunch of content tests have been converted to junit4, so use > ContentShellActivityTestRule rather than inheriting from ContentShellTestBase. > Should be able to just look at some other test using > ContentShellActivityTestRule and copy from that. Done. https://codereview.chromium.org/2754493002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java:44: private void fullyLoadUrl(final String url) throws Throwable { On 2017/03/15 22:08:11, boliu wrote: > not used Done. https://codereview.chromium.org/2754493002/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java:72: } catch (TimeoutException e) { On 2017/03/15 22:08:11, boliu wrote: > add a comment timeout is expected Done. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:112: private boolean mPendingVSyncRequest; On 2017/03/15 22:08:11, boliu wrote: > hmm.. reading over this, I think it makes more sense to have all this logic in > native in BFS? java can just purely pass through for the set paused call. So it couldn't only live in BFS, or WindowAndroid::OnVSync will keep requesting VSync updates and black-holing them (which I guess wouldn't break anything, but would be conceptually strange). We would have to make native WindowAndroid and WindowAndroid::WindowBeginFrameSource both aware of the paused state. Because of this I have a mild preference for leaving as is. Feel free to insist though. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:506: protected void requestVSyncUpdate() { On 2017/03/15 22:08:11, boliu wrote: > keep private Done. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:571: if (mVSyncPaused) nativeSetVSyncPaused(mNativeWindowAndroid, true); On 2017/03/15 22:08:11, boliu wrote: > hmm, this doesn't look correct to me, why is vsync initially paused? what's > unpausing it then? Well, whatever paused it is responsible for unpausing it, it's not initially paused. If something wants to pause the vsync loop before the native WindowAndroid is initialized I don't see why not. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:740: * Pauses/Unpauses the VSync loop. A paused VSync loop will stop the compositor for this window, On 2017/03/15 22:08:11, boliu wrote: > nit: it's not a loop (in my head at least).. so drop the "loop" part Done. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:744: mVSyncPaused = paused; On 2017/03/15 22:08:11, boliu wrote: > generally, these things have an early out when mVSyncPaused and paused are equal Done.
Replying on phone.. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:112: private boolean mPendingVSyncRequest; On 2017/03/16 00:03:07, mthiesse wrote: > On 2017/03/15 22:08:11, boliu wrote: > > hmm.. reading over this, I think it makes more sense to have all this logic in > > native in BFS? java can just purely pass through for the set paused call. > > So it couldn't only live in BFS, or WindowAndroid::OnVSync will keep requesting > VSync updates and black-holing them (which I guess wouldn't break anything, but > would be conceptually strange). We would have to make native WindowAndroid and > WindowAndroid::WindowBeginFrameSource both aware of the paused state. > > Because of this I have a mild preference for leaving as is. Feel free to insist > though. Actually, I see another issue with moving to native. Some of this code runs before native library is loaded, and native may not be ready when paused. OK, add a comment that's why the state is here. Otherwise, I'd ask this to be moved to native like you suggest. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:571: if (mVSyncPaused) nativeSetVSyncPaused(mNativeWindowAndroid, true); On 2017/03/16 00:03:07, mthiesse wrote: > On 2017/03/15 22:08:11, boliu wrote: > > hmm, this doesn't look correct to me, why is vsync initially paused? what's > > unpausing it then? > > Well, whatever paused it is responsible for unpausing it, it's not initially > paused. If something wants to pause the vsync loop before the native > WindowAndroid is initialized I don't see why not. Oh so the assumption is that native start out as unpaused? Why not not assume that and just pass paused here unconditionally?
ui/android lg2m % adding that comment test can't have sleeps though https://codereview.chromium.org/2754493002/diff/120001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java (right): https://codereview.chromium.org/2754493002/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java:76: Thread.sleep(100); err, no sleeps, which is a recipe for flakes maybe we can do this part in a loop?
https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:112: private boolean mPendingVSyncRequest; On 2017/03/16 00:45:44, boliu wrote: > On 2017/03/16 00:03:07, mthiesse wrote: > > On 2017/03/15 22:08:11, boliu wrote: > > > hmm.. reading over this, I think it makes more sense to have all this logic > in > > > native in BFS? java can just purely pass through for the set paused call. > > > > So it couldn't only live in BFS, or WindowAndroid::OnVSync will keep > requesting > > VSync updates and black-holing them (which I guess wouldn't break anything, > but > > would be conceptually strange). We would have to make native WindowAndroid and > > WindowAndroid::WindowBeginFrameSource both aware of the paused state. > > > > Because of this I have a mild preference for leaving as is. Feel free to > insist > > though. > > Actually, I see another issue with moving to native. Some of this code runs > before native library is loaded, and native may not be ready when paused. OK, > add a comment that's why the state is here. > > Otherwise, I'd ask this to be moved to native like you suggest. Done. https://codereview.chromium.org/2754493002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:571: if (mVSyncPaused) nativeSetVSyncPaused(mNativeWindowAndroid, true); On 2017/03/16 00:45:44, boliu wrote: > On 2017/03/16 00:03:07, mthiesse wrote: > > On 2017/03/15 22:08:11, boliu wrote: > > > hmm, this doesn't look correct to me, why is vsync initially paused? what's > > > unpausing it then? > > > > Well, whatever paused it is responsible for unpausing it, it's not initially > > paused. If something wants to pause the vsync loop before the native > > WindowAndroid is initialized I don't see why not. > > Oh so the assumption is that native start out as unpaused? Why not not assume > that and just pass paused here unconditionally? Done. https://codereview.chromium.org/2754493002/diff/120001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java (right): https://codereview.chromium.org/2754493002/diff/120001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/VSyncPausedTest.java:76: Thread.sleep(100); On 2017/03/16 02:28:52, boliu wrote: > err, no sleeps, which is a recipe for flakes > > maybe we can do this part in a loop? Found a better solution.
boliu, you also own content/public/android. Is there anybody else you would like to have review this? I think I only need your review.
mthiesse@chromium.org changed reviewers: + bshe@chromium.org
+bshe for a second set of eyes on the vr_shell/ code.
Description was changed from ========== Disables the standard compositing path while in WebVR Presentation by pausing the vsync loop. This prevents rAFs and other unnecessary compositor work from happening. BUG=698923 ========== to ========== Disables the standard compositing path while in WebVR Presentation by pausing VSync. This prevents rAFs and other unnecessary compositor work from happening. BUG=698923 ==========
On 2017/03/16 15:38:10, mthiesse wrote: > boliu, you also own content/public/android. Is there anybody else you would like > to have review this? I think I only need your review. Don't need anyone else for content or ui. lgtm
On 2017/03/16 16:45:30, boliu wrote: > On 2017/03/16 15:38:10, mthiesse wrote: > > boliu, you also own content/public/android. Is there anybody else you would > like > > to have review this? I think I only need your review. > > Don't need anyone else for content or ui. lgtm vr_shell lgtm
The CQ bit was checked by mthiesse@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by mthiesse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2754493002/#ps180001 (title: "Fix 'unused' member")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489694233195680,
"parent_rev": "2aa9dd13b0ba68a0f493c86f80b1bb0f3ceffc41", "commit_rev":
"b10e886bab7da85506656b4bb83e707cc2170209"}
Message was sent while issue was closed.
Description was changed from ========== Disables the standard compositing path while in WebVR Presentation by pausing VSync. This prevents rAFs and other unnecessary compositor work from happening. BUG=698923 ========== to ========== Disables the standard compositing path while in WebVR Presentation by pausing VSync. This prevents rAFs and other unnecessary compositor work from happening. BUG=698923 Review-Url: https://codereview.chromium.org/2754493002 Cr-Commit-Position: refs/heads/master@{#457552} Committed: https://chromium.googlesource.com/chromium/src/+/b10e886bab7da85506656b4bb83e... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/b10e886bab7da85506656b4bb83e...
Message was sent while issue was closed.
Hi, as I know, in previous Android, VSYNC is not only for compositing, but also sent to renderer to trigger beginMainFrame. So I stopped VSync, I am curious how the WebGL content will be generated(TextureLayer::update will not be called)?
Message was sent while issue was closed.
On 2017/03/29 01:17:07, xing.xu wrote: > Hi, as I know, in previous Android, VSYNC is not only for compositing, but also > sent to renderer to trigger beginMainFrame. > So I stopped VSync, I am curious how the WebGL content will be > generated(TextureLayer::update will not be called)? During webVR presentation we use a separate rendering path that bypasses the compositor. Rendering should happen during vrDisplay#rAF callback, and SubmitFrame() triggers the texture update.
Message was sent while issue was closed.
On 2017/03/29 14:47:40, mthiesse wrote: > On 2017/03/29 01:17:07, xing.xu wrote: > > Hi, as I know, in previous Android, VSYNC is not only for compositing, but > also > > sent to renderer to trigger beginMainFrame. > > So I stopped VSync, I am curious how the WebGL content will be > > generated(TextureLayer::update will not be called)? > > During webVR presentation we use a separate rendering path that bypasses the > compositor. Rendering should happen during vrDisplay#rAF callback, and > SubmitFrame() triggers the texture update. Thanks. Take this https://webvr.info/samples/04-simple-mirroring.html as an example: Before attach phone into Daydream, The VSYNC is working, which will being sent from Browser to Renderer, and initiate beginMainFrame and rendering. In BeginMainFrame, it will call js initWebGL/onAnimationFrame. If VSYNC paused, beginMainFrame will not being called too, so does onAnimationFrame. You know, for any animation related application, it requires a Loop. I didn't see where the Loop is when VSYNC is paused.
Message was sent while issue was closed.
On 2017/03/31 11:06:48, xing.xu wrote: > On 2017/03/29 14:47:40, mthiesse wrote: > > On 2017/03/29 01:17:07, xing.xu wrote: > > > Hi, as I know, in previous Android, VSYNC is not only for compositing, but > > also > > > sent to renderer to trigger beginMainFrame. > > > So I stopped VSync, I am curious how the WebGL content will be > > > generated(TextureLayer::update will not be called)? > > > > During webVR presentation we use a separate rendering path that bypasses the > > compositor. Rendering should happen during vrDisplay#rAF callback, and > > SubmitFrame() triggers the texture update. > > Thanks. > Take this https://webvr.info/samples/04-simple-mirroring.html as an example: > Before attach phone into Daydream, The VSYNC is working, which will being sent > from Browser to Renderer, and initiate beginMainFrame and rendering. In > BeginMainFrame, it will call js initWebGL/onAnimationFrame. > > If VSYNC paused, beginMainFrame will not being called too, so does > onAnimationFrame. > You know, for any animation related application, it requires a Loop. I didn't > see where the Loop is when VSYNC is paused. See crbug.com/704341 for further discussion. As mentioned in my previous post, the rendering loop while presenting needs to use vrDisplay#requestAnimationFrame, not window.requestAnimationFrame. On Android the page is hidden while presenting, so we don't fire rAF callbacks for the main frame.
Message was sent while issue was closed.
On 2017/03/31 14:05:49, mthiesse wrote: > On 2017/03/31 11:06:48, xing.xu wrote: > > On 2017/03/29 14:47:40, mthiesse wrote: > > > On 2017/03/29 01:17:07, xing.xu wrote: > > > > Hi, as I know, in previous Android, VSYNC is not only for compositing, but > > > also > > > > sent to renderer to trigger beginMainFrame. > > > > So I stopped VSync, I am curious how the WebGL content will be > > > > generated(TextureLayer::update will not be called)? > > > > > > During webVR presentation we use a separate rendering path that bypasses the > > > compositor. Rendering should happen during vrDisplay#rAF callback, and > > > SubmitFrame() triggers the texture update. > > > > Thanks. > > Take this https://webvr.info/samples/04-simple-mirroring.html as an example: > > Before attach phone into Daydream, The VSYNC is working, which will being sent > > from Browser to Renderer, and initiate beginMainFrame and rendering. In > > BeginMainFrame, it will call js initWebGL/onAnimationFrame. > > > > If VSYNC paused, beginMainFrame will not being called too, so does > > onAnimationFrame. > > You know, for any animation related application, it requires a Loop. I didn't > > see where the Loop is when VSYNC is paused. > > See crbug.com/704341 for further discussion. > > As mentioned in my previous post, the rendering loop while presenting needs to > use vrDisplay#requestAnimationFrame, not window.requestAnimationFrame. On > Android the page is hidden while presenting, so we don't fire rAF callbacks for > the main frame. Sorry for my ignorance, this is very clear! Thanks! |
