Bug 223056 – Optimize canvas repaints

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

[Simon Fraser (smfr)]

Optimize canvas repaints

[Simon Fraser (smfr)]

Created attachment 422900 [details]
For EWS

[Simon Fraser (smfr)]

Created attachment 422999 [details]
For EWS

[Simon Fraser (smfr)]

Created attachment 423244 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 423245 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 423245 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1322
>          boundingRect.inflate(state().lineWidth / 2);

Should we call inflateStrokeRect() since it handles the miterLimit and lineCap of the stroke?

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2094
> +    if (m_dirtyRect == oldDirtyRect)
> +        canvasBase().didDraw(WTF::nullopt);
> +    else
> +        canvasBase().didDraw(m_dirtyRect);

Maybe

if (m_dirtyRect.contains(oldDirtyRect))
    canvasBase().didDraw(WTF::nullopt);
else {
    m_dirtyRect.unite(dirtyRect);
    canvasBase().didDraw(m_dirtyRect);
}

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2104
> +    return m_dirtyRect == backingStoreBounds();

Is it okay to check for FloatRect equality like this? Should we use some sort of areEssentiallyEqual()? Or maybe comparing enclosingIntRect() instead?

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2549
> +    if (isEntireBackingStoreDirty())
> +        didDraw(WTF::nullopt);
> +    else if (repaintEntireCanvas)
> +        didDrawEntireCanvas();
> +    else
>          didDraw(textRect);

This code was repeated multiple times in this patch. Can't this logic be moved to didDraw()? For example, pass 'repaintEntireCanvas' to didDraw() and make didDraw() check for isEntireBackingStoreDirty().

[Simon Fraser (smfr)]

> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2549
> > +    if (isEntireBackingStoreDirty())
> > +        didDraw(WTF::nullopt);
> > +    else if (repaintEntireCanvas)
> > +        didDrawEntireCanvas();
> > +    else
> >          didDraw(textRect);
> 
> This code was repeated multiple times in this patch. Can't this logic be
> moved to didDraw()? For example, pass 'repaintEntireCanvas' to didDraw() and
> make didDraw() check for isEntireBackingStoreDirty().

I need to avoid computing the rect in this case:
   didDraw(path.fastBoundingRect());

which hampers refactoring this (hence my comment in the change log about WTF::function<> approaches).

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/274461/webkit

[Radar WebKit Bug Importer]

<rdar://problem/75460317>