Closed Bug 573229 Opened 14 years ago Closed 14 years ago

[D2D] Add logic to enable D2D by default

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- beta4+

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(2 files, 1 obsolete file)

Part 2: Enable D2D by default where a DX10 GPU is available v2
5.26 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
Part 1: Allow passing a device into cairo_d2d_create_device
8.25 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review 
We need logic to decide where to enable/disable D2D by default. We have decided for now it will be enabled by default on all systems with DirectX 10 GPUs and higher.
blocking2.0: --- → beta4+
This implements the aforementioned proposal.
Attachment #464976 - Flags: review?(jmuizelaar)
It would be get some ts numbers for this off of tryserver
Comment on attachment 464976 [details] [diff] [review]
Enable D2D by default where a DX10 GPU is available

>+    nsCOMPtr<nsIPrefBranch2> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+
>+#ifdef CAIRO_HAS_D2D_SURFACE
>+    NS_RegisterMemoryReporter(new D2DCacheReporter());

Do we want to register this regardless of whether we use d2d or not?

>+    mD2DDevice = NULL;
>+
>+    DXGICreateFactory1Func createDXGIFactory1 = (DXGICreateFactory1Func)
>+        GetProcAddress(LoadLibraryW(L"dxgi.dll"), "CreateDXGIFactory1");

I expect we should do something cheaper than LoadLibrary to check for XP before we try LoadLibrary()

> #ifdef CAIRO_HAS_DWRITE_FONT
>     nsresult rv;
>     PRBool useDirectWrite = PR_FALSE;
> 
>     rv = pref->GetBoolPref(
>         "gfx.font_rendering.directwrite.enabled", &useDirectWrite);
>     if (NS_FAILED(rv)) {
>         useDirectWrite = PR_FALSE;
>     }
>+
>+    if (mRenderMode == RENDER_DIRECT2D) {
>+      // We need DirectWrite if we're going to do Direct2D.
>+      useDirectWrite = PR_TRUE;
>+    }
>             
>     if (useDirectWrite) {
>         DWriteCreateFactoryFunc createDWriteFactory = (DWriteCreateFactoryFunc)
>             GetProcAddress(LoadLibraryW(L"dwrite.dll"), "DWriteCreateFactory");
> 
>         if (createDWriteFactory) {
>             /**
>              * I need a direct pointer to be able to cast to IUnknown**, I also
>@@ -209,19 +249,21 @@ gfxWindowsPlatform::gfxWindowsPlatform()
> #ifndef CAIRO_HAS_D2D_SURFACE
>                 return;
> #else
>-		mD2DDevice = cairo_d2d_create_device();
>                 if (!mD2DDevice) {
>-                    return;
>+                    mD2DDevice = cairo_d2d_create_device();
>+                    if (!mD2DDevice) {
>+                      return;
>+                    }
>                 }

I'm a little confused by this change.


>+#ifdef CAIRO_HAS_D2D_SURFACE
>+PRBool
>+gfxWindowsPlatform::IsD2DDefaultOn()

I found this name confusing. How about ShouldEnableD2D()?

It seems like the logic for whether to enable d2d is split between here when we get the DXGIFactory. Is there any reason it can't be one place?

It's also not clear why we need keep a reference around to the factory in the Platform class.
It seems to only be used for detection, so can't we drop the reference after we know whether to enable d2d?



>+{
>+    if (!mDXGIFactory) {
>+        return PR_FALSE;
>+    }
>+
>+    nsRefPtr<IDXGIAdapter1> adapter;
>+
>+    HRESULT hr;
>+    
>+    hr = mDXGIFactory->EnumAdapters1(0, getter_AddRefs(adapter));
>+
>+    if (FAILED(hr)) {
>+        return PR_FALSE;
>+    }
>+
>+    LARGE_INTEGER umd;
>+    hr = adapter->CheckInterfaceSupport(__uuidof(ID3D10Device), &umd);

Is this the part where we check for D3D10 hardware? Are you sure it fails on D3D9 hardware?

>+
>+    if (FAILED(hr)) {
>+        return PR_FALSE;
>+    }
>+
>+    return PR_TRUE;
>+}
>+#endif
Attachment #464976 - Flags: review?(jmuizelaar) → review-
(In reply to comment #3)
> Comment on attachment 464976 [details] [diff] [review]
> Enable D2D by default where a DX10 GPU is available
> 
> >+    nsCOMPtr<nsIPrefBranch2> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
> >+
> >+#ifdef CAIRO_HAS_D2D_SURFACE
> >+    NS_RegisterMemoryReporter(new D2DCacheReporter());
> 
> Do we want to register this regardless of whether we use d2d or not?
> 
> >+    mD2DDevice = NULL;
> >+
> >+    DXGICreateFactory1Func createDXGIFactory1 = (DXGICreateFactory1Func)
> >+        GetProcAddress(LoadLibraryW(L"dxgi.dll"), "CreateDXGIFactory1");
> 
> I expect we should do something cheaper than LoadLibrary to check for XP before
> we try LoadLibrary()

I don't think LoadLibraryW is particularly expensive if we're on XP. It just looks around for a library which it won't find. We can pre-check some other way though.

> 
> > #ifdef CAIRO_HAS_DWRITE_FONT
> >     nsresult rv;
> >     PRBool useDirectWrite = PR_FALSE;
> > 
> >     rv = pref->GetBoolPref(
> >         "gfx.font_rendering.directwrite.enabled", &useDirectWrite);
> >     if (NS_FAILED(rv)) {
> >         useDirectWrite = PR_FALSE;
> >     }
> >+
> >+    if (mRenderMode == RENDER_DIRECT2D) {
> >+      // We need DirectWrite if we're going to do Direct2D.
> >+      useDirectWrite = PR_TRUE;
> >+    }
> >             
> >     if (useDirectWrite) {
> >         DWriteCreateFactoryFunc createDWriteFactory = (DWriteCreateFactoryFunc)
> >             GetProcAddress(LoadLibraryW(L"dwrite.dll"), "DWriteCreateFactory");
> > 
> >         if (createDWriteFactory) {
> >             /**
> >              * I need a direct pointer to be able to cast to IUnknown**, I also
> >@@ -209,19 +249,21 @@ gfxWindowsPlatform::gfxWindowsPlatform()
> > #ifndef CAIRO_HAS_D2D_SURFACE
> >                 return;
> > #else
> >-		mD2DDevice = cairo_d2d_create_device();
> >                 if (!mD2DDevice) {
> >-                    return;
> >+                    mD2DDevice = cairo_d2d_create_device();
> >+                    if (!mD2DDevice) {
> >+                      return;
> >+                    }
> >                 }
> 
> I'm a little confused by this change.

Right now we can already have a D2D device here, if D2D is enabled by default, if it's not, we need to create it. If after creating it because it's preffed on manually, we still don't have one, the pref won't work.

> 
> 
> >+#ifdef CAIRO_HAS_D2D_SURFACE
> >+PRBool
> >+gfxWindowsPlatform::IsD2DDefaultOn()
> 
> I found this name confusing. How about ShouldEnableD2D()?

Sure.

> 
> It seems like the logic for whether to enable d2d is split between here when we
> get the DXGIFactory. Is there any reason it can't be one place?

We could move DWrite factory and device creation and such things into ShouldEnableD2D, but I think it's a bit obfuscated to initialize permanent gfxWindowsPlatform members from there. It adds a bit 'implicit' function to ShouldEnableD2D. How about if it's preffed on manually? We still need to see if DirectWrite was enabled after all.

We could make the Pref also get checked in ShouldEnableD2D. But then that wouldn't fix the problem of giving ShouldEnableD2D implicit initialization functions and it would require a little refactoring of how gfxWindowsPlatform handled RenderModes.

> 
> It's also not clear why we need keep a reference around to the factory in the
> Platform class.
> It seems to only be used for detection, so can't we drop the reference after we
> know whether to enable d2d?

The idea was going forward we might want to be able to switch on/off at runtime (fallback scenarios and such) and check the support at any random point. But we could do that for now, I suppose.

> >+    hr = mDXGIFactory->EnumAdapters1(0, getter_AddRefs(adapter));
> >+
> >+    if (FAILED(hr)) {
> >+        return PR_FALSE;
> >+    }
> >+
> >+    LARGE_INTEGER umd;
> >+    hr = adapter->CheckInterfaceSupport(__uuidof(ID3D10Device), &umd);
> 
> Is this the part where we check for D3D10 hardware? Are you sure it fails on
> D3D9 hardware?

The documentation certainly suggests this! But then again, I haven't got a DX9 adapter to double-check.
(In reply to comment #4)
> (In reply to comment #3)
> > I expect we should do something cheaper than LoadLibrary to check for XP before
> > we try LoadLibrary()
> 
> I don't think LoadLibraryW is particularly expensive if we're on XP. It just
> looks around for a library which it won't find. We can pre-check some other way
> though.

LoadLibrary presumably does some IO to see if the dll is around. We want to avoid any IO we
can during startup.

> > >+                    }
> > >                 }
> > 
> > I'm a little confused by this change.
> 
> Right now we can already have a D2D device here, if D2D is enabled by default,
> if it's not, we need to create it. If after creating it because it's preffed on
> manually, we still don't have one, the pref won't work.

I'm still confused by your explanation. Can we only create the d2d device when we are sure we're going to use it?


> > It seems like the logic for whether to enable d2d is split between here when we
> > get the DXGIFactory. Is there any reason it can't be one place?
> 
> We could move DWrite factory and device creation and such things into
> ShouldEnableD2D, but I think it's a bit obfuscated to initialize permanent
> gfxWindowsPlatform members from there. It adds a bit 'implicit' function to
> ShouldEnableD2D. How about if it's preffed on manually? We still need to see if
> DirectWrite was enabled after all.

The Dwrite factory and device creation shouldn't be in ShouldEnableD2D. ShouldEnableD2D should only contain the code needed to determine if we are going to try to use D2D.

> 
> We could make the Pref also get checked in ShouldEnableD2D. But then that
> wouldn't fix the problem of giving ShouldEnableD2D implicit initialization
> functions and it would require a little refactoring of how gfxWindowsPlatform
> handled RenderModes.

It seems reasonable to have the pref checked in ShouldEnableD2D otherwise the suggested
render mode could be passed in as a parameter. If it makes things better we could also not
use the render.mode pref for forcing d2d. I never liked it that much.
 
> > 
> > It's also not clear why we need keep a reference around to the factory in the
> > Platform class.
> > It seems to only be used for detection, so can't we drop the reference after we
> > know whether to enable d2d?
> 
> The idea was going forward we might want to be able to switch on/off at runtime
> (fallback scenarios and such) and check the support at any random point. But we
> could do that for now, I suppose.

Once we've decided to use D2D I don't see much value in trying to decide again later.

> > >+    }
> > >+
> > >+    LARGE_INTEGER umd;
> > >+    hr = adapter->CheckInterfaceSupport(__uuidof(ID3D10Device), &umd);
> > 
> > Is this the part where we check for D3D10 hardware? Are you sure it fails on
> > D3D9 hardware?
> 
> The documentation certainly suggests this! But then again, I haven't got a DX9
> adapter to double-check.

I tried this out on DX9 adapater and if (FAILED(adapter->CheckInterfaceSupport(__uuidof(ID3D10Device), &umd)) is not true.
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > Right now we can already have a D2D device here, if D2D is enabled by default,
> > if it's not, we need to create it. If after creating it because it's preffed on
> > manually, we still don't have one, the pref won't work.
> 
> I'm still confused by your explanation. Can we only create the d2d device when
> we are sure we're going to use it?

This would require combining checking the pref, if we have directwrite, and if we might want to switch it on by default, if we create a separate preference as you suggested later in your comment we can do it more easily.

> > We could move DWrite factory and device creation and such things into
> > ShouldEnableD2D, but I think it's a bit obfuscated to initialize permanent
> > gfxWindowsPlatform members from there. It adds a bit 'implicit' function to
> > ShouldEnableD2D. How about if it's preffed on manually? We still need to see if
> > DirectWrite was enabled after all.
> 
> The Dwrite factory and device creation shouldn't be in ShouldEnableD2D.
> ShouldEnableD2D should only contain the code needed to determine if we are
> going to try to use D2D.

But if DirectWrite works is needed to know if we're going to try to use D2D.

> 
> > 
> > We could make the Pref also get checked in ShouldEnableD2D. But then that
> > wouldn't fix the problem of giving ShouldEnableD2D implicit initialization
> > functions and it would require a little refactoring of how gfxWindowsPlatform
> > handled RenderModes.
> 
> It seems reasonable to have the pref checked in ShouldEnableD2D otherwise the
> suggested
> render mode could be passed in as a parameter. If it makes things better we
> could also not
> use the render.mode pref for forcing d2d. I never liked it that much.

That's how rendermode seems work though, doesn't mean we can't change that.

> > The idea was going forward we might want to be able to switch on/off at runtime
> > (fallback scenarios and such) and check the support at any random point. But we
> > could do that for now, I suppose.
> 
> Once we've decided to use D2D I don't see much value in trying to decide again
> later.

How about when we add fallback? A device might've been reset, we fallback to software, we might still want to try and go back to hardware (i.e. driver updates and such)

> > 
> > The documentation certainly suggests this! But then again, I haven't got a DX9
> > adapter to double-check.
> 
> I tried this out on DX9 adapater and if
> (FAILED(adapter->CheckInterfaceSupport(__uuidof(ID3D10Device), &umd)) is not
> true.

Okay, then I suppose the registry heuristics is the only thing we can do.
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > We could move DWrite factory and device creation and such things into
> > > ShouldEnableD2D, but I think it's a bit obfuscated to initialize permanent
> > > gfxWindowsPlatform members from there. It adds a bit 'implicit' function to
> > > ShouldEnableD2D. How about if it's preffed on manually? We still need to see if
> > > DirectWrite was enabled after all.
> > 
> > The Dwrite factory and device creation shouldn't be in ShouldEnableD2D.
> > ShouldEnableD2D should only contain the code needed to determine if we are
> > going to try to use D2D.
> 
> But if DirectWrite works is needed to know if we're going to try to use D2D.

Why? Can't we assume DirectWrite works if Direct2D does?

> > > The idea was going forward we might want to be able to switch on/off at runtime
> > > (fallback scenarios and such) and check the support at any random point. But we
> > > could do that for now, I suppose.
> > 
> > Once we've decided to use D2D I don't see much value in trying to decide again
> > later.
> 
> How about when we add fallback? A device might've been reset, we fallback to
> software, we might still want to try and go back to hardware (i.e. driver
> updates and such)

It seems like there are some different things here that are getting conflated.
1. Should we use Direct2D
2. Can we use Direct2D
3. Are we using Direct2D

I don't see any reason that "should we use Direct2D" would ever change.
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > We could move DWrite factory and device creation and such things into
> > > > ShouldEnableD2D, but I think it's a bit obfuscated to initialize permanent
> > > > gfxWindowsPlatform members from there. It adds a bit 'implicit' function to
> > > > ShouldEnableD2D. How about if it's preffed on manually? We still need to see if
> > > > DirectWrite was enabled after all.
> > > 
> > > The Dwrite factory and device creation shouldn't be in ShouldEnableD2D.
> > > ShouldEnableD2D should only contain the code needed to determine if we are
> > > going to try to use D2D.
> > 
> > But if DirectWrite works is needed to know if we're going to try to use D2D.
> 
> Why? Can't we assume DirectWrite works if Direct2D does?

In theory, yes, in practice if it turns out not to be true for some reason. It will turn into a crash, which I'd like to avoid.

> 
> > > > The idea was going forward we might want to be able to switch on/off at runtime
> > > > (fallback scenarios and such) and check the support at any random point. But we
> > > > could do that for now, I suppose.
> > > 
> > > Once we've decided to use D2D I don't see much value in trying to decide again
> > > later.
> > 
> > How about when we add fallback? A device might've been reset, we fallback to
> > software, we might still want to try and go back to hardware (i.e. driver
> > updates and such)
> 
> It seems like there are some different things here that are getting conflated.
> 1. Should we use Direct2D
> 2. Can we use Direct2D
> 3. Are we using Direct2D
> 
> I don't see any reason that "should we use Direct2D" would ever change.

I'll try and do a general restructuring in my patch once I've worked up a new working method that makes it all prettier.
Here's some information I dug up on how to detect d3d10 hardware from the registry.

http://msdn.microsoft.com/en-us/library/ff564147%28v=VS.85%29.aspx
if UserModeDriverName has two comma separated values it has d3d10
if it has three it has d3d11
if it has one it has d3d9

We should be able to find the right display device to look at using EnumDisplayDevices
InstalledDisplayDrivers might be a better key to check than UserModeDriverName
As per IRC discussion. No other method than render-mode used yet to force it on (for older D3D9 hardware) or switch it off (on hardware where it's enabled by default). If we want that we'll have to agree on some sort of pref.

Also, where 10.0 is available we need to make another attempt to load 10.1, since 10.0 is a little bit faster. This sadly means that where 10.1 is available we create a device twice, adding a pretty useless 10-20ms startup time for the second creation. But I suppose it's better than having to fail twice for users who don't get D2D.

I'm open to any restructuring suggestions. But I think this is the method that interferes with the current logic the least.
Attachment #464976 - Attachment is obsolete: true
Attachment #465566 - Flags: review?(jmuizelaar)
Comment on attachment 465566 [details] [diff] [review]
Part 2: Enable D2D by default where a DX10 GPU is available v2

Overall this looks better.

>+
>+            if (SUCCEEDED(hr)) {
>+                // We have 10.0, let's try 10.1.
>+                // XXX - This adds an additional 10-20ms for people who are
>+                // getting direct2d. We'd really like to do something more
>+                // clever.

Do you have any evidence that 10.1 makes a difference to Direct2D?

>+        }
>+    }
> #endif
>+
> #ifdef CAIRO_HAS_DWRITE_FONT
>     nsresult rv;
>     PRBool useDirectWrite = PR_FALSE;
> 
>     rv = pref->GetBoolPref(
>         "gfx.font_rendering.directwrite.enabled", &useDirectWrite);
>     if (NS_FAILED(rv)) {
>         useDirectWrite = PR_FALSE;
>     }
>             
>-    if (useDirectWrite) {
>+    if ((useDirectWrite && isVistaOrHigher) || mRenderMode == RENDER_DIRECT2D) {

This condition could perhaps use a bit of documentation. I had to think about it for a bit.
like why we're testing isVistaOrHigher.

>         DWriteCreateFactoryFunc createDWriteFactory = (DWriteCreateFactoryFunc)
>             GetProcAddress(LoadLibraryW(L"dwrite.dll"), "DWriteCreateFactory");
> 

> #else
>-		mD2DDevice = cairo_d2d_create_device();
>                 if (!mD2DDevice) {
>-                    return;
>+		    mD2DDevice = cairo_d2d_create_device();
>+                    if (!mD2DDevice) {
>+                        return;
>+                    }
>                 }

In what situation will this cairo_d2d_create_device() be called?
Comment on attachment 465567 [details] [diff] [review]
Part 1: Allow passing a device into cairo_d2d_create_device

Assuming there are no big changes in the code movement this looks fine.
Attachment #465567 - Flags: review?(jmuizelaar) → review+
(In reply to comment #13)
> Comment on attachment 465566 [details] [diff] [review]
> Part 2: Enable D2D by default where a DX10 GPU is available v2
> 
> Overall this looks better.
> 
> >+
> >+            if (SUCCEEDED(hr)) {
> >+                // We have 10.0, let's try 10.1.
> >+                // XXX - This adds an additional 10-20ms for people who are
> >+                // getting direct2d. We'd really like to do something more
> >+                // clever.
> 
> Do you have any evidence that 10.1 makes a difference to Direct2D?
Yes, I've seen different feature levels make significant differences. I'm not sure exactly about 10.0 vs 10.1 and the numbers though. One difference in 10.1 is native BGRA support I believe. But it's definitely worth investigating further for follow-up if it really is worth it.

> 
> >         DWriteCreateFactoryFunc createDWriteFactory = (DWriteCreateFactoryFunc)
> >             GetProcAddress(LoadLibraryW(L"dwrite.dll"), "DWriteCreateFactory");
> > 
> 
> > #else
> >-		mD2DDevice = cairo_d2d_create_device();
> >                 if (!mD2DDevice) {
> >-                    return;
> >+		    mD2DDevice = cairo_d2d_create_device();
> >+                    if (!mD2DDevice) {
> >+                        return;
> >+                    }
> >                 }
> 
> In what situation will this cairo_d2d_create_device() be called?

When someone has a DX9 card and prefs D2D on manually, our own device creations will fail. But cairo tries more feature levels. I left some tabs in there I see.
Comment on attachment 465566 [details] [diff] [review]
Part 2: Enable D2D by default where a DX10 GPU is available v2

Alright, this seems sane enough.
Attachment #465566 - Flags: review?(jmuizelaar) → review+
I turned this off because of the orange that it was causing.
This patch also makes it impossible to disable d2d on dx10 hardware
Depends on: 587349
Re-enabled. http://hg.mozilla.org/mozilla-central/rev/f2959b949445
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
so the 8/17 nightly will allow manual disable of D2D even with DX10/W7?

i prefer to keep it off until Bug 574976 is resolved.
Yes, just set mozilla.widget.render-mode to 0.
will this disable D2D in just the UI or content as well?

if it's both (which i would like), the setting is perhaps counter-intuitive? maybe split into 2 different prefs?
It's both.
re-open ?

seems to be disabled now,
http://hg.mozilla.org/mozilla-central/rev/cee59a962976
Depends on: 588427
Depends on: 589872
Depends on: 591592
Depends on: 731569
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: