Closed Bug 844954 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect, P1)

x86_64
Windows 8
defect

Tracking

()

VERIFIED FIXED
mozilla22

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

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

Attachments

(2 files, 5 obsolete files)

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.
Attached patch Patch v1. (obsolete) — Splinter Review
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?
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
Attachment #717988 - Flags: review? → review?(bas)
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.
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.
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.
(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.
Blocks: metrov1it4
Whiteboard: feature=defect → feature=defect c=Opening_and_closing u=metro_firefox_user p=0
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)
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).
(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?
(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?
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).
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. (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.
Priority: -- → P1
Depends on: 847485
Depends on: metro-omtc
No longer blocks: metrov1it4
Blocks: metrov1it4
No longer blocks: metrov1backlog
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
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
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-
k no prob, will do.
Attached patch Patch v1 - windows helpers (obsolete) — Splinter Review
RAII HMODULE and helper from loading from system32
Attachment #725439 - Attachment is obsolete: true
Attachment #726407 - Flags: review?(jmathies)
Attachment #726410 - Flags: review?(bas)
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-
Removed whitespace from the file and sysdirlen check fix.
Attachment #726407 - Attachment is obsolete: true
Attachment #727868 - Flags: review+
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+
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.
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.
Flags: needinfo?(jbecerra)
QA Contact: jbecerra
Duplicate of this bug: 811207
Blocks: 848913
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
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)
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.
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.
No longer depends on: 904214
You need to log in before you can comment on or make changes to this bug.