Closed Bug 489718 Opened 11 years ago Closed 11 years ago

image-rendering and text-rendering hints should operate when clipping


(Core :: SVG, defect)

Not set





(Reporter: ryoqun, Assigned: ryoqun)




(Keywords: fixed1.9.1)


(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/2009033100 Ubuntu/9.04 (jaunty) Firefox/3.0.8
Build Identifier: 

When a non-trivial clipping path is rendered, SetAntialiasMode(gfxContext::MODE_ALIASED) is called regardless of mShapeRendering, whose value should be set depending on mShapeRendering. Moreover, gfxContext::MODE_ALIASED is opposite to the default value of mShapeRendering. Thus, in many cases, clipping's edge is not antialiased where actually it should be.

When a trivial clipping path is rendered, this is not affected because the 'if' immediately enclosing SetAntialiasMode(gfxContext::MODE_ALIASED) evaluates to false (In this case, renderMode is nsSVGRenderState::CLIP).

Reproducible: Always

Steps to Reproduce:
1. Open the URL
Actual Results:  
The edges of two darkblue circles in the left image should be anti-aliased.

Expected Results:  
The edges of two darkblue circles in the left image aren't anti-aliased.
Attached patch A patch (obsolete) — Splinter Review
This patch fixes the problem in the way described as this bug's detail.
You need to pick a reviewer for the patch. That way jwatt and I know who's doing what :-) 

Set the review flag for the patch to ? (indicating that review is requested) and enter the email address of the person from whom you are requesting review
Assignee: nobody → ryoqun
Ever confirmed: true
Attachment #374201 - Flags: review?(longsonr)
(In reply to comment #2)
I selected you as reviewer.

This bug also fixes not-anti-aliased rendering of clipPath in the following
Those pages are showcasing new capabilities of upcoming Firefox 3.5, and this
untidiness of rendering is rather apparent to end users, thus, can you flag
"wanted1.9.1" to "+"? I only saw "?" at the combo box, not "+". "?" means this
My only review comment is that you need to write a bit more code to fix this bug completely as you've only done shapes and not text.

The same thing exists in nsSVGGlyphFrame.cpp In that case however you must switch on GetStyleSVG()->mTextRendering and according to only optimizeSpeed (i.e. NS_STYLE_TEXT_RENDERING_OPTIMIZESPEED) should be MODE_ALIASED and everything else would then by MODE_COVERAGE.

You should set the flag to wanted1.9.1? which means you are asking for it and then someone with authority to do so will move it from ? to + which means accepted. 

That flag is only a hint though. The key flag is really to set approval1.9.1? on the patch and to do that you need the following things first...

a) a review + from me 
b) the patch to land on the trunk
Attachment #374201 - Flags: review?(longsonr) → review-
Comment on attachment 374201 [details] [diff] [review]
A patch

Additional code needed for nsSVGGlyphFrame as outlined in the previous comment. Good so far though.
Attached patch Patch v2 (obsolete) — Splinter Review
Here is the improved patch.
Sorry, but my patch had regression. After SetAntialiasMode() is correctly set, gfx->Restore() is called if renderMode != nsSVGRenderState::NORMAL. I fixed this, and tested by opening svg files while changing "shape-rendering". Appropriate-rendering effects are confirmed this time.

However, with my patch applied, and inside and outside <clipPath>, even if I turn on or turn off "optimizeSpeed" of "text-rendering", I couldn't see difference between patched Firefox and unpatched Firerox other than the minor glyph shift mentioned later. It seems that setting SetAntialiasMode(MODE_COVERAGE) doesn't affect glyph rendering at all.

As mentioned before, "optimizedSpeed" affects the rendering result of glyphs a bit regardless with and without my patch. Glyphs are shifted horizontally along one pixel or so. Maybe, hinting part of gfx is using the value of text-rendering? Is this intended/expected behaviour?

I've tested only on Ubuntu 9.04, testing on other platforms by someone else is needed?
Attachment #374201 - Attachment is obsolete: true
Flags: wanted1.9.1?
Attachment #374259 - Flags: review?(longsonr)
Comment on attachment 374259 [details] [diff] [review]
Patch v2

>     if (renderMode == nsSVGRenderState::CLIP_MASK) {
>-      gfx->SetAntialiasMode(gfxContext::MODE_ALIASED);
>+      switch (GetStyleSVG()->mTextRendering) {
>+        gfx->SetAntialiasMode(gfxContext::MODE_ALIASED);
>+        break;
>+      default:
>+        gfx->SetAntialiasMode(gfxContext::MODE_COVERAGE);
>+        break;
>+      }

This code needs to be called whether or not we are in normal mode. You need to move it up a bit so it's outside the normal test.

>       gfx->SetColor(gfxRGBA(1.0f, 1.0f, 1.0f, 1.0f));
>       FillCharacters(&iter, gfx);
>     } else {
>diff -r 8f5ce95d8ad8 layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>--- a/layout/svg/base/src/nsSVGPathGeometryFrame.cpp	Mon Apr 20 14:08:03 2009 -0700
>+++ b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp	Thu Apr 23 20:49:06 2009 +0900
>@@ -442,20 +442,6 @@
>   if (renderMode != nsSVGRenderState::NORMAL) {
>     gfx->Restore();

Can we just put the Restore code at the end of the drawing i.e. just before the return like we do for the NORMAL case. Then we only have one test for normal.
Attachment #374259 - Flags: review?(longsonr) → review-
patch v2.
Attachment #374259 - Attachment is obsolete: true
Attachment #374367 - Flags: review?(longsonr)
Attachment #374367 - Flags: review?(longsonr) → review+
Looks good. Thanks for this. I'll land it tomorrow assuming the tree is (mostly) green.
I see.
Summary: Clipping should depend on mShapeRendering → image-rendering and text-rendering hints should operate when clipping
Checked in (modified for the nsSVGGlyphFrame issue) in comment 7. Thanks Ryo.
Closed: 11 years ago
Resolution: --- → FIXED
You're welcome. The code move in my patch on Comment 8 should go up further. Sorry for my carelessness. And I'm really happy because my patch is checked in. Thanks too!
We could have a reftest here, couldn't we?
Reftest created. At the moment, I couldn't come up with one using pass.svg. Also, it uses two circles. This may sound redundant, but when I use only one circle, SVGClipPathFrame considers itself "trivial" and takes different code path. In that case, "shape-rendering" DOES NOT take effect. I don't know much about why. Maybe, gfx related? At least even in that case, SetAntialiasMode() is called, I guess. As a conlution, I must use two or more or even random <whatever/> elements, because svg frame constructor makes frames for such elements as SVGGenericFrame, and SVGClipPathFrame naively considers itself not-"trivial" when it contains them.

Also, maybe, I should make one for text-rendering too, but as I commented in Comment 6, the effects of text-rendering is hardly noticeable at least on Ubuntu 9.04. So, I omited intentionally.

Finally, about importing this in 1.9.1, my opinion is in Comment 3.
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Attachment #374367 - Attachment description: patch v2 → patch v2 [Checkin: Comment 11]
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing]
Depends on: 525363
Blocks: 525363
No longer depends on: 525363
You need to log in before you can comment on or make changes to this bug.