Closed Bug 1036602 Opened 10 years ago Closed 9 years ago

Support VR rendering in layers

Categories

(Core :: Graphics, defect)

x86
Windows 8.1
defect
Not set
normal

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.)
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)
Attached patch 2 - D3D11 implementation (obsolete) — Splinter Review
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)
Attached patch 3 - GL implementation (obsolete) — Splinter Review
Attachment #8453278 - Flags: feedback?(jmuizelaar)
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?
(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.).
Bas: this is what it looks like when things go fullscreen, fwiw: http://i.imgur.com/3QS7tXY.jpg
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)
Implements the VR effect in D3D11 layers
Attachment #8513640 - Flags: review?(bas)
Attached patch 3 - GL implementation (v2) (obsolete) — Splinter Review
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)
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 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)
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.
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)
(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 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 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-
Updated generic bits with IsSameProcess() protection.
Attachment #8513639 - Attachment is obsolete: true
Attachment #8513639 - Flags: review?(jmuizelaar)
Comment on attachment 8523108 [details] [diff] [review]
Generic layers support for VR (v3)

carrying forward nical's review
Attachment #8523108 - Flags: review+
Attached patch 3 - GL implementation (v3) (obsolete) — Splinter Review
Updated GL implementation with review feedback.
Attachment #8513641 - Attachment is obsolete: true
Attachment #8526206 - Flags: review?(bgirard)
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)
Attachment #8526274 - Flags: review?(bgirard) → review+
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)
Attachment #8537905 - Flags: review?(bas) → review+
Attachment #8513640 - Flags: review?(bas)
You need to log in before you can comment on or make changes to this bug.