Bug 75792 – Correct the bounding rect estimations for stroking text in canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Dongseong Hwang]

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.

[Dongseong Hwang]

This patch calculates the rect using the more precise way.

[Dongseong Hwang]

Created attachment 121571 [details]
patch

This patch calculates the rect using the more precise way.

[Dongseong Hwang]

Created attachment 121572 [details]
patch v.2

This patch can handle stroke(), also.

[Dongseong Hwang]

Created attachment 121573 [details]
patch v.3

[Eric Seidel (no email)]

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

[Dongseong Hwang]

Created attachment 127154 [details]
patch v.2

I attached layout test for stroke with cap and join.

[Matthew Delaney]

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?

[Matthew Delaney]

It would be great if you posted a standalone testcase so that I don't have to apply your patch to try it out.

[Dongseong Hwang]

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.

[Dongseong Hwang]

Created attachment 127346 [details]
patch v.5

Separate a layout test.

[Dongseong Hwang]

Created attachment 127347 [details]
a layout test for patch

[Dongseong Hwang]

(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?

[Simon Fraser (smfr)]

Comment on attachment 127346 [details]
patch v.5

Please put the code change and the test into the same patch in future.

[Simon Fraser (smfr)]

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.

[WebKit Review Bot]

Comment on attachment 127346 [details]
patch v.5

Clearing flags on attachment: 127346

Committed r114679: <http://trac.webkit.org/changeset/114679>

[WebKit Review Bot]

Comment on attachment 127347 [details]
a layout test for patch

Clearing flags on attachment: 127347

Committed r114680: <http://trac.webkit.org/changeset/114680>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.