Bug 223056 - Optimize canvas repaints
Summary: Optimize canvas repaints
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 223203
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-10 21:40 PST by Simon Fraser (smfr)
Modified: 2021-03-15 20:08 PDT (History)
22 users (show)

See Also:


Attachments
For EWS (19.18 KB, patch)
2021-03-10 21:47 PST, Simon Fraser (smfr)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
For EWS (30.17 KB, patch)
2021-03-11 17:19 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (21.80 KB, patch)
2021-03-15 14:50 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (21.89 KB, patch)
2021-03-15 14:51 PDT, Simon Fraser (smfr)
sabouhallawa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2021-03-10 21:40:10 PST
Optimize canvas repaints
Comment 1 Simon Fraser (smfr) 2021-03-10 21:47:17 PST
Created attachment 422900 [details]
For EWS
Comment 2 Simon Fraser (smfr) 2021-03-11 17:19:19 PST
Created attachment 422999 [details]
For EWS
Comment 3 Simon Fraser (smfr) 2021-03-15 14:50:43 PDT
Created attachment 423244 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-03-15 14:51:36 PDT
Created attachment 423245 [details]
Patch
Comment 5 Said Abou-Hallawa 2021-03-15 16:45:25 PDT
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().
Comment 6 Simon Fraser (smfr) 2021-03-15 17:36:47 PDT
> > 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).
Comment 7 Simon Fraser (smfr) 2021-03-15 20:07:53 PDT
https://trac.webkit.org/changeset/274461/webkit
Comment 8 Radar WebKit Bug Importer 2021-03-15 20:08:15 PDT
<rdar://problem/75460317>