Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2358803007:
Refactor LabelButtonAssetBorder to avoid SkArithmeticMode (Closed)
DescriptionRefactor LabelButtonAssetBorder to avoid SkArithmeticMode
SkArithmeticMode is being deprecated in Skia.
The current arithmetic transfer function interpolates linearly between foreground/background:
FG * k + BG * (1 - k)
An equivalent effect can be attained using alpha modulation and kPlus blending:
kPlus(FG * alpha, BG * (1 - alpha)) == FG * alpha + BG * (1 - alpha)
R=reed@google.com, msw@chromium.org
Committed: https://crrev.com/abcc4813b5e92a1c25bac7071b77393a5387860f
Cr-Commit-Position: refs/heads/master@{#421887}
Patch Set 1 #Patch Set 2 : fix sktou8 warning #Patch Set 3 : use kPlus xfermode #
Total comments: 4
Patch Set 4 : review #Messages
Total messages: 43 (27 generated)
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think this will yield different results if the source is not opaque. Not sure if that's significant -- will need to figure it out before landing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. We can attain an equivalent src<->dest interpolation effect using saveLayerAlpha(). R=reed@google.com ========== to ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. We can attain a similar src<->dest interpolation effect using saveLayerAlpha(). R=reed@google.com ==========
On 2016/09/23 13:48:44, f(malita) wrote: > I think this will yield different results if the source is not opaque. Not sure > if that's significant -- will need to figure it out before landing. Did a bit of archeology and it looks like the current non-blending interpolation is intentional. SkLerpXfermode was added to support it, and later replaced with SkArithmeticMode. Some details on https://bugs.chromium.org/p/chromium/issues/detail?id=239121. So I think the non-opaque foreground case is significant, and we'll have to use an image filter.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. We can attain a similar src<->dest interpolation effect using saveLayerAlpha(). R=reed@google.com ========== to ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. The current arithmetic transfer function interpolates linearly between foreground/background: FG * k + BG * (1 - k) An equivalent effect can be attained using alpha modulation and kPlus blending: kPlus(FG * alpha, BG * (1 - alpha)) == FG * alpha + BG * (1 - alpha) R=reed@google.com ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/23 16:12:57, f(malita) wrote: > On 2016/09/23 13:48:44, f(malita) wrote: > > I think this will yield different results if the source is not opaque. Not > sure > > if that's significant -- will need to figure it out before landing. > > Did a bit of archeology and it looks like the current non-blending interpolation > is intentional. SkLerpXfermode was added to support it, and later replaced with > SkArithmeticMode. Some details on > https://bugs.chromium.org/p/chromium/issues/detail?id=239121. > > So I think the non-opaque foreground case is significant, and we'll have to use > an image filter. I figured a way to preserve the transfer function using kPlus + alpha modulation. (incidentally I've also discovered someone else has tried this approach before, and the consensus was that it works: https://codereview.chromium.org/22744003/) I *think* this is no worse than the image filter alternative, PTAL.
Description was changed from ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. The current arithmetic transfer function interpolates linearly between foreground/background: FG * k + BG * (1 - k) An equivalent effect can be attained using alpha modulation and kPlus blending: kPlus(FG * alpha, BG * (1 - alpha)) == FG * alpha + BG * (1 - alpha) R=reed@google.com ========== to ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. The current arithmetic transfer function interpolates linearly between foreground/background: FG * k + BG * (1 - k) An equivalent effect can be attained using alpha modulation and kPlus blending: kPlus(FG * alpha, BG * (1 - alpha)) == FG * alpha + BG * (1 - alpha) R=reed@google.com, msw@chromium.org ==========
fmalita@chromium.org changed reviewers: + msw@chromium.org
lgtm
lgtm, thanks!
Since I could only do some limited testing for this change, Mike suggested I should also try writing a Skia fiddle to verify that the kPlus approach does indeed behave the way we expect. I'm glad to report that it does :) https://fiddle.skia.org/c/056ff0c5482d59dc014b9667b2463176 Each column is an interpolation phase of a blue foreground vs. a red background, composited onto a green backdrop (0%, 25%, 50%, 75% & 100%). Each row uses a different blend method: * row 0 is the current SkArithmeticMode-based impl * row 1 is the kPlus approach, same as this CL * row 2 uses alpha modulation for both layers, then blends with srcOver (I think it was used historically at some point) * row 3 uses alpha modulation for foreground only, then blends with srcOver The important result is that row 0 & 1 are identical (while the other approaches diverge). (a version with non-opaque colors: https://fiddle.skia.org/c/9c15e0b41ec8a04d8421fa1005618106) Anyway, just sharing these encouraging results before landing.
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
fmalita@chromium.org changed reviewers: + sky@chromium.org
On 2016/09/29 03:49:54, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) +sky for owner.
https://codereview.chromium.org/2358803007/diff/40001/ui/views/controls/butto... File ui/views/controls/button/label_button_border.cc (right): https://codereview.chromium.org/2358803007/diff/40001/ui/views/controls/butto... ui/views/controls/button/label_button_border.cc:143: SkToU8(SkScalarRoundToInt(0xff * animation->GetCurrentValue())); CurrentValueBetween(0, 255)? https://codereview.chromium.org/2358803007/diff/40001/ui/views/controls/butto... ui/views/controls/button/label_button_border.cc:152: canvas->sk_canvas()->restore(); All these raw save/restore make me nervous as they are easy to get wrong. Are there any objects that do the savelayer in their constructor and restore in the destructor?
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2358803007/diff/40001/ui/views/controls/butto... File ui/views/controls/button/label_button_border.cc (right): https://codereview.chromium.org/2358803007/diff/40001/ui/views/controls/butto... ui/views/controls/button/label_button_border.cc:143: SkToU8(SkScalarRoundToInt(0xff * animation->GetCurrentValue())); On 2016/09/29 16:05:17, sky wrote: > CurrentValueBetween(0, 255)? Done. https://codereview.chromium.org/2358803007/diff/40001/ui/views/controls/butto... ui/views/controls/button/label_button_border.cc:152: canvas->sk_canvas()->restore(); On 2016/09/29 16:05:17, sky wrote: > All these raw save/restore make me nervous as they are easy to get wrong. Are > there any objects that do the savelayer in their constructor and restore in the > destructor? We have SkAutoCanvasRestore, which restores to the initial save count in dtor, and optionally performs a save() in ctor. It doesn't know about saveLayer()/saveLayerAlpha(), but we could still use it for restoring automatically when exiting the scope. Let me know if patch set #4 looks clearer to you, or we should stick to explicit restore.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/2358803007/#ps60001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. The current arithmetic transfer function interpolates linearly between foreground/background: FG * k + BG * (1 - k) An equivalent effect can be attained using alpha modulation and kPlus blending: kPlus(FG * alpha, BG * (1 - alpha)) == FG * alpha + BG * (1 - alpha) R=reed@google.com, msw@chromium.org ========== to ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. The current arithmetic transfer function interpolates linearly between foreground/background: FG * k + BG * (1 - k) An equivalent effect can be attained using alpha modulation and kPlus blending: kPlus(FG * alpha, BG * (1 - alpha)) == FG * alpha + BG * (1 - alpha) R=reed@google.com, msw@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. The current arithmetic transfer function interpolates linearly between foreground/background: FG * k + BG * (1 - k) An equivalent effect can be attained using alpha modulation and kPlus blending: kPlus(FG * alpha, BG * (1 - alpha)) == FG * alpha + BG * (1 - alpha) R=reed@google.com, msw@chromium.org ========== to ========== Refactor LabelButtonAssetBorder to avoid SkArithmeticMode SkArithmeticMode is being deprecated in Skia. The current arithmetic transfer function interpolates linearly between foreground/background: FG * k + BG * (1 - k) An equivalent effect can be attained using alpha modulation and kPlus blending: kPlus(FG * alpha, BG * (1 - alpha)) == FG * alpha + BG * (1 - alpha) R=reed@google.com, msw@chromium.org Committed: https://crrev.com/abcc4813b5e92a1c25bac7071b77393a5387860f Cr-Commit-Position: refs/heads/master@{#421887} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/abcc4813b5e92a1c25bac7071b77393a5387860f Cr-Commit-Position: refs/heads/master@{#421887} |
