-moz-transform doesn't rotate individual characters in this testcase, except when text is partially repainted

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: dholbert, Assigned: jrmuizel)

Tracking

(Depends on 1 bug, {regression, testcase})

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta3+)

Details

Attachments

(5 attachments)

Reporter

Description

10 years ago
Posted file testcase
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

10 years ago
Requesting blocking, since this is a layout regression from 1.9.1.
Flags: blocking1.9.2?
Reporter

Comment 2

10 years ago
Posted file semi-reference case
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

10 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

10 years ago
Reporter

Comment 5

10 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?
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
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
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
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.
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

9 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

9 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.
Assignee: nobody → jmuizelaar
Assignee

Comment 14

9 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

9 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.
The comment 15 testcase also works if you display the frame in a tab.
This blocks _a_ beta, but not necessarily beta 1.
blocking2.0: beta1+ → beta2+

Comment 18

9 years ago
Posted file testcase #2

Comment 19

9 years ago
Posted file reference for #2

Updated

9 years ago
Keywords: testcase
Duplicate of this bug: 577254
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.
blocking2.0: beta2+ → final+
We've hit this bug on the "What's new" page for Beta 2. (bug 578138)
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);
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+ → ?
(And by "various obvious patches" I really mean "various patches that end up disabling the previous_scaled_font cache entirely".)
blocking2.0: ? → beta3+

Updated

9 years ago
Duplicate of this bug: 582386
Assignee

Comment 30

9 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

9 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

9 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

9 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.
(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.
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.
Assignee

Comment 37

9 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)
Depends on: 583035
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

9 years ago
http://hg.mozilla.org/mozilla-central/rev/36f921c9692f

I'll try to add the reftest today too.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Assignee

Comment 40

9 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
Flags: in-testsuite+

Comment 41

9 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!).
Reporter

Updated

9 years ago
Blocks: 551405

Comment 42

8 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

8 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

8 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

8 years ago
(your testcase attached on the other bug renders correctly for me, fwiw (w/ rotated characters), in a firefox Nightly build)

Comment 46

8 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
Duplicate of this bug: 524250

Comment 48

8 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

8 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

8 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

8 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.
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

8 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.
Duplicate of this bug: 720507

Comment 55

7 years ago
What's the status of the bug upstream?
Duplicate of this bug: 778209
You need to log in before you can comment on or make changes to this bug.