Calculate and track display item opaqueness

Calculate and track display item opaqueness

Performance requires that some opaque chunks (such as the background)
are drawn using a layer set to be opaque. This patch implements this
idea by adding a bit to DrawingDisplayItem for whether the item is
known to be opaque. The "knownToBeOpaque" bit is used to mark layers
as explicitly opaque which lets us pass 2x more tests.

BUG=580355
Committed: https://crrev.com/d82d06a85425c08c0e219659f11cf08b6a6ee6a8
Cr-Commit-Position: refs/heads/master@{#371941}

[pdr., 2016-01-26 17:39:27]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, jbroman@chromium.org, trchen@chromium.org

[pdr., 2016-01-26 17:39:31]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 17:40:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1632263002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1632263002/1

[jbroman, 2016-01-26 17:50:14]

This also seems not too hard to unit test.

https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
(right):

https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp:34: if
(!knownToBeOpaqueItems)
I'd think SkRegion is both simple and reasonably efficient here (it has
optimizations for cases like "you're adding something that's strictly contained
in this rectangle".  Then you can set:

chunk.knownToBeOpaque = opaqueRegion.contains(enclosingIntRect(bounds));

We can still leave a TODO to revisit, but it seems to me to be easier to reason
about, and more likely than a second pass to be acceptably efficient.

https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:44: inline bool
operator==(const PaintChunk& a, const PaintChunk& b)
mind updating operator==?

[jbroman, 2016-01-26 17:52:43]

On 2016/01/26 at 17:50:14, jbroman wrote:
> This also seems not too hard to unit test.
> 
>
https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
(right):
> 
>
https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp:34: if
(!knownToBeOpaqueItems)
> I'd think SkRegion is both simple and reasonably efficient here (it has
optimizations for cases like "you're adding something that's strictly contained
in this rectangle".  Then you can set:
> 
> chunk.knownToBeOpaque = opaqueRegion.contains(enclosingIntRect(bounds));
> 
> We can still leave a TODO to revisit, but it seems to me to be easier to
reason about, and more likely than a second pass to be acceptably efficient.
> 
>
https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):
> 
>
https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:44: inline bool
operator==(const PaintChunk& a, const PaintChunk& b)
> mind updating operator==?

Oh, and PrintTo(const PaintChunk&, std::ostream*) as well.

[commit-bot: I haz the power, 2016-01-26 19:19:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-26 19:19:56]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-01-26 23:36:58]

Description was changed from

==========
WIP: Calculate and track display item opaqueness

Performance requires that some opaque chunks (such as the background)
are drawn using a layer set to be opaque. This patch implements this
idea by adding a bit to DrawingDisplayItem for whether the item is
known to be opaque.

This patch is not yet ready to commit.

BUG=580355
==========

to

==========
Calculate and track display item opaqueness

Performance requires that some opaque chunks (such as the background)
are drawn using a layer set to be opaque. This patch implements this
idea by adding a bit to DrawingDisplayItem for whether the item is
known to be opaque. The "knownToBeOpaque" bit is used to mark layers
as explicitly opaque which lets us pass 2x more tests.

BUG=580355
==========

[pdr., 2016-01-26 23:38:09]

PTAL, this is shaping up to be a real patch so I've removed "WIP" and updated
the change description.

https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
(right):

https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp:34: if
(!knownToBeOpaqueItems)
On 2016/01/26 at 17:50:14, jbroman wrote:
> I'd think SkRegion is both simple and reasonably efficient here (it has
optimizations for cases like "you're adding something that's strictly contained
in this rectangle".  Then you can set:
> 
> chunk.knownToBeOpaque = opaqueRegion.contains(enclosingIntRect(bounds));
> 
> We can still leave a TODO to revisit, but it seems to me to be easier to
reason about, and more likely than a second pass to be acceptably efficient.

Heck yeah, done.

https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://codereview.chromium.org/1632263002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:44: inline bool
operator==(const PaintChunk& a, const PaintChunk& b)
On 2016/01/26 at 17:50:14, jbroman wrote:
> mind updating operator==?

Done.

[pdr., 2016-01-26 23:38:12]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 23:41:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1632263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1632263002/20001

[pdr., 2016-01-27 00:41:01]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-27 00:41:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1632263002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1632263002/40001

[jbroman, 2016-01-27 01:47:14]

https://codereview.chromium.org/1632263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/PaintPrinters.cpp (right):

https://codereview.chromium.org/1632263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/PaintPrinters.cpp:52: *os << "),
knownToBeOpaque=" << chunk.knownToBeOpaque;
Please don't put this field outside of the parentheses. This looks weird:

  PaintChunk(begin=0, end=0, props=..., bounds=...), knownToBeOpaque=true

[commit-bot: I haz the power, 2016-01-27 02:29:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-27 02:29:02]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-01-27 05:07:46]

https://codereview.chromium.org/1632263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/PaintPrinters.cpp (right):

https://codereview.chromium.org/1632263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/PaintPrinters.cpp:52: *os << "),
knownToBeOpaque=" << chunk.knownToBeOpaque;
On 2016/01/27 at 01:47:14, jbroman wrote:
> Please don't put this field outside of the parentheses. This looks weird:
> 
>   PaintChunk(begin=0, end=0, props=..., bounds=...), knownToBeOpaque=true

This was not intentional. Fixed.

https://codereview.chromium.org/1632263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h
(right):

https://codereview.chromium.org/1632263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h:64: //
True if there are no transparent areas. Only used for SlimmingPaintV2.
Jbroman, what do you think about this?

It's very gross but we have several free bits in DisplayItem and bloating
DrawingDisplayItem may hurt memory without spv2. Unless there's a better idea, I
think we should move it.

[pdr., 2016-01-27 05:07:50]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-27 05:09:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1632263002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1632263002/60001

[commit-bot: I haz the power, 2016-01-27 06:34:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-27 06:34:54]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-01-27 16:04:27]

https://codereview.chromium.org/1632263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h
(right):

https://codereview.chromium.org/1632263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h:64: //
True if there are no transparent areas. Only used for SlimmingPaintV2.
On 2016/01/27 at 05:07:46, pdr wrote:
> Jbroman, what do you think about this?
> 
> It's very gross but we have several free bits in DisplayItem and bloating
DrawingDisplayItem may hurt memory without spv2. Unless there's a better idea, I
think we should move it.

Ugh, it probably costs us 4-8 bytes per due to pointer alignment, doesn't it?
You could quickly check to see how much memory in total that is in practice
(it's still not massive compared to the usual size of a DrawingDisplayItem +
associated SkPicture + SkRecord). I'm worried it may be premature optimization.

But if you'd really rather have DisplayItem::m_derivedClassDefinedBit1 or
something, we _could_.

[pdr., 2016-01-27 19:41:47]

On 2016/01/27 at 16:04:27, jbroman wrote:
>
https://codereview.chromium.org/1632263002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h
(right):
> 
>
https://codereview.chromium.org/1632263002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h:64: //
True if there are no transparent areas. Only used for SlimmingPaintV2.
> On 2016/01/27 at 05:07:46, pdr wrote:
> > Jbroman, what do you think about this?
> > 
> > It's very gross but we have several free bits in DisplayItem and bloating
DrawingDisplayItem may hurt memory without spv2. Unless there's a better idea, I
think we should move it.
> 
> Ugh, it probably costs us 4-8 bytes per due to pointer alignment, doesn't it?
You could quickly check to see how much memory in total that is in practice
(it's still not massive compared to the usual size of a DrawingDisplayItem +
associated SkPicture + SkRecord). I'm worried it may be premature optimization.
> 
> But if you'd really rather have DisplayItem::m_derivedClassDefinedBit1 or
something, we _could_.

Lets try this and see how it goes. In the latest patchset I've removed the
comment about moving the bool.

PTAL and lg+cq if you like it.

[jbroman, 2016-01-27 19:46:44]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-01-27 19:46:45]

lgtm

[commit-bot: I haz the power, 2016-01-27 19:48:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1632263002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1632263002/80001

[commit-bot: I haz the power, 2016-01-27 21:52:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-27 21:52:43]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2016-01-27 23:35:13]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-01-27 23:37:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1632263002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1632263002/80001

[commit-bot: I haz the power, 2016-01-28 00:40:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-28 00:41:37]

Description was changed from

==========
Calculate and track display item opaqueness

Performance requires that some opaque chunks (such as the background)
are drawn using a layer set to be opaque. This patch implements this
idea by adding a bit to DrawingDisplayItem for whether the item is
known to be opaque. The "knownToBeOpaque" bit is used to mark layers
as explicitly opaque which lets us pass 2x more tests.

BUG=580355
==========

to

==========
Calculate and track display item opaqueness

Performance requires that some opaque chunks (such as the background)
are drawn using a layer set to be opaque. This patch implements this
idea by adding a bit to DrawingDisplayItem for whether the item is
known to be opaque. The "knownToBeOpaque" bit is used to mark layers
as explicitly opaque which lets us pass 2x more tests.

BUG=580355

Committed: https://crrev.com/d82d06a85425c08c0e219659f11cf08b6a6ee6a8
Cr-Commit-Position: refs/heads/master@{#371941}
==========

[commit-bot: I haz the power, 2016-01-28 00:41:38]

Patchset 5 (id:??) landed as
https://crrev.com/d82d06a85425c08c0e219659f11cf08b6a6ee6a8
Cr-Commit-Position: refs/heads/master@{#371941}