Bug 76015 - [chromium] Use region reported painted opaque for draw culling
Summary: [chromium] Use region reported painted opaque for draw culling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 76211
Blocks: 76858
  Show dependency treegraph
 
Reported: 2012-01-10 17:45 PST by Dana Jansens
Modified: 2012-01-30 16:29 PST (History)
10 users (show)

See Also:


Attachments
Patch (18.52 KB, patch)
2012-01-10 17:47 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (24.88 KB, patch)
2012-01-11 12:09 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (22.82 KB, patch)
2012-01-14 07:30 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (23.11 KB, patch)
2012-01-17 15:16 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (28.92 KB, patch)
2012-01-18 10:41 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (29.52 KB, patch)
2012-01-18 12:42 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (44.97 KB, patch)
2012-01-19 14:10 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (56.06 KB, patch)
2012-01-20 10:18 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (55.96 KB, patch)
2012-01-20 11:11 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (54.34 KB, patch)
2012-01-20 12:27 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (54.54 KB, patch)
2012-01-23 08:08 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (54.53 KB, patch)
2012-01-23 09:58 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (54.40 KB, patch)
2012-01-26 13:52 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (47.95 KB, patch)
2012-01-27 12:32 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (47.08 KB, patch)
2012-01-27 15:30 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (46.95 KB, patch)
2012-01-27 23:14 PST, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-01-10 17:45:32 PST
[chromium] Remember region painted opaque in layers and use for draw culling
Comment 1 Dana Jansens 2012-01-10 17:47:39 PST
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.)
Comment 2 WebKit Review Bot 2012-01-10 18:05:26 PST
Comment on attachment 121952 [details]
Patch

Attachment 121952 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11148286
Comment 3 Adrienne Walker 2012-01-11 08:48:21 PST
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?
Comment 4 W. James MacLean 2012-01-11 09:18:37 PST
(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.
Comment 5 Dana Jansens 2012-01-11 12:09:14 PST
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.
Comment 6 WebKit Review Bot 2012-01-11 12:16:36 PST
Comment on attachment 122066 [details]
Patch

Attachment 122066 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11108525
Comment 7 Dana Jansens 2012-01-14 07:30:18 PST
Created attachment 122546 [details]
Patch

rebased.
Comment 8 Adrienne Walker 2012-01-17 12:31:44 PST
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?
Comment 9 Dana Jansens 2012-01-17 15:16:30 PST
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.
Comment 10 Adrienne Walker 2012-01-17 16:13:15 PST
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".
Comment 11 Dana Jansens 2012-01-18 10:38:12 PST
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?
Comment 12 Dana Jansens 2012-01-18 10:41:53 PST
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).
Comment 13 Adrienne Walker 2012-01-18 12:00:08 PST
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?
Comment 14 Dana Jansens 2012-01-18 12:38:38 PST
(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?
Comment 15 Dana Jansens 2012-01-18 12:42:00 PST
Created attachment 122976 [details]
Patch
Comment 16 Dana Jansens 2012-01-18 13:44:11 PST
Comment on attachment 122976 [details]
Patch

Ok, color issue is sorted. Patch ready for review.
Comment 17 Adrienne Walker 2012-01-18 14:29:13 PST
LGTM!
Comment 18 James Robinson 2012-01-18 15:12:18 PST
Comment on attachment 122976 [details]
Patch

R=me
Comment 19 James Robinson 2012-01-18 15:14:07 PST
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.
Comment 20 Dana Jansens 2012-01-18 15:14:59 PST
Sure I can take that on if no one wants to fight me for it.
Comment 21 WebKit Review Bot 2012-01-18 15:15:39 PST
Comment on attachment 122976 [details]
Patch

Rejecting attachment 122976 [details] from commit-queue.

danakj@chromium.org 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.
Comment 22 WebKit Review Bot 2012-01-18 17:42:44 PST
Comment on attachment 122976 [details]
Patch

Clearing flags on attachment: 122976

Committed r105366: <http://trac.webkit.org/changeset/105366>
Comment 23 WebKit Review Bot 2012-01-18 17:42:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 James Robinson 2012-01-18 18:17:36 PST
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

:/
Comment 25 James Robinson 2012-01-18 18:39:17 PST
Reverted r105366 for reason:

Breaks CCLayerTreeHostImplTest unit test

Committed r105371: <http://trac.webkit.org/changeset/105371>
Comment 26 James Robinson 2012-01-18 18:39:50 PST
Sorry Dana, reverted you in http://trac.webkit.org/changeset/105371
Comment 28 Dana Jansens 2012-01-18 23:42:22 PST
Hmm.. ok I'll have a look. Sorry about that, I guess reworking it changed some stuff or I overlooked something.
Comment 29 Dana Jansens 2012-01-19 14:10:27 PST
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.
Comment 30 Dana Jansens 2012-01-19 14:12:02 PST
(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.
Comment 31 Adrienne Walker 2012-01-19 17:20:39 PST
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()?
Comment 32 Dana Jansens 2012-01-19 17:28:44 PST
(In reply to comment #31)
> How about using m_color.hasAlpha()?

Oh ya, good plan.
Comment 33 WebKit Review Bot 2012-01-19 17:32:26 PST
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
Comment 34 Dana Jansens 2012-01-20 10:14:43 PST
(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.
Comment 35 Dana Jansens 2012-01-20 10:18:51 PST
Created attachment 123340 [details]
Patch
Comment 37 Dana Jansens 2012-01-20 11:11:35 PST
Created attachment 123346 [details]
Patch

Rebased.
Comment 38 Dana Jansens 2012-01-20 12:27:38 PST
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.
Comment 39 Dana Jansens 2012-01-23 08:08:18 PST
Created attachment 123558 [details]
Patch

Rebased.
Comment 40 Dana Jansens 2012-01-23 09:58:38 PST
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.)
Comment 41 Dana Jansens 2012-01-26 13:52:59 PST
Created attachment 124173 [details]
Patch

rebased to ToT
Comment 42 James Robinson 2012-01-27 11:50:32 PST
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?
Comment 43 James Robinson 2012-01-27 11:52:45 PST
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.
Comment 44 Dana Jansens 2012-01-27 11:59:54 PST
(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.
Comment 45 Dana Jansens 2012-01-27 12:01:44 PST
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.
Comment 46 James Robinson 2012-01-27 12:04:00 PST
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).
Comment 47 David Reveman 2012-01-27 12:26:52 PST
(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.
Comment 48 Dana Jansens 2012-01-27 12:32:45 PST
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()
Comment 49 Dana Jansens 2012-01-27 12:35:20 PST
(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.
Comment 50 David Reveman 2012-01-27 12:46:19 PST
(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?
Comment 51 Dana Jansens 2012-01-27 12:50:22 PST
(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.
Comment 52 David Reveman 2012-01-27 12:56:55 PST
(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.
Comment 53 Dana Jansens 2012-01-27 13:05:38 PST
(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.
Comment 54 James Robinson 2012-01-27 13:09:51 PST
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.
Comment 55 David Reveman 2012-01-27 13:13:24 PST
(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.
Comment 56 David Reveman 2012-01-27 13:29:54 PST
(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.
Comment 57 Dana Jansens 2012-01-27 13:32:52 PST
(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.
Comment 58 David Reveman 2012-01-27 13:49:18 PST
(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.
Comment 59 James Robinson 2012-01-27 13:54:30 PST
(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.
Comment 60 Dana Jansens 2012-01-27 13:58:03 PST
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?
Comment 61 Dana Jansens 2012-01-27 15:30:21 PST
Created attachment 124383 [details]
Patch

rebased
Comment 62 James Robinson 2012-01-27 22:43:22 PST
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?
Comment 63 Dana Jansens 2012-01-27 22:53:41 PST
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.
Comment 64 James Robinson 2012-01-27 23:01:43 PST
But we don't use it. You can just rename the variable completely if you want, rather than making another local
Comment 65 Dana Jansens 2012-01-27 23:14:19 PST
Created attachment 124431 [details]
Patch

Thanks. I think a comment will do here.
Comment 66 James Robinson 2012-01-30 14:43:07 PST
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+
Comment 67 WebKit Review Bot 2012-01-30 16:29:19 PST
Comment on attachment 124431 [details]
Patch

Clearing flags on attachment: 124431

Committed r106300: <http://trac.webkit.org/changeset/106300>
Comment 68 WebKit Review Bot 2012-01-30 16:29:27 PST
All reviewed patches have been landed.  Closing bug.