Closed
Bug 912196
Opened 11 years ago
Closed 10 years ago
Add support for ANGLE D3D11 path
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: vlad, Assigned: jgilbert)
References
(Blocks 1 open bug)
Details
(Whiteboard: [games])
Attachments
(7 files, 6 obsolete files)
(deleted),
patch
|
jgilbert
:
review+
jgilbert
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Comment 1•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: [games]
Assignee | ||
Comment 2•11 years ago
|
||
I doubt that it'd make much difference on this, but it's a pretty big backend change, so who knows.
Updated•11 years ago
|
Blocks: gecko-games
Reporter | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
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+
Comment 5•10 years ago
|
||
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.
Comment 6•10 years ago
|
||
(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.
Reporter | ||
Comment 8•10 years ago
|
||
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.
Comment 10•10 years ago
|
||
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.
Comment 11•10 years ago
|
||
Vlad discovered that it works when Antialias is disabled.
Comment 12•10 years ago
|
||
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.
Comment 13•10 years ago
|
||
(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
Comment 14•10 years ago
|
||
Rebased to avoid conflicts
Attachment #8475020 -
Attachment is obsolete: true
Comment 15•10 years ago
|
||
(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)
Comment 16•10 years ago
|
||
Here's another approach to making this work. This should have no impact on our existing tests.
Comment 17•10 years ago
|
||
(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)
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
Comment 20•10 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #19)
> https://tbpl.mozilla.org/?tree=Try&rev=5bc02479bef3
This one still fails.
Comment 21•10 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #20)
> This one still fails.
Yeah, so it seems. I'm investigating.
Comment 22•10 years ago
|
||
(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.
Comment 23•10 years ago
|
||
Comment 24•10 years ago
|
||
Comment 25•10 years ago
|
||
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.
Comment 26•10 years ago
|
||
Here's a try run with d3d11 angle turned on:
https://tbpl.mozilla.org/?tree=Try&rev=54ac92b803d6
Comment 27•10 years ago
|
||
(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?
Comment 28•10 years ago
|
||
Also, can either you or Jeff Gilbert outline what you think the formats that ANGLE should be using in D3D11 and D3D9 modes are?
Assignee | ||
Comment 29•10 years ago
|
||
I filed an ANGLE bug:
https://code.google.com/p/angleproject/issues/detail?id=768
Assignee | ||
Comment 30•10 years ago
|
||
(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
Comment 31•10 years ago
|
||
(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.
Comment 32•10 years ago
|
||
I've landed the first part of this.
https://hg.mozilla.org/integration/mozilla-inbound/rev/98a881d6282c
Keywords: leave-open
Comment 33•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8404038 -
Flags: checkin+
Assignee | ||
Comment 34•10 years ago
|
||
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)
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8509862 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8509863 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 37•10 years ago
|
||
If we r+ this, I'll land it on our ANGLE git repo.
Attachment #8509865 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8509866 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 39•10 years ago
|
||
Attachment #8509867 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8509869 -
Flags: review?(bas)
Assignee | ||
Comment 41•10 years ago
|
||
You might end up reviewing this in another bug as well. We'll just take whichever gets in first.
Attachment #8509871 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 42•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 43•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8509869 -
Flags: review?(bas) → review?(nical.bugzilla)
Comment 44•10 years ago
|
||
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?
Comment 45•10 years ago
|
||
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.
Updated•10 years ago
|
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+
Assignee | ||
Comment 46•10 years ago
|
||
Comment on attachment 8509871 [details] [diff] [review]
0008-Update-mOptions-based-on-SurfaceCaps.patch
This landed elsewhere.
Attachment #8509871 -
Attachment is obsolete: true
Assignee | ||
Comment 47•10 years ago
|
||
Comment on attachment 8509869 [details] [diff] [review]
0007-Add-WrapMode-handling-to-TexturedEffects.patch
This is bug 1088417.
Attachment #8509869 -
Attachment is obsolete: true
Assignee | ||
Comment 48•10 years ago
|
||
Assignee | ||
Comment 49•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9105d35ba70
https://hg.mozilla.org/integration/mozilla-inbound/rev/942e71d1f271
https://hg.mozilla.org/integration/mozilla-inbound/rev/51926ef0c382
https://hg.mozilla.org/integration/mozilla-inbound/rev/42836d13bba5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ccefd960453
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9cb6f547e23
Comment 50•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9105d35ba70
https://hg.mozilla.org/mozilla-central/rev/942e71d1f271
https://hg.mozilla.org/mozilla-central/rev/51926ef0c382
https://hg.mozilla.org/mozilla-central/rev/42836d13bba5
https://hg.mozilla.org/mozilla-central/rev/7ccefd960453
https://hg.mozilla.org/mozilla-central/rev/f9cb6f547e23
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 51•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•