Bug 79428

Summary: [chromium] Port GraphicsLayerChromium over to depend on Web*Layer instead of *LayerChromium
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED WONTFIX    
Severity: Normal CC: enne, nduca, piman, schenney, senorblanco, shawnsingh, vangelis, vollick, wjmaclean
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79425, 79708, 79714, 79723    
Bug Blocks:    

Description James Robinson 2012-02-23 18:19:28 PST
Once https://bugs.webkit.org/show_bug.cgi?id=79425 lands the Web*Layer headers will be in a place that GraphicsLayerChromium can use directly.  To port over, we'll have to expand the WebLayer APIs by a bit.  This is also a good opportunity to take a careful look at our GraphicsLayerChromium <-> compositor interface to see if there's anything we can simplify or clean up.

Here's a list of what would would need to expose to be able to compile GraphicsLayerChromium against WebLayer/WebContentLayer instead of LayerChromium/ContentLayerChromium, under their current LayerChromium names:

==== WebLayer ====

--- Debugging ---
setDebugName()

--- Layer properties ---
setIsDrawable()
setPreserves3D()
setDoubleSided()
setBackgroundColor()
setContentsScale()

--- Invalidation ----
contentChanged()
(These two exist already on WebContentLayer as invalidate() / invalidateRect()):
setNeedsDisplay()
setNeedsDisplayRect()

--- Cleanup / tree manipulation ---
clearRenderSurface()
setChildren()


=== WebContentLayer ===

--- Invalidation ---
pageScaleChanged()

--- Filters ---
setFilters()

--- Animations ---
addAnimation()
pauseAnimation()
removeAnimation()
suspendAnimations()
resumeAnimations()

We'll need to wrap the animation and filter classes in API.  I know Ian has plans for how to do this for animations, does anyone have an idea for filters?  I think that we'll want to specify filters on layers for Aura anyway so this isn't just busy work.
Comment 1 James Robinson 2012-02-23 18:21:27 PST
Note that I'm not expecting this to happen all at once.  We can transition over gradually (at the expense of making GraphicsLayerChromium a big uglier) in order to ease the transition.
Comment 2 Shawn Singh 2012-02-27 20:50:29 PST
OK, I spent some time thinking carefully about how we could simplify and cleanup the compositor layer public API.  Some of these ideals may not be necessary quite yet, but I feel they are eventually the "right" thing to do, and I am not aware of any fundamental reasons they cannot be achieved after a few more refactorings.  Hopefully all this is relevant to the discussion.


1. I would like to make WebLayer a completely pure virtual interface (except for the static create functions)  Then LayerChromium and subclasses simply implement the interface, rather than having the WebLayer.cpp level of indirection between them.


2. Perhaps we can merge video / webGL / canvas2d / image, from the public perspective?  
My rationale:
(a) they all update their own compositor resources
(b) all we need to know is what may have changed from frame to frame, and possibly some "updater" delegate/callback.


3. Do we really need a distinction between WebContentLayer and WebLayer?  We can put ASSERT statements on things like invalidateRect().


4. If both #1, #2, and #3 are all possible, then we can provide 3 clear and simple create() functions:
(a) createContainerLayer() // for empty containers that don't have content.
(b) createContentLayer()    // for the user to provide a paint callback, and we manage the resources via tiling
(c) createExternallyManagedLayer() // designed for user to just point to the compositor resources used.
perhaps (b) and (c) could even be merged if we’re careful?


5. For layer properties (like opacity, transform) --> we clearly need settors, but do we really need gettors?  Can we keep the information flow stubbornly one-way?  If someone really needed to use such a property in their own code, they should already be aware of the property in their own code, right?


6. Do filters fit better as a delegate/callback mechanism, or as an enum that offers a variety of filter types?


7. I think we’re on a collision course with two different uses of the suffix "Impl".  (e.g. WebLayerImpl is not an impl-thread construct)
Comment 3 James Robinson 2012-02-29 14:08:30 PST
(In reply to comment #2)
> OK, I spent some time thinking carefully about how we could simplify and cleanup the compositor layer public API.  Some of these ideals may not be necessary quite yet, but I feel they are eventually the "right" thing to do, and I am not aware of any fundamental reasons they cannot be achieved after a few more refactorings.  Hopefully all this is relevant to the discussion.
> 

Thanks, this is exactly the sort of thing I had in mind.  Responses inline.

> 
> 1. I would like to make WebLayer a completely pure virtual interface (except for the static create functions)  Then LayerChromium and subclasses simply implement the interface, rather than having the WebLayer.cpp level of indirection between them.
> 

I think this is a good goal but won't change much in the short term.

> 
> 2. Perhaps we can merge video / webGL / canvas2d / image, from the public perspective?  
> My rationale:
> (a) they all update their own compositor resources
> (b) all we need to know is what may have changed from frame to frame, and possibly some "updater" delegate/callback.
> 

This is intriguing.  The question then becomes how much API do we need to expose for these different layer types to work.  I'm worried about pushing complexity out of the core API and making callers have to deal with it all.  For instance, WebGL layers need special handling when they are created, when they receive damage (rate limiter), and they push a different set of properties than other layer types.  I think we can unify the last one of these by having the layer expose the union of all properties that any type would need to set.  That might get unwieldy, but I'm not sure in practice how bad it would be.

> 
> 3. Do we really need a distinction between WebContentLayer and WebLayer?  We can put ASSERT statements on things like invalidateRect().
> 

I think we don't really do - a WebLayer today is simply a WebContentLayer that doesn't have any contents.

Incidentally, we do need a generic way to mark a part of a layer as 'dirty' even if it isn't a content layer so that damage tracking, etc work.  I was thinking of piggybacking on the invalidate()/invalidateRect() calls for this for non-Content layers, but I'm not sure how to make it clear what happens after an invalidate() call for a Content vs a non-Content layer.  For a Content layer, invalidate() means "this region should be repainted before being displayed".  For everything else invalidate() means "somebody has damaged this part of the layer, but you don't need to invoke any callbacks".


> 
> 4. If both #1, #2, and #3 are all possible, then we can provide 3 clear and simple create() functions:
> (a) createContainerLayer() // for empty containers that don't have content.
> (b) createContentLayer()    // for the user to provide a paint callback, and we manage the resources via tiling
> (c) createExternallyManagedLayer() // designed for user to just point to the compositor resources used.
> perhaps (b) and (c) could even be merged if we’re careful?
> 

Is the idea that the create*() calls would accept an appropriate Client interface?

> 
> 5. For layer properties (like opacity, transform) --> we clearly need settors, but do we really need gettors?  Can we keep the information flow stubbornly one-way?  If someone really needed to use such a property in their own code, they should already be aware of the property in their own code, right?

I agree, we don't need any getters.  I'll remove them (or not add them) as I go.

> 
> 
> 6. Do filters fit better as a delegate/callback mechanism, or as an enum that offers a variety of filter types?
> 

I'm thinking an enum (or list of enum / value pairs) since we want to be able to evaluate the filter chain from the impl thread / process independently of the client of the API.

> 
> 7. I think we’re on a collision course with two different uses of the suffix "Impl".  (e.g. WebLayerImpl is not an impl-thread construct)

I think so are, but it's probably something we can live with for now.
Comment 4 Stephen White 2012-02-29 14:22:47 PST
(In reply to comment #3)
> (In reply to comment #2)
> > OK, I spent some time thinking carefully about how we could simplify and cleanup the compositor layer public API.  Some of these ideals may not be necessary quite yet, but I feel they are eventually the "right" thing to do, and I am not aware of any fundamental reasons they cannot be achieved after a few more refactorings.  Hopefully all this is relevant to the discussion.
> > 
> 
> Thanks, this is exactly the sort of thing I had in mind.  Responses inline.
> 
> > 
> > 1. I would like to make WebLayer a completely pure virtual interface (except for the static create functions)  Then LayerChromium and subclasses simply implement the interface, rather than having the WebLayer.cpp level of indirection between them.
> > 
> 
> I think this is a good goal but won't change much in the short term.
> 
> > 
> > 2. Perhaps we can merge video / webGL / canvas2d / image, from the public perspective?  
> > My rationale:
> > (a) they all update their own compositor resources
> > (b) all we need to know is what may have changed from frame to frame, and possibly some "updater" delegate/callback.
> > 
> 
> This is intriguing.  The question then becomes how much API do we need to expose for these different layer types to work.  I'm worried about pushing complexity out of the core API and making callers have to deal with it all.  For instance, WebGL layers need special handling when they are created, when they receive damage (rate limiter), and they push a different set of properties than other layer types.  I think we can unify the last one of these by having the layer expose the union of all properties that any type would need to set.  That might get unwieldy, but I'm not sure in practice how bad it would be.
> 
> > 
> > 3. Do we really need a distinction between WebContentLayer and WebLayer?  We can put ASSERT statements on things like invalidateRect().
> > 
> 
> I think we don't really do - a WebLayer today is simply a WebContentLayer that doesn't have any contents.
> 
> Incidentally, we do need a generic way to mark a part of a layer as 'dirty' even if it isn't a content layer so that damage tracking, etc work.  I was thinking of piggybacking on the invalidate()/invalidateRect() calls for this for non-Content layers, but I'm not sure how to make it clear what happens after an invalidate() call for a Content vs a non-Content layer.  For a Content layer, invalidate() means "this region should be repainted before being displayed".  For everything else invalidate() means "somebody has damaged this part of the layer, but you don't need to invoke any callbacks".
> 
> 
> > 
> > 4. If both #1, #2, and #3 are all possible, then we can provide 3 clear and simple create() functions:
> > (a) createContainerLayer() // for empty containers that don't have content.
> > (b) createContentLayer()    // for the user to provide a paint callback, and we manage the resources via tiling
> > (c) createExternallyManagedLayer() // designed for user to just point to the compositor resources used.
> > perhaps (b) and (c) could even be merged if we’re careful?
> > 
> 
> Is the idea that the create*() calls would accept an appropriate Client interface?
> 
> > 
> > 5. For layer properties (like opacity, transform) --> we clearly need settors, but do we really need gettors?  Can we keep the information flow stubbornly one-way?  If someone really needed to use such a property in their own code, they should already be aware of the property in their own code, right?
> 
> I agree, we don't need any getters.  I'll remove them (or not add them) as I go.
> 
> > 
> > 
> > 6. Do filters fit better as a delegate/callback mechanism, or as an enum that offers a variety of filter types?
> > 
> 
> I'm thinking an enum (or list of enum / value pairs) since we want to be able to evaluate the filter chain from the impl thread / process independently of the client of the API.

Keep in mind, although only the shortcut filter functions are currently implemented, the CSS filter spec allows for the reference of a full SVG filter DAG via the url() syntax.  This is currently unimplemented on composited layers on either the Apple or Chrome side, but we will need it if we want to be spec-compliant (it does work on the software path, I think).

Also, if the security concerns about GLSL shaders are addressed, we'll also have to figure out how we want to represent those.

> 
> > 
> > 7. I think we’re on a collision course with two different uses of the suffix "Impl".  (e.g. WebLayerImpl is not an impl-thread construct)
> 
> I think so are, but it's probably something we can live with for now.
Comment 5 Shawn Singh 2012-02-29 15:56:42 PST
(In reply to comment #3)

> > 
> > 2. Perhaps we can merge video / webGL / canvas2d / image, from the public perspective?  
> > My rationale:
> > (a) they all update their own compositor resources
> > (b) all we need to know is what may have changed from frame to frame, and possibly some "updater" delegate/callback.
> > 
> 
> This is intriguing.  The question then becomes how much API do we need to expose for these different layer types to work.  I'm worried about pushing complexity out of the core API and making callers have to deal with it all.  For instance, WebGL layers need special handling when they are created, when they receive damage (rate limiter), and they push a different set of properties than other layer types.  I think we can unify the last one of these by having the layer expose the union of all properties that any type would need to set.  That might get unwieldy, but I'm not sure in practice how bad it would be.
> 

Now I think of it, maybe we can evolve this in a different direction.  In some ways, the real purpose of defining these layer types is to know how we should handle resource updates, scheduling / rate limiting, initialization behavior.  So, maybe *those* are the options we should be exposing in the create().  And then privately in our code, create(...) would be clever enough to instantiate the layer type that best suits the purpose.

> > 
> > 3. Do we really need a distinction between WebContentLayer and WebLayer?  We can put ASSERT statements on things like invalidateRect().
> > 
> 
> I think we don't really do - a WebLayer today is simply a WebContentLayer that doesn't have any contents.
> 
> Incidentally, we do need a generic way to mark a part of a layer as 'dirty' even if it isn't a content layer so that damage tracking, etc work.  I was thinking of piggybacking on the invalidate()/invalidateRect() calls for this for non-Content layers, but I'm not sure how to make it clear what happens after an invalidate() call for a Content vs a non-Content layer.  For a Content layer, invalidate() means "this region should be repainted before being displayed".  For everything else invalidate() means "somebody has damaged this part of the layer, but you don't need to invoke any callbacks".
> 

Would it be robust and clear enough to rely on the existence of the painter delegate to make this assumption?  i.e. if the person who requested to create this layer did not provide a painter delegate, then we assume it does not need to do anything on invalidate().  I know this sounds a bit like the old architecture we had with LayerChromiums, so maybe the real question is, why did we originally remove the painter delegate from LayerChromium, and does that reasoning also apply here?


> 
> > 
> > 4. If both #1, #2, and #3 are all possible, then we can provide 3 clear and simple create() functions:
> > (a) createContainerLayer() // for empty containers that don't have content.
> > (b) createContentLayer()    // for the user to provide a paint callback, and we manage the resources via tiling
> > (c) createExternallyManagedLayer() // designed for user to just point to the compositor resources used.
> > perhaps (b) and (c) could even be merged if we’re careful?
> > 
> 
> Is the idea that the create*() calls would accept an appropriate Client interface?
> 

That wasn't my original reasoning, but it does seem reasonable.  I need to think more about how the delegates provide painting/animating/etc functionality.  But probably our discussion of item 2 trumps this anyway; if multiple client interfaces or multiple create() functions are appropriate, it should become clear after deciding something for item 2.