Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2308683004:
Added WebVR Render path to VrShell (Closed)
DescriptionAdded WebVR Render path to VrShell
This render path isn't used for WebVR just yet because VrShell has not been
added to the main browser activity.
BUG=389343
Committed: https://crrev.com/c5c5ea873213ee72e3d0929b47482681555340c3
Cr-Commit-Position: refs/heads/master@{#417424}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed Michael's feedback. #
Total comments: 7
Patch Set 3 : Addressed feedback #
Messages
Total messages: 29 (12 generated)
bajones@chromium.org changed reviewers: + bshe@chromium.org, klausw@chromium.org, mthiesse@chromium.org
Yes, I know it's a US/CAN holiday weekend. Prompt reviews are not expected. :) Adds the WebVR renderer to VrShell and makes VrShell a GvrDelegate in anticipation of the point that it's added to Chrome and can be used as the GvrApi entry point for the WebVR API. I know that none of you are directory owners, but I'd appreciate you taking a first pass at it before moving on to a more formal review.
Yes I know it's the long weekend, but I'm just watching anime here :P Some meta-level comments: There's no synchronization going on between the compositor and vr shell's DrawFrame right? Is this leading to randomly dropped frames even if everything runs at 60fps? Is there going to be extra delay between grabbing the head pose and getting pixels on screen because of this as well? And should that extra delay be added to the predicted delay? Also, is this rendering path only for WebVR content while in VR Shell? Or is this going to be the path for all WebVR content on Android (all platforms?)? If so, that means pulling in a lot of VR Shell code even when we disable VR Shell, which is probably fine? https://codereview.chromium.org/2308683004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2308683004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:126: gvr::ClockTimePoint target_time = gvr::GvrApi::GetTimePointNow(); Can you move this branch out to a DrawVRShell() function? https://codereview.chromium.org/2308683004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): https://codereview.chromium.org/2308683004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_renderer.cc:176: std::unique_ptr<float> vertices(new float[32]{ Move this out to a constant?
On 2016/09/04 16:04:03, mthiesse wrote: > There's no synchronization going on between the compositor and vr shell's > DrawFrame right? Is this leading to randomly dropped frames even if everything > runs at 60fps? Is there going to be extra delay between grabbing the head pose > and getting pixels on screen because of this as well? And should that extra > delay be added to the predicted delay? Yes, yes, yes, and yes. You also forgot to mention that the compositor texture is way too small to handle a VR renderbuffer, so it's a pixely mess. :) Honestly the output this produces is brutally bad. The fact that we're using the compositor texture here is a stopgap to simply get SOMETHING on screen, though, so those symptoms should all disappear once we get the texture sync story figured out. (What Klaus is looking at now.) The eventual path will be something along the lines of: * WebGL backbuffer is swapped for a Surface/SurfaceTexture pair that VrShell controls when VR presentation starts. * WebGL context draws to that buffer and then calls submitFrame, which is forwarded to the VrShell as SubmitWebVRFrame. (Which in this patch is empty.) * We use that as a signal to sync the SurfaceTexture and draw the frame. I'm sure that we'll hit some snags while implementing it, but that should give us far less CPU and texture copy overhead than going through the full compositor as we do now. > Also, is this rendering path only for WebVR content while in VR Shell? Or is > this going to be the path for all WebVR content on Android (all platforms?)? If > so, that means pulling in a lot of VR Shell code even when we disable VR Shell, > which is probably fine? It's for any WebVR content on Android. _Maybe_ all platforms, once VrShell expands beyond mobile, but Android is the only platform where I need something equivalent to the GvrLayout in order for VR rendering to work. Since VrShell owns (is?) the GvrLayout it makes sense to forward VR presentation code here. As VrShell becomes more complex we may want to split the API interaction bits out into a simplified delegate that both WebVR and the VrShell rely on, but there's also going to be plenty of transitions and interaction between the two, so this feels like the right solution for the time being. > Can you move this branch out to a DrawVRShell() function? > > Move this out to a constant? Good suggestions. I'll catch those in the next update
> * WebGL backbuffer is swapped for a Surface/SurfaceTexture pair that VrShell > controls when VR presentation starts. > * WebGL context draws to that buffer and then calls submitFrame, which is > forwarded to the VrShell as SubmitWebVRFrame. (Which in this patch is empty.) > * We use that as a signal to sync the SurfaceTexture and draw the frame. This feels backwards, wouldn't we want webGL's requestAnimationFrame to fire synchronously inside of VrShell's DrawFrame ideally and not the other way around? I don't think we can control GVR's render loop timing, but we can probably control Chrome's...
On 2016/09/04 16:59:46, mthiesse wrote: > > * WebGL backbuffer is swapped for a Surface/SurfaceTexture pair that VrShell > > controls when VR presentation starts. > > * WebGL context draws to that buffer and then calls submitFrame, which is > > forwarded to the VrShell as SubmitWebVRFrame. (Which in this patch is empty.) > > * We use that as a signal to sync the SurfaceTexture and draw the frame. > > This feels backwards, wouldn't we want webGL's requestAnimationFrame to fire > synchronously inside of VrShell's DrawFrame ideally and not the other way > around? > > I don't think we can control GVR's render loop timing, but we can probably > control Chrome's... That's not a bad idea! Hadn't thought about it that way, but it should work. Does the rendering work need to happen synchronously inside DrawFrame, though? That would have the browser process blocked on the render process, which is verboten in Chrome.
On 2016/09/04 17:05:44, bajones wrote: > On 2016/09/04 16:59:46, mthiesse wrote: > > > * WebGL backbuffer is swapped for a Surface/SurfaceTexture pair that > VrShell > > > controls when VR presentation starts. > > > * WebGL context draws to that buffer and then calls submitFrame, which is > > > forwarded to the VrShell as SubmitWebVRFrame. (Which in this patch is > empty.) > > > * We use that as a signal to sync the SurfaceTexture and draw the frame. > > > > This feels backwards, wouldn't we want webGL's requestAnimationFrame to fire > > synchronously inside of VrShell's DrawFrame ideally and not the other way > > around? > > > > I don't think we can control GVR's render loop timing, but we can probably > > control Chrome's... > > That's not a bad idea! Hadn't thought about it that way, but it should work. > Does the rendering work need to happen synchronously inside DrawFrame, though? > That would have the browser process blocked on the render process, which is > verboten in Chrome. Actually it wouldn't need to be synchronous, we would just need to post back to our GL thread to submit the frame.
lgtm
bajones@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review. Code path isn't utilized yet, just laying the groundwork for future CLs.
On 2016/09/06 18:09:05, bajones wrote: > mailto:sky@chromium.org: Please review. Code path isn't utilized yet, just laying the > groundwork for future CLs. Please get an owner of c/b/android to review this.
bajones@chromium.org changed reviewers: + mariakhomenko@chromium.org
Ah, yeah that would make more sense. Chromium Butler has failed me for owner suggestions. Sorry! mariakhomenko@: Could you review this change?
Description was changed from ========== Added WebVR Render path to VrShell This render path isn't used for WebVR just yet because VrShell has not been added to the main browser activity. BUG=389343 ========== to ========== Added WebVR Render path to VrShell This render path isn't used for WebVR just yet because VrShell has not been added to the main browser activity. BUG=389343 ==========
sky@chromium.org changed reviewers: - sky@chromium.org
sky@chromium.org changed reviewers: + sky@chromium.org
I'm removing myself from the review.
mariakhomenko@chromium.org changed reviewers: + dtrainor@chromium.org
I think Dave would be a better OWNERS reviewer for this code. Dave, could you take a look?
lgtm % nits. https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:74: device::GvrDelegateManager::GetInstance()->Shutdown(); Just making sure, but we're ok if we call Shutdown with no Initialize call right? https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.cc:164: exit(1); Should we just do CHECK_EQ(0, vertex_shader_handle) << error; or something similar? Same for other cases. It doesn't look like we really do exit() anywhere else in chrome/. https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.cc:189: // TODO: Figure out why this need to be restored. Add ldap? https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.cc:202: * Draw the WebVR frame // instead of /*? Style guide seems to push for that. https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.cc:205: // TODO: Figure out why this need to be restored. ldap, crbug? https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.cc:206: GLint old_buffer; A ScopedBufferRestore helper would be cool, but if this is just temporary until you track down the missing rebind probably not worth it. https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_renderer.h (right): https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.h:64: static constexpr size_t TEXCOORD_OFFSET = sizeof(float)*2; spaces around *
sky@chromium.org changed reviewers: - sky@chromium.org
On 2016/09/08 06:16:05, David Trainor wrote: > https://codereview.chromium.org/2308683004/diff/20001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell.cc:74: > device::GvrDelegateManager::GetInstance()->Shutdown(); > Just making sure, but we're ok if we call Shutdown with no Initialize call > right? Yes that's safe. > chrome/browser/android/vr_shell/vr_shell_renderer.cc:164: exit(1); > Should we just do CHECK_EQ(0, vertex_shader_handle) << error; or something > similar? Same for other cases. > > It doesn't look like we really do exit() anywhere else in chrome/. o_O Thanks for catching that! Definitely did not intent to land that debugging code. (Also replaced some similar statements in the same file from a prior commit.)
The CQ bit was checked by bajones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2308683004/#ps40001 (title: "Addressed feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added WebVR Render path to VrShell This render path isn't used for WebVR just yet because VrShell has not been added to the main browser activity. BUG=389343 ========== to ========== Added WebVR Render path to VrShell This render path isn't used for WebVR just yet because VrShell has not been added to the main browser activity. BUG=389343 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Added WebVR Render path to VrShell This render path isn't used for WebVR just yet because VrShell has not been added to the main browser activity. BUG=389343 ========== to ========== Added WebVR Render path to VrShell This render path isn't used for WebVR just yet because VrShell has not been added to the main browser activity. BUG=389343 Committed: https://crrev.com/c5c5ea873213ee72e3d0929b47482681555340c3 Cr-Commit-Position: refs/heads/master@{#417424} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c5c5ea873213ee72e3d0929b47482681555340c3 Cr-Commit-Position: refs/heads/master@{#417424} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
