Closed
Bug 518172
Opened 15 years ago
Closed 14 years ago
-moz-transform doesn't rotate individual characters in this testcase, except when text is partially repainted
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta3+ |
People
(Reporter: dholbert, Assigned: jrmuizel)
References
(Depends on 1 open bug)
Details
(Keywords: regression, testcase)
Attachments
(5 files)
STEPS TO REPRODUCE:
1. Load testcase.
2. Then try selecting a sub-section of the string "rotated text".
2. Ctrl+A to select all.
EXPECTED RESULTS:
The string "rotated text" (including each of its characters) should be fully rotated 90 degrees, the whole time.
ACTUAL RESULTS:
- At pageload, the *string as a whole* is rotated, but the individual characters are not.
- Selecting portions of the string (step 2) makes the whole string flip to the correct orientation, though the un-rotated "r" at the top of the string doesn't get repainted (because it sticks out of the repainted box)
- Select-all re-breaks everything -- i.e. it puts the characters back in their un-rotated orientation, as they were at pageload.
BROKEN:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090922 Minefield/3.7a1pre
WORKING:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
So, this is a regression w.r.t. the 1.9.1 branch.
(Note: I first noticed this bug on bug 467297's testcase)
Reporter | ||
Comment 1•15 years ago
|
||
Requesting blocking, since this is a layout regression from 1.9.1.
Flags: blocking1.9.2?
Keywords: regressionwindow-wanted
Reporter | ||
Comment 2•15 years ago
|
||
Here's a semi-reference case -- I just removed the 'a' character in the second table cell. This shows expected behavior.
That is weird. Shouldn't be hard to track down there.
Reporter | ||
Comment 4•15 years ago
|
||
Looks like a very recent regression:
WORKS:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090914 Minefield/3.7a1pre
BROKEN:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090915 Minefield/3.7a1pre
Regression pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=912c6ae3b70c&tochange=cdcf1519121f
I'm blaming the cairo update, bug 515192 / changeset 7a9262f6dbc0.
Blocks: 515192
Reporter | ||
Updated•15 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Comment 5•15 years ago
|
||
(In reply to comment #1)
> Requesting blocking, since this is a layout regression from 1.9.1.
Switching my earlier blocking request from 1.9.2 to 1.9.3, since this doesn't actually affect 1.9.2. (The regressing bug didn't land on that branch, and I've verified that this doesn't affect the latest 1.9.2 nightly.)
blocking2.0: --- → ?
Flags: blocking1.9.2?
Comment 6•15 years ago
|
||
If it's a regression from a cairo update, we should probably move it to the Graphics component.
Component: Layout → Graphics
QA Contact: layout → thebes
blocking2.0: ? → alpha1
Comment 7•15 years ago
|
||
fwiw, -moz-scale is also broken for text:
- text is not scaled (but as above, when selected it is suddenly scaled)
- bad kerning & letter-spacing between characters (some have the spacing as if it was scaled, some not)
(and I see that on both OS X and Linux)
OS: Linux → All
Updated•15 years ago
|
Summary: -moz-transform doesn't rotate individual characters in this testcase, except when a portion is selected → -moz-transform doesn't rotate individual characters in this testcase, except when text is partially repainted
Comment 9•15 years ago
|
||
I landed reftests for this bug:
http://hg.mozilla.org/mozilla-central/rev/354735881fa8
with one set (without the extra character required to cause the bug) passing and the other set failing.
So if this gets magically fixed by another cairo upgrade, somebody will know to come update the bug. And also so we have tests for it.
Comment 11•15 years ago
|
||
Changing blocking1.9.3: from alpha1 to beta1 per conversation with Joe Drew and Jeff Muizelaar. Still good to fix sooner rather than later, though.
blocking2.0: alpha1 → beta1
Comment 12•15 years ago
|
||
Bug 551405 (mozTextAlongPath stops rotating along mozPathText) is the same visual glitch but occurs in Vladimir Vukićević/roc's HTML canvas demo rather than CSS; is it a duplicate of this bug?
Reporter | ||
Comment 13•15 years ago
|
||
Just noted on Bug 551405 that these two bugs may have the same root cause, but for now, it's worth leaving them separate since the paths to reproducing them (the testcases) are so different.
Updated•15 years ago
|
Assignee: nobody → jmuizelaar
Assignee | ||
Comment 14•15 years ago
|
||
This looks like the ctm isn't getting propagated to the scaled font. I'm not sure who's responsibility this is.
Comment 15•15 years ago
|
||
Using an SVG test case for character rotation, it fails when displayed normally but works when rendered through drawWindow (In my case, a tab preview extension)
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-text-07-t.html
The testcase attached to this bug is no different.
Comment 16•15 years ago
|
||
The comment 15 testcase also works if you display the frame in a tab.
Comment 17•14 years ago
|
||
This blocks _a_ beta, but not necessarily beta 1.
blocking2.0: beta1+ → beta2+
Comment 18•14 years ago
|
||
Comment 19•14 years ago
|
||
Comment 20•14 years ago
|
||
Another test case:
http://disruptive-innovations.com/zoo/rotator/rotator.xul
Comment 22•14 years ago
|
||
I'd note that playing with various examples of animation of transforms shows that this seems to happen only for some transformations (those at right angles) -- a bunch of the animations in http://dbaron.org/css/test/2010/transition-negative-determinant (such as the middle two in the top row) animate correctly through the animation, but then flip to the incorrect result after the animation completes. Some of the ones in the bottom row have the opposite behavior, though, so maybe that explanation doesn't particularly make sense.
Updated•14 years ago
|
blocking2.0: beta2+ → final+
Comment 23•14 years ago
|
||
We've hit this bug on the "What's new" page for Beta 2. (bug 578138)
Comment 24•14 years ago
|
||
The bug goes away if I comment out these two lines in cairo_set_scaled_font (which were added in the regressing changeset):
if (was_previous)
→·······cr->gstate->scaled_font = cairo_scaled_font_reference ((cairo_scaled_font_t *) scaled_font);
Comment 25•14 years ago
|
||
There are various obvious patches that make the bug go away, but I'm really having trouble understanding what the code is *supposed* to be doing -- what invariants previous_scaled_font and scaled_font should maintain, whether _cairo_gstate_unset_scaled_font is what's supposed to be called when those invariants change, etc.
We should fix this before final. It's a pretty bad regression.
e.g. for beta3.
blocking2.0: final+ → ?
Comment 28•14 years ago
|
||
(And by "various obvious patches" I really mean "various patches that end up disabling the previous_scaled_font cache entirely".)
Updated•14 years ago
|
blocking2.0: ? → beta3+
Assignee | ||
Comment 30•14 years ago
|
||
The upstream commit:
commit 7a023a62f7517ad0d54f4d59c99909fadcc05e82
Author: Nicolaus L Helper <nlhepler@gmail.com>
Date: Thu Jun 17 08:56:30 2010 +0100
ft,fc,xlib: LCD filtering patch.
This adds internal API to retrieve the LCD filtering parameters from
fontconfig, or as set on the Screen, and feed them to FreeType when
rendering the glyph.
References:
Bug 10301 - LCD filtering patch
https://bugs.freedesktop.org/show_bug.cgi?id=10301
Tested-by: Brandon Wright <bearoso@gmail.com>
Forward-ported-by: Robert Hooker <sarvatt@gmail.cm>
ickle: The API is clearly not ready for public consumption, the enum are
poorly named, however this stands by itself as enabling system wide
properties.
seems to fix the problem. But it's not at all clear why. The best theory I have so far is that it interacts with font hashing.
I'll try to figure out what broke it in the first place next.
Assignee | ||
Comment 31•14 years ago
|
||
The commit that caused the regression is:
commit dc083ab30a5b781e205354c525ee054982364abd
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Wed May 27 14:54:34 2009 +0100
[cairo] Track the MRU scaled font
When observing applications two patterns emerge. The first is due to
Pango, which wraps each glyph run within a context save/restore. This
causes the scaled font to be evicted after every run and reloaded on the
next. This is caught by the MRU slot on the cairo_scaled_font_map and
prevents a relatively costly traversal of the hash table and holdovers.
The second pattern is by applications that directly manage the rendering
of their own glyphs. The prime example of this is gnome-terminal/vte. Here
the application frequently alternates between a few scaled fonts - which
requires a hash table retrieval every time.
By introducing a MRU slot on the gstate we are able to directly recover
the scaled font around 90% of the time.
Of 110,000 set-scaled-fonts:
4,000 were setting the current font
96,000 were setting to the previous font
2,500 were recovered from the MRU on the cairo_scaled_font_map
7,500 needed a hash retrieval
which compares to ~106,000 hash lookups without the additional MRU slot on
the gstate.
This translates to an elapsed time saving of ~5% when replaying a
gnome-terminal trace using the drm backend.
Assignee | ||
Comment 32•14 years ago
|
||
We seem to be calling with cairo_set_scaled_font() with a font that doesn't have a ctm that matches the context. Which is wrong according to the documentation:
"Except for some translation, the current CTM of the cairo_t should be the same as that of the cairo_scaled_font_t, which can be accessed using cairo_scaled_font_get_ctm()."
This used to work because changing the ctm would cause the scaled_font to be NULLed out and we would recreate the scaled font as needed. When the MRU cache was added we end up actually using the scaled font with the wrong CTM.
As an aside, does anyone know the goal of the whole scaled_font/font distinction? CoreGraphics and DWrite don't seem to have a similar concept.
Assignee | ||
Comment 33•14 years ago
|
||
We seem to always create scaled_fonts with their ctm set to identity. That seems wrong...
The setting up of scaled_fonts is actually platform-specific code --- see gfxMacFonts, gfxPangoFonts/gfxFT2FontBase, gfxDWriteFonts, gfxGDIFont.
My understanding of scaled_fonts is that by fixing the CTM you'd get a certain set of grid-fitted glyph outlines and metrics that are optimized assuming the destination would have the given CTM, but that you'd be able to render with any CTM if you had to. By that interpretation the documentation you quote is a "should" and not a "must".
So we need to clarify with upstream whether it's a "should" or a "must".
If it's a "must", then we need to change our font implementations to recreate the scaled font when a new CTM is seen.
Comment 35•14 years ago
|
||
(In reply to comment #34)
> If it's a "must", then we need to change our font implementations to recreate
> the scaled font when a new CTM is seen.
If we choose not to fix this in cairo, a possible work-around on our side is to use cairo_set_font_face, cairo_set_font_matrix, and cairo_set_font_options.
Comment 36•14 years ago
|
||
My understanding of cairo_scaled_font_t is that it
"Replaces the current font face, font matrix, and font options in
the #cairo_t with those of the #cairo_scaled_font_t."
Whether the scaled font is actually used is an implementation detail and the gstate should only use the provided scaled font if the ctm is suitable.
I wonder how much the optimization in comment 31 would have been necessary if https://bugs.freedesktop.org/show_bug.cgi?id=17399 (bug 453200) were fixed.
Updated•14 years ago
|
Blocks: scale-stretchy
Assignee | ||
Comment 37•14 years ago
|
||
I think we should fix this as soon as possible so this patch disables the previous_scaled_font cache. I've filed bug #583035 about fixing/figuring out this problem properly.
Attachment #461294 -
Flags: review?(karlt)
Comment 38•14 years ago
|
||
Comment on attachment 461294 [details] [diff] [review]
Disable the previous_scaled_font cache
Will be interesting to see whether this affects firefox perf.
Attachment #461294 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 39•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/36f921c9692f
I'll try to add the reftest today too.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 40•14 years ago
|
||
(In reply to comment #39)
> http://hg.mozilla.org/mozilla-central/rev/36f921c9692f
>
> I'll try to add the reftest today too.
Reftests were already there. They've been set to pass in:
http://hg.mozilla.org/mozilla-central/rev/3802d1d04a44
Updated•14 years ago
|
Flags: in-testsuite+
Comment 41•14 years ago
|
||
Bug 551405 (mozTextAlongPath stops rotating along mozPathText) now works for me, maybe it was fixed by karlt's check-in; so if it has a reftest than someone can mark that bug FIXED too (yay!).
Comment 42•13 years ago
|
||
This does not appear to be fixed. I've posted my usecase and screenshot to another bug that seems to be about this same issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=501028
I also asked for people to test my usecase over IRC, and the one person that did try it, got the same broken rendering. Details at the above link.
Thanks all, - Jason
Reporter | ||
Comment 43•13 years ago
|
||
Jason: Are the testcases attached on this bug here broken for you?
If so, it's likely because you're running iceweasel, and iceweasel might be built with system-cairo rather than Mozilla's bundled cairo. (You can verify this by visiting "about:buildconfig" and looking for mention of "system-cairo" in the build options)
The fix to this bug was a fix to cairo code -- so if your Firefox build was built with an external unpatched cairo library, then you'll definitely hit this bug. (This is why Mozilla uses a bundled version of cairo in Firefox.)
Reporter | ||
Comment 44•13 years ago
|
||
(In reply to Daniel Holbert [:dholbert] (vacation 9/17-9/24) from comment #43)
> so if your Firefox build was
er "if your Iceweasel build" was
Reporter | ||
Comment 45•13 years ago
|
||
(your testcase attached on the other bug renders correctly for me, fwiw (w/ rotated characters), in a firefox Nightly build)
Comment 46•13 years ago
|
||
Daniel,
Thanks for the explanation of the cairo builtin vs system. Mine is indeed using system cairo. I've made a bug report for the debian cairo maintainers explaining the issue, and linking to this firefox bug report.
Looks like the test cases on this bug have the same issues for me as when they were originally reported. The first ("testcase") shows the rotated text funky except when selected with the mouse. The "semi-reference case" works fine. (I too experimented with only having rotated text, and it was fine.) "testcase #2" shows broken, but "reference for #2 shows correctly". All as explained in the comments above.
- Jason
Comment 48•13 years ago
|
||
Using Firefox 7 stable in Linux. Using this setup:
http://pastebin.com/UHKrAf8S
I get this bug.
If I delete the non-rotated element, the bug goes away.
If I change the font-size of either of them, the bug goes away.
Keeping the font-size the same, or deleting every style bit except for the transform produces the bug as described by the OP.
Comment 49•13 years ago
|
||
> Using Firefox 7 stable in Linux
Did you get your browser from mozilla.org? Or from your distro?
May be a problem with your Linux distro's Cairo version.
Reporter | ||
Comment 50•13 years ago
|
||
Jordan: your testcase is WORKSFORME using the Mozilla-provided build of Firefox 7, running on Ubuntu x86_64. (First line isn't rotated, second line is.)
If you can reproduce the bug you describe w/ Firefox *downloaded directly from Mozilla* at http://getfirefox.com , please open a new bug on it and CC me.
> May be a problem with your Linux distro's Cairo version.
(indeed -- see comment 43 for more details)
Comment 51•13 years ago
|
||
Meh, nevermind. Works in the official one. Don't know why it'd be different in a bleeding edge distro with zero focus on being 100% FOSS, though.
Comment 52•13 years ago
|
||
Because at least some Linux distros insist on shipping only one copy of cairo rather than including within firefox the version that we've tested and that we ship.
Comment 53•13 years ago
|
||
Well thanks. I took it as an opportunity to switch to UX anyway and its all around much better, no transform bug in sight.
Comment 55•13 years ago
|
||
What's the status of the bug upstream?
You need to log in
before you can comment on or make changes to this bug.
Description
•