Closed Bug 1309277 Opened 8 years ago Closed 8 years ago

Disallow component alpha with D3D9

Categories

(Core :: Graphics, defect, P3)

Product:
Core
Component:
Graphics
Version:
49 Branch
Platform:
Unspecified
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: milan, Assigned: eflores)

References

Details

(Whiteboard: gfx-noted)

Attachments

(2 files)

1309277.patch
9.28 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1309277-verify-DONTLAND.patch
2.23 KB, patch
Details | Diff | Splinter Review 
See bug 1306465, bug 1306953, etc.  We seem to allow component alpha with D3D9, and there is no support for dual sources with D3D9.
Assignee: nobody → edwin
Priority: -- → P3
See Also: → 1306465
Whiteboard: gfx-noted
If we can't untangle this in 51, we need to make sure the patch from bug 1306465 makes it onto 51 (right now it's 50 - and hopefully 49) only.
Component alpha used to work on D3D9 (see bug 593604), has something changed since then?

We might just be broken on some hardware and need to check capabilities.
Run time check would be even better.  Is there a reason to believe that component alpha will work on D3D9 if it failed the run-time check for D3D11 component alpha on the same hardware?
To give more details; today, if we decide not to use D3D11, because of the failed component alpha run time check, we fall back to D3D9 (assuming the fallback preference is set), and proceed to use component alpha.  And we get rendering artifacts.
Those artifacts go away if we disable component alpha on those systems.
It is possible this only happens with E10S enabled.
(In reply to Milan Sreckovic [:milan] from comment #3)
> Run time check would be even better.  Is there a reason to believe that
> component alpha will work on D3D9 if it failed the run-time check for D3D11
> component alpha on the same hardware?

Probably not, but Bas might know more.

(In reply to Milan Sreckovic [:milan] from comment #4)
> To give more details; today, if we decide not to use D3D11, because of the
> failed component alpha run time check, we fall back to D3D9 (assuming the
> fallback preference is set), and proceed to use component alpha.  And we get
> rendering artifacts.
> Those artifacts go away if we disable component alpha on those systems.
> It is possible this only happens with E10S enabled.

Ok, but this isn't the only reason we can end up with d3d9, right?

I think we need a runtime check to determine if we can support component alpha, and then the question is what the right action to take is.

Our current state is somewhat confusing:

We require all Compositors to support component alpha, unless they report their type as LAYERS_BASIC (see ClientLayerManager::AreComponentAlphaLayersEnabled). D3D11 initialization fails if we don't have hardware support for dual source blending, so the obvious solution would be to do the same here and have us fall back to BasicCompositor.

When the backend is LAYERS_BASIC, FrameLayerBuilder does heroics to avoid creating component alpha layers in the first place. It aggressively flattens layers together (losing all of the perf benefits that layers normally give) to try get an opaque background behind all text.

The exception is when APZ is enabled, then we don't do this flattening. Flattening layers would disable async scrolling, and we figured that scroll performance should be the priority when we have the opportunity to do it really well (APZ). We instead just create layers that would be component alpha, but due to ClientLayerManager::AreComponentAlphaLayersEnabled returning false, they just render without subpixel-AA for the text.

For this case, blocking d3d9/11 and falling back to BasicCompositor is pointless, since we still won't get subpixel-AA text on these layers and we've thrown away hardware acceleration for nothing.

If APZ is enabled, then I think we want d3d9/11 initialization to succeed regardless of whether we can have component alpha. We then want to share if component alpha works or not back to layout (probably using TextureFactoryIdentifier to share it over IPC, and then factor this information into AreComponentAlphaLayersEnabled).

If APZ isn't enabled, then we get to pick which of the two approaches to take. Disabling d3d9 and falling back to Basic seems like the safe option for now.
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> (In reply to Milan Sreckovic [:milan] from comment #3)
> ...
> 
> Ok, but this isn't the only reason we can end up with d3d9, right?

Correct.  Until 49, however, there was no way to end up with D3D9 on Vista+ (with default preferences.)

> I think we need a runtime check to determine if we can support component
> alpha, and then the question is what the right action to take is.
> 
> Our current state is somewhat confusing:
> 
> We require all Compositors to support component alpha, unless they report
> their type as LAYERS_BASIC (see
> ClientLayerManager::AreComponentAlphaLayersEnabled). D3D11 initialization
> fails if we don't have hardware support for dual source blending, so the
> obvious solution would be to do the same here and have us fall back to
> BasicCompositor.

Yes, the problem is that we check for dual source blending and if it fails, we decide against D3D11.  If D3D9 doesn't support dual source blending, how would we do the same check there?  Wouldn't it always fail?

> 
> When the backend is LAYERS_BASIC, FrameLayerBuilder does heroics to avoid
> creating component alpha layers in the first place. It aggressively flattens
> layers together (losing all of the perf benefits that layers normally give)
> to try get an opaque background behind all text.
> 
> The exception is when APZ is enabled, then we don't do this flattening.
> Flattening layers would disable async scrolling, and we figured that scroll
> performance should be the priority when we have the opportunity to do it
> really well (APZ). We instead just create layers that would be component
> alpha, but due to ClientLayerManager::AreComponentAlphaLayersEnabled
> returning false, they just render without subpixel-AA for the text.

Except that ClientLayerManager::AreComponentAlphaLayersEnabled() would return true in this "we're falling to D3D9 because D3D11 failed" case.

> 
> For this case, blocking d3d9/11 and falling back to BasicCompositor is
> pointless, since we still won't get subpixel-AA text on these layers and
> we've thrown away hardware acceleration for nothing.
> 
> If APZ is enabled, then I think we want d3d9/11 initialization to succeed
> regardless of whether we can have component alpha. We then want to share if
> component alpha works or not back to layout (probably using
> TextureFactoryIdentifier to share it over IPC, and then factor this
> information into AreComponentAlphaLayersEnabled).
> 
> If APZ isn't enabled, then we get to pick which of the two approaches to
> take. Disabling d3d9 and falling back to Basic seems like the safe option
> for now.

Chances are, we only have this problem with APZ enabled.  This may not be the case, it's just based on bug 1237769, where the same kind of artifact shows up on XP with D3D9+E10S, but not without E10S.  Hard to try to test that theory, that machine seems to have died.

We may have to split this into "something to uplift to 51" and "something that can ride the trains", as the part where we communicate back to layers over IPC and get the correct result eventually feels like something that is better served riding the trains.

Let's keep this bug for the "upliftable patch".
(In reply to Milan Sreckovic [:milan] from comment #6)
> > We require all Compositors to support component alpha, unless they report
> > their type as LAYERS_BASIC (see
> > ClientLayerManager::AreComponentAlphaLayersEnabled). D3D11 initialization
> > fails if we don't have hardware support for dual source blending, so the
> > obvious solution would be to do the same here and have us fall back to
> > BasicCompositor.
> 
> Yes, the problem is that we check for dual source blending and if it fails,
> we decide against D3D11.  If D3D9 doesn't support dual source blending, how
> would we do the same check there?  Wouldn't it always fail?

How? I'm not sure exactly, I can try digging through MSDN to find what we can check.

If we failed d3d11 because of not having component alpha support then yes, it would always fail. But if there are other reasons we can fail to initialize d3d11 (not supporting higher than level 9.3?) then I would expect it to pass for those.


> 
> Except that ClientLayerManager::AreComponentAlphaLayersEnabled() would
> return true in this "we're falling to D3D9 because D3D11 failed" case.

Right, so we need to either disable d3d9 if we can't have component alpha layers, or teach AreComponentAlphaLayersEnabled about the d3d9-but-no-component-alpha case.

 
> Chances are, we only have this problem with APZ enabled.  This may not be
> the case, it's just based on bug 1237769, where the same kind of artifact
> shows up on XP with D3D9+E10S, but not without E10S.  Hard to try to test
> that theory, that machine seems to have died.
> 
> We may have to split this into "something to uplift to 51" and "something
> that can ride the trains", as the part where we communicate back to layers
> over IPC and get the correct result eventually feels like something that is
> better served riding the trains.
> 
> Let's keep this bug for the "upliftable patch".

Ok, so the simplest upliftable patch is a runtime check in the d3d9 initialization that fails if we don't have component alpha. That way we'll fall back to BasicCompositor, ClientLayerManager::AreComponentAlphaLayersEnabled will still be correct and things should all work.
I had a look into detecting this, what we're doing with d3d9 is much simpler than d3d11 (rendering twice rather than setting up a blend state with two inputs), so it seems like it could definitely succeed even if d3d11 failed.

It's also seems somewhat unlikely that any hardware would actually not support what we need, but it's worth checking.

https://msdn.microsoft.com/en-us/library/windows/desktop/bb172513(v=vs.85).aspx

Checking DestBlendCaps on that for D3DPBLENDCAPS_INVSRCCOLOR should tell us if we have what we need.
Once this is dealt with, we can flip the fallback D3D9 preference back (see bug 1306465)
Quick question: is this what would fix the minimize, maximize/restore, and close buttons not appearing if D3D9 and a non-default theme are active? I almost filed a bug for that, but it looks like this might be that bug. 

BTW, a workaround I am using so I can keep the advantages of D3D9 is an addon called Hide Caption Title Bar Plus, which has an option to use custom window control buttons.
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> It's also seems somewhat unlikely that any hardware would actually not
> support what we need, but it's worth checking.
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/bb172513(v=vs.85).
> aspx
> 
> Checking DestBlendCaps on that for D3DPBLENDCAPS_INVSRCCOLOR should tell us
> if we have what we need.

I don't know for sure what you're talking about, I'm afraid, but I do want to point out that my old D3D9 Nvidia card never could handle component alpha rendering, and I had to shut it off to avoid artifacts. I note that Microsoft now often avoids using RGB ClearType in many situations (white text is a big one) and no one seems to be complaining. So I don't think it necessarily needs to be there.
To be clear, I'm not suggesting that we can't get D3D9+E10S+Component alpha to work.

We have reproducible evidence of artifacts with D3D9+E10S, on some hardware, and those problems go away when we disable component alpha through a preference.
It is quite possible that the code is just mixed up in that it checks for the preference value alone, in some places, instead of some other run-time condition.
I have now confirmed that setting layers.componentalpha.enabled to false does not fix the problem with the window control (maximize, minimize, close) buttons not displaying when D3D9 fallback is enabled and a non-default them (or the dark development theme) is used. 

The problem is strange, in that they do appear when there is a closeable message at the bottom of the window (like telling me that I have crash data I haven't sent in.) But they flicker when any overlay is drawn. And they go away completely once the box is closed.
I also note that, in Windows 7, the fog on the title bar actually goes on top of the buttons.
Status: NEW → ASSIGNED
Attachment #8806343 - Flags: review?(matt.woodrow)
I don't have any affected machines available, but this patch should be enough to verify the fix. AFAICT all uses of component alpha have to go through EffectComponentAlpha.
If we have a try or a nightly with this fix, I can get (with a few day turnaround) get somebody to check if this fixes the problem.
Comment on attachment 8806343 [details] [diff] [review]
1309277.patch

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

Looks good! Lets hope it's the right check :)
Attachment #8806343 - Flags: review?(matt.woodrow) → review+
Pushed by eflores@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e844765627a
Allow D3D9 without component alphaa - r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/5e844765627a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: