New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 660187 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug
M56



Sign in to add a comment

Background breaks box-shadow

Reported by sergen....@gmail.com, Oct 27 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36

Steps to reproduce the problem:
Open and reload page.

What is the expected behavior?
Shadow is visible.

What went wrong?
Shadow dissapears after page reload.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 54.0.2840.71  Channel: stable
OS Version: 10.0
Flash Version: Shockwave Flash 23.0 r0
 
background_shadow_issue.html
360 bytes View Download
Cc: jmukthavaram@chromium.org
Labels: -Type-Bug -Pri-2 M56 hasbisect OS-Linux OS-Mac Pri-1 Type-Bug-Regression
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce this issue on windows 7, Mac 10.11.4,Linux Ubuntu 14.04 with Chrome stable version-54.0.2840.71 and Canary -56.0.2906.0.

Manual Bisect:

Bad Build—- 54.0.2827.0--Revision(411497)

Good Build—-54.0.2826.0 --Revision (411209)

Bisect Tool Info:

You are probably looking for a change made after 411335 (known good), but no later than 411338 (first known bad).

CHANGELOG URL:

  https://chromium.googlesource.com/chromium/src/+log/36c4ef576660b52e1a5750b313ae3031ea1b4e81..1cd4e33d451c2b1ff88e3aeebe85d7dae4d979b1

Possible suspect:
https://chromium.googlesource.com/chromium/src/+/3646f9d8a418eabf1e7d8cb29ca446382848e04c

wangxianzhu@ Kindly take a look and please help us to reassign this issue to a right owner if not with respect to this change.

Thanks.
Components: Blink>Paint>Invalidation
Components: -Blink>Paint>Invalidation Blink>Paint
Labels: -OS-Linux -OS-Windows -Pri-1 -Type-Bug-Regression -OS-Mac OS-All Pri-2 Type-Bug
Status: Started (was: Assigned)
Though my CL make the case fail, it actually exposed an old bug about box shadow painting. In some cases (when LayoutBoxModelObject::boxShadowShouldBeAppliedToBackground is true) we paint box shadow along with the bottom background layer. However, we skip painting the bottom background layer if it's obscured by upper layers. In the case, the bottom background layer (background-color: green) is obscured by the background-image layer, so we skip painting the bottom background layer and as a side-effect we also skip painting the box shadow.

It worked before my CL because the background image has wrong hasAlpha flag. We always treated the image as transparent so didn't skip painting the bottom background layer.

In trunk the test case works for the first time when the first paint happens before the background-image is decoded.
Cc: pdr@chromium.org fmalita@chromium.org trchen@chromium.org
How important is it to paint box shadow along with the background-color?

Looked at the history and found that the logic was added by https://chromium.googlesource.com/chromium/src/+/a4a27311f35d1acbafd8ed37da433651c46b146e in 2012 for https://bugs.webkit.org/show_bug.cgi?id=78728 because the new logic was faster than painting background color and shadow separately.

Does the optimization apply to Skia? If not I would like to remove the logic and always paint background color and shadow separately to avoid the complex and fragile logic.
The optimization was done by Apple folks so it's hard to know what impact it has on Skia after all these years. I suggest trying to remove the optimization and see what cluster-telemetry has to say.
Will run both cluster-telemetry and a special test. The real pages may not have enough usage of box-shadow to show the performance difference.
I checked with Mike, and there is no performance benefit to painting the box shadow atomically - all the drawlooper does is iterate over its paints and call the same draw primitive repeatedly.
Using a test containing hundreds of <div>s with the following style:
  div {
    margin: 5px;
    display: inline-block;
    background-color: green;
    box-shadow: 0 10px 10px rgba(0,0,0,.45);
    padding: 1em;
  }
, ran rasterize_and_record_micro on trunk and with patch https://codereview.chromium.org/2464053003/ locally, with command line flags --page-repeat=5 --rasterize-repeat=1000 --record-repeat=1000 to reduce noise. The result is attached. The first 2 runs were on trunk, and the other 2 runs were with patch.

Notable results:
- rasterize_time: slightly better with patch, but the difference may be just noise. Would treat it as no change.
- record_time_caching_disabled and record_time_partial_invalidation: about 40% worse with patch. This should be because of bigger number of display items for separate background and shadow.

Based on the result, I'm inclined to remove the optimization for simpler and more manageable code. The test case is an extreme usage of background+shadow. The increased record time for separate background and shadow may not be significant for real world web pages.

Still waiting for results from cluster-telemetry.

results.html
129 KB View Download
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e

commit eac477641ec5fdf9e3b9ec52d46fc3a908e8503e
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Fri Nov 04 00:38:19 2016

Always paint background and shadow separately

The logic to piggyback shadow on background painting is complex and
fragile. For the bug, we skip painting background-color which is
obscured by background-image but we also slip shadow piggybacked on
painting background. The logic was added in WebKit for some graphics
library that can draw combined background and shadow faster.

It's confirmed that Skia doesn't benefit from the logic. Local perf
test (containing hundreds of divs with simple background and shadow)
showed this CL doesn't affect rasterization performance, and degrade
record performance by 0% to 40% because of more drawing operations to
handle. Considering that the test is an extreme example of background+
shadow which is unlikely to exist in real pages, the performance
impact of this CL on real pages will be acceptable.

Telemetry-cluster showed no regression on 10k web pages.

Remove the logic to simply code and fix the bug.

BUG= 660187 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2464053003
Cr-Commit-Position: refs/heads/master@{#429738}

[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/LayoutTests/fast/css/box-shadow-and-border-radius-expected.png
[add] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/LayoutTests/paint/background/background-and-shadow-expected.html
[add] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/LayoutTests/paint/background/background-and-shadow.html
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/LayoutTests/platform/linux/fast/transforms/shadows-expected.png
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/LayoutTests/platform/mac/fast/transforms/shadows-expected.png
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/LayoutTests/platform/win/fast/transforms/shadows-expected.png
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/layout/LayoutImage.cpp
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/layout/LayoutImage.h
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/layout/LayoutTableCell.h
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/layout/api/LineLayoutBoxModel.h
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/paint/BoxPainter.cpp
[modify] https://crrev.com/eac477641ec5fdf9e3b9ec52d46fc3a908e8503e/third_party/WebKit/Source/core/paint/InlineFlowBoxPainter.cpp

Status: Fixed (was: Started)

Sign in to add a comment