844954 - Defect - Devices that only support DX9 fail to startup in Metro mode

Status: VERIFIED FIXED

(Core :: Graphics, defect, P1)

Description

[Brian R. Bondy [:bbondy]]

Asa shipped me a device that wouldn't startup in Metro mode. It turns out that it only supports DX9.  

We recently fixed startup detection in the delegate execute handler (the thing that decides whether to start in metro mode or desktop mode) to allow DX9 but missed this check in the gfx windows code. 

When dx10 and dx10.1 are note supported it then tries to use cairo w/ dx10.1, dx10, and finally dx9.3. 9.3 is what succeeds on this device but we were bailing early.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

So basically I don't bail early when we know dx10 is not supported. 
Instead I just skip the dx10 creation points. 
Below the code in the patch it calls into cairo create which succeeds in metro mode.

Comment 2

[Brian R. Bondy [:bbondy]]

Hi Bas any chance you could review this soon? I'd like to send back Asa's device after testing on the Nightly and also Asa mentioned a lot of people are waiting for this to test.

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to Brian R. Bondy [:bbondy] from comment #0)
> Asa shipped me a device that wouldn't startup in Metro mode. It turns out
> that it only supports DX9.  
> 
> We recently fixed startup detection in the delegate execute handler (the
> thing that decides whether to start in metro mode or desktop mode) to allow
> DX9 but missed this check in the gfx windows code. 
> 
> When dx10 and dx10.1 are note supported it then tries to use cairo w/
> dx10.1, dx10, and finally dx9.3. 9.3 is what succeeds on this device but we
> were bailing early.

We cannot support the DX9.3 device cairo uses sadly at this point for Direct2D. Changes will have to be made to Azure and other parts of the code to allow 9.3 to work properly.

Comment 4

[Brian R. Bondy [:bbondy]]

What is the reasoning that we can't? I did notice that some things are a bit glitchy by the way on popups. 

This is the line that allows the browser to work currently.
http://dxr.mozilla.org/mozilla-central/gfx/cairo/cairo/src/cairo-d2d-surface.cpp#l333

Maybe we can just add a line like this when dx10.1 and dx10.0 levels fail?

Currently without this patch, a bunch of devices just crash, but with this patch they work although not perfectly but definitely usable.

Comment 5

[Brian R. Bondy [:bbondy]]

metro only is also maybe an option but I'm not sure if we want to offer anything on devices where it won't be perfect.

Comment 6

[Asa Dotzler [:asa]]

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> metro only is also maybe an option but I'm not sure if we want to offer
> anything on devices where it won't be perfect.

We cannot afford to not support these Atom users. These chipsets will come to dominate, I believe, the consumer Windows space as tablets replace netbooks as the consumer Windows device.

Comment 7

[Brian R. Bondy [:bbondy]]

Setting moreinfo flag mainly for Comment 4.

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> metro only is also maybe an option but I'm not sure if we want to offer
> anything on devices where it won't be perfect.

The D3D11 OMTC will support 9.3 devices in combination with -software- content rendering. This means that these users -will- get smooth panning/zooming/etc. And they -will- get the Metro browser, just not Direct2D acceleration.

The idea is that once that's done we'll look into support Direct2D 1.1 where supporting 9.3 devices for content acceleration as well becomes a lot more feasible.

I personally think that's the best route to take at this point.

Comment 9

[Brian R. Bondy [:bbondy]]

Can we land this in the interim so that these users can test? I could do it only when running as Metro?

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> Can we land this in the interim so that these users can test? I could do it
> only when running as Metro?

We could do that, it would be better to make Metro work with D3D9 layers. As these users -will- otherwise run into graphical artifacts and crashes while browsing.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> (In reply to Brian R. Bondy [:bbondy] from comment #9)
> > Can we land this in the interim so that these users can test? I could do it
> > only when running as Metro?
> 
> We could do that, it would be better to make Metro work with D3D9 layers. As
> these users -will- otherwise run into graphical artifacts and crashes while
> browsing.

Bas, can we land your suggestion this week?

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

An IRC comment from Bas: Let's be clear that these users -will- crash and get graphical artifacts while browsing, as well as a large group will experience poor performance (in the context of taking the patch as is).

Comment 13

[Asa Dotzler [:asa]]

(In reply to Milan Sreckovic [:milan] from comment #12)
> An IRC comment from Bas: Let's be clear that these users -will- crash and
> get graphical artifacts while browsing, as well as a large group will
> experience poor performance (in the context of taking the patch as is).

If we can easily identify these crashes and segregate them in socorro, then I think getting "something" working now is important. 

It also sounds like we're going to need the better solution before we ship to consumers.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

So, the suggestion is to land this as is "now", but continue working on it before it reaches aurora?  Bas?

Comment 15

[Asa Dotzler [:asa]]

(In reply to Milan Sreckovic [:milan] from comment #14)
> So, the suggestion is to land this as is "now", but continue working on it
> before it reaches aurora?  Bas?

My "proposal" is we take a fix now that gets more people able to use Metro and that we then figure out how much of what we need to do for a proper solution for v1 of Metro Firefox (which sounds like "make Metro work with D3D9 layers".)

I don't know how much work that is, but I don't think we can go to market with a product that's failing for a significant number of users.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Bas, Brian, what's the best we can cook up for this week?

Comment 17

[Brian R. Bondy [:bbondy]]

Bas, By adding a fallback to D3D10_FEATURE_LEVEL_9_3 in gfxWindowsPlatform.cpp and NOT falling back to cairo's createdevice 9_3 feature level, it seems to work fine.  Is that a good solution for now instead of what the attached patch does?

  hr = createD3DDevice(
                  adapter1, 
                  D3D10_DRIVER_TYPE_HARDWARE,
                  NULL,
                  D3D10_CREATE_DEVICE_BGRA_SUPPORT |
                  D3D10_CREATE_DEVICE_PREVENT_INTERNAL_THREADING_OPTIMIZATIONS,
                  D3D10_FEATURE_LEVEL_9_3,
                  D3D10_1_SDK_VERSION,
                  getter_AddRefs(device));

Let me know which way you prefer for now.

Comment 18

[Bas Schouten (:bas.schouten)]

(In reply to Brian R. Bondy [:bbondy] from comment #17)
> Bas, By adding a fallback to D3D10_FEATURE_LEVEL_9_3 in
> gfxWindowsPlatform.cpp and NOT falling back to cairo's createdevice 9_3
> feature level, it seems to work fine.  Is that a good solution for now
> instead of what the attached patch does?
> 
>   hr = createD3DDevice(
>                   adapter1, 
>                   D3D10_DRIVER_TYPE_HARDWARE,
>                   NULL,
>                   D3D10_CREATE_DEVICE_BGRA_SUPPORT |
>                  
> D3D10_CREATE_DEVICE_PREVENT_INTERNAL_THREADING_OPTIMIZATIONS,
>                   D3D10_FEATURE_LEVEL_9_3,
>                   D3D10_1_SDK_VERSION,
>                   getter_AddRefs(device));
> 
> Let me know which way you prefer for now.

No, this does not make the situation better in any way.

Apparently I wasn't communicating this very clearly before, but to have a working product we can do either 1 of 2 (or maybe 3) things:

1. Wait for OMTC D3D11, this -will- support D3D level 9.3, not with content acceleration but it will support it for metro and support panning/zooming acceleration. This is expected to land in ~2 weeks if all goes well. It will land before the product will ship.
2. Make D3D9 layers work with Metro and use that for the metro app for these people.
(3. Make D3D10 layers work with CPU content/canvas drawing)

Any of those solutions would be hard to do in a week.

If we prefer a non-reftested/slow browser over no browser on metro we can do the option Brian suggested but we should make sure to do it for metro-only. We initially started by allowing 9_3 for all our users and ran into a lot of users having unusably slow or crashy browsers. However it could be in Windows 8 we have a large group of people with 9_3 devices that have little issues, but it's hard to say.

I'm not sure whether crashes will be easy to filter out, it all depends on how they'll show.

Comment 19

[Bas Schouten (:bas.schouten)]

(In reply to Asa Dotzler [:asa] from comment #15)
> (In reply to Milan Sreckovic [:milan] from comment #14)
> > So, the suggestion is to land this as is "now", but continue working on it
> > before it reaches aurora?  Bas?
> 
> My "proposal" is we take a fix now that gets more people able to use Metro
> and that we then figure out how much of what we need to do for a proper
> solution for v1 of Metro Firefox (which sounds like "make Metro work with
> D3D9 layers".)
> 
> I don't know how much work that is, but I don't think we can go to market
> with a product that's failing for a significant number of users.

So just to be perfectly clear, on the roadmap for Metro v1 there already -is- the proper solution (in the form of that I took 9_3 into account when creating OMTC D3D11).

Comment 20

[Brian R. Bondy [:bbondy]]

Asa is waiting ~2 weeks for OMTC  acceptable? I think so and in that case there's no work to do there.  Please let me know if you'd rather me hold onto your device if we wait too until that lands in case there are issues.

Comment 21

[Asa Dotzler [:asa]]

Yes. (In reply to Brian R. Bondy [:bbondy] from comment #20)
> Asa is waiting ~2 weeks for OMTC  acceptable? I think so and in that case
> there's no work to do there.  Please let me know if you'd rather me hold
> onto your device if we wait too until that lands in case there are issues.

Yes. Let's wait on the real fix with OMTC.  This hurts but I'd rather we not spend time on a solution that would only be available for a week or two.

Comment 22

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2

Hey Bas, Asa asked me to try and land this in the meantime while OMTC is being finished up.  Here is the same patch as before but it is only enabled for the Metro browser.

Added this clause so it works as it used to for Desktop.

if (!checkDX10 && !IsRunningInWindowsMetro()) {
  return;
}

Comment 23

[Bas Schouten (:bas.schouten)]

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

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

Sorry, I might not have been clear on this, we need a metro-only version of the -other- patch. The one that actually created the 9_3 device properly in gfxWindowsPlatform.

Comment 24

[Brian R. Bondy [:bbondy]]

k no prob, will do.

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - windows helpers

RAII HMODULE and helper from loading from system32

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - 9.3 feature level and refactoring

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 726407 [details] [diff] [review]
Patch v1 - windows helpers

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

::: xpcom/base/nsWindowsHelpers.h
@@ +91,5 @@
> +  }
> +
> +  static void Release(RawRef aFD) 
> +  { 
> +    if (aFD != Void()) {

nit - white space all over this

@@ +151,5 @@
> +    WCHAR systemPath[MAX_PATH + 1] = { L'\0' };
> +
> +    // If GetSystemPath fails we accept that we'll load the DLLs from the
> +    // normal search path.
> +    GetSystemDirectoryW(systemPath, MAX_PATH + 1);

lets be paranoid and check the result, or check the result of wcslen below for zero.

@@ +162,5 @@
> +      // No need to re-NULL terminate
> +    }
> +
> +    size_t fileLen = wcslen(module);
> +    wcsncpy(systemPath + systemDirLen, module, 

nit - white space

Comment 28

[Bas Schouten (:bas.schouten)]

Comment on attachment 726410 [details] [diff] [review]
Patch v3 - 9.3 feature level and refactoring

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

Discussed on IRC. We need to make sure when 10.0 is successful and we haven't tried 10.1 yet, we still do.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +506,5 @@
>  }
>  
> +#ifdef CAIRO_HAS_D2D_SURFACE
> +HRESULT
> +gfxWindowsPlatform::createDevice(nsRefPtr<IDXGIAdapter1> &adapter1,

Capital C in 'create'

@@ +621,5 @@
> +        featureLevel = D3D10_FEATURE_LEVEL_9_3;
> +        hr = createDevice(adapter1, featureLevel);
> +      }
> +#endif
> +      // If everythin failed, set this pref to 0 to skip fture attempts

nit: typo fture

Comment 29

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - windows helpers

Removed whitespace from the file and sysdirlen check fix.

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - 9.3 feature level and refactoring

More cleanup and implemented review comments.

Comment 31

[Bas Schouten (:bas.schouten)]

Comment on attachment 728018 [details] [diff] [review]
Patch v4 - 9.3 feature level and refactoring

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +576,5 @@
> +    // a createD3DDevice on D3D10_FEATURE_LEVEL_10_0.  We therefore store
> +    // the last used feature level to go direct to that.
> +    int featureLevelIndex = Preferences::GetInt(kFeatureLevelPref, 0);
> +    if (featureLevelIndex >= supportedFeatureLevelsCount || featureLevelIndex < 0)
> +      featureLevelIndex = 0;

nit: curly braces

@@ +606,5 @@
> +    HRESULT hr = E_FAIL;
> +    for (int i = featureLevelIndex; i < supportedFeatureLevelsCount; i++) {
> +      hr = CreateDevice(adapter1, i);
> +      // If it succeeded we found the first available feature level
> +      if (SUCCEEDED(hr))

nit: curly braces

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Gfx code

Implemented review nits, carrying forward r+.
Thanks for all the reviewing work on this!

Comment 33

[Bas Schouten (:bas.schouten)]

(In reply to Brian R. Bondy [:bbondy] from comment #32)
> Created attachment 728360 [details] [diff] [review]
> Patch v2 - Gfx code
> 
> Implemented review nits, carrying forward r+.
> Thanks for all the reviewing work on this!

It's nothing. Sorry I didn't get to it more quickly.

Comment 34

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/925db0605e39
https://hg.mozilla.org/integration/mozilla-inbound/rev/266694bb2f66

Comment 35

[Brian R. Bondy [:bbondy]]

Backed out due to bustage on m-i.  Strangely enough the try build worked fine here:
https://tbpl.mozilla.org/?tree=Try&rev=c34d647211b3

https://hg.mozilla.org/integration/mozilla-inbound/rev/4228409520c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b2a7410c44

Comment 36

[Brian R. Bondy [:bbondy]]

Bustage was due to b2g win emulator builds.  The try build did include it but it was being hidden (I think temporarily due to an unrelated bug) but visible if you manually append &showall=1 to the tbpl try URL.  

Pushed to try again and will re-push to m-i once that succeeds.

Comment 37

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf4cd763bc4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed97a9289b6

Comment 38

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/cf4cd763bc4c
https://hg.mozilla.org/mozilla-central/rev/0ed97a9289b6

Comment 41

[juan becerra [:juanb]]

Brian, what device is it? So I can stop by Asa's desk and borrow it for a moment to verify this.

Comment 42

[juan becerra [:juanb]]

Tested on 2013-03-29 using a nightly built from http://hg.mozilla.org/mozilla-central/rev/8693d1d4c86d
- Verified that Firefox opens in Metro mode on a Dell tablet with an Atom processor. Earlier builds from a few weeks ago did not launch in Metro mode. Today's nightly does.

Comment 43

[Brian R. Bondy [:bbondy]]

Yup I tried this on Asa's acer atom as well, I also had a couple people on IRC confirm that it's working now. Thanks Juan.  I'll be shipping back the device to you Asa Monday if you get this comment in your inbox.

Comment 44

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130812030209
Built from http://hg.mozilla.org/mozilla-central/rev/87c1796bc46c

WFM
Tested on windows 8 using latest nightly for iteration-11. 
- Verified that Firefox opens in Metro mode on a acer tablet with an Atom processor.

Comment 45

[Samvedana (:Samvedana)]

I was just opening metro in atom tablet and it didn't work for me.
I restarted it and tried to open again, but still didn't work. 

I have filed new bug904214 for it.

Please discard comment 44.