Bug 75792 - Correct the bounding rect estimations for stroking text in canvas
Summary: Correct the bounding rect estimations for stroking text in canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-07 23:52 PST by Dongseong Hwang
Modified: 2012-04-19 15:08 PDT (History)
4 users (show)

See Also:


Attachments
patch (3.96 KB, patch)
2012-01-07 23:55 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
patch v.2 (4.69 KB, patch)
2012-01-08 01:30 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
patch v.3 (4.69 KB, patch)
2012-01-08 01:32 PST, Dongseong Hwang
eric: review-
Details | Formatted Diff | Diff
patch v.2 (13.29 KB, patch)
2012-02-15 03:50 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
patch v.5 (4.75 KB, patch)
2012-02-16 03:00 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
a layout test for patch (8.95 KB, patch)
2012-02-16 03:01 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2012-01-07 23:52:22 PST
CanvasRenderingContext2D::drawTextInternal made the bounding rect as big as canvas's size for invalidate.
It is because miters on stroked text may cause the rect so big.
Comment 1 Dongseong Hwang 2012-01-07 23:53:16 PST
This patch calculates the rect using the more precise way.
Comment 2 Dongseong Hwang 2012-01-07 23:55:41 PST
Created attachment 121571 [details]
patch

This patch calculates the rect using the more precise way.
Comment 3 Dongseong Hwang 2012-01-08 01:30:03 PST
Created attachment 121572 [details]
patch v.2

This patch can handle stroke(), also.
Comment 4 Dongseong Hwang 2012-01-08 01:32:29 PST
Created attachment 121573 [details]
patch v.3
Comment 5 Eric Seidel (no email) 2012-02-10 10:35:34 PST
Comment on attachment 121573 [details]
patch v.3

View in context: https://bugs.webkit.org/attachment.cgi?id=121573&action=review

How do we test this?  r- for lack of test.

> Source/WebCore/ChangeLog:3
> +        Canvas more precisely makes the bounding rect for strok rendering.

stroke
Comment 6 Dongseong Hwang 2012-02-15 03:50:38 PST
Created attachment 127154 [details]
patch v.2

I attached layout test for stroke with cap and join.
Comment 7 Matthew Delaney 2012-02-15 10:24:31 PST
Comment on attachment 127154 [details]
patch v.2

View in context: https://bugs.webkit.org/attachment.cgi?id=127154&action=review

> LayoutTests/fast/canvas/script-tests/canvas-strokePath-cap-join.js:2
> +

Looks like this test could use layoutTestController.dumpAsText(), see: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2163
> +    // Fast approximation of the stroke's bounding rect.

I noticed this comment was just copied over from above. Is it still true? How much faster is it than strokeBoundingRect?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:310
> +    void inflateStrokeRect(FloatRect&) const;

Should this functionality live here? It doesn't sound all that specific to canvas to me if it's just estimating the bounding rect of a section of stroked text. Don't we have some set of "text metrics" functions elsewhere?
Comment 8 Matthew Delaney 2012-02-15 10:27:44 PST
It would be great if you posted a standalone testcase so that I don't have to apply your patch to try it out.
Comment 9 Dongseong Hwang 2012-02-16 02:52:42 PST
Comment on attachment 127154 [details]
patch v.2

View in context: https://bugs.webkit.org/attachment.cgi?id=127154&action=review

>> LayoutTests/fast/canvas/script-tests/canvas-strokePath-cap-join.js:2
>> +
> 
> Looks like this test could use layoutTestController.dumpAsText(), see: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

canvas-strokePath-cap-join.html has <script src="../js/resources/js-test-pre.js"></script>
And then js-test-pre.js has layoutTestController.dumpAsText(self.enablePixelTesting);
I used canvas-strokePath-shadow.html as template for this layout test.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2163
>> +    // Fast approximation of the stroke's bounding rect.
> 
> I noticed this comment was just copied over from above. Is it still true? How much faster is it than strokeBoundingRect?

Yes, it is true.
I think more than 10 times faster than skia and qt implementation.
Skia draws it and measure it.
Qt creates stroke path and call Path::boundingRect().

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:310
>> +    void inflateStrokeRect(FloatRect&) const;
> 
> Should this functionality live here? It doesn't sound all that specific to canvas to me if it's just estimating the bounding rect of a section of stroked text. Don't we have some set of "text metrics" functions elsewhere?

I thought it is better to make Path::fastStrokeBoundingRect() similar to Path::fastBoundingRect().
However, Path does not know thickness, join, and cap, and strangely GraphicsContext knows those.

On the other hands, inflateStrokeRect(FloatRect&) needs to know CanvasRenderingContext2d::state.
It is why I put inflateStrokeRect(FloatRect&) as member method.
Comment 10 Dongseong Hwang 2012-02-16 03:00:34 PST
Created attachment 127346 [details]
patch v.5

Separate a layout test.
Comment 11 Dongseong Hwang 2012-02-16 03:01:06 PST
Created attachment 127347 [details]
a layout test for patch
Comment 12 Dongseong Hwang 2012-02-16 03:02:38 PST
(In reply to comment #8)
> It would be great if you posted a standalone testcase so that I don't have to apply your patch to try it out.

Ok, I separated a patch and a layout test.
Do I understand what you mean?
Comment 13 Simon Fraser (smfr) 2012-04-19 14:30:13 PDT
Comment on attachment 127346 [details]
patch v.5

Please put the code change and the test into the same patch in future.
Comment 14 Simon Fraser (smfr) 2012-04-19 14:31:04 PDT
Comment on attachment 127347 [details]
a layout test for patch

I'm pretty sure there's a canvas repaint test that will need new pixel results after this change too.
Comment 15 WebKit Review Bot 2012-04-19 15:05:53 PDT
Comment on attachment 127346 [details]
patch v.5

Clearing flags on attachment: 127346

Committed r114679: <http://trac.webkit.org/changeset/114679>
Comment 16 WebKit Review Bot 2012-04-19 15:08:07 PDT
Comment on attachment 127347 [details]
a layout test for patch

Clearing flags on attachment: 127347

Committed r114680: <http://trac.webkit.org/changeset/114680>
Comment 17 WebKit Review Bot 2012-04-19 15:08:13 PDT
All reviewed patches have been landed.  Closing bug.