Closed Bug 392233 Opened 17 years ago Closed 16 years ago

Text kerning broken by scaling/viewBox (space between letters or letters above each other)

Categories

(Core :: SVG, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: x00000000, Assigned: roc)

References

Details

(Keywords: regression, testcase)

Attachments

(9 files, 6 obsolete files)

168 bytes, image/svg+xml
Details
171 bytes, image/svg+xml
Details
174 bytes, image/svg+xml
Details
177 bytes, image/svg+xml
Details
180 bytes, image/svg+xml
Details
13.58 KB, image/png
Details
1.09 KB, image/svg+xml
Details
1.56 KB, image/svg+xml
Details
69.13 KB, patch
roc
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.2) Gecko/20070305 SeaMonkey/1.1.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a7) Gecko/2007080209 GranParadiso/3.0a7

See testcases. All should render identically.

Also note that the font size is way too small (should be 10% of the height).

This worked in 1.8.x and a few alphas before the current one.


Reproducible: Always

Steps to Reproduce:
1. Load testcases

Actual Results:  
Space between letters resp. letters above each other


Expected Results:  
Normal text
Attachment #276667 - Attachment description: Testcase 1 (viewBox="0 0 100 100", font-size="10") → Testcase 2 (viewBox="0 0 100 100", font-size="10")
If I modify nsSVGGlyphFrame to pass in AppUnitsPerDevPixel() in nsSVGlyphFrame::GetTextRun (with the appropriate scaling elsewhere in the code), I still get spacing artifacts.
Is the spacing bug 383408?
There seem to be these problems for me:

(1) font-size is always treated as CSS or device pixels instead of user
    units (also doesn't change if I add a transform="scale()")
(2) font-size has only a small usable range with discrete sizes (about 9
    to 30 pixels)
(3) character positions are calculated as if the actually used font were
    scaled by the normal pixel / user units ratio
(4) character positions are rounded to whole user units

If I add explicitly font-family="Verdana" or similar (scalable font),
only (4) remains.

My default font is set to Verdana, but the "advanced" font dialog says:

  Proportional: Serif
  Serif: Verdana
  Sans-serif: Helvetica

Actually used font in SVG is Helvetica (bitmap font). A new profile sets
fonts to Times/Helvetica. I have gtk-font-name="Helvetica 9" in
/etc/gtk-2.0/gtkrc to force GTK menus to use Helvetica; normally I use KDE.

Probably this is more than one bug.
Even with scalable fonts, font-size is limited to an integral number of
user units between 1 and 2000.
#4 is definitely due to SVG passing 1 as its AppUnitsPerDevPixel.

Not sure why the font size isn't right.
(In reply to comment #10)
> #4 is definitely due to SVG passing 1 as its AppUnitsPerDevPixel.

#4 wasn't fixed by changing that - see comment 6.
OK. Passing 1 as appunitsperdevpixel will definitely cause advances to be rounded to user units, that is by design. There may still be problems with that fixed, but we should fix that first so we can more easily diagnose remaining problems.
Attached patch test patch (obsolete) — Splinter Review
Not a complete patch - just enough to get text rendering with passing AppUnitsPerDevPixel to MakeTextRun.  Output of testcase stays the same.
Blocks: 321470
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Summary: Text kerning broken if viewBox is used (space between letters or letters above each other) → Text kerning broken by scaling/viewBox (space between letters or letters above each other)
This is an unpleasant regression from Firefox 2, so setting blocking1.9? flag.
Flags: blocking1.9?
Keywords: regression, testcase
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Blocks: 418446
jwatt, tor, rlongson, can you work on this?
Blocks: 383408, 418778
The left side of this testcase shows font-size in the range 1, 2, 3...160. It shows that as font-size increases glyph positioning appear more stable.

The right side of this testcase shows font-size in the range 0.05, 0.10, 0.15...8.55. The first visible step is at the 2.5 boundary, and each step thereafter is at the next .5 boundary (3.5, 4.5, 5.5...).
I'd note that under nsSVGGlyphFrame::GetTextRun, the metrics returned by the GetGlyphOutlineW call in cairo_win32_scaled_font_init_glyph_metrics seem wrong. Taking three values for font-size and comparing the extents for the first glyphs:

0.05 ->  9, 11
8    ->  3,  6
157  -> 88,104

I'd also note that in nsSVGGlyphFrame::nsSVGAutoGlyphHelperContext::Init we don't set the CTM on the computational surface, which perhaps we should be doing.
BTW, the whole right column in the testcase disappears if I press ctrl+<->. Is that a known bug? Seems to be caused by the occurance of font-size="0".
Moving tracking1.9+ back over to blocking1.9+ and setting P1 since we may want to get the extra testing of a beta depending on how this is fixed.
Flags: tracking1.9+ → blocking1.9+
Priority: P2 → P1
gfxFontStyle::gfxFontStyle limits font sizes to FONT_MAX_SIZE which is 2000. This was implemented in bug 383473 and this is why Testcase 5 displays too small.
As to small values. What eventually happens on Windows is that we get into _win32_scaled_font_get_scaled_hfont in cairo-win32-font.c. This converts the font size which is scaled_font->logical_size (a double) into logfont.lfHeight (a long) thereby losing any fractional font sizes.

I don't really know what to do to fix this.

I did think initially that you could multiply the font size by appUnitsPerDevPixel then select the font and scale down the results but that looks like it would

a) probably make the range limit 383473 occur sooner
b) not match fonts properly if you had non-scalable fonts
c) be too complicated for me to implement properly given my limited knowledge of thebes


Assignee: nobody → roc
(In reply to comment #23)
> b) not match fonts properly if you had non-scalable fonts

This is already the case and also a regression from 1.8.1.x. Font matching doesn't account for any scaling if you have non-scalable fonts, see comment #8.
N.B. I think Tor's current patch makes the text disappear in bug 421587 and bug 421584 at least on Windows.
(In reply to comment #19)
> I'd note that under nsSVGGlyphFrame::GetTextRun, the metrics returned by the
> GetGlyphOutlineW call in cairo_win32_scaled_font_init_glyph_metrics seem wrong.
> Taking three values for font-size and comparing the extents for the first
> glyphs:
> 
> 0.05 ->  9, 11

This is almost certainly because the size is being rounded to 0 and Windows treats 0 as requesting some default size.
I think we should
a) pass a reasonable value for appUnitPerDevPixel to MakeTextRun, so we don't unnecessarily round metrics in the textrun code
b) clamp the font-size passed to MakeTextRun to, say, a minimum of 8 and a maximum of 1000 device pixels, but scale metrics and drawing to compensate. So, for example, a requested font-size of 1px would be converted to 8 and then we scale all metrics and drawing down by 8.

The alternative is to always create the textrun using the transformed font size. Problem is then we have to recreate text runs every time we change the CTM, e.g. during a zooming sequence. And it doesn't help us deal with very very small or very very large transformed font sizes. And if we start to support line breaking, then changing the CTM could change line breaks, which is terrible for animation.
Yes, I believe anything less than a font size of 1 gets rounded up to 1 when the CTM on the DC is the identity matrix. If you scale up the transform on the DC you can hand it a smaller font size.

The problem I see with doing scaling the way you suggest is that we essentially accept that our text rendering is always going to be substandard when compared with SL/XAML. In my tests it had absolutely no problem with handling very, very small font size when there's a suitably large CTM to compensate (same for very, very large font size scaled down). The text is always absolutely perfect. Furthermore, when you change the font size and adjust the transform by an exactly mathematical inverse amount, the text always renders identically (sub-sub-pixel identical). This means that if someone has labels that should remain constant size as you zoom, you can easily get that affect by adjusting the font size to compensate. We on the other hand will have text that bobs about like mad during what should be a smooth zoom (and as you can see in the testcase, that isn't just by a pixel or two). I'd just find it hard to swallow doing a [much] poorer job than SL.

I guess the fundamental problem is that we ideally want to have the DC scaled by the exact transform with which the text will be painted to get kerning/hinting/etc. all absolutely perfect. Unfortunately the architecture of both cairo and our own text run architecture seem to preclude that, and as you suggest, there are perf issues to consider.

I guess none of this helps our cause for 1.9, but I just wanted to voice these thoughts.
Oh, and I have a partial patch for passing in appUnitPerDevPixel to MakeTextRun, but it caused some of the text in my testcase to disappear. I need to investigate that, or I can post it here if you want it.
Assignee: roc → nobody
Assignee: nobody → roc
I actually changed my mind, independently :-). And I have a working patch now. Just need to clean it up a bit.

Basically, if the author wants stable glyph positions during zooming, they should use text-rendering:geometricPrecision (which my patch supports). That addresses my concern about line-breaks. Performance may be an issue, but probably not unless you're zooming huge amounts of text. If you are, there are optimizations we could do like copy the font and glyph selection data from the old textrun into the new and just recompute metrics. To deal with very small or very large font sizes, we can still do the clamp-and-scale thing.
Attached patch fix (obsolete) — Splinter Review
Here's my patch. It's not well tested yet but all the testcases in this bug and the dependencies work, and I think it's complete apart from any regressing bugs it may have.

Although it only touches nsSVGGlyphFrame, it's quite a large patch. Because the font size we use for the textrun may differ from the font size we actually want to draw and measure with, we have to scale the context all over the place. This added a lot of code, since context setup was duplicated in many different functions. So I took it upon myself to refactor this class so that all the context setup is in one helper class, which subsumes the other helpers we were using. I think the result is quite pleasing, the new file is a bit shorter than the old file even though it does more.

As part of this restructuring, I changed nsSVGGlyphFrame to hold a reference to its textrun instead of a fontgroup. We don't want to use gfxTextRunCache because that would hold time-based references to each textrun created, and if the scale is being animated that cache could get very large. So we hold a reference to the textrun and delete it eagerly when we no longer need it.

The current code is inconsistent about the kind of context used to create the textrun; sometimes we pass in a screen context, but sometimes we pass in the context for the computational surface. That seems bad, so in this patch I always use a context for a computational surface to create the textrun. It would be better still to always use a context for the kind of surface the frame is targeted at, but I'll leave that to future work.

The new CharacterIterator helper is quite suitable for adding support for RTL, individually positioned glyphs, and intelligent cluster handling, but I've left those for future work as well.

The real work to fix this bug is in EnsureTextRun which subsumes the fontgroup setup code and GetTextRun.

My main concern with this patch is that I might have regressed the SVG text DOM APIs. Unfortunately we don't seem to have any tests for those :-(. I guess I'll write some. Note that even if I hadn't done this refactoring, any fix to this bug would require changes to the code for those APIs and would incur similar risks...
Attachment #286336 - Attachment is obsolete: true
With this patch, very small font sizes (below 0.1) still display with the wrong size. This is entirely a style system issue, the style system stores font sizes in integer appunits.
Attached patch fix v2, now with tests (obsolete) — Splinter Review
Updated patch. There's a small fix to scale the running mAdvanceWidth properly. Also, during my testing I discovered an existing bug in the way we handle getCharNumAtPosition; when glyphs overlap, we should return the last glyph containing the position, not the first.

Another issue is that currently most DOM methods throw when invoked on characters that have not been rendered because we ran out of room in the path. That seems wrong to me. Anyone know if there's a reason for that?

Anyway, this patch contains a reftest for this bug and mochitests for the DOM methods, to increase our confidence that I'm not breaking them.

I'm not sure who should sr this given that tor isn't active. Any suggestions?
Attachment #308516 - Attachment is obsolete: true
Attachment #308558 - Flags: review?(longsonr)
Whiteboard: [needs review]
General:

I think the reftest should go in with the other svg reftests in /mozilla/layout/reftests/svg with a proper file name text-scale-01.svg or somesuch

>diff -NrpU12 mozilla-trunk.23343368cefc/layout/svg/base/src/nsSVGGlyphFrame.cpp mozilla-trunk/layout/svg/base/src/nsSVGGlyphFrame.cpp
>--- mozilla-trunk.23343368cefc/layout/svg/base/src/nsSVGGlyphFrame.cpp	2008-03-11 17:27:58.000000000 +1300
>+++ mozilla-trunk/layout/svg/base/src/nsSVGGlyphFrame.cpp	2008-03-11 17:27:58.000000000 +1300

Nit: This file generally breaks at 80 characters. You have introduced some longer lines at some points. 

>+ * XXX should make this iterate clusters instead
>+ * XXX needs RTL love
>+ * XXX might want to make AdvanceToCharacter constant time (e.g. by
>+ * caching advances and/or the CharacterPosition array across DOM
>+ * API calls) to ensure that calling Get*OfChar (etc) for each character
>+ * in the text is O(N)
>+ */
>+class CharacterIterator
...
> 
> // XXX: This initial straightforward conversion from accessing cairo
> // directly to Thebes doesn't handle clusters.  Pretty much all code
> // that measures or draws single characters (textPath code and some
> // DOM accessors) will need to be reworked.

Just remove this comment now since you have made the same observations in CharacterIterator above

> NS_IMETHODIMP
> nsSVGGlyphFrame::PaintSVG(nsSVGRenderState *aContext, nsRect *aDirtyRect)
> {
...
> 
>   if (HasStroke() && SetupCairoStroke(gfx)) {
>     gfx->NewPath();
>-    LoopCharacters(gfx, text, cp, STROKE);
>+    AddCharactersToPath(&iter, gfx);
>     gfx->Stroke();
>-    gfx->NewPath();

Do we really not need the NewPath? This was put in with bug 337753. Bug 336663 has a testcase.

>   }
>-
>   gfx->Restore();
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsSVGGlyphFrame::GetFrameForPointSVG(float x, float y, nsIFrame** hit)
> {
>+  // XXX should we check 'visibility' style here like we do for painting?
>+

I think this added comment is incorrect. The function checks visibility within its body when appropriate. There are pointer event styles that will allow us to return an invisible frame.

>   } else {
>-    gfx->IdentityMatrix();
>-    extent = gfx->GetUserPathExtent();
>+    extent = gfxRect(0,0,0,0);

Nit: spaces after commas.

>-static int
>-UncompressIndex(int index, PRBool bRightAffinity, const nsTextFragment*fragment)
>-{
>-  // XXX
>-  return index;
>-}
>-

Removal of UncompressIndex is best left for another bug IMHO. 
CompressIndex and GetHighlight only worked with GDI+ prior to textPath being implemented they all need rewriting or removing.

> NS_IMETHODIMP
> nsSVGGlyphFrame::GetExtentOfChar(PRUint32 charnum, nsIDOMSVGRect **_retval)
> {
...
>+  gfxRect rect = tmpCtx.GetUserPathExtent();
>+  return NS_NewSVGRect(_retval, rect);

return NS_NewSVGRect(_retval, tmpCtx.GetUserPathExtent());

> NS_IMETHODIMP_(float)
> nsSVGGlyphFrame::GetBaselineOffset(PRUint16 baselineIdentifier)
> {
...
>   switch (baselineIdentifier) {
>   case BASELINE_HANGING:
>     // not really right, but the best we can do with the information provided
>     // FALLTHROUGH
>   case BASELINE_TEXT_BEFORE_EDGE:
>-    _retval = -float(metrics.mAscent);
>-    break;
>+    return -metrics.mAscent*metricsScale;

You will get double->float conversion warnings under MSVC if you remove the float cast.
metrics.mAscent is a gfxFloat which is a typedef for a double. 
You have done this in a number of places throughout this file.

> NS_IMETHODIMP_(float)
> nsSVGGlyphFrame::GetSubStringLength(PRUint32 charnum, PRUint32 fragmentChars)
> {
...
>-  return float(textRun->GetAdvanceWidth(charnum, fragmentChars, nsnull));
>+  return mTextRun->GetAdvanceWidth(charnum, fragmentChars, nsnull)*metricsScale;

without the float cast you will get a double->float conversion warning.

>+PRBool
>+nsSVGGlyphFrame::EnsureTextRun(float *aDrawScale, float *aMetricsScale)
>+{
...
>+    // The context scale is the ratio of the length of the transformed
>+    // diagonal vector (1,1) to the length of the untransformed diagonal
>+    // (which is sqrt(2)).
>+    gfxPoint p = m.Transform(gfxPoint(1,1)) - m.Transform(gfxPoint(0,0));

Nit: spaces after commas.

>+    float contextScale = sqrt((p.x*p.x + p.y*p.y)/2);

double->float conversion here

>+    // XXX We should use a better surface here! But then we'd have to
>+    // change things so we can ensure we always have the "right" sort o

change things so that we would always have the "right" sort of

>diff -NrpU12 mozilla-trunk.23343368cefc/layout/svg/base/src/nsSVGGlyphFrame.h mozilla-trunk/layout/svg/base/src/nsSVGGlyphFrame.h
>--- mozilla-trunk.23343368cefc/layout/svg/base/src/nsSVGGlyphFrame.h	2008-03-11 17:27:58.000000000 +1300
>+++ mozilla-trunk/layout/svg/base/src/nsSVGGlyphFrame.h	2008-03-11 17:27:58.000000000 +1300
>@@ -35,43 +35,49 @@
...
>+  // Use a power of 2 here. It's not so important to match
>+  // nsIDeviceContext::AppUnitsPerDevPixel, but since we do a lot of
>+  // multiplying by 1/GetTextRunUnitsFactor, it's good for it to be a
>+  // power of 2 to avoid accuracy loss.
>+  static PRInt32 GetTextRunUnitsFactor() { return 64; }

gfxTextRunFactory::Parameters::mAppUnitsPerDevUnit is PRUint32 so this should be too.

That completes my code comments. I would like to give this a try on Windows as it's a big change which is the only reason I haven't given it an r+ yet as the review comments are all pretty trivial.

I'd suggest vlad for sr.
Windows warnings:

c:/src/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp(824) : warning C4244: 'return' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/src/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp(826) : warning C4244: 'return' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/src/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp(829) : warning C4244: 'return' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/src/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp(843) : warning C4244: 'return' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/src/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp(981) : warning C4244: 'return' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/src/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp(1156) : warning C4244: 'initializing' : conversion from 'double' to 'float', possible loss of data
c:/src/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp(1205) : warning C4244: '=' : conversion from 'double' to 'float', possible loss of data
(In reply to comment #35)
> General:
> 
> I think the reftest should go in with the other svg reftests in
> /mozilla/layout/reftests/svg with a proper file name text-scale-01.svg or
> somesuch

Any particular reason SVG does things differently here? If we want to isolate the SVG-specific testcases, why not use layout/reftests/svg/bugs? Having the bug number in the name is useful.

> Nit: This file generally breaks at 80 characters. You have introduced some
> longer lines at some points.

OK I'll fix that.

> Just remove this comment now since you have made the same observations in
> CharacterIterator above

OK

> >-    gfx->NewPath();
> 
> Do we really not need the NewPath? This was put in with bug 337753. Bug 336663
> has a testcase.

Good catch, I'll put that back.

> I think this added comment is incorrect. The function checks visibility within
> its body when appropriate. There are pointer event styles that will allow us
> to return an invisible frame.

You are correct, I'll remove the comment.

> Nit: spaces after commas.

OK

> Removal of UncompressIndex is best left for another bug IMHO. 
> CompressIndex and GetHighlight only worked with GDI+ prior to textPath being
> implemented they all need rewriting or removing.

OK

> return NS_NewSVGRect(_retval, tmpCtx.GetUserPathExtent());

OK

> You will get double->float conversion warnings under MSVC if you remove the
> float cast.
> metrics.mAscent is a gfxFloat which is a typedef for a double. 
> You have done this in a number of places throughout this file.

Sorry, I forgot about that issue. I'll put them back.

> Nit: spaces after commas.

OK

> >+    // XXX We should use a better surface here! But then we'd have to
> >+    // change things so we can ensure we always have the "right" sort o
> 
> change things so that we would always have the "right" sort of

OK

> gfxTextRunFactory::Parameters::mAppUnitsPerDevUnit is PRUint32 so this should
> be too.

Right.
(In reply to comment #38)

> > I think the reftest should go in with the other svg reftests in
> > /mozilla/layout/reftests/svg with a proper file name text-scale-01.svg or
> > somesuch
> 
> Any particular reason SVG does things differently here? If we want to isolate
> the SVG-specific testcases, why not use layout/reftests/svg/bugs? Having the
> bug number in the name is useful.

I don't know. jwatt started it and I just followed suit.

As far as testing is going I'm having lots of problems.

The first testcase in bug 421587 crashes for me. mParent is null in the nsSVGGFrame::GetCanvasTM

http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-align-01-b.html
crashes leaving InitTextRunUniscribe trying to release a gfxContext

Testcase in bug 318597 crashes with the same issue.

Some things work e.g.
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-text-01-b.html

but much of the SVG testsuite which uses text crashes.

OK, so all those temporary contexts need to be heap allocated.
I'm out tomorrow, if you post another patch I'll give it a whirl on Thursday.
Heap allocating the gfxContext will fix most things. What remains after that is:

The first testcase in bug 421587 crashes for me. mParent is null in the
nsSVGGFrame::GetCanvasTM

The testcase in bug 318597 no longer works. This is an interactive text dom testcase.

nsSVGGlyphFrame::GetCharNumAtPosition is wrong. Lets say the mouse is over the first character. First you do a rectangle over the first glyph which matches. Then you do a rectangle over the second glyph, however as you have not cleared the first rectangle you now match that too as both rectangles exist in the context even though the mouse is not over the second glyph.

Since you're going to have to rewrite this anyway, it's probably best to create a temporary gfxPoint variable outside the loop rather than doing the conversion every iteration.

>+    if (tmpCtx.PointInFill(gfxPoint(xPos, yPos))) {
(In reply to comment #38)
> > >-    gfx->NewPath();
> > 
> > Do we really not need the NewPath? This was put in with bug 337753. Bug 336663
> > has a testcase.
> 
> Good catch, I'll put that back.
> 

Can you add a comment. Something along the lines of:

    // We need to clear the context so that we don't leak into chrome
    // See bug 337753.

Then we will all know why that line is there.

btw It's possible the bug 421587 crash I get is some interaction between this patch and my bug 421584 patch.
(In reply to comment #42)
> nsSVGGlyphFrame::GetCharNumAtPosition is wrong. Lets say the mouse is over the
> first character. First you do a rectangle over the first glyph which matches.
> Then you do a rectangle over the second glyph, however as you have not cleared
> the first rectangle you now match that too as both rectangles exist in the
> context even though the mouse is not over the second glyph.

Good catch. I think I forgot to rerun my tests!

> Since you're going to have to rewrite this anyway, it's probably best to create
> a temporary gfxPoint variable outside the loop rather than doing the conversion
> every iteration.
> 
> >+    if (tmpCtx.PointInFill(gfxPoint(xPos, yPos))) {

Done.
The crash in the bug 421587 testcase is a tricky one. We're trying to get the canvas TM during frame construction, via

#5  0x13d5a3d1 in nsSVGGlyphFrame::GetGlobalTransform (this=0x24cab44, aMatrix=0xbfffc088) at /Users/roc/mozilla-trunk/layout/svg/base/src/nsSVGGlyphFrame.cpp:1129
#6  0x13d5a69e in nsSVGGlyphFrame::EnsureTextRun (this=0x24cab44, aDrawScale=0xbfffc1c4, aMetricsScale=0xbfffc1c0) at /Users/roc/mozilla-trunk/layout/svg/base/src/nsSVGGlyphFrame.cpp:1175
#7  0x13d5acb1 in nsSVGGlyphFrame::GetAdvance (this=0x24cab44) at /Users/roc/mozilla-trunk/layout/svg/base/src/nsSVGGlyphFrame.cpp:860
#8  0x13d6b3dc in nsSVGTextFrame::UpdateGlyphPositioning (this=0x24caaa4) at /Users/roc/mozilla-trunk/layout/svg/base/src/nsSVGTextFrame.cpp:428
#9  0x13d6b696 in nsSVGTextFrame::NotifyGlyphMetricsChange (this=0x24caaa4) at /Users/roc/mozilla-trunk/layout/svg/base/src/nsSVGTextFrame.cpp:311
#10 0x13d6b7c9 in nsSVGTextFrame::SetInitialChildList (this=0x24caaa4, aListName=0x0, aChildList=0x24cab44) at /Users/roc/mozilla-trunk/layout/svg/base/src/nsSVGTextFrame.cpp:81

I think nsSVGTextFrame::UpdateGlyphPositioning should be delayed until InitialUpdate at least, when the frame has actually been inserted into the frame tree.
Well, that's not enough. The problem in this testcase is that the text is in the <defs> part, so we still crash because there is no canvas-TM there. Presumably the actual rendering is done via <use> in which case we'll make new nsSVGGlyphFrames which will work correctly. In the meantime I need to figure out how to stop us crashing on glyph frames inside <defs>. BTW is there a reason why we create frames for the stuff inside <defs>?
Attached patch fix v3 (obsolete) — Splinter Review
This should address all your comments.

To handle the text-in-<defs> situation, I allow GetCanvasTM to return null and propagate that through the different implementations of GetCanvasTM. This seems to be the simplest way to detect that we're in <defs>. With this approach, if they do that CharacterIterator will just act as if there are no characters.
Attachment #309014 - Flags: review?(longsonr)
(In reply to comment #46)
> Well, that's not enough. The problem in this testcase is that the text is in
> the <defs> part, so we still crash because there is no canvas-TM there.
> Presumably the actual rendering is done via <use> in which case we'll make new
> nsSVGGlyphFrames which will work correctly. In the meantime I need to figure
> out how to stop us crashing on glyph frames inside <defs>. BTW is there a
> reason why we create frames for the stuff inside <defs>?
> 

The way to detect you're in defs is that the glyph frame should have the NS_STATE_SVG_NONDISPLAY_CHILD bit set. We should avoid calling anything which needs to call GetCanvasTM in such cases. 

We create frames for defs so we get styles as you say such frames may be used by use or clip paths etc.

Comment on attachment 309014 [details] [diff] [review]
fix v3

That's not the way to go for <defs> frames.
Attachment #309014 - Flags: review?(longsonr) → review-
Can you revert all the non nsSVGGlyphFrame changes and check for nondisplay child in the glyphframe. Would having GetGlobalTransform return an identity matrix for nondisplay child work?

The acid test in this case would be that
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-masking-path-04-b.html
works. There you have text within defs and we need to set the position properly in order to get the lower clip path to display.

As well as the testcase in the crashing bug obviously ;-)
(In reply to comment #50)
> Can you revert all the non nsSVGGlyphFrame changes and check for nondisplay
> child in the glyphframe. Would having GetGlobalTransform return an identity
> matrix for nondisplay child work?

No, we have to work harder than that, otherwise we wouldn't use the right matrix when we paint the text as a pattern or use it as a clip path. I guess we could force GetCanvasTM to be called if we're in PaintSVG, otherwise we only call it if we're not a nondisplay-child?
Attachment #308558 - Attachment is obsolete: true
Attachment #308558 - Flags: review?(longsonr)
Seems reasonable. Anything that avoids changes all over the place outside nsSVGGlyphFrame is OK.
OK, I tried that and it seems like a bad approach to try to figure out when it's safe to call GetCanvasTM or not. For example in a call to GetBaselineOffset, if we're a nondisplay-child, should we use the global transform or an identity matrix? We have no way of knowing whether the nsSVGTextFrame::UpdateGlyphPositioning call is being done in a state where it's OK to call GetCanvasTM or not. Maybe we could pass all that information around everywhere, but it just seems simpler to let GetCanvasTM return null than to pass around information about whether it's safe to call it.
Note that if we go with the "pass around information about whether GetCanvasTM is safe" approach, then I think that information has to become a new flag for NotifySVGChanged otherwise nsSVGTextFrame can't know whether it's safe for the call to NotifyGlyphMetricsChange there.
It's a 1:1 correspondance between nondisplay child and can or cannot call GetCanvasTM, or at least it has been up to now. 

What's the functional difference between returning null in all other frame's GetCanvasTMs by putting in code changes everywhere to support that and checking for nondisplay child in nsSVGGlyphFrame GetGlobalTransform which should tell you that a call to GetCanvasTM would return null (or rather assert/crash with the current codebase) so it's not worth doing it.

GetGlobalTransform is the only caller to GetCanvasTM so you have a single test required.

Somehow I get the impression you are trying to explain something I'm not quite getting or vice versa.
> It's a 1:1 correspondance between nondisplay child and can or cannot call
> GetCanvasTM, or at least it has been up to now. 

No it's not, because nondisplay children *can* and do call GetCanvasTM when they're asked to paint because they're being used in a pattern, mask or clip operation. Am I wrong about that?

If I understand correctly, the current situation is that nondisplay children only call GetCanvasTM in code that's only reachable from PaintSVG, GetFrameForPointSVG GetBBox and UpdateCoveredRegion (any others?), and those functions are either never called on nondisplay children, or only called during pattern, mask or clip operations during which GetCanvasTM is safe. That's a rather complicated and fragile invariant, but it's enough.

In the new text code, EnsureTextRun is called along many paths, some of which are reachable via PaintSVG/GetFrameForPointSVG/GetBBox/etc and others which are reached from DOM API calls, so we don't know whether we can call GetCanvasTM or not. Worse, the same is true of nsSVGGlyphFrame::GetBaselineOffset, nsSVGTextFrame::NotifyGlyphMetricsChange, and nsSVGTextFrame::NotifySVGChanged.

Another way of looking at this is that existing SVG code only needs the canvas TM when one is actually available so it's OK to just call it. The new text code sometimes needs the canvas TM when it isn't available, so we want to fall back to an identity matrix but we have no way of telling when we should do that, because one was never needed before.

Let me know if I'm wrong about any of these details.
If that wasn't clear enough, let me be even more concrete:

Suppose we have an outermost <svg> containing a <defs> containing a <marker> containing <text> containing a text node.

Misc changes to the outermost <svg> element can propagate through <defs> and result in nsSGVTextFrame::NotifySVGChanged being called, which leads to nsSVGTextFrame::NotifyGlyphMetricsChange, then nsSVGTextFrame::UpdateGlyphPositioning, nsSVGGlyphFrame::GetBaselineOffset, then nsSVGGlyphFrame::EnsureTextRun. In this situation it is not safe to call GetCanvasTM, we should use the identity matrix.

Use of the marker from outside the <defs> calls nsSVGMarkerFrame::PaintMark, which calls nsSGVTextFrame::NotifySVGChanged, ..., and eventually nsSVGGlyphFrame::EnsureTextRun is called. In this situation, it is safe to call GetCanvasTM --- in fact, it is essential.

So we need a way to tell these (and similar situations) apart so that EnsureTextRun can do the right thing.
(In reply to comment #56)
> > It's a 1:1 correspondance between nondisplay child and can or cannot call
> > GetCanvasTM, or at least it has been up to now. 

Clearly I've muddied the waters, not least my making incorrect statements such as above.

> 
> No it's not, because nondisplay children *can* and do call GetCanvasTM when
> they're asked to paint because they're being used in a pattern, mask or clip
> operation. Am I wrong about that?

You are not wrong.

> 
> If I understand correctly, the current situation is that nondisplay children
> only call GetCanvasTM in code that's only reachable from PaintSVG,
> GetFrameForPointSVG GetBBox and UpdateCoveredRegion (any others?), and those
> functions are either never called on nondisplay children, or only called during
> pattern, mask or clip operations during which GetCanvasTM is safe. That's a
> rather complicated and fragile invariant, but it's enough.

Actually nondisplay children can call GetCanvasTM at almost any time. They can never support calls to UpdateCoveredRegion or GetFrameForPointSVG at any time; anything that wants the covered region is banned for nondisplay children.

Almost any time means only that you can't call GetCanvasTM too early during frame construction as it might not be set up. This currently means no calls from SetInitialChildList, after that you should be OK.

We get one shot to set the x and y position for text fragments which are children of clip paths and we must get the mPositions correct or http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-masking-path-04-b.html won't work.

In 1.8 we did this via InitialUpdate which you are proposing to return to in your latest patch. Unfortunately in 1.9 InitialUpdate is not passed to children of defs frames as nsSVGContainerFrame does not implement nsISVGChildFrame.

We have 3 things muddying the waters in your latest patch

a) returning null in GetCanvasTM which I still don't think is correct but for different reasons than before. If it is returning null we are calling it too early so we need to call it later or set things up properly before we call it.

b) returning an identity matrix in GetCanvasTM at any point. Possibly problematical if clip path children will be misplaced as we will have no later chance to correct that.

c) moving from SetInitialChildList to InitialUpdate in nsSVGTextFrame which may work for ordinary text but not for defs children.

I'm struggling to suggest a best way forward unless we can somehow ensure the canvasTM is set up properly by the time we call SetInitialChildList or indeed set it up within that function for text frames.
> Actually nondisplay children can call GetCanvasTM at almost any time.

How does that work? Seems to me if I just stick <text> in a <defs> and call GetCanvasTM on the <text> or its children, it will call parent->GetCanvasTM on the defs frame, which is just an nsSVGContainerFrame, so it returns null and we crash. (I haven't actually tested this though, don't have a build with my patch in it at the moment.)

If we should always be able to get a canvas TM for elements in <defs>, that'd be great.
You can't stick text directly in a defs at the moment as we do not support tref elements (bridge to be crossed post 1.9 fortunately). You have to stick the text within a marker or clip path and markers and clip paths implementations of GetCanvasTM get their parent transforms from the marked frames or clipped frames and not their defs parents. 

I suspect the marked or clipped frames are not set up when we try to initialise the marker or clip frames; without your patch the methods involved in setup, e.g. GetBaselineOffset did not need to call GetCanvasTM so that didn't matter; now it does.

One idea I had last night is that in TextFrame, if InitialUpdate is late enough that are set up while SetInitialChildList is too early maybe you could modify  NotifyChildrenOfSVGChange so that it took an extra INITIAL_UPDATE flag as nsSVGUtils::NotifyChildrenOfSVGChange forces its way through the defs by recursion.

If InitialUpdate is still too early (and I suspect it might be) then the marked clipped frames are going to need to do an set up the text they point to.

Basically I still think the answer is that we need to call GetCanvasTM late enough that it works and that we should not need to modify the implementation of any of the GetCanvasTM methods for any frame. We do need to modify how or more precisely when, text frames are initialised though, particularly within defs.
Oh and my apologies for anything incorrect or misleading I've previously said that has confused you.
(In reply to comment #60)
> You have to stick the text within a marker or clip path and markers and clip
> paths implementations of GetCanvasTM get their parent transforms from the
> marked frames or clipped frames and not their defs parents. 

Right, that's the case I was thinking of.

It's not just a matter of doing the GetCanvasTM "late enough". For example, if the text is inside a <marker>, then it is only safe to call GetCanvasTM while the marker is in PaintMark or RegionMark, in particular while it has a AutoMarkerReferencer on the stack. AutoMarkerReferencer sets mMarkedFrame to nsnull so we will crash if you call GetCanvasTM after AutoMarkerReferencer destruction.
(In reply to comment #61)
> Oh and my apologies for anything incorrect or misleading I've previously said
> that has confused you.

No worries about that.
Here's an alternative that you might find acceptable: make nsSVGTextFrame::NotifyGlyphMetricsChange not actually call UpdateGlyphPositioning. Instead be lazy and do UpdateGlyphPositioning only when we need it; at that time we should know whether we have a global transform to use.
great idea.

the thing is that, as you were paitiently trying to explain earlier the marker frames only know who their markers are during painting.

if nondisplay do we have to work out where we are every time now as we may be drawn by different marking frames with different transforms?

if display frame there are no problems we are always ok with getcanvastm.
(In reply to comment #65)
> the thing is that, as you were paitiently trying to explain earlier the marker
> frames only know who their markers are during painting.

Yeah, that's right. (More precisely they only know *what they're marking*...)

> if nondisplay do we have to work out where we are every time now as we may be
> drawn by different marking frames with different transforms?

Right. This is currently handled by firing NotifySVGChanged(TRANSFORM) at the marker frame subtree each time before we draw.

> if display frame there are no problems we are always ok with getcanvastm.

Right. I think I can push ahead on this now.
Attached patch fix v4 (obsolete) — Splinter Review
Okay, this does that. Passes all the tests mentioned in this bug.
Attachment #309403 - Flags: review?(longsonr)
Comment on attachment 309403 [details] [diff] [review]
fix v4

This one's a winner ;-)

>diff -NrpU12 mozilla-trunk.d0ddac180c7a/layout/reftests/svg/text-scale-01.svg mozilla-trunk/layout/reftests/svg/text-scale-01.svg
>--- mozilla-trunk.d0ddac180c7a/layout/reftests/svg/text-scale-01.svg	1970-01-01 12:00:00.000000000 +1200
>+++ mozilla-trunk/layout/reftests/svg/text-scale-01.svg	2008-03-15 01:22:26.000000000 +1300

Can you put the bug number in a comment in all the reftest files and add a title element to each one so that the reftests are structured similar to the existing tests.
Also jwatt is trying to encourage us to have public domain licensed reftests.

>diff -NrpU12 mozilla-trunk.d0ddac180c7a/layout/svg/base/src/nsISVGGlyphFragmentLeaf.h mozilla-trunk/layout/svg/base/src/nsISVGGlyphFragmentLeaf.h
>--- mozilla-trunk.d0ddac180c7a/layout/svg/base/src/nsISVGGlyphFragmentLeaf.h	2008-03-15 01:22:26.000000000 +1300
>+++ mozilla-trunk/layout/svg/base/src/nsISVGGlyphFragmentLeaf.h	2008-03-15 01:22:26.000000000 +1300

Interface changing, rev the ID.

>-  NS_IMETHOD_(float) GetBaselineOffset(PRUint16 baselineIdentifier)=0;
>-  NS_IMETHOD_(float) GetAdvance()=0;
>+  NS_IMETHOD_(float) GetBaselineOffset(PRUint16 baselineIdentifier,
>+                                       PRBool aForceGlobalTransform)=0;
>+  NS_IMETHOD_(float) GetAdvance(PRBool aForceGlobalTransform)=0;


> NS_IMETHODIMP
> nsSVGGlyphFrame::PaintSVG(nsSVGRenderState *aContext, nsRect *aDirtyRect)
> {
...
>   if (HasStroke() && SetupCairoStroke(gfx)) {
>+    // We need to clear the context's path because there might be one
>+    // from a previous operation. See bug 337753.
>     gfx->NewPath();

Actually the important thing is to comment the other NewPath below to make sure
nobody removes that. Leaving things in the context can make svg attributes leak
into the surrounding chrome. That's what bug 337753 is about.
It's quite possible the NewPath above is unnecessary although it's too late to remove it now.

>-    LoopCharacters(gfx, text, cp, STROKE);
>+    AddCharactersToPath(&iter, gfx);
>     gfx->Stroke();
>     gfx->NewPath();


>diff -NrpU12 mozilla-trunk.d0ddac180c7a/layout/svg/base/src/nsSVGTextFrame.h mozilla-trunk/layout/svg/base/src/nsSVGTextFrame.h
>--- mozilla-trunk.d0ddac180c7a/layout/svg/base/src/nsSVGTextFrame.h	2008-03-15 01:22:26.000000000 +1300
>+++ mozilla-trunk/layout/svg/base/src/nsSVGTextFrame.h	2008-03-15 01:22:26.000000000 +1300
>@@ -43,30 +43,28 @@
...
>   NS_IMETHOD NotifyRedrawUnsuspended();
>+  // Override these four to ensure that UpdateGlyphPositioning is called
>+  // to bring glyph postions up to date

s/postions up to date/positions up-to-date/

r=longsonr with these minor issues addressed.
Attachment #309403 - Flags: review?(longsonr) → review+
Comment on attachment 309403 [details] [diff] [review]
fix v4

This is not really your thing vlad, but there's no-one else to go to at the moment.
Attachment #309403 - Flags: superreview?(vladimir)
Comment on attachment 309403 [details] [diff] [review]
fix v4

Hrm, why does GetStartPositionOfChar just return the value direct from the position array, but GetEndPositionOfChar takes care to set up a temporary metrics context to call MoveTo on?  (Also, why doesn't it just get the matrix and transform the distance directly?)
(In reply to comment #70)
> (From update of attachment 309403 [details] [diff] [review])
> Hrm, why does GetStartPositionOfChar just return the value direct from the
> position array

Because the value in the array is the start position...

> but GetEndPositionOfChar takes care to set up a temporary
> metrics context to call MoveTo on?

Because we have to compute the destination position by moving along a ray the length of the advance at an angle specified in the position-data array, and this seemed to be the easiest way to do that.

> (Also, why doesn't it just get the matrix
> and transform the distance directly?)

We could, and it would be more efficient, but I just wanted to write the most obvious and simple code here.
This one seems to be a bit higher in risk, but have to ask, should this block beta 5?
I don't think it (or any other SVG bug) has to block beta5, but it can land for beta5 if Vlad finishes his review :-)
Moving to P2.  
Priority: P1 → P2
Vlad points out that the DOM metrics APIs are using the global transform where they shouldn't. I'll add some more tests and fix.
Attachment #309403 - Flags: superreview?(vladimir) → superreview+
Then I pointed out that actually that code is fine because it doesn't call SetupGlobalTransform so mInitialMatrix is always the identity.
Attached patch fix v5 (obsolete) — Splinter Review
nits fixed, improved tests, better comments, good to go.
Attachment #309014 - Attachment is obsolete: true
Attachment #309403 - Attachment is obsolete: true
Attachment #310096 - Flags: superreview+
Attachment #310096 - Flags: review+
checked in
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Backed out, it caused a couple of reftest failures:

REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_osx/mozilla/layout/reftests/svg/pseudo-classes-02.svg
REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_osx/mozilla/layout/reftests/svg/text-in-link-01.svg

Looks like something wrong with whitespace collapsing or something?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Those tests failed because we weren't doing whitespace compression properly. That was because we set up the textrun before SetWhitespaceHandling was called, and *that* was because we'd stopped calling UpdateGlyphPositioning from nsSVGTextFrame::NotifyRedrawUnsuspended, which apparently gets called very early. So I added UpdateGlyphPositioning(PR_FALSE) there, and the bug is fixed. I'll reland with that.
Relanded
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Backed out again. This time we crashed in crashtests:

REFTEST PASS (LOAD ONLY): file:///builds/slave/trunk_osx/mozilla/layout/svg/crashtests/308615-1.svg
../../objdir/dist/Minefield.app/Contents/MacOS/run-mozilla.sh: line 442: 22323 Bus error               "$prog" ${1+"$@"}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fix v6Splinter Review
Turns out that SetupCairoStroke can, in bizarre circumstances (namely in the 308917-1 crashtest), result in mTextRun in this frame being cleared. Presumably similar for SetupCairoFill. So we just create the CharacterIterator after those functions have been called. This fixes the crash and the other crashtests in that directory pass too.

This can be relanded but I don't have time to do it.
Attachment #310096 - Attachment is obsolete: true
Attachment #310209 - Flags: superreview+
Attachment #310209 - Flags: review+
Checking in content/svg/content/test/Makefile.in;
/cvsroot/mozilla/content/svg/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
Checking in layout/reftests/svg/reftest.list;
/cvsroot/mozilla/layout/reftests/svg/reftest.list,v  <--  reftest.list
new revision: 1.66; previous revision: 1.65
done
Checking in layout/svg/base/src/nsISVGGlyphFragmentLeaf.h;
/cvsroot/mozilla/layout/svg/base/src/nsISVGGlyphFragmentLeaf.h,v  <--  nsISVGGlyphFragmentLeaf.h
new revision: 1.14; previous revision: 1.13
done
Checking in layout/svg/base/src/nsSVGGlyphFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp,v  <--  nsSVGGlyphFrame.cpp
new revision: 1.124; previous revision: 1.123
done
Checking in layout/svg/base/src/nsSVGGlyphFrame.h;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.h,v  <--  nsSVGGlyphFrame.h
new revision: 1.33; previous revision: 1.32
done
Checking in layout/svg/base/src/nsSVGTextFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGTextFrame.cpp,v  <--  nsSVGTextFrame.cpp
new revision: 1.70; previous revision: 1.69
done
Checking in layout/svg/base/src/nsSVGTextFrame.h;
/cvsroot/mozilla/layout/svg/base/src/nsSVGTextFrame.h,v  <--  nsSVGTextFrame.h
new revision: 1.13; previous revision: 1.12
done
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Version: unspecified → Trunk
Blocks: 423666
Depends on: 424299
Thanks reed. Glad this stuck.

Someone want to verify the dependencies?
Depends on: 425662
[Firefox 3.0b4 on Ubuntu 8.04beta]  Just confirming that text character spacing is still wrong in the 3.0b4 Linux release for i386 platform.  I'll check again when Ubuntu replaces it with a later Firefox release.
Blocks: 426738
No longer blocks: 426738
Depends on: 426738
Depends on: 423998
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: