Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2079413002:
CreateButtonBackground() has been creating unpremultiplied bitmaps. (Closed)
DescriptionCreateButtonBackground() has been creating unpremultiplied bitmaps.
This has been feeding unpremultiplied SkBitmaps into Skia as if they were premultiplied when drawing UI themes. The attached bug shows an example of how this can go wrong.
It looks like this bug is ~7 years old. While reading the code, I noted down a couple questions worth thinking about if we want to tread into this code again.
As it is, this should be enough to be able to revert the earlier bandaid CL I landed (see the bug). It should also let us start asserting that premultiplied pixels are well-formed, to prevent this sort of problem in the future.
BUG=611002
Committed: https://crrev.com/220747df72f0024bb3438e350f2cef377367ef3d
Cr-Commit-Position: refs/heads/master@{#408484}
Patch Set 1 #Patch Set 2 : further note #
Total comments: 7
Patch Set 3 : update comments #Messages
Total messages: 28 (12 generated)
Description was changed from ========== CreateButtonBackground() has been creating unpremultiplied bitmaps. This has been feeding unpremultiplied SkBitmaps into Skia as if they were premultiplied when drawing UI themes. The attached bug shows an example of how this can go wrong. It looks like this bug is ~7 years old. While reading the code, I noted down a couple questions worth thinking about if we want to tread into this code again. As it is, this should be enough to be able to revert the earlier bandaid CL I landed (see the bug). BUG=611002 ========== to ========== CreateButtonBackground() has been creating unpremultiplied bitmaps. This has been feeding unpremultiplied SkBitmaps into Skia as if they were premultiplied when drawing UI themes. The attached bug shows an example of how this can go wrong. It looks like this bug is ~7 years old. While reading the code, I noted down a couple questions worth thinking about if we want to tread into this code again. As it is, this should be enough to be able to revert the earlier bandaid CL I landed (see the bug). It should also let us start asserting that premultiplied pixels are well-formed, to prevent this sort of problem in the future. BUG=611002 ==========
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079413002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mtklein@chromium.org changed reviewers: + reed@google.com
lgtm might augment the TODO comment about "just drawing", that the image would need to be either premul, or correctly marked as unpremul.
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
On 2016/06/20 at 20:11:47, reed wrote: > lgtm > > might augment the TODO comment about "just drawing", that the image would need to be either premul, or correctly marked as unpremul. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079413002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mtklein@chromium.org changed reviewers: + danakj@chromium.org
danakj@chromium.org changed reviewers: + sky@chromium.org
+sky LGTM https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:132: // TODO: can we just do this with a couple SkCanvas draw calls? TODO format is: TODO(chromiumname): text or TODO(crbug.com/LMNOP): text
LGTM with a better comment. https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:145: // We premultiply bg here. This comment just documents the code, and isn't very helpful. A comment that describes why we need to premultiply helps those looking at the code understand why the code is the way it is.
https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:137: // The math producing dst_row[x] below is a correct SrcOver when bg is Is this one too far away?
https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:137: // The math producing dst_row[x] below is a correct SrcOver when bg is On 2016/06/22 15:19:40, mtklein_C wrote: > Is this one too far away? It wasn't obvious to me that the two relate. Comments with TODO in them are easy to ignore.
https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:132: // TODO: can we just do this with a couple SkCanvas draw calls? On 2016/06/22 at 00:51:27, danakj wrote: > TODO format is: > > TODO(chromiumname): text > or > TODO(crbug.com/LMNOP): text Gotcha. Good point... no one will probably ever care to fix these. https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:137: // The math producing dst_row[x] below is a correct SrcOver when bg is On 2016/06/22 at 15:37:43, sky wrote: > On 2016/06/22 15:19:40, mtklein_C wrote: > > Is this one too far away? > > It wasn't obvious to me that the two relate. Comments with TODO in them are easy to ignore. Fixed up, I think. https://codereview.chromium.org/2079413002/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:145: // We premultiply bg here. On 2016/06/22 at 15:17:40, sky wrote: > This comment just documents the code, and isn't very helpful. A comment that describes why we need to premultiply helps those looking at the code understand why the code is the way it is. Gone.
The CQ bit was checked by mtklein@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com, danakj@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2079413002/#ps40001 (title: "update comments")
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 ========== CreateButtonBackground() has been creating unpremultiplied bitmaps. This has been feeding unpremultiplied SkBitmaps into Skia as if they were premultiplied when drawing UI themes. The attached bug shows an example of how this can go wrong. It looks like this bug is ~7 years old. While reading the code, I noted down a couple questions worth thinking about if we want to tread into this code again. As it is, this should be enough to be able to revert the earlier bandaid CL I landed (see the bug). It should also let us start asserting that premultiplied pixels are well-formed, to prevent this sort of problem in the future. BUG=611002 ========== to ========== CreateButtonBackground() has been creating unpremultiplied bitmaps. This has been feeding unpremultiplied SkBitmaps into Skia as if they were premultiplied when drawing UI themes. The attached bug shows an example of how this can go wrong. It looks like this bug is ~7 years old. While reading the code, I noted down a couple questions worth thinking about if we want to tread into this code again. As it is, this should be enough to be able to revert the earlier bandaid CL I landed (see the bug). It should also let us start asserting that premultiplied pixels are well-formed, to prevent this sort of problem in the future. BUG=611002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== CreateButtonBackground() has been creating unpremultiplied bitmaps. This has been feeding unpremultiplied SkBitmaps into Skia as if they were premultiplied when drawing UI themes. The attached bug shows an example of how this can go wrong. It looks like this bug is ~7 years old. While reading the code, I noted down a couple questions worth thinking about if we want to tread into this code again. As it is, this should be enough to be able to revert the earlier bandaid CL I landed (see the bug). It should also let us start asserting that premultiplied pixels are well-formed, to prevent this sort of problem in the future. BUG=611002 ========== to ========== CreateButtonBackground() has been creating unpremultiplied bitmaps. This has been feeding unpremultiplied SkBitmaps into Skia as if they were premultiplied when drawing UI themes. The attached bug shows an example of how this can go wrong. It looks like this bug is ~7 years old. While reading the code, I noted down a couple questions worth thinking about if we want to tread into this code again. As it is, this should be enough to be able to revert the earlier bandaid CL I landed (see the bug). It should also let us start asserting that premultiplied pixels are well-formed, to prevent this sort of problem in the future. BUG=611002 Committed: https://crrev.com/220747df72f0024bb3438e350f2cef377367ef3d Cr-Commit-Position: refs/heads/master@{#408484} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/220747df72f0024bb3438e350f2cef377367ef3d Cr-Commit-Position: refs/heads/master@{#408484} |
