Bug 71869 - [Chromium] Implement tile-sized painting using SkPicture
Summary: [Chromium] Implement tile-sized painting using SkPicture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks: 72191
  Show dependency treegraph
 
Reported: 2011-11-08 17:54 PST by David Reveman
Modified: 2011-12-06 14:37 PST (History)
10 users (show)

See Also:


Attachments
Patch (15.93 KB, patch)
2011-11-08 18:36 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (16.16 KB, patch)
2011-11-09 14:09 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (20.19 KB, patch)
2011-11-10 12:41 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (19.86 KB, patch)
2011-11-11 15:29 PST, David Reveman
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (19.91 KB, patch)
2011-11-15 12:37 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (17.71 KB, patch)
2011-11-18 15:28 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (17.73 KB, patch)
2011-11-18 15:41 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (14.81 KB, patch)
2011-11-20 16:25 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (14.81 KB, patch)
2011-11-22 16:53 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (14.80 KB, patch)
2011-12-02 10:55 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (18.65 KB, patch)
2011-12-05 17:31 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (18.58 KB, patch)
2011-12-06 13:16 PST, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2011-11-08 17:54:53 PST
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.
Comment 1 David Reveman 2011-11-08 18:36:10 PST
Created attachment 114191 [details]
Patch

Depends on https://bugs.webkit.org/show_bug.cgi?id=71388
Comment 2 Adrienne Walker 2011-11-09 13:50:23 PST
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?
Comment 3 David Reveman 2011-11-09 14:09:10 PST
Created attachment 114366 [details]
Patch
Comment 4 David Reveman 2011-11-09 14:11:26 PST
(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.
Comment 5 David Reveman 2011-11-10 12:41:08 PST
Created attachment 114545 [details]
Patch
Comment 6 WebKit Review Bot 2011-11-10 12:42:49 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 7 David Reveman 2011-11-11 15:29:42 PST
Created attachment 114787 [details]
Patch
Comment 8 WebKit Review Bot 2011-11-12 15:09:09 PST
Comment on attachment 114787 [details]
Patch

Attachment 114787 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10447798
Comment 9 Darin Fisher (:fishd, Google) 2011-11-14 11:12:59 PST
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?
Comment 10 David Reveman 2011-11-14 11:47:58 PST
(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.
Comment 11 David Reveman 2011-11-15 12:37:19 PST
Created attachment 115221 [details]
Patch
Comment 12 Adrienne Walker 2011-11-16 14:37:06 PST
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.
Comment 13 David Reveman 2011-11-18 15:28:14 PST
Created attachment 115891 [details]
Patch
Comment 14 David Reveman 2011-11-18 15:41:21 PST
Created attachment 115895 [details]
Patch
Comment 15 David Reveman 2011-11-20 16:25:18 PST
Created attachment 116007 [details]
Patch
Comment 16 David Reveman 2011-11-21 12:39:39 PST
(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.
Comment 17 David Reveman 2011-11-22 16:53:18 PST
Created attachment 116294 [details]
Patch
Comment 18 David Reveman 2011-12-02 10:55:06 PST
Created attachment 117653 [details]
Patch
Comment 19 David Reveman 2011-12-05 17:31:35 PST
Created attachment 117961 [details]
Patch
Comment 20 James Robinson 2011-12-06 13:07:33 PST
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
Comment 21 David Reveman 2011-12-06 13:16:05 PST
Created attachment 118100 [details]
Patch
Comment 22 David Reveman 2011-12-06 13:18:43 PST
(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.
Comment 23 WebKit Review Bot 2011-12-06 14:37:28 PST
Comment on attachment 118100 [details]
Patch

Clearing flags on attachment: 118100

Committed r102181: <http://trac.webkit.org/changeset/102181>
Comment 24 WebKit Review Bot 2011-12-06 14:37:34 PST
All reviewed patches have been landed.  Closing bug.