Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2384593002:
Encode frame number in pixel data for pose sync (Closed)
DescriptionEncode frame number in pixel data for pose sync
This encodes the pose number of the drawn WebVR frame in a corner pixel
block, this is read by vr_shell.cc to use the corresponding pose when
submitting the frame to GVR for presentation.
Log data shows that the pose is 2-3 frames old by the time it is ready for
submission to GVR. This includes compositor buffering and SurfaceTexture
buffering. The patch helps in getting reprojection working as intended, but
can't do anything about the excessive latency.
BUG=389343
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/dfa8ed4b3b10200b70d9f79cf247b8a1ec23e20e
Cr-Commit-Position: refs/heads/master@{#423367}
Patch Set 1 #Patch Set 2 : Frame pose sync: fix oscillating off-by-one #
Total comments: 19
Patch Set 3 : bajones #4: use contextGL(), auto-restore state, add poseNum to VRPosePtr #
Total comments: 9
Patch Set 4 : bajones #10: restoreStateFromContext, skip pixel read for non-async mode #
Total comments: 32
Patch Set 5 : mthiesse #17: pose_index, ctx->restore*() methods #Patch Set 6 : dcheng #19/20, mthiesse #17: use vector + init pose ring buffer, cleanups #
Total comments: 12
Patch Set 7 : dcheng #22, mthiesse #31: rebase, simpler ring buffer initialization, fix type #
Total comments: 4
Patch Set 8 : bajones #40: add sanity check + pointer zeroing #Messages
Total messages: 59 (19 generated)
Description was changed from ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-4 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. Non-ToT patches assumed to be present: - Required: "Updated to GVR 1.0 SDK" as per commit b181af38ca1ca95ac250256aa067ec1630aff905 from Sep 23 which was rolled back later. - Optional: "treat enable-webvr flag as always on" - Optional: "VrShell: implement insecure content warning display" BUG= ========== to ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. Non-ToT patches assumed to be present: - Required: "Updated to GVR 1.0 SDK" as per commit b181af38ca1ca95ac250256aa067ec1630aff905 from Sep 23 which was rolled back later. - Optional: "treat enable-webvr flag as always on" - Optional: "VrShell: implement insecure content warning display" BUG=389343 ==========
klausw@chromium.org changed reviewers: + bajones@chromium.org, bshe@chromium.org
On 2016/09/30 23:06:10, klausw wrote: > mailto:klausw@chromium.org changed reviewers: > + mailto:bajones@chromium.org, mailto:bshe@chromium.org Mailing for initial feedback about the high-level approach. I'll continue working on implementation details, just want to make sure the approach is considered feasible. Let me know if you want a demo.
Looking pretty good, though the glReadPixels makes the graphics dev in me cry. A few comments so far. https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:279: webvr_head_pose_[(frameNumStarted % mod + mod) % mod] = pose; Is there a reason this isn't simply frameNumStarted % POSE_QUEUE_SIZE? https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:289: //glReadBuffer(GL_BACK); // crashes, null function pointer?! This function is only available in OpenGL ES 3.0 and up, which I doubt we're binding the function pointers for. In OpenGL ES 2.0 glReadPixels will always occur with the bound framebuffer, which is what we want in this case. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:126: ++m_poseNum; This feels very fragile. Let's add a poseId to the Mojo VRPose structure and pass it down that way instead. Oculus has alreday asked for that. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:356: auto ctx = toWebGLRenderingContextBase(m_layer.source()->renderingContext()); Probably just want to cache this on calls to requestPresent. I know we'll need it for other operations in the future. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:358: ctx->scissor(0, 0, 4, 4); why 4x4? we're only reading one pixel https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: ctx->disable(GL_SCISSOR_TEST); This is definitely not going to fly, as it will break various WebGL apps that use values other than this. For the script state bit, it's probably best to dig one level deeper and get the WebGraphicsContext3D that the WebGLRenderingContextBase forwards calls to.
> Looking pretty good, though the glReadPixels makes the graphics dev in me cry. Yes, I'm not liking this myself. According to one log I checked this took about 7ms wallclock time, that's half the frame budget. This shouldn't be necessary if we had a predictable producer/consumer pipeline for the SurfaceTexture. In the alternate patch I'm working on that bypasses the compositor, I only needed to use this for a few frames to ensure that the pipeline had reached steady state, and from that point on it remained in sync with a consistent zero delta. Unfortunately I don't think that's an option for the compositor path since we don't know when it ends up calling swapBuffers, and according to logs the delta kept toggling between 2 and 3 frames delay. One more thing is that I'll investigate trying to drop stale frames in VrShell.java. Easiest approach is to call updateTexImage multiple times to ensure that the FIFO queue gets advanced to the newest frame. A SurfaceTexture's getTimestamp() returns zero if there was no newer frame, I'm not sure if that's guaranteed behavior. But even if not, it should be possible to check for repeated timestamps. That may be able to cut out a frame of latency in the compositor path, but no guarantees at this point. https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:279: webvr_head_pose_[(frameNumStarted % mod + mod) % mod] = pose; On 2016/10/02 20:55:23, bajones wrote: > Is there a reason this isn't simply frameNumStarted % POSE_QUEUE_SIZE? Yes, I've been using a signed int32_t where the mod result can be negative after wraparound, this would be unusable as-is as an array index. The double mod is a common idiom to get around that without assumptions about the C implementation's choice of sign. Originally I had picked signed int for Java compatibility, but if I remember right this is currently C++ only and I could switch it to an unsigned int. https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:289: //glReadBuffer(GL_BACK); // crashes, null function pointer?! On 2016/10/02 20:55:23, bajones wrote: > This function is only available in OpenGL ES 3.0 and up, which I doubt we're > binding the function pointers for. In OpenGL ES 2.0 glReadPixels will always > occur with the bound framebuffer, which is what we want in this case. Good to know, and it does seem to work as expected. I'll reword the comment. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:126: ++m_poseNum; On 2016/10/02 20:55:23, bajones wrote: > This feels very fragile. Let's add a poseId to the Mojo VRPose structure and > pass it down that way instead. Oculus has alreday asked for that. I agree, that this would be much cleaner. I'll look into adding that. Want to add a timestamp while we're at it, or save that for later? I think it would be helpful for performance statistics. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:356: auto ctx = toWebGLRenderingContextBase(m_layer.source()->renderingContext()); On 2016/10/02 20:55:23, bajones wrote: > Probably just want to cache this on calls to requestPresent. I know we'll need > it for other operations in the future. Sounds good. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:358: ctx->scissor(0, 0, 4, 4); On 2016/10/02 20:55:23, bajones wrote: > why 4x4? we're only reading one pixel I need to investigate more. I had tried 2x2 at some point and ended up with wrong values on reading the pixel back, it's possible there's some blurring going on at texture edges. In practical terms it doesn't matter since this area is blacked out by the vignette effect at the eye view border. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: ctx->disable(GL_SCISSOR_TEST); On 2016/10/02 20:55:23, bajones wrote: > This is definitely not going to fly, as it will break various WebGL apps that > use values other than this. For the script state bit, it's probably best to dig > one level deeper and get the WebGraphicsContext3D that the > WebGLRenderingContextBase forwards calls to. I know it's not acceptable, hence the TODO. I just wasn't sure how best to proceed since the ScriptState/ScriptValue abstraction seemed unhelpful for this use case, and the underlying DrawingBuffer was a protected interface which would need a change to the WebAPI internal interface. I'll look into the WebRenderingContext3D to see if that helps. Would it be very controversial to make a public accessor to get the drawing buffer if that turns out to be necessary?
On 2016/10/02 21:25:50, klausw wrote: > > Looking pretty good, though the glReadPixels makes the graphics dev in me cry. > > Yes, I'm not liking this myself. According to one log I checked this took about > 7ms wallclock time, that's half the frame budget. To clarify, this doesn't mean it's 7ms of *added* latency. A big part of this is just going to be forced GPU synchronization where it would have had to wait for that to finish later in the pipeline anyway.
https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: ctx->disable(GL_SCISSOR_TEST); On 2016/10/02 21:25:50, klausw wrote: > On 2016/10/02 20:55:23, bajones wrote: > > This is definitely not going to fly, as it will break various WebGL apps that > > use values other than this. For the script state bit, it's probably best to > dig > > one level deeper and get the WebGraphicsContext3D that the > > WebGLRenderingContextBase forwards calls to. > > I know it's not acceptable, hence the TODO. I just wasn't sure how best to > proceed since the ScriptState/ScriptValue abstraction seemed unhelpful for this > use case, and the underlying DrawingBuffer was a protected interface which would > need a change to the WebAPI internal interface. I'll look into the > WebRenderingContext3D to see if that helps. Would it be very controversial to > make a public accessor to get the drawing buffer if that turns out to be > necessary? Good news, WebGLRenderingContextBase has a public contextGL() accessor which bypasses the protected drawingBuffer() accessor. Assuming that works, I'll switch the code to use that. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webgl/... gpu::gles2::GLES2Interface* contextGL() const { DrawingBuffer* d = drawingBuffer(); if (!d) return nullptr; return d->contextGL(); }
Description was changed from ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. Non-ToT patches assumed to be present: - Required: "Updated to GVR 1.0 SDK" as per commit b181af38ca1ca95ac250256aa067ec1630aff905 from Sep 23 which was rolled back later. - Optional: "treat enable-webvr flag as always on" - Optional: "VrShell: implement insecure content warning display" BUG=389343 ========== to ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. Non-ToT patches assumed to be present: - Required: "Updated to GVR 1.0 SDK" as per commit b181af38ca1ca95ac250256aa067ec1630aff905 from Sep 23 which was rolled back later. - Optional: "treat enable-webvr flag as always on" - Optional: "VrShell: implement insecure content warning display" BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
PTAL, I think I've addressed your comments. It'll now need security review for the mojom change (not requested yet, want to see first if I did it right). Also, does the change to WebGLRenderingContextBase look ok? https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:279: webvr_head_pose_[(frameNumStarted % mod + mod) % mod] = pose; On 2016/10/02 21:25:50, klausw wrote: > On 2016/10/02 20:55:23, bajones wrote: > > Is there a reason this isn't simply frameNumStarted % POSE_QUEUE_SIZE? > > Yes, I've been using a signed int32_t where the mod result can be negative after > wraparound, this would be unusable as-is as an array index. The double mod is a > common idiom to get around that without assumptions about the C implementation's > choice of sign. > > Originally I had picked signed int for Java compatibility, but if I remember > right this is currently C++ only and I could switch it to an unsigned int. Changed to use uint32_t which simplifies this. https://codereview.chromium.org/2384593002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:289: //glReadBuffer(GL_BACK); // crashes, null function pointer?! On 2016/10/02 21:25:50, klausw wrote: > On 2016/10/02 20:55:23, bajones wrote: > > This function is only available in OpenGL ES 3.0 and up, which I doubt we're > > binding the function pointers for. In OpenGL ES 2.0 glReadPixels will always > > occur with the bound framebuffer, which is what we want in this case. > > Good to know, and it does seem to work as expected. I'll reword the comment. Comment updated. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:126: ++m_poseNum; On 2016/10/02 21:25:50, klausw wrote: > On 2016/10/02 20:55:23, bajones wrote: > > This feels very fragile. Let's add a poseId to the Mojo VRPose structure and > > pass it down that way instead. Oculus has alreday asked for that. > > I agree, that this would be much cleaner. I'll look into adding that. Want to > add a timestamp while we're at it, or save that for later? I think it would be > helpful for performance statistics. Done. I saw it has a timestamp already internally, that's just not exposed through the JS-visible VRPose object. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:356: auto ctx = toWebGLRenderingContextBase(m_layer.source()->renderingContext()); On 2016/10/02 20:55:23, bajones wrote: > Probably just want to cache this on calls to requestPresent. I know we'll need > it for other operations in the future. Done, saving the underlying GL context since that's what I'm using. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:358: ctx->scissor(0, 0, 4, 4); On 2016/10/02 21:25:50, klausw wrote: > On 2016/10/02 20:55:23, bajones wrote: > > why 4x4? we're only reading one pixel > > I need to investigate more. I had tried 2x2 at some point and ended up with > wrong values on reading the pixel back, it's possible there's some blurring > going on at texture edges. > > In practical terms it doesn't matter since this area is blacked out by the > vignette effect at the eye view border. I tested, smallest that worked was (0, 0, 1, 3). I think this is due to the WebGL texture size not matching the output texture size and that this can result in interpolated colors in texture sampling. Added a comment to explain what's going on, though I'm not totally sure if I'm understanding it right. https://codereview.chromium.org/2384593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: ctx->disable(GL_SCISSOR_TEST); On 2016/10/03 16:50:17, klausw wrote: > On 2016/10/02 21:25:50, klausw wrote: > > On 2016/10/02 20:55:23, bajones wrote: > > > This is definitely not going to fly, as it will break various WebGL apps > that > > > use values other than this. For the script state bit, it's probably best to > > dig > > > one level deeper and get the WebGraphicsContext3D that the > > > WebGLRenderingContextBase forwards calls to. > > > > I know it's not acceptable, hence the TODO. I just wasn't sure how best to > > proceed since the ScriptState/ScriptValue abstraction seemed unhelpful for > this > > use case, and the underlying DrawingBuffer was a protected interface which > would > > need a change to the WebAPI internal interface. I'll look into the > > WebRenderingContext3D to see if that helps. Would it be very controversial to > > make a public accessor to get the drawing buffer if that turns out to be > > necessary? > > Good news, WebGLRenderingContextBase has a public contextGL() accessor which > bypasses the protected drawingBuffer() accessor. Assuming that works, I'll > switch the code to use that. > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webgl/... > > gpu::gles2::GLES2Interface* contextGL() const { > DrawingBuffer* d = drawingBuffer(); > if (!d) > return nullptr; > return d->contextGL(); > } This seems to work now, though there's some slightly subtle magic going on where I'm relying on the rendering context to auto-restore values via restoreStateAfterClear(). I added comments to explain what's going on.
Definitely better, though it seems like there's some unwanted changes mixed in here. A few more comments. https://codereview.chromium.org/2384593002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2384593002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:10: #include "third_party/gvr-android-sdk/src/ndk/include/vr/gvr/capi/include/gvr.h" I think you've gotten some changes in here that you didn't intend. Make sure your branch has the right upstream. https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: // since we'd be bypassing its clearIfComposited() method. I'd prefer that we explicitly call restoreStateAfterClear() here. This currently only works because of limitations of he fullscreen hack we're using that don't allow any rendering to occur after submitFrame() without it showing up in the HMD, but the spec requires that to work. (It's used for advanced mirroring on desktops, not as useful on mobile.) Even if it happens to work without it for now I'd prefer to double up on the reverts now in favor of accidentally missing them in a future CL. https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRPose.h (right): https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRPose.h:29: // TODO(klausw): expose timestamp and poseNum to JS? We have those in the timestamp is exposed in the VRFrameData object. I don't think poseNum is a good thing to surface to JS, because it's use is really just for tracking render state internally and we don't want developers relying on something that opaque.
Replies below, not changed in code yet. I'll upload an updated version tomorrow. https://codereview.chromium.org/2384593002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2384593002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:10: #include "third_party/gvr-android-sdk/src/ndk/include/vr/gvr/capi/include/gvr.h" On 2016/10/04 06:33:57, bajones wrote: > I think you've gotten some changes in here that you didn't intend. Make sure > your branch has the right upstream. This is intentional though messy. Top-of-tree is still on GVR 0.9.1 which isn't very useful for testing, so I've based it on your earlier GVR 1.0.0 branch. However, some files that were edited since then needed modification to work with the new GVR API. I can hide this part of the diff by rebasing it below the "git cl upload" base, but we'll need these changes at some point. https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: // since we'd be bypassing its clearIfComposited() method. On 2016/10/04 06:33:57, bajones wrote: > I'd prefer that we explicitly call restoreStateAfterClear() here. This currently > only works because of limitations of he fullscreen hack we're using that don't > allow any rendering to occur after submitFrame() without it showing up in the > HMD, but the spec requires that to work. (It's used for advanced mirroring on > desktops, not as useful on mobile.) Even if it happens to work without it for > now I'd prefer to double up on the reverts now in favor of accidentally missing > them in a future CL. I can do that. restoreStateAfterClear() is a protected method though that will need to be made available as public, is that ok? Seems a bit inelegant, would be nice if there were a more generic method to track and restore GL state client-side. https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRPose.h (right): https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRPose.h:29: // TODO(klausw): expose timestamp and poseNum to JS? We have those in the On 2016/10/04 06:33:57, bajones wrote: > timestamp is exposed in the VRFrameData object. I don't think poseNum is a good > thing to surface to JS, because it's use is really just for tracking render > state internally and we don't want developers relying on something that opaque. OK, I'll remove the comment. When you mentioned Oculus being interested, I wasn't sure if you meant via JS or just internally in the WebVR implementation itself. Side note, I guess it would be possible to use timestamps as keys for a pose map to avoid the need for separate pose numbers, but using doubles as keys seems weird even if it should technically work.
PTAL https://codereview.chromium.org/2384593002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2384593002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:10: #include "third_party/gvr-android-sdk/src/ndk/include/vr/gvr/capi/include/gvr.h" On 2016/10/04 06:47:04, klausw wrote: > On 2016/10/04 06:33:57, bajones wrote: > > I think you've gotten some changes in here that you didn't intend. Make sure > > your branch has the right upstream. > > This is intentional though messy. Top-of-tree is still on GVR 0.9.1 which isn't > very useful for testing, so I've based it on your earlier GVR 1.0.0 branch. > However, some files that were edited since then needed modification to work with > the new GVR API. > > I can hide this part of the diff by rebasing it below the "git cl upload" base, > but we'll need these changes at some point. Rebased, this should no longer be visible. https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: // since we'd be bypassing its clearIfComposited() method. On 2016/10/04 06:47:04, klausw wrote: > On 2016/10/04 06:33:57, bajones wrote: > > I'd prefer that we explicitly call restoreStateAfterClear() here. This > currently > > only works because of limitations of he fullscreen hack we're using that don't > > allow any rendering to occur after submitFrame() without it showing up in the > > HMD, but the spec requires that to work. (It's used for advanced mirroring on > > desktops, not as useful on mobile.) Even if it happens to work without it for > > now I'd prefer to double up on the reverts now in favor of accidentally > missing > > them in a future CL. > > I can do that. restoreStateAfterClear() is a protected method though that will > need to be made available as public, is that ok? Seems a bit inelegant, would be > nice if there were a more generic method to track and restore GL state > client-side. I've added a new public method to the rendering context to restore specific state items based on a bitmask, and changed the private restoreStateAfterClear() to use that. What do you think? https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRPose.h (right): https://codereview.chromium.org/2384593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRPose.h:29: // TODO(klausw): expose timestamp and poseNum to JS? We have those in the On 2016/10/04 06:47:04, klausw wrote: > On 2016/10/04 06:33:57, bajones wrote: > > timestamp is exposed in the VRFrameData object. I don't think poseNum is a > good > > thing to surface to JS, because it's use is really just for tracking render > > state internally and we don't want developers relying on something that > opaque. > > OK, I'll remove the comment. When you mentioned Oculus being interested, I > wasn't sure if you meant via JS or just internally in the WebVR implementation > itself. > > Side note, I guess it would be possible to use timestamps as keys for a pose map > to avoid the need for separate pose numbers, but using doubles as keys seems > weird even if it should technically work. Removed the comment.
Description was changed from ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. Non-ToT patches assumed to be present: - Required: "Updated to GVR 1.0 SDK" as per commit b181af38ca1ca95ac250256aa067ec1630aff905 from Sep 23 which was rolled back later. - Optional: "treat enable-webvr flag as always on" - Optional: "VrShell: implement insecure content warning display" BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. Non-ToT patches assumed to be present: - Required: "Updated to GVR 1.0 SDK" as per commit b181af38ca1ca95ac250256aa067ec1630aff905 from Sep 23 which was rolled back later. - Optional: "treat enable-webvr flag as always on" BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
klausw@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for the mojom change, the pose number is needed to match frame image data to the corresponding pose used for rendering. This is required to make async reprojection work properly. Let me know if you have questions.
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:294: return pixels[0] | (pixels[1] << 8) | (pixels[2] << 16); Optionally, you really only need 3 bits, so you could simplify the math a bunch. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:324: // LOG << "klausw: newest_pose=" << webvr_newest_pose_num_ << Remove this https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:76: void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t pose_num) override; This function name feels wrong... How about SetPoseForWebVrFrame(pose, frame_index). https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:164: gvr::Mat4f webvr_head_pose_[POSE_QUEUE_SIZE]; Mild preference for std::array, or std::vector over c-style arrays. Smaller stack sizes are also generally preferable. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:165: int32_t webvr_newest_pose_num_ = 0; Remove unused variable. https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... File device/vr/android/gvr/gvr_device.h (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... device/vr/android/gvr/gvr_device.h:45: uint32_t pose_num_ = 0; I think frame_index_ would be more clear. https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... File device/vr/android/gvr/gvr_device_provider.cc (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... device/vr/android/gvr/gvr_device_provider.cc:68: void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t frameNumStarted) override{}; nit: space after override https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... device/vr/android/gvr/gvr_device_provider.cc:68: void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t frameNumStarted) override{}; Same comment about function signature applies here. https://codereview.chromium.org/2384593002/diff/60001/device/vr/vr_service.mojom File device/vr/vr_service.mojom (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/vr_service.mo... device/vr/vr_service.mojom:16: // given timestamp. The poseNum is a sequential ID that's incremented on Again, I think frameIndex is less confusing. The frame index this pose applies to. https://codereview.chromium.org/2384593002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2384593002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1377: void WebGLRenderingContextBase::restoreStateFromContext(int stateMask) { Rather than a single restoreStateFromContext function which is hard (for me at least) to validate in the general case, I would prefer individual functions for each piece of state you're restoring (ex. restoreScissorBox(), restoreScissorEnabled(), etc). Then you can explicitly restore each piece of state you modify in your function that modifies them.
https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:294: return pixels[0] | (pixels[1] << 8) | (pixels[2] << 16); On 2016/10/04 21:19:16, mthiesse wrote: > Optionally, you really only need 3 bits, so you could simplify the math a bunch. Not sure that would make a big difference - for now I think it's clearer like this, and we're also thinking about using the frame latency for performance diagnostics where it would be a good sanity check to ensure we're not getting way-bigger-than-expected lag. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:324: // LOG << "klausw: newest_pose=" << webvr_newest_pose_num_ << On 2016/10/04 21:19:16, mthiesse wrote: > Remove this Done. https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... File device/vr/android/gvr/gvr_device.h (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... device/vr/android/gvr/gvr_device.h:45: uint32_t pose_num_ = 0; On 2016/10/04 21:19:16, mthiesse wrote: > I think frame_index_ would be more clear. Changed to pose_index_. As discussed, this is strongly associated 1:1 with generated poses, and these may or may not end up being used for submitted frames. For example, before requestPresent, an app can be consuming poses but not using submitFrame(). https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... File device/vr/android/gvr/gvr_device_provider.cc (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... device/vr/android/gvr/gvr_device_provider.cc:68: void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t frameNumStarted) override{}; On 2016/10/04 21:19:16, mthiesse wrote: > Same comment about function signature applies here. renamed to pose_index. https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... device/vr/android/gvr/gvr_device_provider.cc:68: void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t frameNumStarted) override{}; On 2016/10/04 21:19:16, mthiesse wrote: > nit: space after override I can't do that. Presubmit insists on me running "git cl format device", and that deletes the space if I add it. https://codereview.chromium.org/2384593002/diff/60001/device/vr/vr_service.mojom File device/vr/vr_service.mojom (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/vr_service.mo... device/vr/vr_service.mojom:16: // given timestamp. The poseNum is a sequential ID that's incremented on On 2016/10/04 21:19:17, mthiesse wrote: > Again, I think frameIndex is less confusing. The frame index this pose applies > to. Renamed to poseIndex. https://codereview.chromium.org/2384593002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2384593002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1377: void WebGLRenderingContextBase::restoreStateFromContext(int stateMask) { On 2016/10/04 21:19:17, mthiesse wrote: > Rather than a single restoreStateFromContext function which is hard (for me at > least) to validate in the general case, I would prefer individual functions for > each piece of state you're restoring (ex. restoreScissorBox(), > restoreScissorEnabled(), etc). Then you can explicitly restore each piece of > state you modify in your function that modifies them. You convinced me, done.
https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:279: void VrShell::SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t pose_num) { Nit: pass |pose| by const ref https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:284: int32_t GetPixelEncodedFrameNumber() { Let's be uniform with our types through (uint32 everywhere or int32 everywhere). Also, why is this called "frame number" here and "pose number" elsewhere? https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:163: static constexpr int POSE_QUEUE_SIZE = 8; Nit: Chromium-style naming for constants is kPoseQueueSize https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:164: gvr::Mat4f webvr_head_pose_[POSE_QUEUE_SIZE]; It looks like this isn't explicitly initialized, so I think it'll get default initialized. In this case, I think that means that there will be arbitrary values in here. Do we want to zero-initialize it or initialize it to something sensible, since a renderer can specify arbitrary pose numbers? https://codereview.chromium.org/2384593002/diff/60001/device/vr/vr_service.mojom File device/vr/vr_service.mojom (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/vr_service.mo... device/vr/vr_service.mojom:26: uint32 poseNum; Nit: can we move the newly added comment here, so it's next to the thing it defines?
https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... File device/vr/android/gvr/gvr_device_provider.cc (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... device/vr/android/gvr/gvr_device_provider.cc:68: void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t frameNumStarted) override{}; On 2016/10/04 23:08:14, klausw wrote: > On 2016/10/04 21:19:16, mthiesse wrote: > > nit: space after override > > I can't do that. Presubmit insists on me running "git cl format device", and > that deletes the space if I add it. There's a stray semi-colon, that's why clang-format is misformatting this. Remove this, and it should be formatted correctly.
See patchset 6 for changes. mthiesse@, this also includes some updates for your requests which I had missed in patchset 5. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:279: void VrShell::SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t pose_num) { On 2016/10/04 23:17:53, dcheng wrote: > Nit: pass |pose| by const ref Done. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:284: int32_t GetPixelEncodedFrameNumber() { On 2016/10/04 23:17:53, dcheng wrote: > Let's be uniform with our types through (uint32 everywhere or int32 everywhere). > > Also, why is this called "frame number" here and "pose number" elsewhere? Done, using pose index consistently. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:76: void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t pose_num) override; On 2016/10/04 21:19:16, mthiesse wrote: > This function name feels wrong... > > How about SetPoseForWebVrFrame(pose, frame_index). I think it does need "GvrPose" in the name since that's distinct from WebVR's VRPose. Submitting frames needs a GVR pose. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:163: static constexpr int POSE_QUEUE_SIZE = 8; On 2016/10/04 23:17:53, dcheng wrote: > Nit: Chromium-style naming for constants is kPoseQueueSize Done, using kPoseRingBufferSize since it's not actually a queue. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:164: gvr::Mat4f webvr_head_pose_[POSE_QUEUE_SIZE]; On 2016/10/04 21:19:16, mthiesse wrote: > Mild preference for std::array, or std::vector over c-style arrays. Smaller > stack sizes are also generally preferable. Done, using std::vector. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:164: gvr::Mat4f webvr_head_pose_[POSE_QUEUE_SIZE]; On 2016/10/04 23:17:53, dcheng wrote: > It looks like this isn't explicitly initialized, so I think it'll get default > initialized. In this case, I think that means that there will be arbitrary > values in here. Do we want to zero-initialize it or initialize it to something > sensible, since a renderer can specify arbitrary pose numbers? Good point, added explicit initialization to identity matrices. https://codereview.chromium.org/2384593002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:165: int32_t webvr_newest_pose_num_ = 0; On 2016/10/04 21:19:16, mthiesse wrote: > Remove unused variable. Done. https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... File device/vr/android/gvr/gvr_device_provider.cc (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/android/gvr/g... device/vr/android/gvr/gvr_device_provider.cc:68: void SetGvrPoseForWebVr(gvr::Mat4f pose, uint32_t frameNumStarted) override{}; On 2016/10/04 23:19:06, dcheng wrote: > On 2016/10/04 23:08:14, klausw wrote: > > On 2016/10/04 21:19:16, mthiesse wrote: > > > nit: space after override > > > > I can't do that. Presubmit insists on me running "git cl format device", and > > that deletes the space if I add it. > > There's a stray semi-colon, that's why clang-format is misformatting this. > Remove this, and it should be formatted correctly. Ah, thanks for spotting it. Fixed. https://codereview.chromium.org/2384593002/diff/60001/device/vr/vr_service.mojom File device/vr/vr_service.mojom (right): https://codereview.chromium.org/2384593002/diff/60001/device/vr/vr_service.mo... device/vr/vr_service.mojom:26: uint32 poseNum; On 2016/10/04 23:17:53, dcheng wrote: > Nit: can we move the newly added comment here, so it's next to the thing it > defines? Done.
mojo LGTM with nits https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:160: webvr_head_pose_.resize(kPoseRingBufferSize); resize() takes a second argument, so I think this can just say: resize(kPoseRingBufferSize, kIdentity) https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:161: const gvr::Mat4f identity = {{{1.0f, 0.0f, 0.0f, 0.0f}, Nit: prefer constexpr to const (also prefix with k) https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:331: int32_t webvr_pose_frame = GetPixelEncodedPoseIndex(); Nit: match types with uint32_t here as well.
https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:301: glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels); After you start presenting, aren't the poses and frames delivered in order, and with a 1:1 mapping? Do you ever expect GetPixelEncodedPoseIndex to not return 1 higher than it previously returned outside of requestPresent/whatever ends presentation? If not, could you do the pixel read once to synchronize after requestPresent(), and just assume increasing IDs after?
On Oct 5, 2016 06:14, <mthiesse@chromium.org> wrote: > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:301: glReadPixels(0, 0, 1, > 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels); > After you start presenting, aren't the poses and frames delivered in > order, and with a 1:1 mapping? Do you ever expect > GetPixelEncodedPoseIndex to not return 1 higher than it previously > returned outside of requestPresent/whatever ends presentation? If not, > could you do the pixel read once to synchronize after requestPresent(), > and just assume increasing IDs after? No, the frame delay oscillates between 2 and 3. I had tried adding extra updateTexImage calls in VrShell.java to drop stale frames, that didn't seem to help. I assume it's due to drawFrame being triggered by vsync while JS drawing often takes more than one frame. I was able to use this shortcut in the experimental no-compositor patch which calls drawFrame exacrly once pernsubmitFrame. > > https://codereview.chromium.org/2384593002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Oct 5, 2016 06:14, <mthiesse@chromium.org> wrote: > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:301: glReadPixels(0, 0, 1, > 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels); > After you start presenting, aren't the poses and frames delivered in > order, and with a 1:1 mapping? Do you ever expect > GetPixelEncodedPoseIndex to not return 1 higher than it previously > returned outside of requestPresent/whatever ends presentation? If not, > could you do the pixel read once to synchronize after requestPresent(), > and just assume increasing IDs after? No, the frame delay oscillates between 2 and 3. I had tried adding extra updateTexImage calls in VrShell.java to drop stale frames, that didn't seem to help. I assume it's due to drawFrame being triggered by vsync while JS drawing often takes more than one frame. I was able to use this shortcut in the experimental no-compositor patch which calls drawFrame exacrly once pernsubmitFrame. > > https://codereview.chromium.org/2384593002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Oct 5, 2016 07:11, "Klaus Weidner" <klausw@google.com> wrote: > > On Oct 5, 2016 06:14, <mthiesse@chromium.org> wrote: > > > > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > > chrome/browser/android/vr_shell/vr_shell.cc:301: glReadPixels(0, 0, 1, > > 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels); > > After you start presenting, aren't the poses and frames delivered in > > order, and with a 1:1 mapping? Do you ever expect > > GetPixelEncodedPoseIndex to not return 1 higher than it previously > > returned outside of requestPresent/whatever ends presentation? If not, > > could you do the pixel read once to synchronize after requestPresent(), > > and just assume increasing IDs after? > > No, the frame delay oscillates between 2 and 3. I had tried adding extra updateTexImage calls in VrShell.java to drop stale frames, that didn't seem to help. I assume it's due to drawFrame being triggered by vsync while JS drawing often takes more than one frame. > > I was able to use this shortcut in the experimental no-compositor patch which calls drawFrame exacrly once per submitFrame. FWIW, the no-compositor patch sets RENDERMODE_WHEN_DIRTY in VrShell.java, then calls drawFrame for each submitFrame call manually. But that wouldn't work here. The swapBuffers call which adds a frame to the SurfaceTexture comes from the Compositor, and that happens an unpredictable amount of time after submitFrame. > > https://codereview.chromium.org/2384593002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Oct 5, 2016 07:11, "Klaus Weidner" <klausw@google.com> wrote: > > On Oct 5, 2016 06:14, <mthiesse@chromium.org> wrote: > > > > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > > chrome/browser/android/vr_shell/vr_shell.cc:301: glReadPixels(0, 0, 1, > > 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels); > > After you start presenting, aren't the poses and frames delivered in > > order, and with a 1:1 mapping? Do you ever expect > > GetPixelEncodedPoseIndex to not return 1 higher than it previously > > returned outside of requestPresent/whatever ends presentation? If not, > > could you do the pixel read once to synchronize after requestPresent(), > > and just assume increasing IDs after? > > No, the frame delay oscillates between 2 and 3. I had tried adding extra updateTexImage calls in VrShell.java to drop stale frames, that didn't seem to help. I assume it's due to drawFrame being triggered by vsync while JS drawing often takes more than one frame. > > I was able to use this shortcut in the experimental no-compositor patch which calls drawFrame exacrly once per submitFrame. FWIW, the no-compositor patch sets RENDERMODE_WHEN_DIRTY in VrShell.java, then calls drawFrame for each submitFrame call manually. But that wouldn't work here. The swapBuffers call which adds a frame to the SurfaceTexture comes from the Compositor, and that happens an unpredictable amount of time after submitFrame. > > https://codereview.chromium.org/2384593002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Oct 5, 2016 07:40, "Klaus Weidner" <klausw@google.com> wrote: > > On Oct 5, 2016 07:11, "Klaus Weidner" <klausw@google.com> wrote: > > > > On Oct 5, 2016 06:14, <mthiesse@chromium.org> wrote: > > > > > > > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > > > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > > > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > > > chrome/browser/android/vr_shell/vr_shell.cc:301: glReadPixels(0, 0, 1, > > > 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels); > > > After you start presenting, aren't the poses and frames delivered in > > > order, and with a 1:1 mapping? Do you ever expect > > > GetPixelEncodedPoseIndex to not return 1 higher than it previously > > > returned outside of requestPresent/whatever ends presentation? If not, > > > could you do the pixel read once to synchronize after requestPresent(), > > > and just assume increasing IDs after? > > > > No, the frame delay oscillates between 2 and 3. I had tried adding extra updateTexImage calls in VrShell.java to drop stale frames, that didn't seem to help. I assume it's due to drawFrame being triggered by vsync while JS drawing often takes more than one frame. > > > > I was able to use this shortcut in the experimental no-compositor patch which calls drawFrame exacrly once per submitFrame. > > FWIW, the no-compositor patch sets RENDERMODE_WHEN_DIRTY in VrShell.java, then calls drawFrame for each submitFrame call manually. But that wouldn't work here. The swapBuffers call which adds a frame to the SurfaceTexture comes from the Compositor, and that happens an unpredictable amount of time after submitFrame. Anyway, I'm open to experiment with this further, i.e. drawing from onFrameAvailable instead of onDrawFrame, but can we do that as a followup patch? I'd prefer not to complicate this with more invasive changes at this stage. > > > https://codereview.chromium.org/2384593002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Oct 5, 2016 07:40, "Klaus Weidner" <klausw@google.com> wrote: > > On Oct 5, 2016 07:11, "Klaus Weidner" <klausw@google.com> wrote: > > > > On Oct 5, 2016 06:14, <mthiesse@chromium.org> wrote: > > > > > > > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > > > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > > > > > https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... > > > chrome/browser/android/vr_shell/vr_shell.cc:301: glReadPixels(0, 0, 1, > > > 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels); > > > After you start presenting, aren't the poses and frames delivered in > > > order, and with a 1:1 mapping? Do you ever expect > > > GetPixelEncodedPoseIndex to not return 1 higher than it previously > > > returned outside of requestPresent/whatever ends presentation? If not, > > > could you do the pixel read once to synchronize after requestPresent(), > > > and just assume increasing IDs after? > > > > No, the frame delay oscillates between 2 and 3. I had tried adding extra updateTexImage calls in VrShell.java to drop stale frames, that didn't seem to help. I assume it's due to drawFrame being triggered by vsync while JS drawing often takes more than one frame. > > > > I was able to use this shortcut in the experimental no-compositor patch which calls drawFrame exacrly once per submitFrame. > > FWIW, the no-compositor patch sets RENDERMODE_WHEN_DIRTY in VrShell.java, then calls drawFrame for each submitFrame call manually. But that wouldn't work here. The swapBuffers call which adds a frame to the SurfaceTexture comes from the Compositor, and that happens an unpredictable amount of time after submitFrame. Anyway, I'm open to experiment with this further, i.e. drawing from onFrameAvailable instead of onDrawFrame, but can we do that as a followup patch? I'd prefer not to complicate this with more invasive changes at this stage. > > > https://codereview.chromium.org/2384593002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:301: glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, pixels); On 2016/10/05 13:14:06, mthiesse wrote: > After you start presenting, aren't the poses and frames delivered in order, and > with a 1:1 mapping? Do you ever expect GetPixelEncodedPoseIndex to not return 1 > higher than it previously returned outside of requestPresent/whatever ends > presentation? If not, could you do the pixel read once to synchronize after > requestPresent(), and just assume increasing IDs after? Acknowledged.
https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:161: const gvr::Mat4f identity = {{{1.0f, 0.0f, 0.0f, 0.0f}, Might as well re-use SetIdentityM in vr_math.cc Actually, maybe we should just have a constexpr identity matrix in vr_math.cc, then we can replace this and calls to SetIdentityM with matrix = kIdentityMatrix
PTAL at patchset 7. I've rebased to ToT which now includes GVR 1.0.0. https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:160: webvr_head_pose_.resize(kPoseRingBufferSize); On 2016/10/05 07:34:44, dcheng wrote: > resize() takes a second argument, so I think this can just say: > > resize(kPoseRingBufferSize, kIdentity) Done. https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:161: const gvr::Mat4f identity = {{{1.0f, 0.0f, 0.0f, 0.0f}, On 2016/10/05 15:40:01, mthiesse wrote: > Might as well re-use SetIdentityM in vr_math.cc > > Actually, maybe we should just have a constexpr identity matrix in vr_math.cc, > then we can replace this and calls to SetIdentityM with matrix = kIdentityMatrix I changed it to use SetIdentityM. Somehow I was under the mistaken impression that a GVR pose matrix was a different type from what vr_math uses, but both are just gvr::Mat4f. I'm a bit hesitant about a shared kIdentityMatrix, will someone manage to cast away the constness and apply an in-place transform to it? That would lead to extremely nasty bugs if it's a small transform. I'd feel a bit more comfortable with a GetIdentityM() function so that you could use it in expressions or initialization without direct sharing. Or is that overly paranoid? In any case, I think this should be a separate refactor since it gets used in quite a few places. https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:161: const gvr::Mat4f identity = {{{1.0f, 0.0f, 0.0f, 0.0f}, On 2016/10/05 07:34:44, dcheng wrote: > Nit: prefer constexpr to const (also prefix with k) Acknowledged, but it's no longer const (or constexpr) due to using SetIdentityM on it. https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:331: int32_t webvr_pose_frame = GetPixelEncodedPoseIndex(); On 2016/10/05 07:34:44, dcheng wrote: > Nit: match types with uint32_t here as well. Done. I'm surprised the compiler didn't complain about this one, I think it would have caused out-of-bounds array access after half a year of continuous VR presentation due to the mod result going negative.
lgtm
Description was changed from ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. Non-ToT patches assumed to be present: - Required: "Updated to GVR 1.0 SDK" as per commit b181af38ca1ca95ac250256aa067ec1630aff905 from Sep 23 which was rolled back later. - Optional: "treat enable-webvr flag as always on" BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
The CQ bit was checked by klausw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:161: const gvr::Mat4f identity = {{{1.0f, 0.0f, 0.0f, 0.0f}, On 2016/10/05 17:02:10, klausw wrote: > On 2016/10/05 15:40:01, mthiesse wrote: > > Might as well re-use SetIdentityM in vr_math.cc > > > > Actually, maybe we should just have a constexpr identity matrix in vr_math.cc, > > then we can replace this and calls to SetIdentityM with matrix = > kIdentityMatrix > > I changed it to use SetIdentityM. Somehow I was under the mistaken impression > that a GVR pose matrix was a different type from what vr_math uses, but both are > just gvr::Mat4f. > > I'm a bit hesitant about a shared kIdentityMatrix, will someone manage to cast > away the constness and apply an in-place transform to it? That would lead to > extremely nasty bugs if it's a small transform. I'd feel a bit more comfortable > with a GetIdentityM() function so that you could use it in expressions or > initialization without direct sharing. Or is that overly paranoid? In any case, > I think this should be a separate refactor since it gets used in quite a few > places. Casting away const-ness would be a Bad Thing™ to do and mutating a const object is undefined behavior. We shouldn't be doing that: if there is something in Chrome doing that, we should fix it =) https://codereview.chromium.org/2384593002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:331: int32_t webvr_pose_frame = GetPixelEncodedPoseIndex(); On 2016/10/05 17:02:10, klausw wrote: > On 2016/10/05 07:34:44, dcheng wrote: > > Nit: match types with uint32_t here as well. > > Done. I'm surprised the compiler didn't complain about this one, I think it > would have caused out-of-bounds array access after half a year of continuous VR > presentation due to the mod result going negative. We suppress a lot of unsigned / signed conversion warnings.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
LGTM once you've addressed a couple more comments. https://codereview.chromium.org/2384593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:212: Let's null these out on ExitPresent. https://codereview.chromium.org/2384593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:352: // Write the frame number for the pose used into a bottom left pixel block. Check that we're presenting and early terminate if we're not. Otherwise we may try calling GL commands on a context that doesn't exist.
https://codereview.chromium.org/2384593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2384593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:212: On 2016/10/05 20:49:05, bajones (OOO-Paternity leave) wrote: > Let's null these out on ExitPresent. Done, specifically in forceExitPresent (which gets called from exitPresent) to ensure it's also called in error paths. https://codereview.chromium.org/2384593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:352: // Write the frame number for the pose used into a bottom left pixel block. On 2016/10/05 20:49:05, bajones (OOO-Paternity leave) wrote: > Check that we're presenting and early terminate if we're not. Otherwise we may > try calling GL commands on a context that doesn't exist. Done. The original code unconditionally called contorller()->submitFrame and sets m_canUpdateFramePose = true, was that necessary or is it ok to skip this on early exit?
The CQ bit was checked by klausw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, mthiesse@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2384593002/#ps140001 (title: "bajones #40: add sanity check + pointer zeroing")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by klausw@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: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
The CQ bit was checked by ddorwin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dcheng, please review mojom changes.
Whoops, disregard that, didn't see the mojo LGTM, and my extension lies :P
On 2016/10/06 00:18:47, mthiesse wrote: > dcheng, please review mojom changes. mthiesse@, dcheng already LGTMed the mojo changes as of #22, I haven't changed that part since then. I thought the "changed after l-g-t-m" email is just informational?
Message was sent while issue was closed.
Description was changed from ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Encode frame number in pixel data for pose sync This encodes the pose number of the drawn WebVR frame in a corner pixel block, this is read by vr_shell.cc to use the corresponding pose when submitting the frame to GVR for presentation. Log data shows that the pose is 2-3 frames old by the time it is ready for submission to GVR. This includes compositor buffering and SurfaceTexture buffering. The patch helps in getting reprojection working as intended, but can't do anything about the excessive latency. BUG=389343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/dfa8ed4b3b10200b70d9f79cf247b8a1ec23e20e Cr-Commit-Position: refs/heads/master@{#423367} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/dfa8ed4b3b10200b70d9f79cf247b8a1ec23e20e Cr-Commit-Position: refs/heads/master@{#423367} |
