Bug 38783 - [chromium] Implement h/w accelerated compositing for LayerChromium layers
Summary: [chromium] Implement h/w accelerated compositing for LayerChromium layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-07 16:57 PDT by Vangelis Kokkevis
Modified: 2010-05-20 10:31 PDT (History)
9 users (show)

See Also:


Attachments
Proposed patch (61.18 KB, patch)
2010-05-13 11:45 PDT, Vangelis Kokkevis
abarth: review-
Details | Formatted Diff | Diff
Proposed patch with style issues fixed (60.74 KB, patch)
2010-05-13 13:04 PDT, Vangelis Kokkevis
dglazkov: review-
Details | Formatted Diff | Diff
Proposed patch - smaller this time (53.40 KB, patch)
2010-05-17 17:49 PDT, Vangelis Kokkevis
fishd: review-
Details | Formatted Diff | Diff
Patch addressing review comments (53.23 KB, patch)
2010-05-18 11:10 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 2010-05-07 16:57:23 PDT
Switch the current implementation of the ACCELERATED_COMPOSITING path in chromium to do the compositing on the GPU rather than the CPU.
Comment 1 Vangelis Kokkevis 2010-05-13 11:45:49 PDT
Created attachment 56003 [details]
Proposed patch

Sorry this patch got to be a bit bigger than I intended to.  There are still a few things that will change in subsequent CLs such as:
1. Layers don't need to hang on to their GC and Canvas.  The can create one temporarily, render into it and upload the results to the texture.
2. Setting a 1x1 dirty region to kick off the render logic for the browser will go away once the compositor moves to its own thread.
Comment 2 WebKit Review Bot 2010-05-13 11:47:03 PDT
Attachment 56003 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
s] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:471:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:472:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:473:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:474:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:502:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:560:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:567:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:642:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:643:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:644:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:645:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:646:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:647:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:648:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:649:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:651:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:652:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:656:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:662:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:668:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:691:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 45 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Adam Barth 2010-05-13 12:28:10 PDT
Comment on attachment 56003 [details]
Proposed patch

Please run check-webkit-style before posting your patch (or use webkit-patch to upload your patch, which runs the style checker for you).
Comment 4 Vangelis Kokkevis 2010-05-13 13:04:10 PDT
Created attachment 56010 [details]
Proposed patch with style issues fixed
Comment 5 Adam Barth 2010-05-13 13:14:26 PDT
(When you upload a patch, please check the "this is a patch" box so we get the links we need to review your patch.)
Comment 6 Dimitri Glazkov (Google) 2010-05-14 07:58:36 PDT
Comment on attachment 56010 [details]
Proposed patch with style issues fixed

Vangelis, I am sorry -- I gave it my best. But this patch is too large to be reviewed like this. Can you please split it up into smaller bits? Ideally, into a progression of patches that makes logical sense.
Comment 7 Kenneth Russell 2010-05-14 13:16:28 PDT
Comment on attachment 56010 [details]
Proposed patch with style issues fixed

This generally looks good to me. It looks like the code is mostly interdependent so it seems to me it will be difficult to split it up into multiple patches. I'd support landing it all at once, given that it's been demonstrated to work.

The only high level comment I have is that compositeLayersRecursive does two pieces of work: updating the layer's contents and drawing it to the screen. I think it would be better to split these into two different phases, even though doing so would involve two traversals of the layer hierarchy. That way the updating of the layer's contents, which must be done on the main thread, would be more easily split off from the drawing, which might be more profitably done on a background thread in the future.

A few minor comments.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:215
 +      FloatPoint3D test2 = renderMatrix.mapPoint(FloatPoint3D(0.5, -0.5, 0));
Test code should be removed.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:155
 +  static GLuint createLayerTexture()
Should refactor with createTextureObject().


WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:265
 +      offset+= 3 * sizeof(GLfloat);
Need space between offset and += .

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:416
 +      FloatPoint3D test2 = renderMatrix.mapPoint(FloatPoint3D(0.5, -0.5, 0));
More test code.


WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:720
 +  }
Formatting.
Comment 8 Vangelis Kokkevis 2010-05-17 17:49:34 PDT
Created attachment 56297 [details]
Proposed patch - smaller this time

This patch now covers the code necessary for the compositor. I'm afraid it's still a fairly large patch but it's hard to break it up any further. Please let me know if you'd like me to walk you through the code.
Comment 9 Vangelis Kokkevis 2010-05-17 17:50:38 PDT
(In reply to comment #7)
> A few minor comments.
> 
> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:215
>  +      FloatPoint3D test2 = renderMatrix.mapPoint(FloatPoint3D(0.5, -0.5, 0));
> Test code should be removed.
> 

Done.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:155
>  +  static GLuint createLayerTexture()
> Should refactor with createTextureObject().
> 

Done. Removed createTextureObject() as it wasn't getting used.

> 
> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:265
>  +      offset+= 3 * sizeof(GLfloat);
> Need space between offset and += .
> 

Done.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:416
>  +      FloatPoint3D test2 = renderMatrix.mapPoint(FloatPoint3D(0.5, -0.5, 0));
> More test code.
> 

Removed.

> 
> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:720
>  +  }
> Formatting.

Fixed.
Comment 10 Darin Fisher (:fishd, Google) 2010-05-18 08:44:31 PDT
Comment on attachment 56297 [details]
Proposed patch - smaller this time

Overall, looks great.  Just a few minor issues:

WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:535
 +      // The position of they layer is the center of quad.
nit: "they layer" -> "the layer" ?

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:264
 +              // Scrolling works as follows: We render a quad with the current root layer contents
nit: indent by 4 spaces instead of 8.  true, this means the
body is not visually separate from the "|| (scrollDelta..."
bit, but that's what webkit style would have you do.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:359
 +      std::map<LayerChromium*, unsigned int>::iterator textureId;
webkit avoids the container classes from STL.  please use HashMap
from WTF instead.
Comment 11 Vangelis Kokkevis 2010-05-18 11:10:04 PDT
Created attachment 56390 [details]
Patch addressing review comments

Replaced std::map with wtf's HashMap and took care of the other issues as well.
Comment 12 Vangelis Kokkevis 2010-05-18 11:10:49 PDT
(In reply to comment #10)
> (From update of attachment 56297 [details])
> Overall, looks great.  Just a few minor issues:

Thanks for the quick review! Please see new patch.

> 
> WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:535
>  +      // The position of they layer is the center of quad.
> nit: "they layer" -> "the layer" ?
> 

Done.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:264
>  +              // Scrolling works as follows: We render a quad with the current root layer contents
> nit: indent by 4 spaces instead of 8.  true, this means the
> body is not visually separate from the "|| (scrollDelta..."
> bit, but that's what webkit style would have you do.
> 

Done. 

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:359
>  +      std::map<LayerChromium*, unsigned int>::iterator textureId;
> webkit avoids the container classes from STL.  please use HashMap
> from WTF instead.

Done.
Comment 13 Kenneth Russell 2010-05-18 11:22:40 PDT
Comment on attachment 56390 [details]
Patch addressing review comments

Looks good. Couple of very minor comments, not necessary to address now.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:310
 +      glClearColor(0, 0, 1, 1);
Presumably the blue color is for debugging purposes?

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:328
 +      // so we need to send the blending mode appropriately.
send -> set
Comment 14 WebKit Commit Bot 2010-05-19 22:59:21 PDT
Comment on attachment 56390 [details]
Patch addressing review comments

Clearing flags on attachment: 56390

Committed r59822: <http://trac.webkit.org/changeset/59822>
Comment 15 WebKit Commit Bot 2010-05-19 22:59:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Eric Seidel (no email) 2010-05-20 00:12:25 PDT
This broke lots of tests on Gtk:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r59822%20(13001)/results.html
Comment 17 Eric Seidel (no email) 2010-05-20 00:14:43 PDT
It's unclear to me if the Gtk bot just wedged itself or if this is actually responsible for the regression.
Comment 18 Vangelis Kokkevis 2010-05-20 10:31:45 PDT
(In reply to comment #17)
> It's unclear to me if the Gtk bot just wedged itself or if this is actually responsible for the regression.

I'd be surprised too if this patch caused the problem. All the code sits behind an #if WTF_USE_ACCELERATED_COMPOSITING which doesn't get defined yet in regular builds.