Closed
Bug 1036602
Opened 10 years ago
Closed 9 years ago
Support VR rendering in layers
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: vlad, Assigned: vlad)
References
Details
Attachments
(4 files, 6 obsolete files)
15.20 KB,
patch
|
Details | Diff | Splinter Review | |
12.22 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
19.41 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
Rendering VR content requires the browser to apply a device-specific post processing pass. This should be done in the compositor layer. ContainerLayer is extended to take an optional HMDInfo that provides all the information about the needed post-processing, and then does a separate render pass -- buffering all its child rendering into an intermediate surface, and then applying the post-process pass. (Design doc forthcoming.)
Assignee | ||
Comment 1•10 years ago
|
||
This patch addds the basic infrastructure for rendering a Container Layer to VR, applying post-processing effects after children are rendered.
Attachment #8453276 -
Flags: feedback?(jmuizelaar)
Assignee | ||
Updated•10 years ago
|
Attachment #8453276 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 2•10 years ago
|
||
Bas, one weird thing I'm seeing with this -- whenever I take the VR quad drawing path while fullscreen, I see the window borders around the fullscreen window grayed out (e.g. the space is there and in light gray, but it's not rendered.. all of my content is in the middle. But the surfaces involved are all full-screen-sized, not inner-window-sized). Any ideas what could cause that?
Attachment #8453277 -
Flags: feedback?(bas)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8453278 -
Flags: feedback?(jmuizelaar)
Comment 4•10 years ago
|
||
Comment on attachment 8453276 [details] [diff] [review] 1 - Generic layers support for VR/HMDInfo Review of attachment 8453276 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Effects.h @@ +99,5 @@ > > +struct EffectVRDistortion : public Effect > +{ > + EffectVRDistortion(gfx::vr::HMDInfo* aHMD, > + CompositingRenderTarget* aRenderTarget) I think it would make more sense to have EffectVRDistortion be a secondary effect, so that we don't need to pass in the RT/TextureSource. I think that would make sharing the code in ContainerLayerComposite easier too. ::: gfx/layers/Layers.cpp @@ +964,4 @@ > void > ContainerLayer::DefaultComputeEffectiveTransforms(const Matrix4x4& aTransformToSurface) > { > + bool keep3D = !!(GetContentFlags() & CONTENT_PRESERVE_3D); I assume we're only going to do this for VR, not for all preserve-3d transforms. @@ +968,4 @@ > Matrix residual; > Matrix4x4 idealTransform = GetLocalTransform() * aTransformToSurface; > + if (!keep3D) > + idealTransform.ProjectTo2D(); Won't this break rendering since our default projection matrix will cull anything with |z| > 1?
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #4) > > +struct EffectVRDistortion : public Effect > > +{ > > + EffectVRDistortion(gfx::vr::HMDInfo* aHMD, > > + CompositingRenderTarget* aRenderTarget) > > I think it would make more sense to have EffectVRDistortion be a secondary > effect, so that we don't need to pass in the RT/TextureSource. I think that > would make sharing the code in ContainerLayerComposite easier too. So I thought about this, but I think there are a number of problems. Any other secondary effects wouldn't really work with it (or be appropriate). That's okay, we can assert that. The biggest problem is that the shaders in use for this, their layout, the rendering etc. are all totally different than the rest of the code in DrawQuad. It's more straightforward to have a separate path that can write that in a straightforward way, instead of there being a branching path in DrawQuad that complicates the rest of the code (e.g. which input layout you select, how you bind/update buffers, etc.).
Assignee | ||
Comment 6•10 years ago
|
||
Bas: this is what it looks like when things go fullscreen, fwiw: http://i.imgur.com/3QS7tXY.jpg
Assignee | ||
Comment 7•10 years ago
|
||
Not sure who the best reviewer for this would be. This adds EffectVRDistortion and adds a HMDInfo slot on ContainerLayer; also extends ContainerLayerComposite to take a side branch when a layer is to be rendered with VR.
Attachment #8453276 -
Attachment is obsolete: true
Attachment #8453276 -
Flags: feedback?(matt.woodrow)
Attachment #8453276 -
Flags: feedback?(jmuizelaar)
Attachment #8513639 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 8•10 years ago
|
||
Implements the VR effect in D3D11 layers
Attachment #8513640 -
Flags: review?(bas)
Assignee | ||
Comment 9•10 years ago
|
||
GL implementation of VR effect
Attachment #8453277 -
Attachment is obsolete: true
Attachment #8453278 -
Attachment is obsolete: true
Attachment #8453277 -
Flags: feedback?(bas)
Attachment #8453278 -
Flags: feedback?(jmuizelaar)
Attachment #8513641 -
Flags: review?(bgirard)
Assignee | ||
Comment 10•10 years ago
|
||
Note that the new set of patches got rid of the DefaultComputeEffectiveTransform changes -- those are in a later patch that adds support for CSS rendering in VR, but that's not the goal of this.
Comment 11•10 years ago
|
||
Comment on attachment 8513639 [details] [diff] [review] 1 - Generic layers support for VR/HMDInfo (v2) Review of attachment 8513639 [details] [diff] [review]: ----------------------------------------------------------------- Jeff doesn't seem like the best person to review this. Requesting additional review from nical.
Attachment #8513639 -
Flags: review?(nical.bugzilla)
Comment 12•10 years ago
|
||
These patch are going to inevitably regress and perhaps require maintenance when we make changes down the road, as with anything of course. I'm just curious who's going to be responsible for dealing with the regressions and the maintenance so that we can set our expectations. Will we get in tree coverage? I'm hoping to find time to write compositor side GTest at some point. Perhaps this could test this piece of code as well.
Comment 13•10 years ago
|
||
I feel the VR distortion should be a secondary effect added to the effect chain. Fwiw, if we only need to support it for simple, textured effects, that's fine, we can just assert it doesn't get combined with something else, but longer term it seems like the superior approach. What do you think, Vlad?
Flags: needinfo?(vladimir)
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #12) > These patch are going to inevitably regress and perhaps require maintenance > when we make changes down the road, as with anything of course. I'm just > curious who's going to be responsible for dealing with the regressions and > the maintenance so that we can set our expectations. Would be me for the near future, and then we'll have to see after -- if this becomes part of the web platform, then it would be general platform maintenance, otherwise it should get ripped out :) I tried to be fairly careful and have a limited number of touch points with existing code, so near-term maintenance should be limited to any high level architectural changes (e.g. any changes to the effect system, how render targets are managed, etc.) instead of having to untangle vr/non-vr code in the same spot. > Will we get in tree coverage? I'm hoping to find time to write compositor > side GTest at some point. Perhaps this could test this piece of code as well. Yep, we should definitely get some testing. There is provision in forcing this effect on even without hardware connected, so we should be able to do some testing. The intent initially is to land this behind a pref (well, the toplevel stuff that would trigger this would be behind a pref). I'd need to think about how to test this though, since we can't easily do stuff like a reftest (given the distortion effect, it's going to be hard to create a precise ref rendering). (In reply to Bas Schouten (:bas.schouten) from comment #13) > I feel the VR distortion should be a secondary effect added to the effect > chain. Fwiw, if we only need to support it for simple, textured effects, > that's fine, we can just assert it doesn't get combined with something else, > but longer term it seems like the superior approach. > > What do you think, Vlad? Yeah, matt had the same question, I responded in comment #5. Even if we don't combine it with any other effects, having to do the integration with the main primary effects would complicate the shaders and the code flow because the shaders involved here are very different. But it's possible. However, combined with BenWa's comment #12, it seems better (at least for now) to have it be its own toplevel effect for both maintenance and simplicity. We can always move it to a secondary effect when things stabilize more.
Flags: needinfo?(vladimir)
Comment 15•10 years ago
|
||
Comment on attachment 8513639 [details] [diff] [review] 1 - Generic layers support for VR/HMDInfo (v2) Review of attachment 8513639 [details] [diff] [review]: ----------------------------------------------------------------- I am not super found of the raw pointer serialization for the HMD info. Please at least add the checks in LayersTransactionParent (to avoid the fuzzer issue and potential security problem), and if the info can be made cross-process-safe sooner rather than later it'd be good. More generally there are a bunch of XXX in there, I hope they won't stick around for too long. ::: gfx/layers/Effects.h @@ +124,5 @@ > + > + // The viewport for each eye in the source and > + // destination textures. > + gfx::IntRect mViewports[2]; > +}; nit: trailing space. ::: gfx/layers/ipc/LayerTransactionParent.cpp @@ +382,4 @@ > specific.get_ContainerLayerAttributes(); > containerLayer->SetPreScale(attrs.preXScale(), attrs.preYScale()); > containerLayer->SetInheritedScale(attrs.inheritedXScale(), attrs.inheritedYScale()); > + containerLayer->SetVRHMDInfo(reinterpret_cast<mozilla::gfx::VRHMDInfo*>(attrs.hmdInfo())); Please add something like: if (!IsSameProcess()) { NS_WARNING("VR currently not supported in cross-process compositing"); return false; } Otherwise our ipdl fuzzing tests will try to put crazy stuff in there and it will get filed as a security bug. ::: gfx/layers/ipc/LayersMessages.ipdlh @@ +227,4 @@ > float preYScale; > float inheritedXScale; > float inheritedYScale; > + uint64_t hmdInfo; // XXX need to pass something useful here, not a bare pointer Yeah
Attachment #8513639 -
Flags: review?(nical.bugzilla) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8513641 [details] [diff] [review] 3 - GL implementation (v2) Review of attachment 8513641 [details] [diff] [review]: ----------------------------------------------------------------- Minor stuff, r- to have another look (particularly for not using lazy load, fail if VR fails and one time leaks). ::: gfx/layers/opengl/CompositorOGL.cpp @@ +376,5 @@ > console->LogStringMessage(msg.get()); > } > > + if (!InitializeVR()) > + return false; That feels excessive no? I'd go further and say that we should do this lazily since you're adding shader compilation on the startup path. ::: gfx/layers/opengl/CompositorOGL.h @@ +153,4 @@ > nsTArray<GLuint> mUnusedTextures; > }; > > +struct CompositorOGLVRObjects { Optional: A quick comment what the program here does would be nice. ::: gfx/layers/opengl/CompositorOGLVR.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*- tab-width shouldn't be set to 20. Style guide suggests: /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ /* vim: set ts=8 sts=2 et sw=2 tw=80: */ @@ +34,5 @@ > +#include "mozilla/mozalloc.h" // for operator delete, etc > +#include "nsAString.h" > +#include "nsIConsoleService.h" // for nsIConsoleService, etc > +#include "nsIWidget.h" // for nsIWidget > +#include "nsLiteralString.h" // for NS_LITERAL_STRING You're including quite a bit more then you're using by the looks of it. @@ +56,5 @@ > +#include "libdisplay/GonkDisplay.h" // for GonkDisplay > +#include <ui/Fence.h> > +#endif > + > +#define BUFFER_OFFSET(i) ((char *)nullptr + (i)) This isn't used and it's pretty nasty, please remove it. @@ +102,5 @@ > + vs << " vTexCoord0.y = 1.0 - vTexCoord0.y;\n"; > + vs << " vTexCoord1.y = 1.0 - vTexCoord1.y;\n"; > + vs << " vTexCoord2.y = 1.0 - vTexCoord2.y;\n"; > + vs << " vGenericAttribs = aGenericAttribs;\n"; > + vs << "}\n"; Wouldn't it be strictly better to have a string literal for this? @@ +145,5 @@ > + GLint status, length; > + nsCString logStr; > + > + GLuint shaders[2]; > + shaders[0] = gl()->fCreateShader(LOCAL_GL_VERTEX_SHADER); Is it your intention to one time leak your GL resources? @@ +181,5 @@ > + if (!status) { > + gl()->fGetProgramiv(prog, LOCAL_GL_INFO_LOG_LENGTH, &length); > + logStr.SetLength(length); > + gl()->fGetProgramInfoLog(prog, length, &length, logStr.BeginWriting()); > + printf_stderr("GL shader failed to link:\n%s\n", logStr.BeginReading()); Shouldn't these be switched to NS_WARNING or similar to not log in release? @@ +306,5 @@ > + gl()->fBindBuffer(LOCAL_GL_ELEMENT_ARRAY_BUFFER, mVR.mDistortionIndices[eye]); > + gl()->fDrawElements(LOCAL_GL_TRIANGLES, mVR.mDistortionIndexCount[eye], LOCAL_GL_UNSIGNED_SHORT, 0); > + } > + > + // XXX not sure if we should disable all of this It's hard to find a good answer online. I'd leave it in for now.
Attachment #8513641 -
Flags: review?(bgirard) → review-
Assignee | ||
Comment 17•10 years ago
|
||
Updated generic bits with IsSameProcess() protection.
Attachment #8513639 -
Attachment is obsolete: true
Attachment #8513639 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8523108 [details] [diff] [review] Generic layers support for VR (v3) carrying forward nical's review
Attachment #8523108 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
Updated GL implementation with review feedback.
Attachment #8513641 -
Attachment is obsolete: true
Attachment #8526206 -
Flags: review?(bgirard)
Assignee | ||
Comment 20•10 years ago
|
||
Now with proper cleanup. Keeping the ostringstream stuff for flexibility later, since we're likely to need to dynamically generate this shader fairly soon.
Attachment #8526206 -
Attachment is obsolete: true
Attachment #8526206 -
Flags: review?(bgirard)
Attachment #8526274 -
Flags: review?(bgirard)
Updated•10 years ago
|
Attachment #8526274 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Generic part landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c080f66ed9d2
Keywords: leave-open
Assignee | ||
Comment 23•9 years ago
|
||
Minor addition; fix up the genshaders.sh script so that we can easily specify debug flags. Also rename .fx files to .hlsl (not shown in this patch).
Attachment #8537905 -
Flags: review?(bas)
Updated•9 years ago
|
Attachment #8537905 -
Flags: review?(bas) → review+
Assignee | ||
Comment 24•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/729b6b10b8ae remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/64503d093aa0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/458fe91b6392 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e22881d38aa8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e44dd59ae35
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 25•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/729b6b10b8ae https://hg.mozilla.org/mozilla-central/rev/64503d093aa0 https://hg.mozilla.org/mozilla-central/rev/458fe91b6392 https://hg.mozilla.org/mozilla-central/rev/e22881d38aa8 https://hg.mozilla.org/mozilla-central/rev/9e44dd59ae35
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•8 years ago
|
Attachment #8513640 -
Flags: review?(bas)
You need to log in
before you can comment on or make changes to this bug.
Description
•