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

VERIFIED FIXED in mozilla22

Status

()

P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla22
x86_64
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=Opening_and_closing u=metro_firefox_user p=2 status=verified )

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 717988 [details] [diff] [review]
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.
Assignee: nobody → netzen
Attachment #717988 - Flags: review?

Updated

6 years ago
Blocks: 831889
Summary: Devices that only support DX9 fail to startup in Metro mode → Defect - Devices that only support DX9 fail to startup in Metro mode
Whiteboard: feature=defect
(Assignee)

Updated

6 years ago
Attachment #717988 - Flags: review? → review?(bas)
(Assignee)

Comment 2

6 years ago
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.
(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.
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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

6 years ago
(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.

Updated

6 years ago
Blocks: 842108
Whiteboard: feature=defect → feature=defect c=Opening_and_closing u=metro_firefox_user p=0
(Assignee)

Comment 7

6 years ago
Setting moreinfo flag mainly for Comment 4.
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
(Assignee)

Comment 9

6 years ago
Can we land this in the interim so that these users can test? I could do it only when running as Metro?
(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.
(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?
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

6 years ago
(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.
So, the suggestion is to land this as is "now", but continue working on it before it reaches aurora?  Bas?

Comment 15

6 years ago
(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.
Bas, Brian, what's the best we can cook up for this week?
(Assignee)

Comment 17

6 years ago
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.
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
(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).
(Assignee)

Comment 20

6 years ago
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

6 years ago
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.

Updated

6 years ago
Priority: -- → P1
(Assignee)

Updated

6 years ago
Depends on: 847485
(Assignee)

Updated

6 years ago
Depends on: 792576
(Assignee)

Updated

6 years ago
No longer blocks: 842108

Updated

6 years ago
Blocks: 838081
(Assignee)

Updated

6 years ago
Blocks: 842108
No longer blocks: 838081
(Assignee)

Updated

6 years ago
Whiteboard: feature=defect c=Opening_and_closing u=metro_firefox_user p=0 → feature=defect c=Opening_and_closing u=metro_firefox_user p=2

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 22

6 years ago
Created attachment 725439 [details] [diff] [review]
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;
}
Attachment #717988 - Attachment is obsolete: true
Attachment #717988 - Flags: review?(bas)
Attachment #725439 - Flags: review?(bas)
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.
Attachment #725439 - Flags: review?(bas) → review-
(Assignee)

Comment 24

6 years ago
k no prob, will do.
(Assignee)

Comment 25

6 years ago
Created attachment 726407 [details] [diff] [review]
Patch v1 - windows helpers

RAII HMODULE and helper from loading from system32
Attachment #725439 - Attachment is obsolete: true
Attachment #726407 - Flags: review?(jmathies)
(Assignee)

Comment 26

6 years ago
Created attachment 726410 [details] [diff] [review]
Patch v3 - 9.3 feature level and refactoring
Attachment #726410 - Flags: review?(bas)

Comment 27

6 years ago
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
Attachment #726407 - Flags: review?(jmathies) → review+
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
Attachment #726410 - Flags: review?(bas) → review-
(Assignee)

Comment 29

6 years ago
Created attachment 727868 [details] [diff] [review]
Patch v2 - windows helpers

Removed whitespace from the file and sysdirlen check fix.
Attachment #726407 - Attachment is obsolete: true
Attachment #727868 - Flags: review+
(Assignee)

Comment 30

6 years ago
Created attachment 728018 [details] [diff] [review]
Patch v4 - 9.3 feature level and refactoring

More cleanup and implemented review comments.
Attachment #726410 - Attachment is obsolete: true
Attachment #728018 - Flags: review?(bas)
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
Attachment #728018 - Flags: review?(bas) → review+
(Assignee)

Comment 32

6 years ago
Created attachment 728360 [details] [diff] [review]
Patch v2 - Gfx code

Implemented review nits, carrying forward r+.
Thanks for all the reviewing work on this!
Attachment #728018 - Attachment is obsolete: true
Attachment #728360 - Flags: review+
(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.
(Assignee)

Comment 36

6 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/cf4cd763bc4c
https://hg.mozilla.org/mozilla-central/rev/0ed97a9289b6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Flags: needinfo?(jbecerra)
QA Contact: jbecerra
(Assignee)

Updated

6 years ago
Duplicate of this bug: 811207

Updated

6 years ago
Blocks: 848913
(Assignee)

Updated

6 years ago
Duplicate of this bug: 810176
Brian, what device is it? So I can stop by Asa's desk and borrow it for a moment to verify this.
Flags: needinfo?(jbecerra) → needinfo?(netzen)
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.
Status: RESOLVED → VERIFIED
Whiteboard: feature=defect c=Opening_and_closing u=metro_firefox_user p=2 → feature=defect c=Opening_and_closing u=metro_firefox_user p=2 status=verified
(Assignee)

Comment 43

6 years ago
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.
Flags: needinfo?(netzen)

Updated

5 years ago
No longer blocks: 844608
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.

Updated

5 years ago
Depends on: 904214
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.

Updated

5 years ago
No longer depends on: 904214
You need to log in before you can comment on or make changes to this bug.