797013 - CheckInterfaceSupport fails on some hardware in metrofx

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jim Mathies [:jimm]]

We currently force accel on in metro, but failures can still crop up when we initialize the d3d layers backend. When this happens, layer manager in MetroWidget is left as null, and ultimately the browser just crashes. 

VMWare 8.0 is a good test case with the Win8 RTM. 

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp#572

More generally we need a better fall back when we can't create needed rendering resources.

Comment 1

[Brian R. Bondy [:bbondy]]

I think we have to enumerate more than the first adapter, and then return once we find the first one that supports dx10.  I think we only check the first adapter currently.

Comment 2

[Jim Mathies [:jimm]]

Attachment: dxdiag for vmware / win8 rtm

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1

I couldn't find a good reason why CheckInterfaceSupport was failing, but I did find that simply calling D3D10CreateDevice1 directly works.  So that function is failing with interface not supported, but it is supported.  I did find some strange notes in the documentation, but nothing specific to the problem we're seeing on Windows 8.  It only mentions DX11 interfaces, but we're using DX10 interfaces here.  Maybe it's a bug in Win8 or something.  The workaround seems to be just to try creating the object though which is what this patch does.

With this patch Metro builds work in vmware workstation 8.0.  Without it, it does not work and ends in a crash.

> Note  You can use CheckInterfaceSupport only to check whether a Direct3D 
> 10.x interface is supported, and only on Windows Vista SP1 and later 
> versions of the operating system. If you try to use CheckInterfaceSupport 
> to check whether a Direct3D 11.x and later version interface is supported, 
> CheckInterfaceSupport returns DXGI_ERROR_UNSUPPORTED. Therefore, do not use 
> CheckInterfaceSupport. Instead, to verify whether the operating system supports 
> a particular interface, try to create the interface. 

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

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to Brian R. Bondy [:bbondy] from comment #3)
> Created attachment 677273 [details] [diff] [review]
> Patch v1
> 
> I couldn't find a good reason why CheckInterfaceSupport was failing, but I
> did find that simply calling D3D10CreateDevice1 directly works.  So that
> function is failing with interface not supported, but it is supported.  I
> did find some strange notes in the documentation, but nothing specific to
> the problem we're seeing on Windows 8.  It only mentions DX11 interfaces,
> but we're using DX10 interfaces here.  Maybe it's a bug in Win8 or
> something.  The workaround seems to be just to try creating the object
> though which is what this patch does.
> 
> With this patch Metro builds work in vmware workstation 8.0.  Without it, it
> does not work and ends in a crash.
> 
> > Note  You can use CheckInterfaceSupport only to check whether a Direct3D 
> > 10.x interface is supported, and only on Windows Vista SP1 and later 
> > versions of the operating system. If you try to use CheckInterfaceSupport 
> > to check whether a Direct3D 11.x and later version interface is supported, 
> > CheckInterfaceSupport returns DXGI_ERROR_UNSUPPORTED. Therefore, do not use 
> > CheckInterfaceSupport. Instead, to verify whether the operating system supports 
> > a particular interface, try to create the interface. 
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/bb174524%28v=vs.
> 85%29.aspx

Startup time reasons mean we can't just take this patch. Although D3D10 hardware is getting more common we'd need to measure ts carefully first on non D3D10 Windows systems.

Comment 5

[Brian R. Bondy [:bbondy]]

So I think failures are fast, but I think we should do one of 2 things here:

1) Add a pref that remembers that you have neither DX10 nor DX10.1 support. Should this ever expire like on hardware change? If so do you know the best way to detect this change?
or 2) Conditionally only do this in Metro. 

The patch may actually lead to a win for non startup reasons since acceleration will be used. But ya I see the startup concern.

Comment 6

[Brian R. Bondy [:bbondy]]

Another alternative I thought of is doing #1 as per Comment 5, but reset the pref if CheckInterface actually succeeds.

Comment 9

[Brian R. Bondy [:bbondy]]

I found the bug we previously talked about this and set it to block this.  Bug 725508.  I'm going to go with Comment 6 I think, unless you have objections.

Comment 11

[Matthew Morley]

I'm new to nightlies and would like to help out. I can't get the 'Metro' version to load here at all.

What logs / info can I send to help you lot?

MaFt

Comment 12

[Brian R. Bondy [:bbondy]]

Thanks for the offer but the problem is already identified. I'm actually working on a new patch for this now and this will likely land this week.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2

So this patch is similar to the last one, but it doesn't have the potential startup perf hit. 

In particular it will only ever try to create the D10 device once in failure conditions. If that fails we will never check again... that is unless CheckInterfaceSupport returns true. Then we'll check again.

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 678977 [details] [diff] [review]
Patch v2

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

Hrm, I wonder if we can also have this pref reset if the adapter/driver changes :s. I'm a little worried someone on a device where this fails. But maybe that group is too small to really worry about.

Comment 15

[Brian R. Bondy [:bbondy]]

I'm not sure how to detect that case, but if they have a device that always fails the CheckInterfaceSupport call, then they are in the same boat as before this patch.  So this patch doesn't make it worse.  It just opens the door to more devices.

Maybe we could store the last "adapter description" as seen in about:support in registry and then if it is different from what exists currently we would reset?

Comment 16

[Brian R. Bondy [:bbondy]]

Maybe ditto adapter drivers.
If you agree that this patch makes it no worse, could we do that in another bug for both adapter description and adapter drivers? I can still do it this week but a lot of people are waiting on this fix.

Comment 17

[Bas Schouten (:bas.schouten)]

Comment on attachment 678977 [details] [diff] [review]
Patch v2

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

Hrm, I wonder if we can also have this pref reset if the adapter/driver changes :s. I'm a little worried someone on a device where this fails. But maybe that group is too small to really worry about.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +565,5 @@
>                // They won't get acceleration.
>                return;
>              }
> +
> +            bool checkDX10 = 

nit: whitespace

Comment 18

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07eea74cea2b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07eea74cea2b