Last Comment Bug 710521 - Refactor gfxFont to separate out drawing stroke and drawing to path
: Refactor gfxFont to separate out drawing stroke and drawing to path
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla12
Assigned To: Edwin Flores [:eflores] [:edwin]
:
Mentors:
Depends on: 745676 771853
Blocks: 719286 719288
  Show dependency treegraph
 
Reported: 2011-12-13 20:18 PST by Edwin Flores [:eflores] [:edwin]
Modified: 2012-07-07 19:26 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (33.37 KB, patch)
2011-12-13 20:18 PST, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Review
Patch #2 (34.43 KB, patch)
2011-12-14 14:34 PST, Edwin Flores [:eflores] [:edwin]
roc: review+
Details | Diff | Review
Patch #3 (36.04 KB, text/plain)
2011-12-18 18:53 PST, Edwin Flores [:eflores] [:edwin]
no flags Details
Patch Numero Quattro (35.82 KB, patch)
2011-12-18 19:39 PST, Edwin Flores [:eflores] [:edwin]
roc: review+
Details | Diff | Review
Same patch, just rebased (42.86 KB, patch)
2012-01-18 15:06 PST, Edwin Flores [:eflores] [:edwin]
roc: review+
Details | Diff | Review

Description Edwin Flores [:eflores] [:edwin] 2011-12-13 20:18:47 PST
Created attachment 581522 [details] [diff] [review]
Patch
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-13 20:45:15 PST
+            mTextRun->Draw(mThebes,
+                           point,
+                           gfxFont::GLYPH_STROKE,
+                           0,
+                           mTextRun->GetLength(),
+                           nsnull,
+                           nsnull);
         else
             // mOp == TEXT_DRAW_OPERATION_FILL
             mTextRun->Draw(mThebes,
                            point,
+                           gfxFont::GLYPH_FILL,
                            0,
                            mTextRun->GetLength(),
                            nsnull,
                            nsnull);

Combine these into a single Draw call with a conditional expression for the flags.

+        if (aDrawMode == gfxFont::GLYPH_FILL)
+            DrawPartialLigature(font, aContext, start, ligatureRunStart, &pt, aProvider);

+        if (aDrawMode == gfxFont::GLYPH_FILL)
+            DrawPartialLigature(font, aContext, ligatureRunEnd, end, &pt, aProvider);

{} around if body

Not being able to draw partial ligatures with stroking sucks a bit, but we'll deal I guess. We could pass the draw mode into DrawPartialLigature, which draws the glyph with some clipping to cut it up into pieces, but that wouldn't work well with stroking.

+    typedef enum {
+        GLYPH_FILL      = 1,
+        GLYPH_STROKE    = 2,
+        GLYPH_PATH      = 4
+    } DrawMode;

Need to document that GLYPH_FILL and GLYPH_STROKE can be used together, but they can't be used with GLYPH_PATH. Actually, add NS_ASSERTIONs about that to gfxTextRun:::Draw and gfxFont::Draw. Also document that GLYPH_STROKE destroys the current path in the context.

Otherwise looks good!! Please post a new patch.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-13 20:45:40 PST
+        GLYPH_FILL      = 1,
+        GLYPH_STROKE    = 2,
+        GLYPH_PATH      = 4

Also the indenting before = is unnecessary.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-13 20:51:44 PST
CCing font guys for feedback.

This is a relatively simple change to avoid using text-to-path when we just need to stroke the text. We're doing this so that when we introduce fonts with SVG glyphs, stroking can work well. (It's difficult/impossible to get a path for an SVG glyph in general.)
Comment 4 Edwin Flores [:eflores] [:edwin] 2011-12-14 14:34:08 PST
Created attachment 581791 [details] [diff] [review]
Patch #2
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-14 14:53:05 PST
Comment on attachment 581791 [details] [diff] [review]
Patch #2

Review of attachment 581791 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff!

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +2796,5 @@
>          }
>  
> +        mTextRun->Draw(mThebes,
> +                       point,
> +                       mOp == nsCanvasRenderingContext2D::TEXT_DRAW_OPERATION_STROKE ? gfxFont::GLYPH_STROKE : gfxFont::GLYPH_FILL,

80 column line limit
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 00:59:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/2674bffd5575
Comment 7 Matt Brubeck (:mbrubeck) 2011-12-18 08:18:12 PST
Backed out on inbound (along with other patches in the same push) because of Mac and Win7 reftest failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff088809cd2a
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 13:54:22 PST
data:text/html,%3C%21DOCTYPE%20html%3E%3Ctitle%3ELoading%20reftest%20analyzer%20for%20Mozilla-Inbound%20Rev3%20WINNT%205.1%20mozilla-inbound%20opt%20test%20reftest%3C/title%3E%3Clink%20rel%3D%22stylesheet%22%20href%3D%22https%3A//tbpl.mozilla.org/css/style.css%22%3E%3Cbody%3E%3Cp%20class%3D%22loading%22%3ERetrieving%20reftest%20log...%3C/p%3E%3Cscript%20src%3D%22https%3A//tbpl.mozilla.org/js/jquery.js%22%3E%3C/script%3E%3Cscript%20src%3D%22https%3A//tbpl.mozilla.org/js/NetUtils.js%22%3E%3C/script%3E%3Cscript%3E%0Afunction%20encode%28s%29%20%7B%20return%20escape%28s%29.replace%28/%3D/g%2C%20%22%253d%22%29%20%7D%0ANetUtils.loadText%28%22https%3A%5C/%5C/tbpl.mozilla.org%5C/php%5C/getLogExcerpt.php%3Fid%3D8012816%26type%3Dreftest%22%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20function%28log%29%20%7B%20window.location.replace%28%22https%3A//hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml%23log%3D%22%20+%20encode%28encode%28log%29%29%29%20%7D%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20function%28%29%20%7B%20%24%28%22p%22%29.removeClass%28%22loading%22%29.text%28%22Fetching%20reftest%20log%20failed.%22%29%20%7D%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20function%28%29%20%7B%20%24%28%22p%22%29.removeClass%28%22loading%22%29.text%28%22Fetching%20reftest%20log%20timed%20out.%22%29%20%7D%29%3B%0A%3C/script%3E

REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/scrolling/iframe-border-radius.html | image comparison (==) 

The test failure is caused by the reference stroking all three characters at once, while the test strokes the characters individually. This gives different rendering when the strokes on the characters partially overlap. I'm not sure why this patch changed the behavior here, but we can probably work around it by putting spaces between the characters in the tests so they don't overlap.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 16:47:25 PST
Argh, posted wrong URL
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 17:14:49 PST
My guess at the problem was also wrong.

My next guess is that Quartz and D2D are rasterizing path("ABC") differently from path("A") U path("B") U path("C") even where the glyphs don't overlap.
Comment 11 Edwin Flores [:eflores] [:edwin] 2011-12-18 18:53:40 PST
Created attachment 582732 [details]
Patch #3

Just updates the reftest. Draws each letter separately to get rid of weird (negligible) artefacts from drawing more than one glyph at a time.
Comment 12 Edwin Flores [:eflores] [:edwin] 2011-12-18 19:08:56 PST
Hmm, ignore that last patch
Comment 13 Edwin Flores [:eflores] [:edwin] 2011-12-18 19:39:37 PST
Created attachment 582738 [details] [diff] [review]
Patch Numero Quattro

Again, just updated the failing reftest.
Comment 14 Edwin Flores [:eflores] [:edwin] 2012-01-18 15:06:10 PST
Created attachment 589664 [details] [diff] [review]
Same patch, just rebased
Comment 16 Marco Bonardo [::mak] 2012-01-26 16:05:31 PST
https://hg.mozilla.org/mozilla-central/rev/eba8a5ac183f
Comment 17 :Ms2ger 2012-01-27 09:50:25 PST
Comment on attachment 589664 [details] [diff] [review]
Same patch, just rebased

>--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+    bool doUseIntermediateSurface = NeedToUseIntermediateSurface()
>+        || NeedIntermediateSurfaceToHandleGlobalAlpha(style);

Note: we put || at the end of the line, not at the start.

Note You need to log in before you can comment on or make changes to this bug.