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

Issue 624175 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue skia:402
issue 684627



Sign in to add a comment

Skia blur to sigma conversion function wrong?

Project Member Reported by est...@chromium.org, Jun 28 2016

Issue description

Skia used to take a blur radius, which is defined thusly in the CSS standard:

"""
…for a long, straight shadow edge, this should create a color transition the length of the blur distance that is perpendicular to and centered on the shadow’s edge, and that ranges from the full shadow color at the radius endpoint inside the shadow to fully transparent at the endpoint outside it.
"""

Then Skia was changed to take sigma, and a conversion function was introduced as shown here[1]. However, using this conversion function creates a shadow with approximately twice the size it should have according to the CSS definition.

This conversion function was copied to ui/gfx/skia_utils.cc[2] where it is causing us to mis-apply blur in gfx::CreateShadowDrawLooper.

This conversion function was also semi-copied to Blink's SkiaUtils.h[3] but the constant was corrected to suit the CSS definition of blur radius.

I don't know if any code is actually using the conversion function that Skia provides but the skia_utils.cc version is causing bugs. Yet without investigating I can't be sure that the code that uses this isn't relying on those bugs. So for now I'm going to introduce a corrected version of gfx::CreateShadowDrawLooper and TODO the investigation/correction process of existing code.

[1] https://cs.chromium.org/chromium/src/third_party/skia/src/effects/SkBlurMask.cpp?rcl=1467126994&l=22
[2] https://cs.chromium.org/chromium/src/ui/gfx/skia_util.cc?rcl=0&l=135
[3] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h?rcl=0&l=114
 

Comment 1 by reed@google.com, Jun 29 2016

Cc: fmalita@chromium.org
Newer versions of the spec add some clarification: https://www.w3.org/TR/css3-background/#shadow-blur

"the resulting shadow must approximate (with each pixel being within 5% of its expected value) the image that would be generated by applying to the shadow a Gaussian blur with a standard deviation equal to half the blur radius"

Sounds like σ ~= R / 2, so Skia's kBLUR_SIGMA_SCALE should be pretty close?

An interesting (if somewhat old) post from dbaron on this subject: http://dbaron.org/log/20110225-blur-radius

Comment 3 by est...@chromium.org, Jun 29 2016

Then maybe Skia is doing something with the sigma value that is different from what the spec writers assumed?

Experimentally, it seems the blink version of the conversion function works the way you'd expect. In Blink, when I create a box-shadow with 10px of blur, the shadow is visible for 10px. For the bit of native code I'm working on, switching to the Blink version of this conversion function creates a similar effect (blur radius 5 ~= 5px visible shadow).

Here's another bit of Skia code (not entirely sure what it's used for) that seems to convert in the other direction[1]:

float blurRadius = 3.f*SkScalarCeilToScalar(brre.getSigma()-1/6.0f);

Unless I'm misunderstanding, this is saying

R / 3 + 1/6 = σ or roughly R / 3 = σ, and that's closer to the blink version of the conversion function.

[1] https://cs.chromium.org/chromium/src/third_party/skia/src/effects/SkBlurMaskFilter.cpp?rcl=1467199289&l=1131
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 29 2016

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

commit bf0e5b5fd9b152e45eb4bd48e061c0d2fdfaffe6
Author: estade <estade@chromium.org>
Date: Wed Jun 29 22:40:17 2016

Adjust sizes of material design shadows based on Sebastien's feedback.

This CL changes the size of the shadows on the call to action buttons
as they're hovered. This better matches Sebastien's original intent
(and he has approved the resulting screenshots).

Several changes occur simultaneously:
 1. The blur radius and offset are changed from 2dp to 1dp.
 2. A bug in gfx::CreateShadowDrawLooper is corrected. See  crbug.com/624175 .
    This bug was somewhat papered over by the (correct) clipping margins that
    gfx::ShadowValue::GetMargin calculated, but it still manifested as a
    shadow that cut off too abruptly. I sussed this out after Sebastien
    spotted the visual defect.
 3. The way the shadow is painted in BorderShadowLayerDelegate is slightly
    tweaked. The half pixel offset doesn't seem necessary any more with the
    above fixes.

BUG= 571500 , 624175 

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

[modify] https://crrev.com/bf0e5b5fd9b152e45eb4bd48e061c0d2fdfaffe6/ui/gfx/skia_util.cc
[modify] https://crrev.com/bf0e5b5fd9b152e45eb4bd48e061c0d2fdfaffe6/ui/gfx/skia_util.h
[modify] https://crrev.com/bf0e5b5fd9b152e45eb4bd48e061c0d2fdfaffe6/ui/views/animation/ink_drop_painted_layer_delegates.cc
[modify] https://crrev.com/bf0e5b5fd9b152e45eb4bd48e061c0d2fdfaffe6/ui/views/controls/button/md_text_button.cc

Blockedon: skia:402
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 23 2016

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

commit f48b1a4cf3a2181780d1326fb5ac53c7ace86551
Author: chrishtr <chrishtr@chromium.org>
Date: Fri Sep 23 03:51:56 2016

Adjust paint invalidation rects to match the actual radius of rastered blur.

Skia actually renders to a larger radius than just blur().

BUG= 648963 , 624175 

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

[modify] https://crrev.com/f48b1a4cf3a2181780d1326fb5ac53c7ace86551/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/f48b1a4cf3a2181780d1326fb5ac53c7ace86551/third_party/WebKit/Source/core/style/ShadowData.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 5 2017

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

commit 36f01b0163a66ba5174589f0e766ff80f28ae748
Author: estade <estade@chromium.org>
Date: Thu Jan 05 01:49:08 2017

Add shadow/corner radius to message center notifications.

Both toasts (standalone bubbles) and notifications inside the message center get new shadows.

BUG= 676223 , 624175 

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

[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ash/common/system/web_notification/ash_popup_alignment_delegate.cc
[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/gfx/BUILD.gn
[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/gfx/image/image_skia_operations.cc
[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/gfx/image/image_skia_operations.h
[add] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/gfx/shadow_util.cc
[add] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/gfx/shadow_util.h
[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/message_center/views/message_view.cc
[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/message_center/views/message_view_factory.cc
[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/message_center/views/toast_contents_view.h
[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/views/border.cc
[modify] https://crrev.com/36f01b0163a66ba5174589f0e766ff80f28ae748/ui/wm/core/shadow.cc

What's left?

1) ShadowBorder still uses the old version. It's only used in the app list now.
2) RenderText uses the old version
2a) used by Label::SetShadows. All but one of the calls to SetShadows specify 0 blur, so there's no difference which CreateShadowDrawLooper we use. (The last is TouchCalibratorView.)
2b) used by Textfield::SetShadows, which itself is unused and may be removed.
2c) used by Canvas::DrawStringRectWithShadows, likewise unused.
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 5 2017

Blockedon: 684627
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 24 2017

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

commit a972d37e1b2a724300e3104f947f8acc5829951b
Author: estade <estade@chromium.org>
Date: Tue Jan 24 19:57:23 2017

Remove another use of old CreateShadowDrawLooper.

gfx::RenderText::set_shadows is only used by Label and in turn
LabelButton. Only two places actually set a non-zero blur value, and
both are updated in this patch.

BUG= 624175 

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

[modify] https://crrev.com/a972d37e1b2a724300e3104f947f8acc5829951b/chrome/browser/ui/views/profiles/new_avatar_button.cc
[modify] https://crrev.com/a972d37e1b2a724300e3104f947f8acc5829951b/ui/gfx/render_text.cc
[modify] https://crrev.com/a972d37e1b2a724300e3104f947f8acc5829951b/ui/views/button_drag_utils.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 9 2017

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

commit 9ef3c96947ed010f07e1134c633b5761050fdf33
Author: estade <estade@chromium.org>
Date: Thu Mar 09 05:16:59 2017

Update last few users of deprecated CreateShadowDrawLooper and remove
it.

As a general rule, doubling the blur values passed in will yield nearly
identical visuals. The main difference is that ShadowValues::GetMargins()
will now return the correct value because of the corrected blur values.
For example, ShadowBorder will now set the correct margins. Previous to
this change, the ShadowBorders in the AppList were clipped because of
the incorrect margins.

BUG= 684627 , 624175 
TBR=sadrul@chromium.org

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

[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ash/common/system/tray/tray_details_view.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ash/magnifier/partial_magnification_controller.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/chrome/browser/ui/views/message_center/message_center_frame_view.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/content/browser/web_contents/aura/gesture_nav_simple.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/app_list/app_list_constants.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/gfx/image/image_skia_operations.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/gfx/render_text.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/gfx/shadow_util.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/gfx/skia_paint_util.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/gfx/skia_paint_util.h
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/views/animation/ink_drop_painted_layer_delegates.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/views/bubble/bubble_border.cc
[modify] https://crrev.com/9ef3c96947ed010f07e1134c633b5761050fdf33/ui/views/controls/button/toggle_button.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Mar 9 2017

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

commit 816c1c01dce669bd39ab3c3f769a9af8ee7a7e05
Author: msw <msw@chromium.org>
Date: Thu Mar 09 23:55:18 2017

Revert of Make download item drags look like bookmark drags. (patchset #1 id:1 of https://codereview.chromium.org/2740433005/ )

Reason for revert:
http://crbug.com/700161

Original issue's description:
> Make download item drags look like bookmark drags.
>
> As discussed in https://codereview.chromium.org/2654853002/
>
> BUG= 624175 
>
> Review-Url: https://codereview.chromium.org/2740433005
> Cr-Commit-Position: refs/heads/master@{#455507}
> Committed: https://chromium.googlesource.com/chromium/src/+/4e4eae4cbe6136b538a3ea8297b07ef365d06337

TBR=sky@chromium.org,estade@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 624175 

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

[modify] https://crrev.com/816c1c01dce669bd39ab3c3f769a9af8ee7a7e05/chrome/browser/download/drag_download_item_views.cc
[modify] https://crrev.com/816c1c01dce669bd39ab3c3f769a9af8ee7a7e05/ui/base/dragdrop/drag_utils.cc
[modify] https://crrev.com/816c1c01dce669bd39ab3c3f769a9af8ee7a7e05/ui/base/dragdrop/drag_utils.h

Status: Fixed (was: Assigned)
(above reverted CL was only tangentially related)
Project Member

Comment 17 by bugdroid1@chromium.org, Mar 27 2017

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

commit 2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24
Author: estade <estade@chromium.org>
Date: Mon Mar 27 18:20:08 2017

Remove gfx::Canvas::DrawStringRectWithHalo.

This was not used anywhere in production. We have a faux halo effect in
a couple places (dragged bookmarks, avatar button) which is accomplished
with stacked shadows. This is something I might try to change, but I
don't believe the current halo code is going to help with that.

The only current uses of DrawStringRectWithHalo are in tests and example
code which can be removed without harm.

BUG= 624175 

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

[modify] https://crrev.com/2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24/ui/compositor/layer_unittest.cc
[modify] https://crrev.com/2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24/ui/gfx/canvas.h
[modify] https://crrev.com/2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24/ui/gfx/canvas_notimplemented.cc
[modify] https://crrev.com/2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24/ui/gfx/canvas_skia.cc
[modify] https://crrev.com/2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24/ui/gfx/render_text.cc
[modify] https://crrev.com/2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24/ui/gfx/render_text.h
[delete] https://crrev.com/1697bb35e2553952e934208ebbca82af75a15c89/ui/gfx/test/data/compositor/string_with_halo.png
[modify] https://crrev.com/2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24/ui/views/examples/text_example.cc
[modify] https://crrev.com/2a2a1f9d36c51c3a4762cd7ddaf9d4a2d142ca24/ui/views/examples/text_example.h

Cc: herb@google.com

Comment 19 by herb@google.com, Aug 10 2017

Cc: -reed@chromium.org

Sign in to add a comment