Closed Bug 1102896 Opened 5 years ago Closed 5 years ago

Rendering-issues in Customization-panel with Direct2D 1.1

Categories

(Core :: Layout, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- disabled
firefox37 + fixed
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: elbart, Assigned: tnikkel, NeedInfo)

References

Details

Attachments

(1 file)

While in Customization-mode, scrolling the right panel while the "New e10s Window"/"New non-10s Window"-button is in the right-panel causes the button-labels to not be rendered for a few seconds.

Only happens when the right pane is scrollable.

Does not happen when gfx.direct2d.use1_1 is set to false.

Regression-window:
Last good revision: bf5fcc0c4b27 (2014-09-15)
First bad revision: 55b46de164d8 (2014-09-16)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bf5fcc0c4b27&tocha
nge=55b46de164d8

When all the D2D1.1-stuff landed. :(

Using a debug-build (2014-11-21-mozilla-central-debug), this comes up when scrolling:

[GFX3]: Invalid draw target type specified.
[GFX3]: Invalid draw target type specified.
[Parent 3464] WARNING: Failed to allocate a TextureClient, falling back to Buffe
rTextureClient.: file c:\builds\moz2_slave\m-cen-w32-d-000000000000000000\build\
src\gfx\layers\client/TextureClient.cpp, line 389
[GFX3]: Invalid draw target type specified.
Blocks: 902952
Picking up any button before the e10s-button in the Customization-pane also disables the rendering of its label and the labels of the following items in the customization pane for a few seconds.
Maybe it's a related problem.
The scaleY-transform is at fault:
https://hg.mozilla.org/mozilla-central/rev/4ce5da6bd6d6#l6.17
I can reproduce this. It's not very high impact, but it should be fixed. Note this is unrelated to the debug output mentioned in the initial post.
Assignee: nobody → bas
I know a little bit about the cause here, however to diagnose the deeper issue is being really hard considering the complexity of the customization menu, is there someone who perhaps might be able to produce a reduced testcase for this?
When this problem occurs we hit the assertion here: https://hg.mozilla.org/mozilla-central/file/2a193b7f395c/layout/base/FrameLayerBuilder.cpp#l3709

Roc told me this can cause a variety of problems, and I suspect it causes this invalid attempt to draw subpixel-AA'ed text on transparent pixels too.
Flags: needinfo?(roc)
We need to get some movement on this. This is a layout bug but it's causing a problem with Direct2D 1.1. We will have to decide on what our best course of action here is.
Timothy, roc is not going to have time to look at this on the short term, any chance you could have a look?
Flags: needinfo?(tnikkel)
I can reproduce, but haven't had a chance to look any deeper yet.
Status: UNCONFIRMED → NEW
Ever confirmed: true
When I reproduce the problem in a debug build I do not get the assertion from comment 5. And I've seen that assertion firing before with no observable problems. Bas, do you still think this is a layout bug given this?
Flags: needinfo?(bas)
(In reply to Timothy Nikkel (:tn) from comment #9)
> When I reproduce the problem in a debug build I do not get the assertion
> from comment 5. And I've seen that assertion firing before with no
> observable problems. Bas, do you still think this is a layout bug given this?

I can't get this to occur without the assertion firing. Having said that, I can confirm this is a layout bug, because text here is being drawn over pixels which are transparent, in which case permit subpixel AA should not be set to true.

All of those things happen in layout land :-).
Flags: needinfo?(bas)
Milan, it looks like this might hold up D2D 1.1 so we may need to more actively find someone who understands this code and can look at it. Or point me at evidence that my diagnosis is somehow wrong, but I can't seem to find any.
Flags: needinfo?(milan)
Assignee: bas → nobody
Component: Graphics → Layout
Flags: needinfo?(milan)
Tracking as this may be required to enabled D2D 1.1, which is currently enabled in 37+.

Jet - Can you please help find an owner?
Attached patch patchSplinter Review
The problem is that when we draw an inactive layer we clobber the subpixel AA setting on the draw target, so any later items will paint with the wrong setting.
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel)
Flags: needinfo?(roc)
Attachment #8568472 - Flags: review?(matt.woodrow)
No longer blocks: 1134293
Attachment #8568472 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/9d4ffa427cf8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Is it possible to get a test case for this?
Will this patch be uplifted to aurora and beta? It also fixes bug 1090554.
Uplift perhaps? :-)
Flags: needinfo?(tnikkel)
Comment on attachment 8568472 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 902952
[User impact if declined]: text for icons in customization mode disappears, and likely in some other cases too
[Describe test coverage new/current, TreeHerder]: nope
[Risks and why]: sets the proper AA mode, should be pretty low risk
[String/UUID change made/needed]: none
Flags: needinfo?(tnikkel)
Attachment #8568472 - Flags: approval-mozilla-beta?
Attachment #8568472 - Flags: approval-mozilla-aurora?
Bas, why would the problem go away when D2D 1.1 is disabled?  We had a number of things "regress" when we enabled D2D 1.1, and at least some (e.g., this one) turned out not to be directly related to that; just wondering if there is a lesson here to help us see which of the other D2D 1.1 tagged issues are not directly D2D 1.1 issues...
Flags: needinfo?(bas)
(In reply to Timothy Nikkel (:tn) from comment #19)
> [Describe test coverage new/current, TreeHerder]: nope

Given that we have no test coverage, can you please verify that the fix is good on m-c before uplift?
Flags: needinfo?(tnikkel)
I checked on my machine on m-c and it was fixed, but that doesn't really add much new information since I also checked that it fixed the bug when I made the patch.
Flags: needinfo?(tnikkel)
(In reply to Milan Sreckovic [:milan] from comment #20)
> Bas, why would the problem go away when D2D 1.1 is disabled?  We had a
> number of things "regress" when we enabled D2D 1.1, and at least some (e.g.,
> this one) turned out not to be directly related to that; just wondering if
> there is a lesson here to help us see which of the other D2D 1.1 tagged
> issues are not directly D2D 1.1 issues...

Bas would know better but there appears to be a path in the d2d 1 code for drawing subpixel AA text over transparent pixels that there isn't in the corresponding d2d 1.1 code. Specifically DrawTargetD2D::FillGlyphs is mostly the same as DrawTargetD2D1::FillGlyphs except that DrawTargetD2D::FillGlyphs calls FillGlyphsManual specifically for subpixel AA text on transparent surfaces. Why this is I cannot speak to.
(In reply to Timothy Nikkel (:tn) from comment #22)
> I checked on my machine on m-c and it was fixed, but that doesn't really add
> much new information since I also checked that it fixed the bug when I made
> the patch.

Thanks Timothy.

Elbart - As the reporter, if you have time, can you please confirm that you can no longer reproduce this issue in the latest Nightly build?
Flags: needinfo?(elbart)
Comment on attachment 8568472 [details] [diff] [review]
patch

The fix has been testing on m-c. Let's get this into Beta 2. Beta+ Aurora+
Attachment #8568472 - Flags: approval-mozilla-beta?
Attachment #8568472 - Flags: approval-mozilla-beta+
Attachment #8568472 - Flags: approval-mozilla-aurora?
Attachment #8568472 - Flags: approval-mozilla-aurora+
(In reply to Milan Sreckovic [:milan] from comment #20)
> Bas, why would the problem go away when D2D 1.1 is disabled?  We had a
> number of things "regress" when we enabled D2D 1.1, and at least some (e.g.,
> this one) turned out not to be directly related to that; just wondering if
> there is a lesson here to help us see which of the other D2D 1.1 tagged
> issues are not directly D2D 1.1 issues...

Because of differences in the subpixel AA code, D2D 1.0 would give other (more subtle) artifacts than D2D 1.1. If you see a black box where you're expecting text, it's likely related to this.
Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.