Bug 123043 – Remote Layer Tree: Double-buffering and minimization of repaints

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

We should not repaint the entirety of a layer every time it is dirtied.

[Tim Horton]

Created attachment 214611 [details]
patch

[WebKit Commit Bot]

Attachment 214611 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/mac/WebLayer.h', u'Source/WebCore/platform/graphics/mac/WebLayer.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp', u'Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerBackingStore.h', u'Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerBackingStore.mm']" exit_code: 1
Source/WebCore/platform/graphics/mac/WebLayer.h:47:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 214611 [details]
patch

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

> Source/WebCore/platform/graphics/mac/WebLayer.mm:64
> +    bool isTiledLayer = [layer isKindOfClass:[CATiledLayer class]];

Move this down.

> Source/WebCore/platform/graphics/mac/WebLayer.mm:117
> +    // If we have no dirty rects, repaint the whole layer.

Comment is the reverse of the immediately following statement which is confusing.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerBackingStore.mm:124
> +        Region cleanRegion(layerBounds);
> +        cleanRegion.subtract(m_dirtyRegion);
> +
> +        RetainPtr<CGImageRef> frontImage = m_frontBuffer->makeCGImage();
> +
> +        for (const auto& rect : cleanRegion.rects())
> +            context->drawNativeImage(frontImage.get(), m_frontBuffer->size(), ColorSpaceDeviceRGB, rect, rect);

Seems like this could be much slower than copying over the entire layer in many cases.

[Tim Horton]

(In reply to comment #3)
> (From update of attachment 214611 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214611&action=review
> 
> > Source/WebCore/platform/graphics/mac/WebLayer.mm:64
> > +    bool isTiledLayer = [layer isKindOfClass:[CATiledLayer class]];
> 
> Move this down.
> 
> > Source/WebCore/platform/graphics/mac/WebLayer.mm:117
> > +    // If we have no dirty rects, repaint the whole layer.
> 
> Comment is the reverse of the immediately following statement which is confusing.

OK, I'll flip it around.

> > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerBackingStore.mm:124
> > +        Region cleanRegion(layerBounds);
> > +        cleanRegion.subtract(m_dirtyRegion);
> > +
> > +        RetainPtr<CGImageRef> frontImage = m_frontBuffer->makeCGImage();
> > +
> > +        for (const auto& rect : cleanRegion.rects())
> > +            context->drawNativeImage(frontImage.get(), m_frontBuffer->size(), ColorSpaceDeviceRGB, rect, rect);
> 
> Seems like this could be much slower than copying over the entire layer in many cases.

Yeah. I meant to use the five-or-less rect set, not the raw set.

[Tim Horton]

Created attachment 214622 [details]
revised

[Simon Fraser (smfr)]

Comment on attachment 214622 [details]
revised

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

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerBackingStore.mm:99
> +        setNeedsDisplay();

It's odd to call setNeedsDisplay() inside of display().

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerBackingStore.mm:108
> +        setNeedsDisplay(indicatorRect);

Ditto.

[Tim Horton]

http://trac.webkit.org/changeset/157658