Bug 69441 – [chromium] Keep track of the paint rect in layers

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

This is part 1 of adding compositor support for --show-paint-rects.  Note that the "dirty rect" (area marked as needing repaint) is not necessarily the same as the "paint rect" (area that is actually repainted), even though they will often be the same.  In addition to visualizing paint rects, keeping track of this will be useful for computing a good scissor rect as well.

[Shawn Singh]

Created attachment 109982 [details]
Patch

[Shawn Singh]

Reviewers please look closely at changes in TiledLayerChromium... the "contentRectToLayerRect" conversion may do clipping as a side effect.  Is this still correct, or is it more correct to manually translate the rect without clipping?

[James Robinson]

Comment on attachment 109982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109982&action=review

I think that this should be the return value of whatever function is calculating it and not state on the layer.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:218
> +    FloatRect m_actuallyRepaintedRect;

This isn't really state on the layer, is it? Isn't it an output from updateCompositorResources()?

Also I'm a bit confused about the name - if this is about repainting, why isn't it calculated by paintContentsIfDirty()? It sounds more like this is talking about the portion of the layer that was changed due to texture uploads and the like, which isn't necessarily the same thing as the portion that was painted.

[Shawn Singh]

(In reply to comment #3)
> (From update of attachment 109982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109982&action=review
> 
> I think that this should be the return value of whatever function is calculating it and not state on the layer.

I disagree... it really is part of the state of the layer.  This rect is going to be pushed to the CCLayerImpl, and would eventually affect scissor and draw rects.  Even if we use a return value, we have to store this value so it can be propagated in pushPropertiesTo.  The CCLayerTreeHost, which calls paintContentsIfDirty and updateCompositorResources, is not the right place to receive those values.

> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:218
> > +    FloatRect m_actuallyRepaintedRect;
> 
> This isn't really state on the layer, is it? Isn't it an output from updateCompositorResources()?
> 
> Also I'm a bit confused about the name - if this is about repainting, why isn't it calculated by paintContentsIfDirty()? It sounds more like this is talking about the portion of the layer that was changed due to texture uploads and the like, which isn't necessarily the same thing as the portion that was painted.

It will be more efficient to discuss this in person first =)

[Shawn Singh]

Created attachment 110013 [details]
renamed repaint to update

[Shawn Singh]

Please note my original question about contentRectToLayerRect.

Discussed offline with jamesr.  As I understand, we agreed that the rest of the patch is good the way it is; cleaning up the way we manage "transient state" will need to be done in a separate change.  Please let me know if that's not what we agreed.

[James Robinson]

Comment on attachment 110013 [details]
renamed repaint to update

View in context: https://bugs.webkit.org/attachment.cgi?id=110013&action=review

Getting close! You probably want to read the feedback bottom up as the interesting comments are near the bottom of the patch.

> Source/WebCore/ChangeLog:4
> +        Added a FloatRect to LayerChromium that tracks what region was actually
> +        updated in a layer.

bug titles should ideally describe what the patch is doing or what problem it's solving, not how. also, it's very helpful to tag chromium-specific patches with [chromium] so that people looking through the commit log for regressions on other platforms (for example) can know that this patch only impacts chromium. so maybe something like "[chromium] Track the updated portion of the layer in updateCompositingResources()"

> Source/WebCore/ChangeLog:10
> +        Tests will be added in a separate patch after the
> +        GraphicsContext3D and allocator can be mocked.

FYI we can mock out GraphicsContext3D today I believe - take a look at MockGraphicsContext3D and CompositorMockGraphicsContext3D in WebKit/chromium/tests/.  TextureAllocator is pretty thin and designed to be mocked easily if you're so inclined.  I'm not sure if there's all that much to test here though since the value's not being used for anything.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:84
> +    m_actuallyUpdatedRect = FloatRect(0.0f, 0.0f, bounds().width(), bounds().height());

Could be FloatRect(FloatPoint::zero(), bounds());

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:218
> +    FloatRect m_actuallyUpdatedRect;

sorry to rathole on the name but this still feels awkward.  why isn't this m_updatedRect? is there another variable that we are trying to distinguish this from with the 'actually' qualifier?

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:71
> +    m_actuallyUpdatedRect = FloatRect(0.0f, 0.0f, bounds().width(), bounds().height());

could be written as:

FloatRect(FloatPoint::zero(), bounds());

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:140
> +    m_actuallyUpdatedRect = FloatRect(0.0f, 0.0f, bounds().width(), bounds().height());

same here - this could be written as FloatRect(FloatPoint::zero(), bounds());

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:95
> +        m_actuallyUpdatedRect = FloatRect(0.0f, 0.0f, bounds().width(), bounds().height());

ditto re: FloatRect(). since this is repeated 4x should it be in the base class to be the default behavior so that subclasses can override it with more specific bounds if they have it? if we add a new layer type we'll have to remember to add this incantation.

maybe one way to do this would be to have a virtual updatedRect() getter on LayerChromium that returns this and override that in TiledLayerChromium with a function that calculates the value it uses.

[Vangelis Kokkevis]

Comment on attachment 110013 [details]
renamed repaint to update

View in context: https://bugs.webkit.org/attachment.cgi?id=110013&action=review

You'll need some code to clear out the update rects every update cycle, otherwise they'll stick around.  You'll probably need to do this in CCLayerTreeHost::updateCompositorResources() before calling updateCompositorResources(layer, context, allocator)

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:70
> +    // Plugin layers do their own repainting, so we assume it updates the entire layer on every update.

I'm not sure I agree with this one. Unlike other layers, for plugins we have no way of knowing whether any updates too place, at least not here.  I would not mark it as updated.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:209
> +            totalUpdatedContent.unite(sourceRect);

I don't think you need to aggregate the individual tiles. m_updateRect is the rect you need.

[Shawn Singh]

Actually I think the new patch (coming in a few moments) might have indirectly addressed some of your comments, since those points in code are no longer needed.

However there are 2 things to point out:

- We will need to keep the update rect until we have the opportunity to push it to the CCLayerImpl during commit

- The new patch also proposes to re-name "m_updateRect" in the TiledLayerChromium, because it is currently a misnomer.  That rect is really just a temporary content rect needed for re-computing tile indices, valid only between prepareToUpdate and updateCompositorResources.  What we really want is the m_paintRect, which happens to be the same as the update rect for TiledLayerChromium.


(In reply to comment #8)
> (From update of attachment 110013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110013&action=review
> 
> You'll need some code to clear out the update rects every update cycle, otherwise they'll stick around.  You'll probably need to do this in CCLayerTreeHost::updateCompositorResources() before calling updateCompositorResources(layer, context, allocator)
> 
> > Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:70
> > +    // Plugin layers do their own repainting, so we assume it updates the entire layer on every update.
> 
> I'm not sure I agree with this one. Unlike other layers, for plugins we have no way of knowing whether any updates too place, at least not here.  I would not mark it as updated.
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:209
> > +            totalUpdatedContent.unite(sourceRect);
> 
> I don't think you need to aggregate the individual tiles. m_updateRect is the rect you need.

[Shawn Singh]

Created attachment 110084 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 110084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110084&action=review

A high level comment:  Overall for our purposes, we really want to track what areas of the layer get repainted as painting is an expensive operation.  It so happens that for TiledLayerChromium's, the repaint rects are (almost?) identical to the upload rects (what we use to upload the texture contents, which is what you're tracking now). So, for tiled layers m_paintRect is actually the most correct region to keep track of as that's what's getting sent to the texture updater that triggers a webkit repaint.  

Image layers don't cost much in terms of repainting but it would be good to keep track of when we upload new textures for them which can be done in ImageLayerChromium::updateTextureRect() .  Similarly with Canvas2DLayerChromium and WebGLLayerChromium, we can catch the rects they upload on their updateCompositorResources() call.  PluginLayerChromium is the one I'm a bit uncertain of as I'm not sure how we'll know whether it gets a new texture or not.

To avoid having to make a distinction between paintedRect and updatedRect we can call this new rect something like modifiedRect to actually capture the intersection of the two that's useful for us.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:113
> +    virtual const FloatRect updateRect() const { return FloatRect(FloatPoint::zero(), bounds()); }

Canvas2D, WebGL and Image layers don't always update their contents.  Their updateRect should be set based on whether they actually did change.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:165
> +    virtual void updateCompositorResources(GraphicsContext3D*, TextureAllocator*) { ASSERT_NOT_REACHED(); }

I suspect this assert will now trigger at least for PluginLayerChromium's

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:92
> +    m_contentToPaint = IntRect();

Not a huge fan of this new name. It's been conventional to use the Rect suffix for variables that hold rects.  

Also, how do we now get the updateRect for tiled layers?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:90
> +    // The original rect that indicates what should be painted

Not sure I fully understand what you mean by the "original"

[Shawn Singh]

Thanks for the comments.  Here is my summary response:

- So clearly the virtual function approach does not work - oops.  I'll revert the approach to the previous patch.

- I'm not clear on what you meant by the "intersection" of paint rect and update rect.  Can you please clarify?  If you are OK with it, I would prefer to stick with "paint rect" and "update rect" only, adding a "modified rect" may be further confusing.

- I think at this point, the right thing to do is simply track both paint rect and update rect.   Its quite straightforward, and I'm working on it right now.

- Can you please suggest a better name for the rect "m_contentToPaint"?  how about "m_contentRect"?   The only purpose of this rect is simply to store the content rect temporarily so that we can iterate over tile indices properly.

[James Robinson]

(In reply to comment #11)
> (From update of attachment 110084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110084&action=review
> 
> A high level comment:  Overall for our purposes, we really want to track what areas of the layer get repainted as painting is an expensive operation.  It so happens that for TiledLayerChromium's, the repaint rects are (almost?) identical to the upload rects (what we use to upload the texture contents, which is what you're tracking now). So, for tiled layers m_paintRect is actually the most correct region to keep track of as that's what's getting sent to the texture updater that triggers a webkit repaint.  
> 
> Image layers don't cost much in terms of repainting but it would be good to keep track of when we upload new textures for them which can be done in ImageLayerChromium::updateTextureRect() .  Similarly with Canvas2DLayerChromium and WebGLLayerChromium, we can catch the rects they upload on their updateCompositorResources() call.  PluginLayerChromium is the one I'm a bit uncertain of as I'm not sure how we'll know whether it gets a new texture or not.
> 

So now I don't know what we are trying to track here.  What's this for?  If it's for the scissoring optimization then we need to know what parts of the layer may be different from previous frames, since they have to be drawn.  If we're trying to track paint time for some sort of development tool then that sounds like something different.  Can you describe the use case that you think this data will be used for?  I'd much rather start with use cases and then figure out what data we need vs adding some data without a clear use for it.

[Shawn Singh]

I think both your use cases are intended.  Here's a summary of the two goals:

1. analysis of various performance overheads.  It is useful to visualize dirty rects, paint rects, update rects, draw rects, and the scissor rect.  Each one (and the combination of them) helps narrow down the cause of performance issues.  I personally have already found these rects useful to find bugs (for example, https://bugs.webkit.org/show_bug.cgi?id=67009) and performance overheads. As we try to improve performance, these rects will be useful, both human visualization and automated (like computing overdraw) for debugging and testing our performance improvements.

2. computing a scissor rect.  (others are waiting on this optimization, so this is priority)  The scissor rect depends on draw rects.  the draw rects can be optimized by knowing the update rects (and dirty rects, since the flow of information will not be perfect)

In order to move forward with these use cases, we want to track the update rect.  Vangelis wants to make available visualizing paint rects, because this information will immediately help other groups.  So, I think the solution is to track both, and this one small patch can easily do that =)

Also just discussed offline with Vangelis and agreed on tracking and visualizing both.  James, does all this sound good to you?

[James Robinson]

What exactly does "tracking" mean for 1?  Is something other than the compositor the consumer of that data?  Who will be doing the tracking (us, web developers, someone else?)  There are lots of interesting things to track depending on who is consuming the data and what they are tracking.  Should we hook that up to the tracing system perhaps?

For scissoring the thing we need to know is what portion of each layer has changed in the updateCompositorResources() call.  For contents textures this is easy - we directly control what textures get uploaded.  The situation is more complicated for plugin layers since the model there is that the texture ID is owned by the plugin. The plugin notifies the compositor when the texture is updated, but it's possible that we haven't yet received that notification when we go to draw from the texture.  Maybe we need to add a copy here?  I think we'll get into the same place here for video layers soon as well when using a h/w stack.

[Shawn Singh]

(In reply to comment #15)
> What exactly does "tracking" mean for 1?  Is something other than the compositor the consumer of that data?  Who will be doing the tracking (us, web developers, someone else?)  There are lots of interesting things to track depending on who is consuming the data and what they are tracking.  Should we hook that up to the tracing system perhaps?
> 

by "tracking" actually I just meant simply that we have to add that state in the classes.  Examples of "consumers" of this information:  the compositor (including our HUD and scissor rect), the Inspector for web devs to analyze their page performance, and unit tests.

> For scissoring the thing we need to know is what portion of each layer has changed in the updateCompositorResources() call.  For contents textures this is easy - we directly control what textures get uploaded.  The situation is more complicated for plugin layers since the model there is that the texture ID is owned by the plugin. The plugin notifies the compositor when the texture is updated, but it's possible that we haven't yet received that notification when we go to draw from the texture.  Maybe we need to add a copy here?  I think we'll get into the same place here for video layers soon as well when using a h/w stack.

My thoughts on this:

Overall I think what you're talking about are secondary optimizations.  Any scissoring is better than no scissoring.  The most critical one is the root layer (i.e. tiled layer).  In my old version of the scissor, it was good enough to use the dirty rect, because webkit safely marked video/webgl/plugin/canvas as the entire layer being dirty.  However, piping the dirty rect the way that I originally did was not a good idea.  The cleanest yet realistic approach is to compute scissor based on update rect, and when that information is not available, assume that the entire layer needs redraw (which is what webkit's dirty rects were doing anyway).  Of course those rects may still be culled by visibility/overdraw/etc. on the impl side.

Optimizing canvas (and maybe plugin) could be a useful secondary gain.  And optimizing webGL and video, should be low priority for now, because most likely when they exist, the entire layer changes most of the time anyway.

I'm not sure what copy you're referring to... but I'm quite certain that no texture copies will be necessary, right?

New patch should be coming soon... =)

[James Robinson]

If we always assume the entire plugin layer has changed for the purpose of calculating the scissor, then no I don't think we'll need to add a copy.

[Shawn Singh]

Created attachment 110258 [details]
Patch

[Shawn Singh]

Notes about this most recent patch:

(1) After discussing with Vangelis offline, we realized that the sourceRect aggregation in TiledLayerChromium.cpp should probably stay there, because it may result in a updateRect that is different from paintRect.  However, i'm not clear on what case this is true?

(2) m_paintRect is actually updated by existing code, which is why it doesn't appear in the patch.  m_paintRect currently is only applicable to TiledLayerChromium, and is updated in prepareToUpdate()  (which occurs during paintContentsIfDirty()).

(3) There is one last design choice I would like to discuss.  Instead of putting m_paintRect directly in the base class, maybe it is appropriate where it originally was, in the TiledLayerChromium... because that is currently the only layer type that uses the concept of a paint rect.

If we do that, then the paint rect changes would disappear in this patch, but in the part 2 (visualization) patch, we will have to add a specialized pushPropertiesTo for the TiledLayerChromium to give the paint rect to the draw side of things.

I can see arguments on both design choices.  I'm leaning towards the latter (removing paint rect from this patch).  What do you all think?

[Shawn Singh]

Can reviewers please answer point #3 in the previous comment?

If the answer is "no, keep it the way it is", then can you please review this?  Otherwise I will submit a new patch ASAP...

[James Robinson]

For the way we're using the term "paint" meaning "call into WebCore via a paintContents() callback", it only makes sense on TiledLayerChromium so I think that it makes sense to move the m_paintRect down to that class.

[Shawn Singh]

Created attachment 110435 [details]
Patch

[Adrienne Walker]

Comment on attachment 110435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110435&action=review

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:215
> +    // FIXME: Prove/disprove whether totalUpdatedContent will always be the same as m_paint. For now, assuming its not.
> +    m_updateRect = FloatRect(m_tiler->contentRectToLayerRect(totalUpdatedContent));

This seems like something you should figure out before you land this patch.  It's my assumption that the paint rect will be identical to the update rect.  We can't update something we haven't painted, and we consider all tiles that we've painted when doing updates.

[Shawn Singh]

(In reply to comment #23)
> (From update of attachment 110435 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110435&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:215
> > +    // FIXME: Prove/disprove whether totalUpdatedContent will always be the same as m_paint. For now, assuming its not.
> > +    m_updateRect = FloatRect(m_tiler->contentRectToLayerRect(totalUpdatedContent));
> 
> This seems like something you should figure out before you land this patch.  It's my assumption that the paint rect will be identical to the update rect.  We can't update something we haven't painted, and we consider all tiles that we've painted when doing updates.


Yeah, I agree.  Vangelis, since we had discussed this earlier, what do you think?  After our discussion I had second thoughts and could not think of the case where they are different.  I think they are the same.

If they are the same, then I'll remove that aggregation put m_update = FloatRect(m_paint);

[Vangelis Kokkevis]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 110435 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=110435&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:215
> > > +    // FIXME: Prove/disprove whether totalUpdatedContent will always be the same as m_paint. For now, assuming its not.
> > > +    m_updateRect = FloatRect(m_tiler->contentRectToLayerRect(totalUpdatedContent));
> > 
> > This seems like something you should figure out before you land this patch.  It's my assumption that the paint rect will be identical to the update rect.  We can't update something we haven't painted, and we consider all tiles that we've painted when doing updates.
> 
> 
> Yeah, I agree.  Vangelis, since we had discussed this earlier, what do you think?  After our discussion I had second thoughts and could not think of the case where they are different.  I think they are the same.
> 
> If they are the same, then I'll remove that aggregation put m_update = FloatRect(m_paint);

I agree that the updateRect computed as a union of update rects is the same as the paint rect but it's not an accurate measure of how many pixels we've uploaded as textures, if that's what we're after. It's really the bounding box around the updated area. For example, consider this layer:

X X X 
X O X 
X X X 

where X are dirty tiles and O is a clean tile.  Uploads will only happen for the dirty tiles so there will be a total of 8 tile-sized texture uploads whereas the updateRect, being a union of all the X's will contain the surface of 9 tiles.

That said, if we regard the update rect as the region of the layer that changed within the last frame, then I think it makes sense to track it separately from the paint rect (which tracks the area that we asked webkit to redraw).  In that case, for tiled layers, paintRect == updateRect but for WebGL, canvas, video paintRect = 0 and updateRect is equal to the entire layer when something changes in it (a new video frame, webgl / canvas marked dirty).

[James Robinson]

(In reply to comment #25)
> (In reply to comment #24)

> I agree that the updateRect computed as a union of update rects is the same as the paint rect but it's not an accurate measure of how many pixels we've uploaded as textures, if that's what we're after. It's really the bounding box around the updated area. For example, consider this layer:
> 
> X X X 
> X O X 
> X X X 
> 
> where X are dirty tiles and O is a clean tile.  Uploads will only happen for the dirty tiles so there will be a total of 8 tile-sized texture uploads whereas the updateRect, being a union of all the X's will contain the surface of 9 tiles.
> 

Actually I'm pretty sure we paint and upload all 9 in this case because we union damage when it's received and we union 'damage' from evicted tiles.

[Vangelis Kokkevis]

Comment on attachment 110435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110435&action=review

> Source/WebCore/ChangeLog:3
> +        [chromium] Tracking paint rects and update rects in LayerChromium.

I'm not seeing where the paint rects are being tracked.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:306
> +    m_updateRect = FloatRect();

Don't you want to push the update rect over to the impl side before you clear it?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:88
> +    IntRect m_contentRect;

I think a better name for this would be m_visibleContentRect.  m_contentRect as used elsewhere in the code usually stores the entire content area of the layer.  We could also change the parameter name in TiledLayerChromium::prepareToUpdate() to match it.

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:139
> +    m_updateRect = FloatRect(FloatPoint::zero(), bounds());

I think it makes sense for the update rect to capture the area that was changed between frames so in the case of the video layer it would be the entire frame, if it was marked dirty.

[Vangelis Kokkevis]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> 
> > I agree that the updateRect computed as a union of update rects is the same as the paint rect but it's not an accurate measure of how many pixels we've uploaded as textures, if that's what we're after. It's really the bounding box around the updated area. For example, consider this layer:
> > 
> > X X X 
> > X O X 
> > X X X 
> > 
> > where X are dirty tiles and O is a clean tile.  Uploads will only happen for the dirty tiles so there will be a total of 8 tile-sized texture uploads whereas the updateRect, being a union of all the X's will contain the surface of 9 tiles.
> > 
> 
> Actually I'm pretty sure we paint and upload all 9 in this case because we union damage when it's received and we union 'damage' from evicted tiles.

Ah yes, you're right!

[Shawn Singh]

Alright then, I'll upload the (hopefully) final patch tonight, where m_updateRect = m_paintRect.

(In reply to comment #27)
> (From update of attachment 110435 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110435&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [chromium] Tracking paint rects and update rects in LayerChromium.
> 
> I'm not seeing where the paint rects are being tracked.
> 

Vestigal cruft, I'll remove it.

> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:306
> > +    m_updateRect = FloatRect();
> 
> Don't you want to push the update rect over to the impl side before you clear it?
> 

I was going to wait until part 2 for that.  I had originally intended this first patch to be very brief and quick... But yes, it will be pushed once we add the same property to CCLayerImpl.

> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:88
> > +    IntRect m_contentRect;
> 
> I think a better name for this would be m_visibleContentRect.  m_contentRect as used elsewhere in the code usually stores the entire content area of the layer.  We could also change the parameter name in TiledLayerChromium::prepareToUpdate() to match it.
> 

OK will change

> > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:139
> > +    m_updateRect = FloatRect(FloatPoint::zero(), bounds());
> 
> I think it makes sense for the update rect to capture the area that was changed between frames so in the case of the video layer it would be the entire frame, if it was marked dirty.

This code is already a rect that represents the entire layer, right?  point (0,0) and bounds (width,height)
Same should be true for WebGL and canvas2D...  Did I overlook something?

[Vangelis Kokkevis]

Comment on attachment 110435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110435&action=review

>>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:139
>>> +    m_updateRect = FloatRect(FloatPoint::zero(), bounds());
>> 
>> I think it makes sense for the update rect to capture the area that was changed between frames so in the case of the video layer it would be the entire frame, if it was marked dirty.
> 
> This code is already a rect that represents the entire layer, right?  point (0,0) and bounds (width,height)
> Same should be true for WebGL and canvas2D...  Did I overlook something?

Doh, sorry, I misread the code! You are right.

[Shawn Singh]

Created attachment 110463 [details]
Patch

[Shawn Singh]

Can someone please review this latest version?  Hopefully it could be the final version.

[Shawn Singh]

Created attachment 111114 [details]
Added update rect to CCLayerImpl also

[James Robinson]

Comment on attachment 111114 [details]
Added update rect to CCLayerImpl also

View in context: https://bugs.webkit.org/attachment.cgi?id=111114&action=review

I think the updateRect for tiled layers is wrong - shouldn't it be in layer space? The easiest way to check this is to see what the expected and actual update rectangle are for an animated image layer that's scaled in CSS.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:211
> +    m_updateRect = FloatRect(m_paintRect);

m_paintRect is in content space, not layer space. Is that what we expect? All of the other m_updateRects are in layer space, not content space.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:251
> +    // Reset any state that should be cleared for the next update.
> +    m_paintRect = IntRect();

Why do we clear the paint rect here? We didn't push that to the CCLayerImpl

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:350
> +void TiledLayerChromium::prepareToUpdate(const IntRect& visibleContentRect)

Now that the parameter name and member variable have nearly the same name (modulo the "m_") I find this function much harder to read. I think the original name was actually better - the caller was requesting that we prepare to update the region corresponding with a given content rect. The fact that the content rect represents a visible area is the caller's concern, not the concern of this function - and in fact when we start doing tile prerendering the passed-in content rect will not necessarily have much to do with what's currently visible.

I think the parameter name 'contentRect' was better and the member variable should be something like 'requestedUpdateRect' to reflect that we might not end up actually updating that exact rectangle.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:69
>      // Prepare data needed to update textures that intersect with contentRect.
> -    void prepareToUpdate(const IntRect& contentRect);
> -    // Update invalid textures that intersect with contentRect provided in prepareToUpdate().
> -    void updateRect(GraphicsContext3D*, LayerTextureUpdater*);
> +    void prepareToUpdate(const IntRect& visibleContentRect);

The comment and signature are out of sync

[Shawn Singh]

Created attachment 111472 [details]
addressed all of jamesr comments

[James Robinson]

Comment on attachment 111472 [details]
addressed all of jamesr comments

R=me

[WebKit Review Bot]

Comment on attachment 111472 [details]
addressed all of jamesr comments

Clearing flags on attachment: 111472

Committed r97780: <http://trac.webkit.org/changeset/97780>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.