Bug 76015 – [chromium] Use region reported painted opaque for draw culling

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Remember region painted opaque in layers and use for draw culling

[Dana Jansens]

Created attachment 121952 [details]
Patch

What do you think?

- Allows marking a subset of a layer as opaque and passes it thru to the impl side.
- Integrates it into the CCDrawQuad::drawsOpaque() logic so it will be used for the culling.
- Sets the rect after painting a contents layer with the opaque rect reported back to us
  by skia stuff (it depends on https://bugs.webkit.org/show_bug.cgi?id=74352.)

[WebKit Review Bot]

Comment on attachment 121952 [details]
Patch

Attachment 121952 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11148286

[Adrienne Walker]

Comment on attachment 121952 [details]
Patch

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

This is good stuff.  (Could you also some culling-related unit tests involving partially opaque quads?)

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:53
> +    bool drawsOpaque() const { return m_opaqueInSharedQuadState && m_quadOpaque && opacity() == 1; }

Would it be better to expose an opaque rect per quad, so that partially opaque quads can still contribute to culling?

[W. James MacLean]

(In reply to comment #3)
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:53
> > +    bool drawsOpaque() const { return m_opaqueInSharedQuadState && m_quadOpaque && opacity() == 1; }
> 
> Would it be better to expose an opaque rect per quad, so that partially opaque quads can still contribute to culling?

Ditto. If we know the opaqueRect() for a quad, and that quad ends up being drawn, then the opaqueRect() should be added to the occludedRegion in CCQuadCuller so that the opaque part of this layer can help cull quads beneath it.

[Dana Jansens]

Created attachment 122066 [details]
Patch

Expose opaqueRect() on CCDrawQuad, which is the opaque sub-region of the quad. It will be empty if any of the other old drawsOpaque() flags are false.

Added some unit tests for this.

[WebKit Review Bot]

Comment on attachment 122066 [details]
Patch

Attachment 122066 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11108525

[Dana Jansens]

Created attachment 122546 [details]
Patch

rebased.

[Adrienne Walker]

Comment on attachment 122546 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:415
> +    textureUpdater()->prepareToUpdate(m_paintRect, m_tiler->tileSize(), m_tiler->hasBorderTexels(), contentsScale(), &m_opaqueRect);

This doesn't look right to me.  The texture updater records the opaque rect for the current paint rect, not for the entire layer.  So, if you have a giant opaque layer and then the cursor blinks, the opaque rect will be set to be very small.  Would it make sense to store the opaque rect per tile?

[Dana Jansens]

Created attachment 122822 [details]
Patch

- Store an opaque rect in each tile. Constructed from the opaque rect given back from paints.
- CCDrawQuad has an opaqueRect which is typically empty but can be changed by subclass. This is used when opacity == 1 but isOpaque isn't true
- Each tile's opaque rect is passed to the CCTileDrawQuad, and set on the CCDrawQuad for the above case.

[Adrienne Walker]

Comment on attachment 122822 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:277
>          tiledLayer->syncTextureId(i, j, tile->managedTexture()->textureId());
> +        tiledLayer->setTileOpaqueRect(i, j, tile->m_opaqueRect);

Can you combine these two functions, so that we only have one function to sync a tile over?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:373
> +        if (tile->i() >= left && tile->i() <= right && tile->j() >= top && tile->j() <= bottom)
> +            tile->m_opaqueRect = IntRect();

I'm not sure these two lines are necessary.  Don't you set the opaque rect for everything you paint below?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:431
> +            // Save what was opaque in the tile.
> +            IntRect tileRect = m_tiler->tileRect(tile);
> +            tile->m_opaqueRect = intersection(tileRect, paintedOpaqueRect);

I think this could still be better for the "blinking cursor" case where the whole tile isn't painted, but the opaque rect should really continue to be "the whole tile" and not just "the tiny opaque bit that was just painted".

[Dana Jansens]

Comment on attachment 122822 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:277
>> +        tiledLayer->setTileOpaqueRect(i, j, tile->m_opaqueRect);
> 
> Can you combine these two functions, so that we only have one function to sync a tile over?

done. pushTileProperties().

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:373
>> +            tile->m_opaqueRect = IntRect();
> 
> I'm not sure these two lines are necessary.  Don't you set the opaque rect for everything you paint below?

If texture memory runs out it early exists, and thought in this case the tile shouldn't be marked opaque. I see the tiler gets reset() though.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:431
>> +            tile->m_opaqueRect = intersection(tileRect, paintedOpaqueRect);
> 
> I think this could still be better for the "blinking cursor" case where the whole tile isn't painted, but the opaque rect should really continue to be "the whole tile" and not just "the tiny opaque bit that was just painted".

yes, made it better. thanks!

i made it check if the new area is a subset of the old opaque area. that seems enough to me. i was thinking maybe to use the same idea as OpaqueRegionSkia with extending rects, but this is mostly for redrawing things already drawn (but differently) so typically the first paint should mark what is opaque and future updates are more likely to change content without changing opaque-ness. so containment seems like enough. do you agree?

[Dana Jansens]

Created attachment 122963 [details]
Patch

Comments addressed.

Found a flakyness in CCTiledLayerImpl test. The CCSharedQuadState's ownptr was being destroyed when getQuads() finished, but the shared state was expected to be there still for the test to use (referenced from the quads).

[Adrienne Walker]

Comment on attachment 122963 [details]
Patch

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

Thanks for those changes.  Other than one tiny comment, this all looks great to me. (Also, good catch on the CCSharedQuadState in CCTiledLayerImplTest.  Oops.)

> Source/WebCore/platform/graphics/chromium/cc/CCTileDrawQuad.cpp:51
> +    m_opaqueRect = opaqueRect;

Minor aside: For solid color quads, should we also set the opaque rect so that checkerboarded quads still cull what's beneath them?

[Dana Jansens]

(In reply to comment #13)
> Minor aside: For solid color quads, should we also set the opaque rect so that checkerboarded quads still cull what's beneath them?

Yes we should.

Aside aside, I was wondering about SolidColor quads a bit while I was doing this. Right now..

layer is opaque, background color is not => solid color not drawn opaque

Is this intended?

[Dana Jansens]

Created attachment 122976 [details]
Patch

[Dana Jansens]

Comment on attachment 122976 [details]
Patch

Ok, color issue is sorted. Patch ready for review.

[Adrienne Walker]

LGTM!

[James Robinson]

Comment on attachment 122976 [details]
Patch

R=me

[James Robinson]

General comment - can someone make sure we have get good tracing coverage for all culling that we do? We want to be able to figure out how much this is or isn't buying us on various pages, and be able to see if culling is involved on pages that show rendering issues.

[Dana Jansens]

Sure I can take that on if no one wants to fight me for it.

[WebKit Review Bot]

Comment on attachment 122976 [details]
Patch

Rejecting attachment 122976 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 122976 [details]
Patch

Clearing flags on attachment: 122976

Committed r105366: <http://trac.webkit.org/changeset/105366>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This broke CCLayerTreeHostImplTest.blendingOffWhenDrawingOpaqueLayers: 
third_party/WebKit/Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:192: Failure
Value of: testBlendingDrawQuad->needsBlending()
  Actual: true
Expected: m_blend
Which is: false
third_party/WebKit/Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:192: Failure
Value of: testBlendingDrawQuad->needsBlending()
  Actual: true
Expected: m_blend
Which is: false
third_party/WebKit/Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:192: Failure
Value of: testBlendingDrawQuad->needsBlending()
  Actual: true
Expected: m_blend
Which is: false
third_party/WebKit/Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:192: Failure
Value of: testBlendingDrawQuad->needsBlending()
  Actual: true
Expected: m_blend
Which is: false
third_party/WebKit/Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:192: Failure
Value of: testBlendingDrawQuad->needsBlending()
  Actual: true
Expected: m_blend
Which is: false
third_party/WebKit/Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:192: Failure
Value of: testBlendingDrawQuad->needsBlending()
  Actual: true
Expected: m_blend
Which is: false
third_party/WebKit/Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:192: Failure
Value of: testBlendingDrawQuad->needsBlending()
  Actual: true
Expected: m_blend
Which is: false
third_party/WebKit/Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:192: Failure
Value of: testBlendingDrawQuad->needsBlending()
  Actual: true
Expected: m_blend
Which is: false

:/

[James Robinson]

Reverted r105366 for reason:

Breaks CCLayerTreeHostImplTest unit test

Committed r105371: <http://trac.webkit.org/changeset/105371>

[James Robinson]

Sorry Dana, reverted you in http://trac.webkit.org/changeset/105371

[James Robinson]

I think this also broke a number of layout tests:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=compositing%2Fgeometry%2Fforeground-offset-change.html%2Ccompositing%2Foverflow%2Foverflow-scroll.html%2Ccompositing%2Foverflow%2Fscrollbar-painting.html%2Cfast%2Flayers%2Fscroll-with-transform-composited-layer.html%2Cfast%2Frepaint%2Fblock-selection-gap-in-composited-layer.html

see how there's some compositor blue showing through?

[Dana Jansens]

Hmm.. ok I'll have a look. Sorry about that, I guess reworking it changed some stuff or I overlooked something.

[Dana Jansens]

Created attachment 123192 [details]
Patch

For enne review again:

- LayoutTest fail: tile's opaque rect needed to be intersected with the layerVisibleRect
- Unit test fail: Solid color quads can now be considered opaque if their color is opaque so the CCLayerTreeHostImpl.
- Unit test fail: Solid color quads are not opaque if alpha != 255, not if alpha != 1.
- Better distinction between tileBounds() and tileRect() from m_tiler when deciding if the tile is fully opaque or not.
- Turn on opaque tracking always. This is reflected in the LayerTextureUpdaterTest unit test. - Increasingly concerned about not tracking paintedOpaque areas for opaque layers, as its all kinds of unclear what happens if the opaque flag is flipping on/off. This should simplify future code and potential problems a lot.

Passes all DRT and unit tests for sure on my end now.

[Dana Jansens]

(In reply to comment #29)
> - Unit test fail: Solid color quads can now be considered opaque if their color is opaque so the CCLayerTreeHostImpl

unit tests needed to be adjusted to handle this. Added some tests to include combinations of the opaque flag and opaque colors.

[Adrienne Walker]

Comment on attachment 123192 [details]
Patch

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

I like these changes.  Always tracking opacity seems to be a lot cleaner, and will certainly help once we start animating opacity on the impl thread.

> Source/WebCore/platform/graphics/chromium/cc/CCSolidColorDrawQuad.cpp:41
> +    if (m_color.alpha() != 255)

How about using m_color.hasAlpha()?

[Dana Jansens]

(In reply to comment #31)
> How about using m_color.hasAlpha()?

Oh ya, good plan.

[WebKit Review Bot]

Comment on attachment 123192 [details]
Patch

Attachment 123192 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11307087

New failing tests:
fast/repaint/block-selection-gap-in-composited-layer.html
compositing/iframes/iframe-in-composited-layer.html

[Dana Jansens]

(In reply to comment #33)
> New failing tests:
> fast/repaint/block-selection-gap-in-composited-layer.html
> compositing/iframes/iframe-in-composited-layer.html

These are both cases of turning off blending for supposedly "opaque" stuff creating different pixel values (off-by-one in each channel). So they should be rebaselined for chromium.

I believe this is the Skia off-by-one bug[1] baked into the existing expectations. Turning off blending hides the Skia bug for these tests as we no longer blend the pixels with incorrect 254 alpha.
- In the iframe test its the AA'd pixels on the corners of the scrollbars
- In the block selection its the translucent selection area.

Patch with new baselines incoming. I am trying to try it on the mac/win bots also if they stop timing out.

[Dana Jansens]

Created attachment 123340 [details]
Patch

[Dana Jansens]

[1] The skia bug is referenced at http://code.google.com/p/skia/issues/detail?id=420&thanks=420&ts=1324086997 and http://codereview.appspot.com/5494076/

[Dana Jansens]

Created attachment 123346 [details]
Patch

Rebased.

[Dana Jansens]

Created attachment 123360 [details]
Patch

- Mark fast/repaint/block-selection-gap-in-composited-layer.html as IMAGE on MAC and WIN temporarily.
- Undo text changes, only rebaseline the png.

[Dana Jansens]

Created attachment 123558 [details]
Patch

Rebased.

[Dana Jansens]

Created attachment 123570 [details]
Patch

- Compute the tile's opaque rect after checking the tile exists.  (Became a problem when I enabled paint-culling in next CL.)

[Dana Jansens]

Created attachment 124173 [details]
Patch

rebased to ToT

[James Robinson]

Comment on attachment 124173 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:92
> +    canvasPainter.skiaContext()->setTrackOpaqueRegion(true);

why are we enabling tracking when we know the whole layer is opaque? that seems wasteful and it means that we'll lose opaque-ness on layers where we know for external reasons that it's opaque but the skia paint tracker isn't able to detect it automatically

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:55
> +    IntRect opaqueRect() const

this is a bit big to stick in the header

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:59
> +        if (m_sharedQuadState->isOpaque() && m_quadOpaque)

hmm, when do the shared quad state's isOpaque() bit and m_quadOpaque differ? for AA'd edge quads?

[James Robinson]

I don't really understand the motivation for the tracking change. Is the idea that tracking is always going to find opaque rects or opaque layers, so we don't need any other hints? It seems to me to be really valuable to be able to specify that you know a layer is going to be opaque and know that value will be used without having to worry about keeping extra state or about hitting weird edge cases in the tracking code.

[Dana Jansens]

(In reply to comment #43)
> I don't really understand the motivation for the tracking change. Is the idea that tracking is always going to find opaque rects or opaque layers, so we don't need any other hints? It seems to me to be really valuable to be able to specify that you know a layer is going to be opaque and know that value will be used without having to worry about keeping extra state or about hitting weird edge cases in the tracking code.

It's possible for a layer to go from opaque() to non-opaque() at which point it would be really nice to have the opaque painted area known. Otherwise you need to dirty the whole layer at that time to force a repaint and collect the data.

Also there ended up being an increasing number of if (!opaque()) {} in the code making it less clear what is going on and appearing less maintainable over time.

The perks of enabling the tracking is that the layer can change its opaque state at will and everything continues to work with no extra code anywhere else, and simplicity from a single code path that is always used in culling etc - except in the place where the current opaque rect is computed. Do these seem reasonable to you?

> > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:59
> > +        if (m_sharedQuadState->isOpaque() && m_quadOpaque)

> hmm, when do the shared quad state's isOpaque() bit and m_quadOpaque differ? for AA'd edge quads?

m_quadOpaque is something used by CCDrawQuad subclasses to override and say "this is not opaque". CCSolidColorQuad does this, as the layer may be opaque but it can be assigned a non-opaque color for that quad. isOpaque() comes from the source layer state.

[Dana Jansens]

To further clarify: opaque() still trumps the painted opaque tracking. Painting opaque is tracked so if opaque() goes away we have something else to fall back to.

[James Robinson]

If a layer goes from opaque to non-opaque then it means that the whole layer has to be repainting anyway, since it means that every pixel of the layer has gone from having an alpha value of 0 to potentially some other value.  That's not really a loss.  Doesn't setOpaque() invalidate the whole layer? (if not it probably should).

[David Reveman]

(In reply to comment #46)
> If a layer goes from opaque to non-opaque then it means that the whole layer has to be repainting anyway, since it means that every pixel of the layer has gone from having an alpha value of 0 to potentially some other value.  That's not really a loss.  Doesn't setOpaque() invalidate the whole layer? (if not it probably should).

Doesn't going from opaque to non-opaque just mean that some pixels might have changed alpha value to something other than 1.0? Basically, "opaque" means all pixels are guaranteed to be opaque and any other state would fall into the "non-opaque" case. That was my understanding.

[Dana Jansens]

Created attachment 124350 [details]
Patch

- move CCDrawQuad::opaqueRect() to the .cpp
- undo tracking opaque paints when opaque() == true
- call setNeedsDisplay() on setOpaque() - instead of just setNeedsCommit()

[Dana Jansens]

(In reply to comment #47)
> (In reply to comment #46)
> > If a layer goes from opaque to non-opaque then it means that the whole layer has to be repainting anyway, since it means that every pixel of the layer has gone from having an alpha value of 0 to potentially some other value.  That's not really a loss.  Doesn't setOpaque() invalidate the whole layer? (if not it probably should).
> 
> Doesn't going from opaque to non-opaque just mean that some pixels might have changed alpha value to something other than 1.0? Basically, "opaque" means all pixels are guaranteed to be opaque and any other state would fall into the "non-opaque" case. That was my understanding.

Yes it doesn't mean they are necessarily non-opaque now, but I'll defer to jamesr's choice here. He is right that it would be an unusual case to change opaque-state without changing painted contents. If it becomes a problem we can always revisit it.

[David Reveman]

(In reply to comment #48)
> Created an attachment (id=124350) [details]
> Patch
> 
> - move CCDrawQuad::opaqueRect() to the .cpp
> - undo tracking opaque paints when opaque() == true
> - call setNeedsDisplay() on setOpaque() - instead of just setNeedsCommit()

So if the opaqueness of small portion of the layer change we invalidate the whole layer. Is that really what we want?

[Dana Jansens]

(In reply to comment #50)
> (In reply to comment #48)
> > Created an attachment (id=124350) [details] [details]
> > Patch
> > 
> > - move CCDrawQuad::opaqueRect() to the .cpp
> > - undo tracking opaque paints when opaque() == true
> > - call setNeedsDisplay() on setOpaque() - instead of just setNeedsCommit()
> 
> So if the opaqueness of small portion of the layer change we invalidate the whole layer. Is that really what we want?

The flag is an all-or-nothing thing. It changes for things like video layers changing formats, or an image changing its source. It's not something we compute from the painted contents or the like. I think it should be okay.

[David Reveman]

(In reply to comment #51)
> (In reply to comment #50)
> > (In reply to comment #48)
> > > Created an attachment (id=124350) [details] [details] [details]
> > > Patch
> > > 
> > > - move CCDrawQuad::opaqueRect() to the .cpp
> > > - undo tracking opaque paints when opaque() == true
> > > - call setNeedsDisplay() on setOpaque() - instead of just setNeedsCommit()
> > 
> > So if the opaqueness of small portion of the layer change we invalidate the whole layer. Is that really what we want?
> 
> The flag is an all-or-nothing thing. It changes for things like video layers changing formats, or an image changing its source. It's not something we compute from the painted contents or the like. I think it should be okay.

ah, ok this isn't used for contents layers. that's probably fine then. however, i'm not sure about implicitly calling setNeedsDisplay in setOpaque. we're not doing that in any of the other setters (e.g. setOpacity). seems inconsistent.

[Dana Jansens]

(In reply to comment #52)
> ah, ok this isn't used for contents layers. that's probably fine then. however, i'm not sure about implicitly calling setNeedsDisplay in setOpaque. we're not doing that in any of the other setters (e.g. setOpacity). seems inconsistent.

i think the difference is setOpacity() doesn't reflect on the contents of the layer. just how it is presented.

[James Robinson]

Yeah, in my view setOpaque() is a bit of a different API.  It's the caller saying "I know that every pixel in this layer is going to be opaque, no matter what you think". Changing that promise should definitely trigger a repaint, IMO.

[David Reveman]

(In reply to comment #53)
> (In reply to comment #52)
> > ah, ok this isn't used for contents layers. that's probably fine then. however, i'm not sure about implicitly calling setNeedsDisplay in setOpaque. we're not doing that in any of the other setters (e.g. setOpacity). seems inconsistent.
> 
> i think the difference is setOpacity() doesn't reflect on the contents of the layer. just how it is presented.

ok, that makes some sense. considering that setOpaque technically doesn't necessarily mean that the contents changed, my vote is still for not having setOpaque do an implicit invalidation.

[David Reveman]

(In reply to comment #54)
> Yeah, in my view setOpaque() is a bit of a different API.  It's the caller saying "I know that every pixel in this layer is going to be opaque, no matter what you think". Changing that promise should definitely trigger a repaint, IMO.

Hm, I think that's the same as what Dana and I are saying. What I don't understand is why it would have to trigger a repaint. Lets say we have a layer type with a special shader that can introduce opacity in the shader. The opaqueness of that layer could change without us having to repaint anything.

[Dana Jansens]

(In reply to comment #56)
> Hm, I think that's the same as what Dana and I are saying. What I don't understand is why it would have to trigger a repaint. Lets say we have a layer type with a special shader that can introduce opacity in the shader. The opaqueness of that layer could change without us having to repaint anything.

To let the opaque paint tracker figure out what is opaque so we can continue to cull behind the layer.

[David Reveman]

(In reply to comment #57)
> (In reply to comment #56)
> > Hm, I think that's the same as what Dana and I are saying. What I don't understand is why it would have to trigger a repaint. Lets say we have a layer type with a special shader that can introduce opacity in the shader. The opaqueness of that layer could change without us having to repaint anything.
> 
> To let the opaque paint tracker figure out what is opaque so we can continue to cull behind the layer.

That only applies to content layers that make use of setOpaque(), right? IMO, it seems better to always use the paint tracker so we don't have to repaint when setOpaque() changes.

[James Robinson]

(In reply to comment #58)
> (In reply to comment #57)
> > (In reply to comment #56)
> > > Hm, I think that's the same as what Dana and I are saying. What I don't understand is why it would have to trigger a repaint. Lets say we have a layer type with a special shader that can introduce opacity in the shader. The opaqueness of that layer could change without us having to repaint anything.
> > 
> > To let the opaque paint tracker figure out what is opaque so we can continue to cull behind the layer.
> 
> That only applies to content layers that make use of setOpaque(), right? IMO, it seems better to always use the paint tracker so we don't have to repaint when setOpaque() changes.

But we *HAVE* to repaint when setOpaque() changes, since that means that the caller is telling you that the pixels values are different than they used to be.

[Dana Jansens]

TBH repainting won't cause correctness issues and James likes it. This is blocking paint culling - which appears to half video memory/bandwidth use on chromium os. So I'd rather move ahead and try get culling in for M18, and we can come back to this detail?

[Dana Jansens]

Created attachment 124383 [details]
Patch

rebased

[James Robinson]

Comment on attachment 124383 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:447
> +            IntRect fixedSizeTileRectWithBorders = m_tiler->tileRect(tile);
> +            IntRect sourceRect = fixedSizeTileRectWithBorders;

what's up with the extra var here? what was wrong with the old code?

[Dana Jansens]

Thanks!

(In reply to comment #62)
> (From update of attachment 124383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124383&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:447
> > +            IntRect fixedSizeTileRectWithBorders = m_tiler->tileRect(tile);
> > +            IntRect sourceRect = fixedSizeTileRectWithBorders;
> 
> what's up with the extra var here? what was wrong with the old code?

There's two tileRect's in close proximity now and they are very different (one is the real bounds, one inlcudes borders and is the default tile size). It wasn't very clear to me at first that they were different. So I thought a variable name could clarify. Could be a comment instead if you like.

[James Robinson]

But we don't use it. You can just rename the variable completely if you want, rather than making another local

[Dana Jansens]

Created attachment 124431 [details]
Patch

Thanks. I think a comment will do here.

[James Robinson]

Comment on attachment 124431 [details]
Patch

R=me.

When I r+/cq- you don't need to overwrite the r+ with a new patch - you can land the patch with the suggestions as-is on the existing r+

[WebKit Review Bot]

Comment on attachment 124431 [details]
Patch

Clearing flags on attachment: 124431

Committed r106300: <http://trac.webkit.org/changeset/106300>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.