Closed Bug 1153609 Opened 5 years ago Closed 4 years ago

Text is rendered white/transparency in rotated div

Categories

(Core :: Graphics, defect, blocker)

37 Branch
x86_64
Windows 7
defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox37 --- wontfix
firefox38 + wontfix
firefox39 + wontfix
firefox40 + verified
firefox41 --- verified
firefox-esr31 --- unaffected
firefox-esr38 - wontfix

People

(Reporter: del, Assigned: bas.schouten)

References

(Depends on 1 open bug)

Details

(Keywords: regression, testcase, Whiteboard: [gfx-noted])

Attachments

(5 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150402191859

Steps to reproduce:

Open http://www.cego-online.de/startseite.html


Actual results:

Text renders white in tilted divs


Expected results:

Should render black. This is the case in IE und Chrome on Windows and used to be the case also in Firefox for the last two years. Tested on Windows7 and 8.
the problem appears depended on browser width.

[Tracking Requested - why for this release]: regressed since firefox37

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=165c3fd176ec&tochange=d954ed24e795

triggered by: Bug 902952
Blocks: 902952
Status: UNCONFIRMED → NEW
Component: General → Graphics
Ever confirmed: true
Flags: needinfo?(bas)
Keywords: regression
[Tracking Requested - why for this release]:
Regression window with force enable d2d1.1:

user_pref("gfx.canvas.azure.backends", "direct2d1.1,direct2d,skia,cairo");
user_pref("gfx.content.azure.backends", "direct2d1.1,direct2d,cairo");
user_pref("gfx.direct2d.use1_1", true);

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3749c4d4e76b&tochange=8e28464849fa

Via local build:
Last good: 20e97c8496e4
First bad: 7b9201731195

triggered by: 7b9201731195	Matt Woodrow — Bug 1046550 - Part 3: Use Direct2D 1.1 when preffed on. r=bas
Blocks: 1046550
Matt, Bas, can you help here? Thanks
Flags: needinfo?(matt.woodrow)
[Tracking Requested - why for this release]:

Tracking requested for 39+
Attached file another testcase
I think this should be fixed in 38 for next 38ESR.
Severity: normal → blocker
Keywords: testcase
Summary: Text is rendered white in rotated div → Text is rendered white/transparency in rotated div
Whiteboard: [gfx-noted]
The patch that is the target of the regression hunt had a problem in it that eventually got resolved in bug 1145585, it may be worth doing a build with that fix included, and see if the problem in this bug still shows up.
(In reply to Milan Sreckovic [:milan] from comment #9)
> The patch that is the target of the regression hunt had a problem in it that
> eventually got resolved in bug 1145585, it may be worth doing a build with
> that fix included, and see if the problem in this bug still shows up.

I think that you are talking about different bug.
Nope, same bug.  Bug 1046550, part 3, mentioned in comment 3 had a bug in it, and it got resolved in bug 1145585.  The descriptions of the symptoms is different, but since we identified that particular patch as a culprit, I thought it worth while to check if it's a culprit even when correct.
(In reply to Milan Sreckovic [:milan] from comment #11)
> Nope, same bug.  Bug 1046550, part 3, mentioned in comment 3 had a bug in
> it, and it got resolved in bug 1145585.  The descriptions of the symptoms is
> different, but since we identified that particular patch as a culprit, I
> thought it worth while to check if it's a culprit even when correct.


Bug 1145585 was landing in Nightly 39.0a1. 
And I can still reproduce this on Nihjtly40.0a1... It means that Bug 1145585 did not fix this.
Milan, what are you planning for this bug? Thanks
Flags: needinfo?(milan)
I would wontfix for 38.
Flags: needinfo?(milan)
Milan is this something that your team is still intending to work on for 39? It still doesn't have an owner. Thanks!
Flags: needinfo?(milan)
It depends on how a couple of larger problems with 39 go; once we resolve them, we can get on this one.
This is a result of us taking the not-rectilinear-matrix path in DrawTargetD2D1::PushClipRect, when there is alpha.  If the opacity is 1 it works.  If we skip the mTransform.IsRectilinear() test in PushClipRect and take the default route, it works.  It also works for D2D.
Bas, can't imagine this would take you a long time to sort out.
Assignee: nobody → bas
Flags: needinfo?(milan)
Flags: needinfo?(bas)
Actually, lets keep needinfo on Bas to tag this one as important to sort out quickly.
Flags: needinfo?(bas)
At one point Bas thought this could be in layout, and not passing the correct information that we use for subpixel AA, which we're dealing with differently with D2D 1.1.

Bas, probably a different bug, but given that it's text and D2D 1.1, wonder if bug 1160070 is a related problem.
This patch lets us repush all layers with their backgrounds copied when drawing subpixel AA'ed text to a transparent surface. This can be wasteful since the last layer that was pushed could already have had its pixels where the glyphs will be drawn made opaque, however we have no way of knowing this so we have to always repush the layers.

This might be a small performance hit for drawing some things. We'll have to see if that's acceptable or we need to come up with something else.

Note that there's still a small layout bug here, when you rotate the div enough, layout will still tell us to use subpixel AA, even if the bounds of the glyphs in device space no longer only cover the opaque different. The artifacts of this are minimal and not too disturbing, but they can be seen if you rotate the div a little more (say 4 degrees).

The contract of permit subpixel AA is that is should only be set to true when the rectangular bounds of the glyphs are filled with opaque pixels.
Flags: needinfo?(bas)
Attachment #8611626 - Flags: review?(jmuizelaar)
(In reply to Milan Sreckovic [:milan] from comment #20)
> At one point Bas thought this could be in layout, and not passing the
> correct information that we use for subpixel AA, which we're dealing with
> differently with D2D 1.1.
> 
> Bas, probably a different bug, but given that it's text and D2D 1.1, wonder
> if bug 1160070 is a related problem.

I somewhat doubt it since I have never been able to reproduce that I think.
Comment on attachment 8611626 [details] [diff] [review]
Push opaque layers when drawing subpixel AA'ed text to a transparent surface with complex clips pushed

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

Yuck.
Attachment #8611626 - Flags: review?(jmuizelaar) → review+
This is what I get with the 45 degree red text example and patch in attachment 8611626 [details] [diff] [review].
Comment on attachment 8611626 [details] [diff] [review]
Push opaque layers when drawing subpixel AA'ed text to a transparent surface with complex clips pushed

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

Feedback- based on the artifacts with this patch.
Attachment #8611626 - Flags: feedback-
This patch fixes glitches with non-rectilinear transforms by preceding the clips with a top level clip to the tight, non-rectangular glyph bounds in device space.
Attachment #8611626 - Attachment is obsolete: true
Attachment #8612581 - Flags: review?(jmuizelaar)
Comment on attachment 8612581 [details] [diff] [review]
Push opaque layers when drawing subpixel AA'ed text to a transparent surface with complex clips pushed v2

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

This is even worse...
Attachment #8612581 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #28)
> ...
> This is even worse...

But the results are good :)
https://hg.mozilla.org/mozilla-central/rev/3ad4a417e095
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Milan, can you comment on the stability of this fix? Should we won't fix it for FF39?
Flags: needinfo?(milan)
(In reply to Ritu Kothari (:ritu) from comment #32)
> Milan, can you comment on the stability of this fix? Should we won't fix it
> for FF39?

We could uplift to 40, I wouldn't uplift to 39 myself.
I'm with Bas on this; if we can uplift to 40 this week, the risk/reward balance is OK.

Before we do, and given the issues we've had in the past, and the fact that we'd be uplifting, Bas, I'd like a follow up patch that actually checks the results of the D2D calls and does the appropriate thing.  For example, we ignore the status of ID2D1DeviceContext::GetGlyphRunWorldBounds calls, nor PushLayer, we assume path->Open succeeds, etc.  If we "know" these can never fail, let's put in MOZ_RELEASE_ASSERT or something like that, which will give us a quick indication if we're wrong in those assumptions.  Or just protect against it.  If we need a separate bug for it, let's do that.
Flags: needinfo?(milan)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bas)
(In reply to Milan Sreckovic [:milan] from comment #34)
> I'm with Bas on this; if we can uplift to 40 this week, the risk/reward
> balance is OK.
> 
> Before we do, and given the issues we've had in the past, and the fact that
> we'd be uplifting, Bas, I'd like a follow up patch that actually checks the
> results of the D2D calls and does the appropriate thing.  For example, we
> ignore the status of ID2D1DeviceContext::GetGlyphRunWorldBounds calls, nor
> PushLayer, we assume path->Open succeeds, etc.  If we "know" these can never
> fail, let's put in MOZ_RELEASE_ASSERT or something like that, which will
> give us a quick indication if we're wrong in those assumptions.  Or just
> protect against it.  If we need a separate bug for it, let's do that.

PushLayer actually can't return an error value (it's void), so for that one it doesn't matter, for path->Open we'd crash right away on a null deref on the sink so we're essentially already 'release asserting' and an additional check would be somewhat wasteful.

For GetGlyphRunWorldBounds I agree that a release check is a fair idea.
Flags: needinfo?(bas) → needinfo?(jmuizelaar)
Agree.
Flags: needinfo?(jmuizelaar)
OK, sounds like you are agreeing that we should wontfix for 39. It is good to know the details, thanks!
Milan, could you fill the uplift request to 40? Thanks. We will take it.
Flags: needinfo?(milan)
Comment on attachment 8612581 [details] [diff] [review]
Push opaque layers when drawing subpixel AA'ed text to a transparent surface with complex clips pushed v2

Approval Request Comment
[Feature/regressing bug #]: D2D 1.1
[User impact if declined]: Disappearing text in rotated divs with transparency.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: There is new code; on the other hand, the majority is triggered in the special case above that would fail.
[String/UUID change made/needed]:
Flags: needinfo?(milan)
Attachment #8612581 - Flags: approval-mozilla-aurora?
Comment on attachment 8612581 [details] [diff] [review]
Push opaque layers when drawing subpixel AA'ed text to a transparent surface with complex clips pushed v2

Let's try with that in aurora. As usual with graphic changes, the best way to get feedback is to push the feature to as many users as possible.
Attachment #8612581 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1153530
[Tracking Requested - why for this release]: It should be fixed to ESR38
Flags: needinfo?(milan)
See also Bug 1177039
Blocks: 1177039
I'm not sure this qualifies as a type of a problem we update ESR with?
Flags: needinfo?(milan)
Duplicate of this bug: 1177039
Depends on: 1180379
QA Whiteboard: [good first verify]
I have reproduced this bug with Firefox nightly 40.0a1(20150413030203)on Windows 7 64 Bit. 
UA: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0

The fix is verified for latest Beta 40.0b3(20150709163524) and latest Aurora 41.0a2(20150711004006)

UA: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
UA: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0

Updating the status to verified fixed!
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][testday-20150710]
Setting the status flags to verified based on the previous comment, and a quick check on my Windows 7 machine with Firefox 40 Beta 2.
(In reply to Milan Sreckovic [:milan] (PTO through 7/28) from comment #46)
> I'm not sure this qualifies as a type of a problem we update ESR with?

We do take non stability fixes like this in ESR but usually wait for feedback on the issue from ESR users themselves. I don't think we should take the fix unless we need to. Marking ESR38- until we know otherwise.
Depends on: 1212409
You need to log in before you can comment on or make changes to this bug.