Bug 38783 – [chromium] Implement h/w accelerated compositing for LayerChromium layers

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

[Vangelis Kokkevis]

Switch the current implementation of the ACCELERATED_COMPOSITING path in chromium to do the compositing on the GPU rather than the CPU.

[Vangelis Kokkevis]

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.

[WebKit Review Bot]

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.

[Adam Barth]

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).

[Vangelis Kokkevis]

Created attachment 56010 [details]
Proposed patch with style issues fixed

[Adam Barth]

(When you upload a patch, please check the "this is a patch" box so we get the links we need to review your patch.)

[Dimitri Glazkov (Google)]

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.

[Kenneth Russell]

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.

[Vangelis Kokkevis]

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.

[Vangelis Kokkevis]

(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.

[Darin Fisher (:fishd, Google)]

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.

[Vangelis Kokkevis]

Created attachment 56390 [details]
Patch addressing review comments

Replaced std::map with wtf's HashMap and took care of the other issues as well.

[Vangelis Kokkevis]

(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.

[Kenneth Russell]

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

[WebKit Commit Bot]

Comment on attachment 56390 [details]
Patch addressing review comments

Clearing flags on attachment: 56390

Committed r59822: <http://trac.webkit.org/changeset/59822>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This broke lots of tests on Gtk:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r59822%20(13001)/results.html

[Eric Seidel (no email)]

It's unclear to me if the Gtk bot just wedged itself or if this is actually responsible for the regression.

[Vangelis Kokkevis]

(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.