Bug 44148 – [chromium] Composited layers should handle their own drawing

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

[Vangelis Kokkevis]

As the code currently stands, LayerRendererChromium (the compositor) handles the rendering of all the various layer types. It handles setting the shader for each layer, binding a _single_ texture and rendering a quad for it. As more and more layer types are added, this architecture proves rather limiting. For example: 
* The video layer now needs to use 3 textures for the color space conversion and there's no easy way to accommodate them
* Layers making use of masks require multiple textures

In addition, with each special layer type, new cruft is added to the compositor's draw code to accommodate for any special rendering needs. The end result is a complex and difficult to maintain draw logic which all lives in the compositor.

The proposed solution is to move the layer drawing code into the actual layer classes so that each layer type is free to setup its own shader and textures.  This will greatly simplify the compositor code and make it more extensible.

[Vangelis Kokkevis]

Created attachment 64665 [details]
Proposed patch

[WebKit Review Bot]

Attachment 64665 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/LayerRendererChromium.h:58:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vangelis Kokkevis]

Created attachment 64734 [details]
Proposed patch - fixed style issue

[Kenneth Russell]

Comment on attachment 64734 [details]
Proposed patch - fixed style issue

The code reorganization looks good in general. Trusting that your testing covers the changes since it's impossible for a review to catch all issues in a change this large.

A few mostly stylistic issues.

The naming of CanvasLayerChromium::CanvasLayerSharedValues, ContentLayerChromium::ContentLayerSharedValues and LayerChromium::SharedValues is inconsistent. Given that you already have scoping within the class, I recommend renaming them all to SharedValues. These should all be structs, not classes, and though this isn't mentioned in the WebKit coding guidelines, the members shouldn't have the "m_" prefix.

WebCore/platform/graphics/chromium/LayerRendererChromium.h:153
 +      CanvasLayerChromium::CanvasLayerSharedValues m_canvasLayerSharedValues;
I recommend changing these to OwnPtr<T>, and instantiating them in LayerRendererChromium::initializeSharedObjects. (For example: m_layerSharedValues = adoptPtr(new LayerChromium::SharedValues()).) This will let you make the OpenGL context current before initializing them. You can then clear out the OwnPtrs in cleanupSharedObjects, again giving you the chance to make the OpenGL context current. Then you can move the logic in initializeSharedObjects and cleanupSharedObjects in the LayerChromium subclasses into the constructors and destructors of the SharedValues structs. This will encapsulate this logic more cleanly.


WebCore/platform/graphics/chromium/LayerRendererChromium.h:159
 +  #define GLC(x) { x, LayerRendererChromium::debugGLCall(#x, __FILE__, __LINE__); }
Does this macro definition style handle the trailing semicolon at the invocations on all platforms? Could you use it with assignments (like "GLC(val = glGetUniformLocation(...))")? Do you need to parenthesize (x) as I believe is common in some macros?


WebCore/platform/graphics/chromium/LayerChromium.h:202
 +      static const unsigned m_texCoordAttribLocation;
These should be prefixed with s_.

[Vangelis Kokkevis]

Created attachment 64810 [details]
Proposed patch addressing review comments

[Vangelis Kokkevis]

Thanks for the review and sorry about the size of this beast.

(In reply to comment #4)
> (From update of attachment 64734 [details])
> The code reorganization looks good in general. Trusting that your testing covers the changes since it's impossible for a review to catch all issues in a change this large.
> 
> A few mostly stylistic issues.
> 
> The naming of CanvasLayerChromium::CanvasLayerSharedValues, ContentLayerChromium::ContentLayerSharedValues and LayerChromium::SharedValues is inconsistent. Given that you already have scoping within the class, I recommend renaming them all to SharedValues. These should all be structs, not classes, and though this isn't mentioned in the WebKit coding guidelines, the members shouldn't have the "m_" prefix.

Done. Classes renamed to SharedValues. I did however keep them as classes, made all the members private and added accessors for them.  That way, code using the shared values cannot inadvertently change them.

> 
> WebCore/platform/graphics/chromium/LayerRendererChromium.h:153
>  +      CanvasLayerChromium::CanvasLayerSharedValues m_canvasLayerSharedValues;
> I recommend changing these to OwnPtr<T>, and instantiating them in LayerRendererChromium::initializeSharedObjects. (For example: m_layerSharedValues = adoptPtr(new LayerChromium::SharedValues()).) This will let you make the OpenGL context current before initializing them. You can then clear out the OwnPtrs in cleanupSharedObjects, again giving you the chance to make the OpenGL context current. Then you can move the logic in initializeSharedObjects and cleanupSharedObjects in the LayerChromium subclasses into the constructors and destructors of the SharedValues structs. This will encapsulate this logic more cleanly.

I like this idea a lot. I eliminated the initialize/cleanupSharedObjects method and added the logic in the SharedValues constructor/destructor.

> 
> 
> WebCore/platform/graphics/chromium/LayerRendererChromium.h:159
>  +  #define GLC(x) { x, LayerRendererChromium::debugGLCall(#x, __FILE__, __LINE__); }
> Does this macro definition style handle the trailing semicolon at the invocations on all platforms? Could you use it with assignments (like "GLC(val = glGetUniformLocation(...))")? Do you need to parenthesize (x) as I believe is common in some macros?
> 

I parenthesized (x) so now expressions of the type GLC(x = gl*()) work.  Trailing semicolon seems to compile fine with both GCC and msvc.

> 
> WebCore/platform/graphics/chromium/LayerChromium.h:202
>  +      static const unsigned m_texCoordAttribLocation;
> These should be prefixed with s_.

Done.

[Kenneth Russell]

Comment on attachment 64810 [details]
Proposed patch addressing review comments

Looks good. r=me. I noticed that the method list in the ChangeLog is slightly out of date; this isn't critical, but you can fix this by saving off your text edits to the ChangeLog, svn reverting it, and running prepare-ChangeLog again. Feel free to commit using webkit-patch after this rather than submitting another patch for the commit queue. Removing cq? since you are a committer and can set it yourself if you want.

[Vangelis Kokkevis]

Committed r65717: <http://trac.webkit.org/changeset/65717>