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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
SVG
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Ryo Onodera, Assigned: Ryo Onodera)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
fixed1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

9 years ago
Created attachment 374201 [details] [diff] [review]
A patch

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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

9 years ago
Attachment #374201 - Flags: review?(longsonr)
(Assignee)

Comment 3

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

Comment 6

9 years ago
Created attachment 374259 [details] [diff] [review]
Patch v2

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
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1?
(Assignee)

Updated

9 years ago
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) {
>+      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.
Attachment #374259 - Flags: review?(longsonr) → review-
(Assignee)

Comment 8

9 years ago
Created attachment 374367 [details] [diff] [review]
patch v2
[Checkin: Comment 11]

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

Comment 10

9 years ago
I see.
Summary: Clipping should depend on mShapeRendering → image-rendering and text-rendering hints should operate when clipping
Checked in http://hg.mozilla.org/mozilla-central/rev/0fae24feb57a (modified for the nsSVGGlyphFrame issue) in comment 7. Thanks Ryo.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

9 years ago
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?
(Assignee)

Comment 15

9 years ago
Created attachment 374574 [details] [diff] [review]
A patch by hg export for reftest

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+
Attachment #374367 - Flags: approval1.9.1+

Updated

9 years ago
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]
checked in http://hg.mozilla.org/mozilla-central/rev/b749959e07f3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing]
checked in http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8e420e12ed63
Keywords: fixed1.9.1
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.