Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1514723003:
Fix bug where custom cursors would dissappear when screen was rotated (Closed)
DescriptionFix bug where custom cursors would disappear when screen was rotated for ozone based platforms. This was caused by commit cd0a8ae72380dc461eea58084325d9d981942192. The change modified the cursor SkBitmap to always be in unpremultiplied alpha format. However, the SkCanvas::drawBitmap() operation fails silently if the SkBitmap is in unpremultiplied format and the ui::ScaleAndRotateCursorBitmapAndHotpoint() function relies on SkCanvas::drawBitmap() to rotate the cursor. This fails and the cursor disappears.
This fix temporarily converts the cursor bitmap back to premultiplied alpha before rotating. It also adds a DCHECK to SkBitmapOperations::Rotate() to document the alpha type requirement.
BUG=533748
Committed: https://crrev.com/fd9f15be2e290d54095d1b3c6b2285d89447e984
Cr-Commit-Position: refs/heads/master@{#366423}
Patch Set 1 #Patch Set 2 : Change to actually convert SkBitmap #Patch Set 3 : Improve comments. #Patch Set 4 : Remove unnecessary CHECK() statement. #
Total comments: 2
Patch Set 5 : Fix typo in comment. #
Total comments: 2
Patch Set 6 : Fix variable name. #
Total comments: 1
Patch Set 7 : Change DCHECK to DCHECK_NE #Messages
Total messages: 24 (11 generated)
The CQ bit was checked by kylechar@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/1514723003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514723003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Fix bug where custom cursors would dissappear when screen was rotated with ozone. This was caused by a change that used unpremultiplied alpha, fix by converting bitmap back to premultiplied alpha. BUG=533748 ========== to ========== Fix bug where custom cursors would disappear when screen was rotated for ozone based platforms. This was caused by commit cd0a8ae72380dc461eea58084325d9d981942192. The change modified the cursor SkBitmap to always be in unpremultiplied alpha format. However, the SkCanvas::drawBitmap() operation fails silently if the SkBitmap is in unpremultiplied format and the ui::ScaleAndRotateCursorBitmapAndHotpoint() function relies on SkCanvas::drawBitmap() to rotate the cursor. This fails and the cursor disappears. This fix temporarily converts the cursor bitmap back to premultiplied alpha before rotating. It also adds a DCHECK to SkBitmapOperations::Rotate() to document the alpha type requirement. BUG=533748 ==========
alexst@chromium.org changed reviewers: + alexst@chromium.org
https://codereview.chromium.org/1514723003/diff/60001/ui/base/cursor/cursor_u... File ui/base/cursor/cursor_util.cc (right): https://codereview.chromium.org/1514723003/diff/60001/ui/base/cursor/cursor_u... ui/base/cursor/cursor_util.cc:46: // SkBitmapOperations::Rotate() needs the the bitmap to have premultiplied double the
kylechar@chromium.org changed reviewers: + reed@google.com
Hi Mike, I was talking with sadrul@ and he mentioned you might know if there are any problems with converting the bitmap alpha type from unpremul -> premul -> unpremul in order to rotate it. https://codereview.chromium.org/1514723003/diff/60001/ui/base/cursor/cursor_u... File ui/base/cursor/cursor_util.cc (right): https://codereview.chromium.org/1514723003/diff/60001/ui/base/cursor/cursor_u... ui/base/cursor/cursor_util.cc:46: // SkBitmapOperations::Rotate() needs the the bitmap to have premultiplied On 2015/12/17 20:56:28, alexst (slow to review) wrote: > double the Done.
lgtm just curious, why must we keep the cursor in unpremul form?
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/1514723003/diff/80001/ui/base/cursor/cursor_u... File ui/base/cursor/cursor_util.cc (right): https://codereview.chromium.org/1514723003/diff/80001/ui/base/cursor/cursor_u... ui/base/cursor/cursor_util.cc:22: bool ConvertSkBitmapAlphaType(SkBitmap* bitmap, SkAlphaType alphaType) { alpha_type (see https://google.github.io/styleguide/cppguide.html#Variable_Names)
On 2015/12/17 22:13:44, reed1 wrote: > lgtm > > just curious, why must we keep the cursor in unpremul form? It looks like the system expects the cursor bitmap in that format. See https://codereview.chromium.org/1251173003 and https://code.google.com/p/chromium/issues/detail?id=432043 for more context.
https://codereview.chromium.org/1514723003/diff/80001/ui/base/cursor/cursor_u... File ui/base/cursor/cursor_util.cc (right): https://codereview.chromium.org/1514723003/diff/80001/ui/base/cursor/cursor_u... ui/base/cursor/cursor_util.cc:22: bool ConvertSkBitmapAlphaType(SkBitmap* bitmap, SkAlphaType alphaType) { On 2015/12/18 18:00:09, sadrul (slow until new year) wrote: > alpha_type (see > https://google.github.io/styleguide/cppguide.html#Variable_Names) Whoops. Spend the last two years writing mostly Java... Fixed.
lgtm Another option would be to do the necessary conversions in SkBitmapOperations::Rotate() instead. But we shouldn't need to do this in too many places, and it's better for the callers to be aware of the cost, so the DCHECK() you have in ::Rotate() is good. Thanks. https://codereview.chromium.org/1514723003/diff/100001/ui/gfx/skbitmap_operat... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/1514723003/diff/100001/ui/gfx/skbitmap_operat... ui/gfx/skbitmap_operations.cc:761: DCHECK(source.info().alphaType() != kUnpremul_SkAlphaType); Sorry, missed this in earlier review, but: use DCHECK_NE() instead.
Patchset #7 (id:120001) has been deleted
On 2015/12/21 at 16:32:12, sadrul wrote: > lgtm > > Another option would be to do the necessary conversions in SkBitmapOperations::Rotate() instead. But we shouldn't need to do this in too many places, and it's better for the callers to be aware of the cost, so the DCHECK() you have in ::Rotate() is good. Thanks. > > https://codereview.chromium.org/1514723003/diff/100001/ui/gfx/skbitmap_operat... > File ui/gfx/skbitmap_operations.cc (right): > > https://codereview.chromium.org/1514723003/diff/100001/ui/gfx/skbitmap_operat... > ui/gfx/skbitmap_operations.cc:761: DCHECK(source.info().alphaType() != kUnpremul_SkAlphaType); > Sorry, missed this in earlier review, but: use DCHECK_NE() instead. Thanks. Changed the DCHECK to DCHECK_NE.
The CQ bit was checked by sadrul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1514723003/#ps140001 (title: "Change DCHECK to DCHECK_NE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514723003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514723003/140001
Message was sent while issue was closed.
Description was changed from ========== Fix bug where custom cursors would disappear when screen was rotated for ozone based platforms. This was caused by commit cd0a8ae72380dc461eea58084325d9d981942192. The change modified the cursor SkBitmap to always be in unpremultiplied alpha format. However, the SkCanvas::drawBitmap() operation fails silently if the SkBitmap is in unpremultiplied format and the ui::ScaleAndRotateCursorBitmapAndHotpoint() function relies on SkCanvas::drawBitmap() to rotate the cursor. This fails and the cursor disappears. This fix temporarily converts the cursor bitmap back to premultiplied alpha before rotating. It also adds a DCHECK to SkBitmapOperations::Rotate() to document the alpha type requirement. BUG=533748 ========== to ========== Fix bug where custom cursors would disappear when screen was rotated for ozone based platforms. This was caused by commit cd0a8ae72380dc461eea58084325d9d981942192. The change modified the cursor SkBitmap to always be in unpremultiplied alpha format. However, the SkCanvas::drawBitmap() operation fails silently if the SkBitmap is in unpremultiplied format and the ui::ScaleAndRotateCursorBitmapAndHotpoint() function relies on SkCanvas::drawBitmap() to rotate the cursor. This fails and the cursor disappears. This fix temporarily converts the cursor bitmap back to premultiplied alpha before rotating. It also adds a DCHECK to SkBitmapOperations::Rotate() to document the alpha type requirement. BUG=533748 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fix bug where custom cursors would disappear when screen was rotated for ozone based platforms. This was caused by commit cd0a8ae72380dc461eea58084325d9d981942192. The change modified the cursor SkBitmap to always be in unpremultiplied alpha format. However, the SkCanvas::drawBitmap() operation fails silently if the SkBitmap is in unpremultiplied format and the ui::ScaleAndRotateCursorBitmapAndHotpoint() function relies on SkCanvas::drawBitmap() to rotate the cursor. This fails and the cursor disappears. This fix temporarily converts the cursor bitmap back to premultiplied alpha before rotating. It also adds a DCHECK to SkBitmapOperations::Rotate() to document the alpha type requirement. BUG=533748 ========== to ========== Fix bug where custom cursors would disappear when screen was rotated for ozone based platforms. This was caused by commit cd0a8ae72380dc461eea58084325d9d981942192. The change modified the cursor SkBitmap to always be in unpremultiplied alpha format. However, the SkCanvas::drawBitmap() operation fails silently if the SkBitmap is in unpremultiplied format and the ui::ScaleAndRotateCursorBitmapAndHotpoint() function relies on SkCanvas::drawBitmap() to rotate the cursor. This fails and the cursor disappears. This fix temporarily converts the cursor bitmap back to premultiplied alpha before rotating. It also adds a DCHECK to SkBitmapOperations::Rotate() to document the alpha type requirement. BUG=533748 Committed: https://crrev.com/fd9f15be2e290d54095d1b3c6b2285d89447e984 Cr-Commit-Position: refs/heads/master@{#366423} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fd9f15be2e290d54095d1b3c6b2285d89447e984 Cr-Commit-Position: refs/heads/master@{#366423} |
