User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.8) 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 2. 3. 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.

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

(In reply to comment #2) I selected you as reviewer. This bug also fixes not-anti-aliased rendering of clipPath in the following pages. http://people.mozilla.org/~prouget/demos/mashup/video.xhtml http://people.mozilla.org/~roc/clipPath.xhtml 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 request?

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 http://www.w3.org/TR/SVG11/painting.html#ShapeRenderingProperty 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

Comment on attachment 374201 [details] [diff] [review] A patch Additional code needed for nsSVGGlyphFrame as outlined in the previous comment. Good so far though.

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?

Comment on attachment 374259 [details] [diff] [review] Patch v2 > if (renderMode == nsSVGRenderState::CLIP_MASK) { >- gfx->SetAntialiasMode(gfxContext::MODE_ALIASED); >+ switch (GetStyleSVG()->mTextRendering) { >+ case NS_STYLE_TEXT_RENDERING_OPTIMIZESPEED: >+ 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.

patch v2.

Looks good. Thanks for this. I'll land it tomorrow assuming the tree is (mostly) green.

I see.

Checked in http://hg.mozilla.org/mozilla-central/rev/0fae24feb57a (modified for the nsSVGGlyphFrame issue) in comment 7. Thanks Ryo.

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?

Do we need this in 1.9.1?

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.

checked in http://hg.mozilla.org/mozilla-central/rev/b749959e07f3

checked in http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8e420e12ed63