Bug 71869 – [Chromium] Implement tile-sized painting using SkPicture

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

Add texture uploader that paints tile-sized chunks using SkPicture recording and playback. Expose setting which allows this texture updater to be turned on/off.

[David Reveman]

Created attachment 114191 [details]
Patch

Depends on https://bugs.webkit.org/show_bug.cgi?id=71388

[Adrienne Walker]

Comment on attachment 114191 [details]
Patch

I am still worried about the memory behavior of keeping around a CPU-side buffer of pixels for every tile that ever gets painted.  When does this get freed?

[David Reveman]

Created attachment 114366 [details]
Patch

[David Reveman]

(In reply to comment #2)
> (From update of attachment 114191 [details])
> I am still worried about the memory behavior of keeping around a CPU-side buffer of pixels for every tile that ever gets painted.  When does this get freed?

Sorry, I meant to fix that. Latest patch allocates the buffer in ::prepareRect and frees after using it in ::updateRect.

[David Reveman]

Created attachment 114545 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[David Reveman]

Created attachment 114787 [details]
Patch

[WebKit Review Bot]

Comment on attachment 114787 [details]
Patch

Attachment 114787 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10447798

[Darin Fisher (:fishd, Google)]

Comment on attachment 114787 [details]
Patch

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

> Source/WebKit/chromium/public/WebSettings.h:136
> +    virtual void setPerTilePainting(bool) = 0;

why do you need to convey this setting in two places?  WebSettings and WebLayerTreeView::Settings?

[David Reveman]

(In reply to comment #9)
> (From update of attachment 114787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114787&action=review
> 
> > Source/WebKit/chromium/public/WebSettings.h:136
> > +    virtual void setPerTilePainting(bool) = 0;
> 
> why do you need to convey this setting in two places?  WebSettings and WebLayerTreeView::Settings?

No, I don't think we do. This is my first time adding a compositor setting. What's the rational used to deciding the place a setting belongs to? I couldn't tell based on existing settings.

[David Reveman]

Created attachment 115221 [details]
Patch

[Adrienne Walker]

Comment on attachment 115221 [details]
Patch

This looks great.  I wonder a little bit if this should be the default path, because it fixes the "paint aggregator" problem.

[David Reveman]

Created attachment 115891 [details]
Patch

[David Reveman]

Created attachment 115895 [details]
Patch

[David Reveman]

Created attachment 116007 [details]
Patch

[David Reveman]

(In reply to comment #9)
> (From update of attachment 114787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114787&action=review
> 
> > Source/WebKit/chromium/public/WebSettings.h:136
> > +    virtual void setPerTilePainting(bool) = 0;
> 
> why do you need to convey this setting in two places?  WebSettings and WebLayerTreeView::Settings?

Removed the WebLayerTreeView::Settings changes from the latest patch. Only in WebSettings now.

[David Reveman]

Created attachment 116294 [details]
Patch

[David Reveman]

Created attachment 117653 [details]
Patch

[David Reveman]

Created attachment 117961 [details]
Patch

[James Robinson]

Comment on attachment 117961 [details]
Patch

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

This looks great, let's do it!

> Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.h:66
> +    explicit BitmapSkPictureCanvasLayerTextureUpdater(PassOwnPtr<LayerPainterChromium>, bool useMapTexSubImage);

you don't need 'explicit' for 2-arg c'tors

[David Reveman]

Created attachment 118100 [details]
Patch

[David Reveman]

(In reply to comment #20)
> (From update of attachment 117961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117961&action=review
> 
> This looks great, let's do it!

Yay!

> 
> > Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.h:66
> > +    explicit BitmapSkPictureCanvasLayerTextureUpdater(PassOwnPtr<LayerPainterChromium>, bool useMapTexSubImage);
> 
> you don't need 'explicit' for 2-arg c'tors

Latest patch removes unnecessary 'explicit' from c'tor.

[WebKit Review Bot]

Comment on attachment 118100 [details]
Patch

Clearing flags on attachment: 118100

Committed r102181: <http://trac.webkit.org/changeset/102181>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.