Closed Bug 912196 Opened 11 years ago Closed 10 years ago

Add support for ANGLE D3D11 path

Categories

(Core :: Graphics: CanvasWebGL, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: vlad, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [games])

Attachments

(7 files, 6 obsolete files)

4.12 KB, patch
jgilbert
: review+
jgilbert
: checkin+
Details | Diff | Splinter Review
4.79 KB, patch
u480271
: review+
Details | Diff | Splinter Review
8.83 KB, patch
u480271
: review+
Details | Diff | Splinter Review
5.22 KB, patch
u480271
: review+
Details | Diff | Splinter Review
1.01 KB, patch
u480271
: review+
Details | Diff | Splinter Review
937 bytes, patch
u480271
: review+
Details | Diff | Splinter Review
1.34 KB, patch
u480271
: review+
Details | Diff | Splinter Review
ANGLE now has a D3D11 backend.  We should be able to turn it on, and extend our D3D11 layers code to use it thus not needing to touch D3D9 on platforms that have D3D11.

This depends on the angle update in bug 883478.
Assignee: nobody → dglastonbury
May I ask what the current status of this is? I'm profiling a rendering stress test code for Emscripten ( see https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes/10kCubes.html ), and am seeing large portions of time consumed in testing D3D9 context lost events (probably same as bugs 875222 and 846536). Interestingly, running with the webgl.prefer-native-gl gives me better performance on Windows 8.1, but not by much, since there I'm getting a large penalty of surfaces being routed GPU->CPU->GPU via glReadPixels. I'd be interested in working on/testing out with an Angle D3D11 backend to compare how it would perform.
Whiteboard: [games]
I doubt that it'd make much difference on this, but it's a pretty big backend change, so who knows.
Blocks: gecko-games
Assignee: dglastonbury → nobody
Attached patch Use ANGLE D3D11Splinter Review
So this seems to work.. I'm going to send it through tryserver shortly.  Only works with OMTC on though, which is why there's a check in there.  I don't really want to figure out why it doesn't work without OMTC...
Assignee: nobody → vladimir
Attachment #8404038 - Flags: review?(jgilbert)
Comment on attachment 8404038 [details] [diff] [review]
Use ANGLE D3D11

Review of attachment 8404038 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +464,5 @@
>    nsRefPtr<ID3D10Device1> device;
>    HRESULT hr =
>      createD3DDevice(adapter1, D3D10_DRIVER_TYPE_HARDWARE, nullptr,
> +#ifdef DEBUG
> +                    D3D10_CREATE_DEVICE_DEBUG |

Yay. I was just talking to someone about needing to do this for GL as well.
Attachment #8404038 - Flags: review?(jgilbert) → review+
This patch doesn't seem to work for me.

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> I doubt that it'd make much difference on this, but it's a pretty big
> backend change, so who knows.

Using D3D11 would let us do synchronization using DXGIKeyedMutex which should work better than the current glFinish() approach.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> This patch doesn't seem to work for me.

I see d3d11 call happening but nothing shows up on the screen. I'm on Intel Ivybridge graphics on win7.
I'll take a look.
FWIW -- it does work for me with NVIDIA graphics.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #8)
> FWIW -- it does work for me with NVIDIA graphics.

You must have a lucky setup because I also have nVidia graphics and it fails.
FWIW I've determined using nVidia nSight that the WebGL content is being rendered, yet it looks like the SRV in the compositor (maybe?) doesn't have a texture attached to it.
Vlad discovered that it works when Antialias is disabled.
Attached patch Use BGR/BGRA formats for ANGLE (obsolete) — Splinter Review
When using Antialiasing, ie. MSAA, the resolve to a texture via FramebufferBlit fails because it's an error to blit from RGBA to BGRA format.

This patch is a quick proof-of-concept hack that forces creation of BGR/BGRA format renderbuffer/textures in GLScreenBuffer.
(In reply to Dan Glastonbury :djg :kamidphish from comment #12)
> Created attachment 8475020 [details] [diff] [review]
> Use BGR/BGRA formats for ANGLE
> 
> When using Antialiasing, ie. MSAA, the resolve to a texture via
> FramebufferBlit fails because it's an error to blit from RGBA to BGRA format.
> 
> This patch is a quick proof-of-concept hack that forces creation of BGR/BGRA
> format renderbuffer/textures in GLScreenBuffer.

This patch causes reftest failures:
https://tbpl.mozilla.org/?tree=Try&rev=4f65dcafe2ff
Attached patch Use BGR/BGRA formats for ANGLE (obsolete) — Splinter Review
Rebased to avoid conflicts
Attachment #8475020 - Attachment is obsolete: true
Depends on: 1068169
(In reply to Dan Glastonbury :djg :kamidphish from comment #12)
> Created attachment 8475020 [details] [diff] [review]
> Use BGR/BGRA formats for ANGLE
> 
> When using Antialiasing, ie. MSAA, the resolve to a texture via
> FramebufferBlit fails because it's an error to blit from RGBA to BGRA format.
> 
> This patch is a quick proof-of-concept hack that forces creation of BGR/BGRA
> format renderbuffer/textures in GLScreenBuffer.

Dan, any guesses as to what/why this is breaking?
Flags: needinfo?(dglastonbury)
Attached patch reorder-formats (obsolete) — Splinter Review
Here's another approach to making this work. This should have no impact on our existing tests.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> (In reply to Dan Glastonbury :djg :kamidphish from comment #12)
> > Created attachment 8475020 [details] [diff] [review]
> > Use BGR/BGRA formats for ANGLE
> > 
> > When using Antialiasing, ie. MSAA, the resolve to a texture via
> > FramebufferBlit fails because it's an error to blit from RGBA to BGRA format.
> > 
> > This patch is a quick proof-of-concept hack that forces creation of BGR/BGRA
> > format renderbuffer/textures in GLScreenBuffer.
> 
> Dan, any guesses as to what/why this is breaking?

What is the question? Why is this patch breaking? On DX9 or DX11?
Flags: needinfo?(dglastonbury)
Assignee: vladimir → dglastonbury
Status: NEW → ASSIGNED
(In reply to Dan Glastonbury :djg :kamidphish from comment #19)
> https://tbpl.mozilla.org/?tree=Try&rev=5bc02479bef3

This one still fails.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #20)
> This one still fails.

Yeah, so it seems. I'm investigating.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #20)
> (In reply to Dan Glastonbury :djg :kamidphish from comment #19)
> > https://tbpl.mozilla.org/?tree=Try&rev=5bc02479bef3
> 
> This one still fails.

So it was glCopyTexSubImage2D that was failing. The spec says that it behaves like glReadPixels up to just after the RGBA conversion process and then behaves like glTexSubImage2D.

I've "fixed it" by changing ANGLE's internal validation to accept an FBO with BGRA format. Stepping through the code path, ANGLE does correct conversion and the failing tests pass. It have no idea if what I've changed is "legal" though.
I spoke with :jgilbert at length about the path I was trying. It appears it's the wrong direction. I think :jmuizel approach of making ANGLE use RGB instead of BGR would be the smoothest way forward.
Here's a try run with d3d11 angle turned on:
https://tbpl.mozilla.org/?tree=Try&rev=54ac92b803d6
(In reply to Dan Glastonbury :djg :kamidphish from comment #25)
> I spoke with :jgilbert at length about the path I was trying. It appears
> it's the wrong direction. I think :jmuizel approach of making ANGLE use RGB
> instead of BGR would be the smoothest way forward.

Can you elaborate as to why?
Also, can either you or Jeff Gilbert outline what you think the formats that ANGLE should be using in D3D11 and D3D9 modes are?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #28)
> Also, can either you or Jeff Gilbert outline what you think the formats that
> ANGLE should be using in D3D11 and D3D9 modes are?

So the issue is with glBlitFramebuffer with a read buffer with non-zero sample bits.
In this case, an INVALID_OPERATION is generated if "the formats of the read and
draw framebuffers are not identical".

With D3D11, ANGLE considers the read buffer to have format RGBA8, and the draw buffer to have format BGRA8. Under these inputs, the spec says it should generate INVALID_OP.

The existence of EGL_HI_colorformats.txt[1] makes it seem like the EGLSurface should be treated as RGB-ordered, but I can't find any spec language to back this up.

If we assume EGLSurface can have BGR-ordered formats, then it will not be possible to create a multisampled renderbuffer of format BGR8 with which to blit onto a BGR-ordered EGLSurface that has no alpha channel.

That said, as found in bug 1048108, ANGLE doesn't really support no-alpha EGLSurfaces right now, so we're always having to fake that we don't have an alpha channel on ANGLE.

For the short term, we may be able to, on ANGLE, 'arbitrarily' try to use BGR formats instead of RGB, including using BGRA8 instead of the missing BGR8 format.

For the long term, we need to either add an extension for BGR8, or make ANGLE treat EGLSurfaces as RGB-orderer.

[1] https://www.khronos.org/registry/egl/extensions/HI/EGL_HI_colorformats.txt
(In reply to Jeff Muizelaar [:jrmuizel] from comment #28)
> Also, can either you or Jeff Gilbert outline what you think the formats that
> ANGLE should be using in D3D11 and D3D9 modes are?

D3D9 should have BGR because it's the only choice: D3DFMT_R8G8B8, D3DFMT_A8R8G8B8, D3DFMT_X8R8G8B8. (These are all BGR)

(http://msdn.microsoft.com/en-us/library/windows/desktop/bb172558%28v=vs.85%29.aspx)

Starting with DXGI in DX10, it appears Microsoft changes to RGB order with a note saying DX10 impl. may support BGR. I take that to mean use RGB in DX10 and up.

http://msdn.microsoft.com/en-us/library/windows/desktop/cc627090%28v=vs.85%29.aspx

Ideally, we should test RGB vs BGR with DX11, but it appears hard to get the correct tracking of R/B layout throughout our code.
Attachment #8404038 - Flags: checkin+
Assignee: dglastonbury → jgilbert
Attachment #8488616 - Attachment is obsolete: true
Attachment #8491850 - Attachment is obsolete: true
Attachment #8492055 - Attachment is obsolete: true
Attachment #8509861 - Flags: review?(dglastonbury)
Attachment #8509862 - Flags: review?(dglastonbury)
Attachment #8509863 - Flags: review?(dglastonbury)
If we r+ this, I'll land it on our ANGLE git repo.
Attachment #8509865 - Flags: review?(dglastonbury)
You might end up reviewing this in another bug as well. We'll just take whichever gets in first.
Attachment #8509871 - Flags: review?(dglastonbury)
Keywords: leave-open
Comment on attachment 8509861 [details] [diff] [review]
0001-Work-around-no-alpha-on-ANGLE-in-WebGL-not-GLContext.patch

Review of attachment 8509861 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits.

::: dom/canvas/WebGLContext.cpp
@@ +1844,4 @@
>      container->UnlockCurrentImage();
>      return ok;
>  }
> +s

Was ist s?

@@ +1848,3 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  
> +s

Was ist s?
Attachment #8509861 - Flags: review?(dglastonbury) → review+
Attachment #8509869 - Flags: review?(bas) → review?(nical.bugzilla)
Comment on attachment 8509869 [details] [diff] [review]
0007-Add-WrapMode-handling-to-TexturedEffects.patch

Review of attachment 8509869 [details] [diff] [review]:
-----------------------------------------------------------------

I am not sure why we support repeat at all in the compositor

::: gfx/layers/composite/TiledContentHost.cpp
@@ +494,5 @@
>    }
>  
> +  RefPtr<TexturedEffect> effect = CreateTexturedEffect(source, sourceOnWhite,
> +                                                       aFilter, true,
> +                                                       WrapMode::REPEAT);

Why are we using repeat for tiles?
So it looks like the only reason we support repeat in the compositor is for buffer rotation. All other layer types should just use clamp. I think you can just simplify all of that logic in there because use repeat will disappear when tiling becomes the default everywhere, and I don't think D3D11 ANGLE needs repeat in the compositor.
Attachment #8509869 - Flags: review?(nical.bugzilla) → review-
Attachment #8509862 - Flags: review?(dglastonbury) → review+
Attachment #8509863 - Flags: review?(dglastonbury) → review+
Attachment #8509865 - Flags: review?(dglastonbury) → review+
Attachment #8509866 - Flags: review?(dglastonbury) → review+
Attachment #8509867 - Flags: review?(dglastonbury) → review+
Attachment #8509871 - Flags: review?(dglastonbury) → review+
Depends on: 1088417
Comment on attachment 8509871 [details] [diff] [review]
0008-Update-mOptions-based-on-SurfaceCaps.patch

This landed elsewhere.
Attachment #8509871 - Attachment is obsolete: true
Comment on attachment 8509869 [details] [diff] [review]
0007-Add-WrapMode-handling-to-TexturedEffects.patch

This is bug 1088417.
Attachment #8509869 - Attachment is obsolete: true
Depends on: 1090985
Blocks: 1089139
You need to log in before you can comment on or make changes to this bug.