Don't invalidate paint on background image change if it's obscured

Don't invalidate paint on background image change if it's obscured

BUG=622368
TEST=fast/repaint/obscured-background-no-repaint.html

Committed: https://crrev.com/3646f9d8a418eabf1e7d8cb29ca446382848e04c
Cr-Commit-Position: refs/heads/master@{#411337}

[Xianzhu, 2016-08-08 23:35:15]

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

[Xianzhu, 2016-08-08 23:35:37]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org

[commit-bot: I haz the power, 2016-08-08 23:35:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 00:34:42]

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

[commit-bot: I haz the power, 2016-08-09 00:34:43]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Stephen Chennney, 2016-08-09 14:22:31]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2016-08-09 14:22:32]

Good idea, more work to do.

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/repaint/obscured-background-no-repaint.html
(right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/repaint/obscured-background-no-repaint.html:54:
setTimeout(finishRepaintTest, 300);
Timeouts should be avoided if at all possible. They lead to test flakiness and
add to trybot time. Is there any way to restructure this to avoid the timeout?

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1529: if (isBackground &&
(isDocumentElement() || backgroundStolenForBeingBody()))
if (isBackground) {
  if (isDocumentElement() || ...
    return false;

  invalidateBackgroundObscurationStatus();
  ...
}

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:392: bool hasAlpha =
m_source.frameHasAlphaAtIndex(index);
Based on trying a similar change I believe this has a very large impact on
performance. Bug crbug.com/593430 should be fixed instead, or run cluster
telemetry to verify performance.

[eae, 2016-08-09 14:41:47]

eae@chromium.org changed reviewers:
+ eae@chromium.org

[eae, 2016-08-09 14:41:48]

LGTM w/nit for core/layout

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1528: bool isBackground =
&layers == &styleRef().backgroundLayers();
NIT: Comparing references like this is a little odd; how about adding a ==
operator to the FillLayers calls?

[chrishtr, 2016-08-09 16:05:17]

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1491: ShapeValue*
shapeOutsideValue = style()->shapeOutside();
I guess you're fixing a bug also that this code didn't use to execute? What was
the effect of this? Add
a test?

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutImage.cpp (left):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutImage.cpp:237: if
(!m_imageResource->cachedImage())
?

[Xianzhu, 2016-08-09 16:26:05]

Just found a problem of this CL: LayoutBox::foregroundIsKnownToBeOpaqueInRect()
depends on compositing state which is not proper to be called from
imageChanged(). We need to set a flag in imageChanged() and check it during
paint invalidation.

[Xianzhu, 2016-08-09 18:51:48]

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/repaint/obscured-background-no-repaint.html
(right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/repaint/obscured-background-no-repaint.html:54:
setTimeout(finishRepaintTest, 300);
On 2016/08/09 14:22:31, Stephen Chennney wrote:
> Timeouts should be avoided if at all possible. They lead to test flakiness and
> add to trybot time. Is there any way to restructure this to avoid the timeout?

Unfortunately there seems no other ways for such tests containing animated gifs.
The test is designed to be stable: even if the test is slow causing the animated
gif not to update frames in time, it will still produce the expected result (no
paint invalidation).

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1491: ShapeValue*
shapeOutsideValue = style()->shapeOutside();
On 2016/08/09 16:05:17, chrishtr wrote:
> I guess you're fixing a bug also that this code didn't use to execute? What
was
> the effect of this? Add
> a test?

There might be a bug if background and shape-outside share the same image that
we would miss needed layout when shape-outside image changes. However this can't
be reproduced because we seem to always use different images for background and
shape-outside, perhaps because shape-outside's image request has different
referrer rule.

However, the change makes the code not depend on the special referrer rule.

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1528: bool isBackground =
&layers == &styleRef().backgroundLayers();
On 2016/08/09 14:41:48, eae wrote:
> NIT: Comparing references like this is a little odd; how about adding a ==
> operator to the FillLayers calls?

We have FillLayer::operator== that compares the values which is not desired
here.

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1529: if (isBackground &&
(isDocumentElement() || backgroundStolenForBeingBody()))
On 2016/08/09 14:22:31, Stephen Chennney wrote:
> if (isBackground) {
>   if (isDocumentElement() || ...
>     return false;
> 
>   invalidateBackgroundObscurationStatus();
>   ...
> }

Done.

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutImage.cpp (left):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutImage.cpp:237: if
(!m_imageResource->cachedImage())
On 2016/08/09 16:05:17, chrishtr wrote:
> ?

This is combined into the condition at line 219.

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:392: bool hasAlpha =
m_source.frameHasAlphaAtIndex(index);
On 2016/08/09 14:22:31, Stephen Chennney wrote:
> Based on trying a similar change I believe this has a very large impact on
> performance. Bug crbug.com/593430 should be fixed instead, or run cluster
> telemetry to verify performance.

Which benchmark do you think the best to reflect the performance of this?

I thought of letting DeferredImageDecoder notify ImageSource when it finishes
decoding a frame, but not sure the proper way to do this. Besides the hasAlpha
flag that is always true for deferred decoded images, the isComplete flag is
also always false because of lacking of the notification.

(Static analysis of performance:

1. The above line is basically equivalent to the following which should be fast
(except for the lock that I'm not sure about its impact):

  // In ImageSource::frameHasAlphaAtIndex():
  if (!m_decoder)
    return true;
  // In DeferredImageDecoder::frameHasAlphaAtIndex():
  if (m_actualDecoder) // false
    ...
  if (!m_frameGenerator->isMultiFrame()) {
    // In ImageFrameGenerator
    MutexLocker lock(m_alphaMutex);
    if (index < m_hasAlpha.size())
      return m_hasAlpha[index];
  }
  return true;

2. In case hasAlpha == false, it will be cached below, and will be used from
cache in the future.)

[Stephen Chennney, 2016-08-09 19:06:04]

OK on the testing issues. See below for notes on perf. At this point I am not
arguing against landing this.

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:392: bool hasAlpha =
m_source.frameHasAlphaAtIndex(index);
On 2016/08/09 18:51:48, Xianzhu wrote:
> On 2016/08/09 14:22:31, Stephen Chennney wrote:
> > Based on trying a similar change I believe this has a very large impact on
> > performance. Bug crbug.com/593430 should be fixed instead, or run cluster
> > telemetry to verify performance.
> 
> Which benchmark do you think the best to reflect the performance of this?

I would try a cluster telemetry run. Here's the one I did:
https://00e9e64bac8558f036879b6a7b17bf91e4e821e538348abc6c-apidata.googleuser...

I think you need an @google account to see it.

Your change is better than mine in that you will stop checking once you discover
that it is opaque, so I would be willing for sure to take the perf hit given the
expected gain from reduced invalidation.
 
> I thought of letting DeferredImageDecoder notify ImageSource when it finishes
> decoding a frame, but not sure the proper way to do this. Besides the hasAlpha
> flag that is always true for deferred decoded images, the isComplete flag is
> also always false because of lacking of the notification.

Right. The issue is that decode now happens on a raster thread, so getting a
message back to the renderer is challenging. It was on my agenda to look into it
but I never got there.

> 
> (Static analysis of performance:
> 
> 1. The above line is basically equivalent to the following which should be
fast
> (except for the lock that I'm not sure about its impact):
> 
>   // In ImageSource::frameHasAlphaAtIndex():
>   if (!m_decoder)
>     return true;
>   // In DeferredImageDecoder::frameHasAlphaAtIndex():
>   if (m_actualDecoder) // false
>     ...
>   if (!m_frameGenerator->isMultiFrame()) {
>     // In ImageFrameGenerator
>     MutexLocker lock(m_alphaMutex);
>     if (index < m_hasAlpha.size())
>       return m_hasAlpha[index];
>   }
>   return true;
> 
> 2. In case hasAlpha == false, it will be cached below, and will be used from
> cache in the future.)

[Xianzhu, 2016-08-09 19:11:05]

On 2016/08/09 19:06:04, Stephen Chennney wrote:
> OK on the testing issues. See below for notes on perf. At this point I am not
> arguing against landing this.
> 
>
https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):
> 
>
https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:392: bool hasAlpha
=
> m_source.frameHasAlphaAtIndex(index);
> On 2016/08/09 18:51:48, Xianzhu wrote:
> > On 2016/08/09 14:22:31, Stephen Chennney wrote:
> > > Based on trying a similar change I believe this has a very large impact on
> > > performance. Bug crbug.com/593430 should be fixed instead, or run cluster
> > > telemetry to verify performance.
> > 
> > Which benchmark do you think the best to reflect the performance of this?
> 
> I would try a cluster telemetry run. Here's the one I did:
>
https://00e9e64bac8558f036879b6a7b17bf91e4e821e538348abc6c-apidata.googleuser...
> 

The ct link seem just for one-time use and can't be shared. What's the task id
of your test?

[Stephen Chennney, 2016-08-09 19:20:04]

On 2016/08/09 19:11:05, Xianzhu wrote:
> On 2016/08/09 19:06:04, Stephen Chennney wrote:
> > OK on the testing issues. See below for notes on perf. At this point I am
not
> > arguing against landing this.
> > 
> >
>
https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:392: bool
hasAlpha
> =
> > m_source.frameHasAlphaAtIndex(index);
> > On 2016/08/09 18:51:48, Xianzhu wrote:
> > > On 2016/08/09 14:22:31, Stephen Chennney wrote:
> > > > Based on trying a similar change I believe this has a very large impact
on
> > > > performance. Bug crbug.com/593430 should be fixed instead, or run
cluster
> > > > telemetry to verify performance.
> > > 
> > > Which benchmark do you think the best to reflect the performance of this?
> > 
> > I would try a cluster telemetry run. Here's the one I did:
> >
>
https://00e9e64bac8558f036879b6a7b17bf91e4e821e538348abc6c-apidata.googleuser...
> > 
> 
> The ct link seem just for one-time use and can't be shared. What's the task id
> of your test?

Sorry. Perf run 598.

[Xianzhu, 2016-08-09 19:41:42]

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

[commit-bot: I haz the power, 2016-08-09 19:42:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 21:30:24]

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

[commit-bot: I haz the power, 2016-08-09 21:30:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2016-08-09 22:07:54]

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutImage.cpp (left):

https://codereview.chromium.org/2229643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutImage.cpp:237: if
(!m_imageResource->cachedImage())
On 2016/08/09 at 18:51:48, Xianzhu wrote:
> On 2016/08/09 16:05:17, chrishtr wrote:
> > ?
> 
> This is combined into the condition at line 219.

Ah ok.

https://codereview.chromium.org/2229643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2229643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1499: if
(!invalidatePaintOfLayerRectsForImage(image, style()->backgroundLayers(), true))
Why is it ok to delete this?

[Xianzhu, 2016-08-09 23:07:14]

https://codereview.chromium.org/2229643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2229643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1499: if
(!invalidatePaintOfLayerRectsForImage(image, style()->backgroundLayers(), true))
On 2016/08/09 22:07:54, chrishtr wrote:
> Why is it ok to delete this?

Line 1499 became line 1487-1494. Line 1500 became line 1496-1507. Expanded
because the two calls had few common lines and the bool parameter control the
behavior of the function looked not good.

The conditions are made independent because one image may be shared between mask
and background. Even if we setShouldDoFullPaintInvalidation for mask image, we
still need to invalidateBackgroundObscurationStatus if the image is used in
background.

[chrishtr, 2016-08-09 23:13:31]

lgtm

[Xianzhu, 2016-08-10 18:09:11]

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

[commit-bot: I haz the power, 2016-08-10 18:09:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 20:22:29]

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

[commit-bot: I haz the power, 2016-08-10 20:22:33]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-08-10 22:30:21]

Ct task results show flaky results.

Run 1136:
record_time_caching_disabled (ms)	0.438%	0.437%
record_time (ms)			0.312%	0.311%
record_time_construction_disabled (ms)	0.119%	0.119%
record_time_sk_null_canvas (ms)		0%	0%
record_time_subsequence_caching_disabled (ms)	-0.024%	-0.024%
record_time_painting_disabled (ms)	-0.074%	-0.074%
rasterize_time (ms)			-0.204%	-0.204%

Run 1140 (seems not trustworthy):
record_time (ms)			4.329%	4.237%
record_time_subsequence_caching_disabled (ms)	2.278%	2.252%
record_time_caching_disabled (ms)	2.165%	2.142%
record_time_painting_disabled (ms)	1.324%	1.315%
record_time_construction_disabled (ms)	1.151%	1.145%
rasterize_time (ms)			0.039%	0.039%
record_time_sk_null_canvas (ms)		0%	0%

Still waiting for result of another run.

[Xianzhu, 2016-08-10 22:30:31]

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

[commit-bot: I haz the power, 2016-08-10 22:32:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-08-10 22:35:22]

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

[commit-bot: I haz the power, 2016-08-10 22:36:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 03:03:18]

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

[commit-bot: I haz the power, 2016-08-11 03:03:20]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-08-11 15:30:37]

The new ct task (1145) shows progressions:

record_time_sk_null_canvas (ms)		0%	0%
rasterize_time (ms)			-0.058%	-0.058%
record_time_painting_disabled (ms)	-0.133%	-0.134%
record_time_subsequence_caching_disabled (ms)	-0.268%	-0.268%
record_time (ms)			-0.280%	-0.281%
record_time_caching_disabled (ms)	-0.699%	-0.702%
record_time_construction_disabled (ms)	-0.718%	-0.720%

The differences between runs seem just flakiness.

[Xianzhu, 2016-08-11 15:31:06]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-11 15:31:07]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2229643002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-11 15:31:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 15:36:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-11 15:38:48]

Description was changed from

==========
Don't invalidate paint on background image change if it's obscured

BUG=622368
TEST=fast/repaint/obscured-background-no-repaint.html
==========

to

==========
Don't invalidate paint on background image change if it's obscured

BUG=622368
TEST=fast/repaint/obscured-background-no-repaint.html

Committed: https://crrev.com/3646f9d8a418eabf1e7d8cb29ca446382848e04c
Cr-Commit-Position: refs/heads/master@{#411337}
==========

[commit-bot: I haz the power, 2016-08-11 15:38:50]

Patchset 7 (id:??) landed as
https://crrev.com/3646f9d8a418eabf1e7d8cb29ca446382848e04c
Cr-Commit-Position: refs/heads/master@{#411337}