Bug 44148 - [chromium] Composited layers should handle their own drawing
Summary: [chromium] Composited layers should handle their own drawing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Vangelis Kokkevis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-17 19:28 PDT by Vangelis Kokkevis
Modified: 2010-08-19 17:47 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (104.84 KB, patch)
2010-08-17 20:37 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Proposed patch - fixed style issue (104.95 KB, patch)
2010-08-18 10:51 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Proposed patch addressing review comments (106.62 KB, patch)
2010-08-18 22:55 PDT, Vangelis Kokkevis
kbr: review+
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-08-17 19:28:29 PDT
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.
Comment 1 Vangelis Kokkevis 2010-08-17 20:37:46 PDT
Created attachment 64665 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-08-17 20:41:19 PDT
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.
Comment 3 Vangelis Kokkevis 2010-08-18 10:51:48 PDT
Created attachment 64734 [details]
Proposed patch - fixed style issue
Comment 4 Kenneth Russell 2010-08-18 17:19:43 PDT
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_.
Comment 5 Vangelis Kokkevis 2010-08-18 22:55:28 PDT
Created attachment 64810 [details]
Proposed patch addressing review comments
Comment 6 Vangelis Kokkevis 2010-08-18 22:59:32 PDT
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.
Comment 7 Kenneth Russell 2010-08-19 11:47:42 PDT
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.
Comment 8 Vangelis Kokkevis 2010-08-19 17:47:09 PDT
Committed r65717: <http://trac.webkit.org/changeset/65717>