Bug 53715 - [chromium] Import PaintAggregator
Summary: [chromium] Import PaintAggregator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-03 14:07 PST by Darin Fisher (:fishd, Google)
Modified: 2012-01-31 14:57 PST (History)
6 users (show)

See Also:


Attachments
v1 patch (84.00 KB, patch)
2011-02-03 14:16 PST, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff
v2 patch (80.68 KB, patch)
2011-02-03 15:07 PST, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff
Patch (35.47 KB, patch)
2012-01-12 15:15 PST, Nat Duca
no flags Details | Formatted Diff | Diff
Patch for landing (35.44 KB, patch)
2012-01-31 10:21 PST, Nat Duca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2011-02-03 14:07:42 PST
[chromium] Import PaintAggregator and introduce WebPaintSurface API

This bug is about importing the Chromium code that collects invalidation
and scrolling commands and generates updates to the software backingstore.

It makes sense to move this code into WebKit so that we can more easily
exercise it with layout tests.  We may also want to make use of the paint
aggregator to reduce updates to composited layers.
Comment 1 Darin Fisher (:fishd, Google) 2011-02-03 14:16:02 PST
Created attachment 81114 [details]
v1 patch

The corresponding changes to TestShell are here:
http://codereview.chromium.org/6283019

(Those are not yet ready for review, but they give you an idea of how WebPaintSurface may be implemented.)
Comment 2 Darin Fisher (:fishd, Google) 2011-02-03 14:18:31 PST
@brettw, it'd be great to get your input on the plugin related changes.  This is my attempt to bring your plugin painting optimization into WebKit.
Comment 3 WebKit Review Bot 2011-02-03 14:23:01 PST
Attachment 81114 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7689755
Comment 4 Darin Fisher (:fishd, Google) 2011-02-03 15:07:15 PST
Created attachment 81117 [details]
v2 patch
Comment 5 James Robinson 2011-02-03 16:45:11 PST
Comment on attachment 81117 [details]
v2 patch

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

Seems pretty sane to me.  We're supposed to use 2-clause BSD these days

> Source/WebKit/chromium/src/painting/PaintController.cpp:201
> +    context.setStrokeColor(colors[colorSelector++ & 3], ColorSpaceDeviceRGB);

% 3, & 3 will allow the value '3' which will index out of bounds here
Comment 6 WebKit Review Bot 2011-02-03 20:14:28 PST
Attachment 81114 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/WebPopupMenuImpl.h:131:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/src/WebPopupMenuImpl.h:132:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/src/painting/PaintController.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/src/painting/PaintController.cpp:207:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/WebViewImpl.h:209:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/src/WebViewImpl.h:210:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/src/painting/PaintAggregator.cpp:205:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/painting/PaintAggregator.cpp:248:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/chromium/tests/PaintAggregatorTest.cpp:282:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/tests/PaintAggregatorTest.cpp:299:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/painting/PaintAggregator.h:83:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 11 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 WebKit Review Bot 2011-02-03 20:15:57 PST
Attachment 81117 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/WebKit.gyp', u'Sour..." exit_code: 1

Source/WebKit/chromium/src/WebPopupMenuImpl.h:131:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/src/WebPopupMenuImpl.h:132:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/src/painting/PaintController.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/src/painting/PaintController.cpp:207:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/WebViewImpl.h:209:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/src/WebViewImpl.h:210:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/src/painting/PaintAggregator.cpp:205:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/painting/PaintAggregator.cpp:248:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/chromium/tests/PaintAggregatorTest.cpp:282:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/tests/PaintAggregatorTest.cpp:299:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/painting/PaintAggregator.h:83:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 11 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Darin Fisher (:fishd, Google) 2011-02-04 09:56:27 PST
(In reply to comment #5)
> (From update of attachment 81117 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81117&action=review
> 
> Seems pretty sane to me.  We're supposed to use 2-clause BSD these days

ah, right.  thanks!

> 
> > Source/WebKit/chromium/src/painting/PaintController.cpp:201
> > +    context.setStrokeColor(colors[colorSelector++ & 3], ColorSpaceDeviceRGB);
> 
> % 3, & 3 will allow the value '3' which will index out of bounds here

whoops... yeah, that's a typo in my translation of the chromium code :-/  thanks for catching it.
Comment 9 Nat Duca 2011-02-07 17:44:04 PST
Comment on attachment 81114 [details]
v1 patch

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

> Source/WebKit/chromium/public/WebPaintSurface.h:61
> +    // and the buffer may be deleted once flush() returns.

May be deleted? What happens if I don't delete a WebPaintBuffer? Can i efficiently paint with it next frame?

> Source/WebKit/chromium/public/WebPlugin.h:78
> +    virtual WebPaintBuffer* cloneBackingBuffer() { return 0; }

The name of this sort of confused me at first...

I'm probably missing some critical detail, but can we reverse the impedence of this and say paintIntoSurface(WebPaintSurface*)?

> Source/WebKit/chromium/src/WebViewImpl.h:360
> +    void scheduleInvalidation(const WebCore::IntRect&);

Can we remove WebViewImpl::invalidateRootLayerRect/scrollRootLayerRect completely and put their implementation behind scheduleInvalidation/etc? This might allow ChromeClient to be agnostic about compositing.

> Source/WebKit/chromium/src/painting/PaintController.h:54
> +class PaintController : public WebPaintSurfaceClient {

Hmm... isn't paintController ~= LayerChromium, plus an update loop?

If we leave the updateTimer on WebView, we could fuse scheduleComposite with this update. That might then pave the way to another step which is fusing PaintController with LayerChromium.

I'm trying to figure out the step after this where PaintController and LayerChromium fuse together a bit.

> Source/WebKit/chromium/src/painting/Painter.h:49
> +public:

If you hoist animate and layout of these (which are sort of global rather than RenderLayer-specific iirw) then you have something nearly identical to TilePaintInterface, which is Enne's abstraction for painting... maybe we can fuse those together.
Comment 10 Darin Fisher (:fishd, Google) 2011-02-08 13:06:47 PST
(In reply to comment #9)
> (From update of attachment 81114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81114&action=review
> 
> > Source/WebKit/chromium/public/WebPaintSurface.h:61
> > +    // and the buffer may be deleted once flush() returns.
> 
> May be deleted? What happens if I don't delete a WebPaintBuffer? Can i efficiently paint with it next frame?

We have a choice.  There is nothing that technically precludes it.  We
probably just want to strongly encourage people to not modify the
WebPaintBuffer while it is being flushed :-)


> > Source/WebKit/chromium/public/WebPlugin.h:78
> > +    virtual WebPaintBuffer* cloneBackingBuffer() { return 0; }
> 
> The name of this sort of confused me at first...

Yeah, I had a hard time coming up with a good name.  "clone" seemed
better than "create" (sounds like it would return something that is
new and uninitialized) or "get" (sounds like it would not be something
the caller should need to free later).  "clone" on the other hand
sounds like it is a copy, which should be freed later.

I used the term "backing buffer" to mimic the term already used to
get the backing texture.


> I'm probably missing some critical detail, but can we reverse the impedence of this and say paintIntoSurface(WebPaintSurface*)?

Maybe.  The problem with doing this is that we need to keep the buffer
alive until flush is called.  I could fix that by making it so that
WebPaintBuffer is reference counted such that WebPaintSurface::paint
would just take an owning reference.

Yeah, this might be a lot nicer.  It helps avoid the nasty problem of
exposing the canvas of the underlying WebPaintBuffer of the plugin.

I'll noodle on this some more...


> > Source/WebKit/chromium/src/WebViewImpl.h:360
> > +    void scheduleInvalidation(const WebCore::IntRect&);
> 
> Can we remove WebViewImpl::invalidateRootLayerRect/scrollRootLayerRect completely and put their implementation behind scheduleInvalidation/etc? This might allow ChromeClient to be agnostic about compositing.

Yes, we probably can.  I'll give that a try.


> > Source/WebKit/chromium/src/painting/PaintController.h:54
> > +class PaintController : public WebPaintSurfaceClient {
> 
> Hmm... isn't paintController ~= LayerChromium, plus an update loop?

Good question.  I need to learn more about LayerChromium.


> If we leave the updateTimer on WebView, we could fuse scheduleComposite with this update. That might then pave the way to another step which is fusing PaintController with LayerChromium.

I definitely want to avoid adding complexity to WebView though...
I had the idea of writing a PaintController unit test :)


> I'm trying to figure out the step after this where PaintController and LayerChromium fuse together a bit.

Yeah... this is something I haven't thought enough about yet.


> > Source/WebKit/chromium/src/painting/Painter.h:49
> > +public:
> 
> If you hoist animate and layout of these (which are sort of global rather than RenderLayer-specific iirw) then you have something nearly identical to TilePaintInterface, which is Enne's abstraction for painting... maybe we can fuse those together.

Interesting.  I'll take a look.
Comment 11 Nat Duca 2011-02-08 13:20:30 PST
> I definitely want to avoid adding complexity to WebView though...
> I had the idea of writing a PaintController unit test :)
> 
Hmm! Like that bit, lots...

Thinking out loud...

WebViewPainter { -> recieves damage, knows how to paint webview onto websurface
  invalidate/scroll/needsAnimate;
  PaintController*; // forward invalidations here
  paint aggregator;
  WebView*;
  WebSurface*;
}

PaintController { -> schedules updates ("update controller"), now for just sw path, soon for hw rendering case too
  anim/layout/paint scheduling
}

WebView {
  PaintController;
}

ChromeClient {
  SoftwarePainter*; // forward invalidations directly here? idk...
}
Comment 12 Brett Wilson (Google) 2011-02-08 23:45:09 PST
The API looks good overall. I'm also a bit confused about the "clone" part. So the caller needs to free the buffer? How can we avoid copying the backing store pixels in this case?

Sorry if I missed something obvious, I'm a bit rushed at the moment.
Comment 13 Darin Fisher (:fishd, Google) 2011-02-09 12:48:54 PST
(In reply to comment #12)
> The API looks good overall. I'm also a bit confused about the "clone" part. So the caller needs to free the buffer? How can we avoid copying the backing store pixels in this case?

Not exactly.  The caller deletes the handle--the WebPaintBuffer.  The implementation of WebPaintBuffer will most likely be reference counted.

I used the term "clone" to indicate that the returned pointer should be freed, but that doesn't mean it has to be deep-copied :)
Comment 14 Adam Barth 2011-05-05 01:10:18 PDT
Comment on attachment 81117 [details]
v2 patch

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

I'm not sure whether you're still interested in this patch, but there are a number of style nits reported by the bot.  I'm going to mark this R- for now because we'll at least need to clean up the style problems.  If you're still interested in the patch, please feel encouraged to upload a new version for review.

> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:320
> +#if WEBKIT_USING_PAINTSURFACE

This doesn't seem like the right style of define.  Wouldn't USE(PAINTSURFACE) or ENABLE(PAINTSURFACE) be more appropriate?
Comment 15 Nat Duca 2012-01-12 15:15:36 PST
Created attachment 122319 [details]
Patch
Comment 16 Nat Duca 2012-01-12 15:16:29 PST
Will pull the surface stuff to a new patch. This imported the paint aggregator in a nice way. Lets get that landed.

@fishd: r?
Comment 17 Darin Fisher (:fishd, Google) 2012-01-20 16:01:44 PST
Comment on attachment 122319 [details]
Patch

OK... did you confirm that all/any downstream paint_aggregator.cc changes that may have occurred since the original patch were incorporated into this copy?
Comment 18 Nat Duca 2012-01-20 16:53:28 PST
(In reply to comment #17)
> (From update of attachment 122319 [details])

Yeah, I was amazed to find it hasn't been touched since January 2011 except for moves relating to content refactoring.
Comment 19 Nat Duca 2012-01-31 10:21:42 PST
Created attachment 124768 [details]
Patch for landing
Comment 20 WebKit Review Bot 2012-01-31 14:57:34 PST
Comment on attachment 124768 [details]
Patch for landing

Clearing flags on attachment: 124768

Committed r106399: <http://trac.webkit.org/changeset/106399>
Comment 21 WebKit Review Bot 2012-01-31 14:57:40 PST
All reviewed patches have been landed.  Closing bug.