Closed
Bug 1153609
Opened 9 years ago
Closed 9 years ago
Text is rendered white/transparency in rotated div
Categories
(Core :: Graphics, defect)
Tracking
()
VERIFIED
FIXED
mozilla41
People
(Reporter: del, Assigned: bas.schouten)
References
Details
(Keywords: regression, testcase, Whiteboard: [gfx-noted])
Attachments
(5 files, 1 obsolete file)
455.52 KB,
application/zip
|
Details | |
384 bytes,
text/html
|
Details | |
454 bytes,
text/html
|
Details | |
9.24 KB,
image/jpeg
|
Details | |
7.22 KB,
patch
|
jrmuizel
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
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
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox-esr31:
--- → unaffected
tracking-firefox38:
--- → ?
tracking-firefox39:
--- → ?
Component: General → Graphics
Ever confirmed: true
Flags: needinfo?(bas)
Keywords: regression
Comment 3•9 years ago
|
||
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
Comment 4•9 years ago
|
||
Comment 6•9 years ago
|
||
[Tracking Requested - why for this release]: Tracking requested for 39+
Updated•9 years ago
|
Comment 7•9 years ago
|
||
Comment 8•9 years ago
|
||
I think this should be fixed in 38 for next 38ESR.
Severity: normal → blocker
Keywords: testcase
Updated•9 years ago
|
Summary: Text is rendered white in rotated div → Text is rendered white/transparency in rotated div
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.
Comment 10•9 years ago
|
||
(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.
Comment 12•9 years ago
|
||
(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.
Updated•9 years ago
|
I would wontfix for 38.
Flags: needinfo?(milan)
Comment 15•9 years ago
|
||
OK, thanks!
Comment 16•9 years ago
|
||
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)
Updated•9 years ago
|
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.
Assignee | ||
Comment 21•9 years ago
|
||
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)
Assignee | ||
Comment 22•9 years ago
|
||
(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 24•9 years ago
|
||
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-
Assignee | ||
Comment 27•9 years ago
|
||
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 28•9 years ago
|
||
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 :)
Comment 31•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3ad4a417e095
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
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)
Assignee | ||
Comment 33•9 years ago
|
||
(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)
Assignee | ||
Comment 35•9 years ago
|
||
(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)
Comment 37•9 years ago
|
||
OK, sounds like you are agreeing that we should wontfix for 39. It is good to know the details, thanks!
Comment 38•9 years ago
|
||
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 40•9 years ago
|
||
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+
Comment 43•9 years ago
|
||
[Tracking Requested - why for this release]: It should be fixed to ESR38
Comment hidden (spam) |
Comment 45•9 years ago
|
||
See also Bug 1177039
I'm not sure this qualifies as a type of a problem we update ESR with?
Flags: needinfo?(milan)
Updated•9 years ago
|
QA Whiteboard: [good first verify]
Comment 48•9 years ago
|
||
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]
Comment 49•9 years ago
|
||
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.
Comment 50•9 years ago
|
||
(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.
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•