Background breaks box-shadow
Reported by
sergen....@gmail.com,
Oct 27 2016
|
|||||
Issue descriptionUserAgent: 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
,
Nov 1 2016
,
Nov 1 2016
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.
,
Nov 1 2016
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.
,
Nov 1 2016
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.
,
Nov 1 2016
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.
,
Nov 1 2016
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.
,
Nov 1 2016
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.
,
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
,
Nov 4 2016
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by jmukthavaram@chromium.org
, Nov 1 2016Labels: -Type-Bug -Pri-2 M56 hasbisect OS-Linux OS-Mac Pri-1 Type-Bug-Regression
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Unconfirmed)