Closed Bug 875329 Opened 11 years ago Closed 11 years ago

transforms within SVG-in-OpenType glyphs are ignored when SVG display-list painting is enabled

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Version:
17 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(4 files, 7 obsolete files)

TrueType font with an SVG glyph for U+263A
432.25 KB, application/x-font-ttf
Details
the SVG document that is embedded in the above font
6.75 KB, image/svg+xml
Details
reftest for use of transforms within SVG-in-OT glyphs.
116.59 KB, patch
heycam
: review+
Details | Diff | Splinter Review
don't ignore transforms when rendering SVG-in-OT glyphs.
86.40 KB, patch
heycam
: review+
Details | Diff | Splinter Review
The attached font is a copy of the AndroidEmoji font with an SVG image provided for the character U+263A (☺), so when gfx.font_rendering.opentype_svg.enabled is set to true, it should render a colorful smiley in place of the original monochrome outline version. However, if svg.display-lists.painting.enabled is also true (which is the default, since bug 776054), the transform used to scale the artwork appropriately for the glyph bounding box is completely ignored (as are any other transforms used within the SVG content). Because this particular artwork was designed in a 64-unit user space square, it appears minuscule within the 1000-unit-high glyph coordinate space that the SVG-glyph code uses. Switching off display-list painting fixes the problem and the glyph artwork scales nicely to fit the expected glyph bounds.
For reference, this is a copy of the SVG document that is embedded in the font to provide the glyph for U+263A. (Note that because of the transform used to move the artwork largely above the baseline to negative y-coordinates, it will be mainly out of view if loaded as a standalone document.)
A simple testcase for the SVG-in-OpenType rendering, assuming the font file is placed in the same directory: <!DOCTYPE html> <html> <head> <meta charset="utf-8"> <style type="text/css"> @font-face { font-family: foo; src: url("GeckoEmoji.ttf"); } body { font-family: foo, sans-serif; font-size: 32px; } </style> </head> <body> foo&#x263a;&#x263a;&#x263a;bar </body> </html> With display-list painting DISabled, it looks good... with it enabled, not so much.
GetCanvasTM does not return a CTM that takes into account transform="" on the element or any ancestors when display list painting is turned on, since that transform should already have been applied before PaintSVG is called. Do we need PaintSVG to take a flag that indicates whether it is being called from under a display list item's Paint method? Currently we are checking for NS_STATE_SVG_NONDISPLAY_CHILD in GetCanvasTM, which is the other case where we need to set up our entire transform chain. That doesn't apply SVG glyph element here, though.
If you have: <svg ...> <g transform="..."> <g glyphid="123"> ... </g> </g> </svg> should the transform on the parent <g> be applied when you paint the glyph with ID 123?
Offhand, I'm inclined to think it should *not* ... Roc, Edwin, WDYT?
That is also my first thought. I think it's going to be tricky, as we have to thread through the frame that we want to stop the transform-looking-up at through all PaintSVG and GetCanvasTM(FOR_PAINTING) calls.
Attached patch partial patch (obsolete) — — Splinter Review
Something like this, for example, which doesn't seem to work yet, but which illustrates what would be needed.
(Forgot to add the transform root to the relevant PaintSVG call in nsSVGUtils::PaintSVGGlyph, but that didn't seem to change the rendering either.)
Hmm, but what about other attributes that are set outside the element that's actually being used as the glyph? E.g., if you have <svg ...> <g fill="yellow" stroke="blue"> <g glyphid="123"> ... </g> </g> </svg> should the paths within the glyph inherit the stroke and fill that were specified outside it? Probably so... but in that case, maybe transforms *should* apply too.
My thinking went down that path, too, but transform="" is kind of like a non-inherited property (and in the future, transform="" will be a presentation attribute for the 'transform' property), and I think that's the difference. If you have a filter applied to an ancestor element of the <g glyphid>, then I don't think that should affect how the glyph is rendered. 'fill' and 'stroke', OTOH, are inherited, so the values of those properties on the <g glyphid> are actually different and should influence the rendering.
Yes, I can see a difference between properties like fill and stroke that are *inherited* and something like transform (or filter) that applies because it is a modification of the context within which the element is rendered. So arguably it makes sense for the inherited attributes to continue to be inherited, but the context modifications are irrelevant when the glyph is rendered within a different context. On the other hand, the element being used as the glyph is not entirely divorced from its original context, in that it can refer to other parts of the document (e.g. to use as a filter). So it's not like we're "excerpting" the glyph subtree alone and completely ignoring its owning SVG document. We could also argue that the rendering of the <g glyphid...> when used as a glyph should ideally be exactly the same as the rendering of that same element within its "host" SVG document. A designer might use Inkscape (for example) to create a document with a number of graphical elements that are then each tagged with separate glyph IDs; they'd probably expect those glyphs to render just like they did within the original document, regardless of whether an effect such as a filter or transform had been applied "above" or "below" the level at which the glyphid attributes are specified. Not sure what will seem most natural to authors...
Against comment #12, I see no particular reason for authors to apply "transform" to the ancestors of glyphs, and I don't expect to see that in the wild. So I think we only need to choose some behavior and stick to it (and specify it). I agree with comment 11.
I'd imagined that people might write their SVG documents so that they could see all of the glyphs at once, like a font specimen, probably by using transforms to place them around the canvas. That'd be useful for developing the glyphs in the browser or a plain SVG authoring tool. And then when the individual glyphs were used, the transforms from the ancestors wouldn't be used. You could get the same effect without having to rely on the ancestor transforms not applying, using <use> elements to display the glyphs at particular positions on the canvas, but it'd be nicer not to have to.
Version: unspecified → 18 Branch
Version: 18 Branch → 17 Branch
Blocks: 719286
This is a rebased version of your patch with the addition of a !aTransformRoot check in various implementations of GetCanvasTM() where it was missing. With this, my SVG-in-OT examples that include transforms seem to be rendering fine (regardless of the setting of svg.display-lists.painting.enabled); and tryserver says that the SVG reftests still pass (https://tbpl.mozilla.org/?tree=Try&rev=ba88f5209bc6).
Attachment #799440 - Flags: review?(cam)
Assignee: nobody → jfkthame
BTW, the page at http://people.mozilla.org/~jkew/opentype-svg/GeckoEmoji.html can serve as an example for this. With current Nightly, svg.display-lists.painting.enabled=false is needed to work around the ignored-transform bug; but with the tryserver build (above) this is no longer necessary.
A quick thought before I review properly tomorrow: what about an approach that, rather than passing the transform root through the recursive GetCanvasTM calls, we set a state bit on the transform root frame and have the GetCanvasTM implementations check that instead? Is it worth avoiding the extra argument passing?
Flags: needinfo?(jfkthame)
Wouldn't that make it harder for GetCanvasTM to know whether or not there is -any- transform root in effect? Which often seems to be important for knowing whether we can take the "early exit" that just returns nsSVGIntegrationUtils::GetCSSPxToDevPxMatrix(this). So as far as I can see right now, I think it's better to pass it down.
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #18) > So as far as I can see right now, I think it's better to pass it down. OK.
Comment on attachment 799440 [details] [diff] [review] don't ignore transforms when rendering SVG-in-OT glyphs Review of attachment 799440 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I'd like to see a test added. r=me with a test, and the two comments below. ::: layout/svg/nsISVGChildFrame.h @@ +44,5 @@ > NS_DECL_QUERYFRAME_TARGET(nsISVGChildFrame) > > // Paint this frame - aDirtyRect is the area being redrawn, in frame > // offset pixel coordinates > NS_IMETHOD PaintSVG(nsRenderingContext* aContext, Can you document the whole purpose of aTransformRoot here? ::: layout/svg/nsSVGFilterPaintCallback.h @@ +21,5 @@ > * to mess with context state. > * The context will be configured to use the "user space" coordinate > * system. > * @param aDirtyRect the dirty rect *in user space pixels* > */ Could you add a @param line for aTransformRoot?
Attachment #799440 - Flags: review?(cam) → review+
Added comments as requested (do these make sense?); reftest to follow in a separate patch. Carrying forward r=heycam.
Attachment #799440 - Attachment is obsolete: true
Those comments look OK.
Attachment #801119 - Flags: review+
Attachment #760787 - Attachment is obsolete: true
Here's a test with three glyph definitions that use transforms in various ways. These all render incorrectly with current trunk (unless display-list rendering is disabled), but correctly once the patch here is applied.
Attachment #801126 - Flags: review?(cam)
Attachment #801126 - Attachment is obsolete: true
Attachment #801126 - Flags: review?(cam)
Comment on attachment 801132 [details] [diff] [review] reftest for use of transforms within SVG-in-OT glyphs. Review of attachment 801132 [details] [diff] [review]: ----------------------------------------------------------------- This is good, but can you just add one more subtest that puts a transform on an ancestor of an element with an id="glyph<n>", to test that the aTransformRoot handling stops at the right place? ::: layout/reftests/text-svgglyphs/reftest.list @@ +18,5 @@ > pref(gfx.font_rendering.opentype_svg.enabled,true) == svg-glyph-cachedopacity.svg svg-glyph-cachedopacity-ref.svg > pref(gfx.font_rendering.opentype_svg.enabled,true) == svg-glyph-objectvalue.svg svg-glyph-objectvalue-ref.svg > pref(gfx.font_rendering.opentype_svg.enabled,true) fails == svg-glyph-mask.svg svg-glyph-mask-ref.svg # bug 872483 > pref(gfx.font_rendering.opentype_svg.enabled,true) == svg-glyph-paint-server.svg svg-glyph-paint-server-ref.svg # bug 801467 > +pref(gfx.font_rendering.opentype_svg.enabled,true) == svg-glyph-transform.svg svg-glyph-transform-ref.svg # bug # bug 875329 s/# bug //
(In reply to Cameron McCormack (:heycam) from comment #25) > Comment on attachment 801132 [details] [diff] [review] > reftest for use of transforms within SVG-in-OT glyphs. > > Review of attachment 801132 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is good, but can you just add one more subtest that puts a transform on > an ancestor of an element with an id="glyph<n>", to test that the > aTransformRoot handling stops at the right place? Duh - good point, of course we should test that. And in fact...it turns out that doesn't work right. :( Will poke at it some more and see if I can figure out why. We should also have a test that accumulates multiple stages of transform within the glyph element. (That -does- work properly.)
(In reply to Jonathan Kew (:jfkthame) from comment #26) > (In reply to Cameron McCormack (:heycam) from comment #25) > > Comment on attachment 801132 [details] [diff] [review] > > reftest for use of transforms within SVG-in-OT glyphs. > > > > Review of attachment 801132 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This is good, but can you just add one more subtest that puts a transform on > > an ancestor of an element with an id="glyph<n>", to test that the > > aTransformRoot handling stops at the right place? > > Duh - good point, of course we should test that. And in fact...it turns out > that doesn't work right. :( Will poke at it some more and see if I can > figure out why. The problem here is that mCanvasTM for each of the frames gets set up during initial reflow of the SVG document, triggered from gfxSVGGlyphsDocument::SetupPresentation. As this is a reflow of the entire document, not of individual glyph elements, there's no per-glyph-element transform root set up, and so the TMs go all the way to the top of the document. And then during subsequent drawing, the GetCanvasTM functions see that mCanvasTM already exists, and simply use it, ignoring aTransformRoot. A possible solution might be to have GetCanvasTM remember the value of aTransformRoot for which mCanvasTM was set up, so they can discard and re-create it if called with a different root. I'll experiment with this and see how it goes. (Another option I considered was to avoid storing the TM during that initial reflow, when aFor is FOR_OUTERSVG_TM, and only create mCanvasTM on the first actual paint. However, that would still have problems in the - tricky but theoretically possible - case where one glyph definition includes another glyph with an added transform, as the root would vary depending which glyph is being painted.)
This resolves the problem of transforms from outside the glyph being included when painting. It should have no impact on non-SVGGlyph documents, as aTransformRoot will just remain null there. For glyph documents, I'm assuming the usual case is that reflow (without a transform root) happens once, then painting (with a root) happens many times, so caching the matrix is only really significant for the latter. The case where the same element is being painted by multiple glyphs with different transform roots should be rare enough that we don't need to care about the cost of discarding and re-creating the matrix.
Attachment #801471 - Flags: review?(cam)
You can remove all the cached mCanvasTM values by calling NotifySVGChanged(TRANSFORM_CHANGED) from the root frame if you need to. That's what we do for painting patterns or gradients where the different patterns/gradients may reference the same content with different transforms applied.
Hmm... I guess we could do that before painting the glyph (e.g. in nsSVGUtils::PaintSVGGlyph), but in general this would have a greater performance impact than the patch here, which only clears the cached value if the root is different (which usually won't be the case).
Something still isn't right here, even with the cached transforms disabled. In this example: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> <defs> <rect id="a" x="250" y="-750" width="100" height="500" /> </defs> <!-- the red glyphs should be 2x the size of the blue and green ones --> <g id="glyph4"> <use xlink:href="#a" fill="blue" /> </g> <g transform="scale(2)"> <g id="glyph5"> <use xlink:href="#a" fill="green" /> </g> </g> <g id="glyph6" > <g transform="scale(2)"> <use xlink:href="#a" fill="red" /> </g> </g> <g id="glyph7"> <rect x="250" y="-750" width="100" height="500" fill="blue" /> </g> <g transform="scale(2)"> <g id="glyph8"> <rect x="250" y="-750" width="100" height="500" fill="green" /> </g> </g> <g id="glyph9"> <g transform="scale(2)"> <rect x="250" y="-750" width="100" height="500" fill="red" /> </g> </g> </svg> I'm seeing glyph8 being rendered at 2x size (like the red glyphs), even though the scale(2) is *outside* the element identified as the glyph. But in the case of glyph5 (similar, but with a <use> element for its content rather than a literal <rect>), the outside scaling is correctly ignored.
(In reply to Jonathan Kew (:jfkthame) from comment #30) > Hmm... I guess we could do that before painting the glyph (e.g. in > nsSVGUtils::PaintSVGGlyph), but in general this would have a greater > performance impact than the patch here, which only clears the cached value > if the root is different (which usually won't be the case). Memory use would be lower though as you're not storing all those frame pointers.
OK, so there were a couple of problems remaining: (a) in nsSVGPathGeometryFrame::GetCanvasTM, we weren't checking the transform root; this accounts for the unexpected difference between glyph5 and glyph8 in comment 31. (b) the GetCanvasTM implementations should be checking aTransformRoot against the current frame (not its parent) when deciding whether to prepend to the parent's matrix; otherwise we'll fail to apply a transform attribute that is specified directly on the glyph element itself. I'll prepare an updated patch that incorporates these fixes, and a more extensive set of tests.
(In reply to Robert Longson from comment #32) > (In reply to Jonathan Kew (:jfkthame) from comment #30) > > Hmm... I guess we could do that before painting the glyph (e.g. in > > nsSVGUtils::PaintSVGGlyph), but in general this would have a greater > > performance impact than the patch here, which only clears the cached value > > if the root is different (which usually won't be the case). > > Memory use would be lower though as you're not storing all those frame > pointers. True - though if memory use is more critical than performance here, maybe we shouldn't be caching the canvasTM at all? :heycam, WDYT?
Updated to test more variations, including transform outside the glyph (which should be ignored).
Attachment #801625 - Flags: review?(cam)
Attachment #801132 - Attachment is obsolete: true
Attachment #801132 - Flags: review?(cam)
And an updated patch that makes those tests actually pass. This folds in the cached-mCanvasTM patch from comment 28, as well as the bug-fixes mentioned in comment 33. Question for review: should we pay the memory cost of adding the mTransformRootForCanvasTM member vars, or the perf cost of always discarding cached TMs when rendering SVG glyphs? (See discussion above.)
Attachment #801629 - Flags: review?(cam)
Attachment #801119 - Attachment is obsolete: true
Attachment #801471 - Attachment is obsolete: true
Attachment #801471 - Flags: review?(cam)
I think we should begin with discarding cached TMs when rendering SVG glyphs, as that's what we're doing in other SVG resource element situations. If performance is a problem we can revisit.
Comment on attachment 801625 [details] [diff] [review] reftest for use of transforms within SVG-in-OT glyphs. Review of attachment 801625 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/text-svgglyphs/reftest.list @@ +18,5 @@ > pref(gfx.font_rendering.opentype_svg.enabled,true) == svg-glyph-cachedopacity.svg svg-glyph-cachedopacity-ref.svg > pref(gfx.font_rendering.opentype_svg.enabled,true) == svg-glyph-objectvalue.svg svg-glyph-objectvalue-ref.svg > pref(gfx.font_rendering.opentype_svg.enabled,true) fails == svg-glyph-mask.svg svg-glyph-mask-ref.svg # bug 872483 > pref(gfx.font_rendering.opentype_svg.enabled,true) == svg-glyph-paint-server.svg svg-glyph-paint-server-ref.svg # bug 801467 > +pref(gfx.font_rendering.opentype_svg.enabled,true) == svg-glyph-transform.svg svg-glyph-transform-ref.svg # bug # bug 875329 Duplicate "# bug" still here.
Attachment #801625 - Flags: review?(cam) → review+
Comment on attachment 801629 [details] [diff] [review] don't ignore transforms when rendering SVG-in-OT glyphs. Review of attachment 801629 [details] [diff] [review]: ----------------------------------------------------------------- So I think I agree with Robert. Let's do the same thing that nsSVGMaskFrame, nsSVGMarkerFrame, etc. do and call NotifySVGChanged(nsISVGChildFrame::TRANSFORM_CHANGED) on the glyph to be painted, just before we call in to PaintSVG at the end of nsSVGUtils::PaintSVGGlyph. We probably need to have an SVG-wide solution to this at some point, so let's not have a workaround specific for SVG glyphs (which also increases every SVG frame size by a word) if we can help it. ::: layout/svg/nsSVGInnerSVGFrame.cpp @@ +140,5 @@ > (xOrYIsPercentage || > (widthOrHeightIsPercentage && svg->HasViewBoxRect()))) { > aFlags |= TRANSFORM_CHANGED; > } > + aFlags |= TRANSFORM_CHANGED; Is this a deliberate change?
Attachment #801629 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #39) > So I think I agree with Robert. Let's do the same thing that > nsSVGMaskFrame, nsSVGMarkerFrame, etc. do and call > NotifySVGChanged(nsISVGChildFrame::TRANSFORM_CHANGED) on the glyph to be > painted, just before we call in to PaintSVG at the end of > nsSVGUtils::PaintSVGGlyph. OK, thanks; will update accordingly. > > + aFlags |= TRANSFORM_CHANGED; > > Is this a deliberate change? Oops. No!
OK, here's the version that calls NotifySVGChanged on the glyph before painting it. This also seems to work fine, as expected.
Attachment #802806 - Flags: review?(cam)
Attachment #801629 - Attachment is obsolete: true
Comment on attachment 802806 [details] [diff] [review] don't ignore transforms when rendering SVG-in-OT glyphs. Looks good, thanks!
Attachment #802806 - Flags: review?(cam) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd077e283f6a (patch) https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf68e5c75ca (reftest) (Note that due to bug 871961, the reftest will not actually be run on TBPL at present. However, it has been confirmed to pass locally and on all tryserver platforms.)
Target Milestone: --- → mozilla26
https://hg.mozilla.org/mozilla-central/rev/fd077e283f6a https://hg.mozilla.org/mozilla-central/rev/4cf68e5c75ca
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: