Bug 23065 – Enable incremental painting for canvas

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Whenever something draws in a <canvas>, the entire canvas is repainted. This makes drawing with large canvas elements very slow.

There was an attempt to fix this in <http://trac.webkit.org/changeset/29448>, but it was turned off.

[Simon Fraser (smfr)]

<rdar://problem/6151191>

[Simon Fraser (smfr)]

Created attachment 26350 [details]
Patch, testcase, changelog.

[Darin Adler]

Comment on attachment 26350 [details]
Patch, testcase, changelog.

> -const char* defaultFont = "10px sans-serif";
> +static const char* defaultFont = "10px sans-serif";

This is still not doing what the original author probably intended. This is a variable, not a constant. It should be:

    static const char defaultFont[] = 10px sans-serif";

or

    static const char* const defaultFont = 10px sans-serif";

> +class CanvasStrokeStyleApplier : public StrokeStyleApplier {
> +public:
> +    CanvasStrokeStyleApplier(CanvasRenderingContext2D* canvasContext)
> +        : m_canvasContext(canvasContext)
> +    {
> +    }
> +    
> +    virtual void strokeStyle(GraphicsContext* c)
> +    {
> +        c->setStrokeThickness(m_canvasContext->lineWidth());
> +
> +        LineCap cap;
> +        if (parseLineCap(m_canvasContext->lineCap(), cap))
> +            c->setLineCap(cap);
> +
> +        LineJoin join;
> +        if (parseLineJoin(m_canvasContext->lineJoin(), join))
> +            c->setLineJoin(join);
> +
> +        c->setMiterLimit(m_canvasContext->miterLimit());
> +    }
> +
> +    CanvasRenderingContext2D* m_canvasContext;
> +};

Both strokeStyle and m_canvasContext should be private, not public. Because ... because why not? I say always make things private unless they're needed publicly.

> -void CanvasRenderingContext2D::willDraw(const FloatRect& r)
> +void CanvasRenderingContext2D::willDraw(const FloatRect& r, bool applyTransform, bool applyShadow, bool applyClip)

Functions that take a bunch of booleans make the code hard to read. This should either use an enum for each boolean or a flags word. Either is easier to read at the call site because of the use of named constants.

> +    if (fill)
> +        m_canvas->willDraw(textRect);
> +    else
> +        // When stroking text, pointy miters can extend outside of textRect, so we
> +        // punt and dirty the whole canvas.
> +        m_canvas->willDraw(FloatRect(0, 0, m_canvas->width(), m_canvas->height()));

Need braces around multi-line else body here (number of lines includes things like comments).

Would be better to have a helper function for "will draw the entire canvas". Code that constructs the rectangle is confusing.

>      CanvasStyle* drawStyle = fill ? state().m_fillStyle.get() : state().m_strokeStyle.get();
>      if (drawStyle->canvasGradient() || drawStyle->canvasPattern()) {
> +        // FIXME The rect is not big enough for miters on stroked text

We should have a colon after the FIXME. We should have a period at the end of the sentence.

I don't understand why it's OK to check in with this FIXME. Will this result in incorrect canvas drawing?

> +// Map rect r from srcRect to an equivalent rect in destRect

Sentence-capitalized comment needs a period.

> +inline FloatRect mapRect(const FloatRect& r, const FloatRect& srcRect, const FloatRect& destRect)
> +{
> +    float widthScale = destRect.width() / srcRect.width();
> +    float heightScale = destRect.height() / srcRect.height();
> +    return FloatRect(destRect.x() + (r.x() - srcRect.x()) * widthScale,
> +                     destRect.y() + (r.y() - srcRect.y()) * heightScale,
> +                     r.width() * widthScale, r.height() * heightScale);
> +}

Why is this inline? Is the behavior with 0 width and 0 height acceptable?

Does this really work reliably? Our previous attempts to turn this on have had bad results. Is the test you added extensive enough to prove we won't have trouble? Did you test with the sites that exercise canvas heavily, like the one that does the 3D Doom-style game?

I'm going to say r=em, because none of these comments seem like showstoppers to me

[Sam Weinig]

+        LineCap cap;
+        if (parseLineCap(m_canvasContext->lineCap(), cap))
+            c->setLineCap(cap);
+
+        LineJoin join;
+        if (parseLineJoin(m_canvasContext->lineJoin(), join))
+            c->setLineJoin(join);

It seems a little silly to call these parse methods here, since we store the LineCap and LineJoin data internally already.  For instance, m_canvasContext->lineCap() calls lineCapName(state().m_lineCap) for the sake of the API.  We can just add accessors that get at the data directly to avoid this round trip.

[Simon Fraser (smfr)]

(In reply to comment #3)
> (From update of attachment 26350 [details] [review])

> >      CanvasStyle* drawStyle = fill ? state().m_fillStyle.get() : state().m_strokeStyle.get();
> >      if (drawStyle->canvasGradient() || drawStyle->canvasPattern()) {
> > +        // FIXME The rect is not big enough for miters on stroked text
> 
> We should have a colon after the FIXME. We should have a period at the end of
> the sentence.
> 
> I don't understand why it's OK to check in with this FIXME. Will this result in
> incorrect canvas drawing?

This is not a new issue with this patch; it's an existing problem. I filed bug 23067.

[Simon Fraser (smfr)]

Test sites:
http://jsmsxdemo.googlepages.com/jsmsx.html
http://caimansys.com/painter/

[Simon Fraser (smfr)]

Created attachment 26353 [details]
Final patch

Final patch for reference.

[Oliver Hunt]

Comment on attachment 26353 [details]
Final patch

Instead of <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> use <!DOCTYPE html>

otherwise r=me!

[Simon Fraser (smfr)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/canvas/canvas-incremental-repaint.html
	A	LayoutTests/platform/mac/fast/canvas/canvas-incremental-repaint-expected.checksum
	A	LayoutTests/platform/mac/fast/canvas/canvas-incremental-repaint-expected.png
	A	LayoutTests/platform/mac/fast/canvas/canvas-incremental-repaint-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/html/CanvasRenderingContext2D.cpp
	M	WebCore/html/CanvasRenderingContext2D.h
	M	WebCore/html/HTMLCanvasElement.cpp
	M	WebCore/platform/graphics/FloatRect.cpp
	M	WebCore/platform/graphics/FloatRect.h
Committed r39538