Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
317053003:
Fixes for SkAlphaTypes. (Closed)
|
Created:
6 years, 6 months ago by scroggo Modified:
6 years, 6 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionFixes for SkAlphaTypes.
When using allocPixels to set the info and create a new pixelRef in one
shot, create the pixelRef with the corrected info of the bitmap (i.e.
alphatype).
Simplify the check to determine whether to copy the genID in SkBitmap::
copyTo's same configs/same sizes case. The old logic (only checking the
height and assuming the others must be the same) relied on a deep
(mis?)understanding of the rest of the function, and I cannot convince
myself that it has to be true, given that readPixels may have switched
src to a different SkBitmap. Instead, just compare the SkImageInfos,
which is much clearer. (This also caught a bug where a 565 bitmap had
the wrong alphatype, leading to the allocPixels fixes.)
If readPixels succeeds but the source is unpremultiplied, fail out
of copyTo, since readPixels assumes premultiplied.
When copying from 8888 to 4444 or in the fallback case of drawing,
return false if the alphatype is unpremul, which would have failed
anyway.
BUG=skia:2012
Committed: https://skia.googlesource.com/skia/+/0187dc2155830c1ae3390d97463d5dd3971eca41
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rename info parameter to requestedInfo. #Messages
Total messages: 10 (0 generated)
https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:972: if (fPixelRef->info().alphaType() == kUnpremul_SkAlphaType) { Brian, I've cc'ed you mainly to make sure I'm doing the right thing in this case.
https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:972: if (fPixelRef->info().alphaType() == kUnpremul_SkAlphaType) { On 2014/06/04 20:28:27, scroggo wrote: > Brian, > > I've cc'ed you mainly to make sure I'm doing the right thing in this case. I suppose... I can't think of any better way to handle it.
I like the renaming parts. Not sure about the unpremul part. https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:416: bool SkBitmap::allocPixels(const SkImageInfo& info, SkPixelRefFactory* factory, perhaps rename this to origInfo or requestedInfo, to make your point even clearer. https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:447: bool SkBitmap::installPixels(const SkImageInfo& info, void* pixels, size_t rb, SkColorTable* ct, ditto about renaming this too. https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:972: if (fPixelRef->info().alphaType() == kUnpremul_SkAlphaType) { On 2014/06/04 20:39:32, bsalomon wrote: > On 2014/06/04 20:28:27, scroggo wrote: > > Brian, > > > > I've cc'ed you mainly to make sure I'm doing the right thing in this case. > > I suppose... I can't think of any better way to handle it. Is this because pixelref::readPixels is poorly defined, or is it defined that it must return 32bit-premul?
https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:416: bool SkBitmap::allocPixels(const SkImageInfo& info, SkPixelRefFactory* factory, On 2014/06/05 12:38:45, reed1 wrote: > perhaps rename this to origInfo or requestedInfo, to make your point even > clearer. Done. https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:447: bool SkBitmap::installPixels(const SkImageInfo& info, void* pixels, size_t rb, SkColorTable* ct, On 2014/06/05 12:38:45, reed1 wrote: > ditto about renaming this too. Done. https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:972: if (fPixelRef->info().alphaType() == kUnpremul_SkAlphaType) { On 2014/06/05 12:38:45, reed1 wrote: > On 2014/06/04 20:39:32, bsalomon wrote: > > On 2014/06/04 20:28:27, scroggo wrote: > > > Brian, > > > > > > I've cc'ed you mainly to make sure I'm doing the right thing in this case. > > > > I suppose... I can't think of any better way to handle it. > > Is this because pixelref::readPixels is poorly defined, or is it defined that it > must return 32bit-premul? I suppose the former. readPixels was written before we had any notion of unpremul, and does not take it into account. The implementation specifically creates a premultiplied bitmap.
On 2014/06/05 15:33:28, scroggo wrote: > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp > File src/core/SkBitmap.cpp (right): > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > src/core/SkBitmap.cpp:416: bool SkBitmap::allocPixels(const SkImageInfo& info, > SkPixelRefFactory* factory, > On 2014/06/05 12:38:45, reed1 wrote: > > perhaps rename this to origInfo or requestedInfo, to make your point even > > clearer. > > Done. > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > src/core/SkBitmap.cpp:447: bool SkBitmap::installPixels(const SkImageInfo& info, > void* pixels, size_t rb, SkColorTable* ct, > On 2014/06/05 12:38:45, reed1 wrote: > > ditto about renaming this too. > > Done. > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > src/core/SkBitmap.cpp:972: if (fPixelRef->info().alphaType() == > kUnpremul_SkAlphaType) { > On 2014/06/05 12:38:45, reed1 wrote: > > On 2014/06/04 20:39:32, bsalomon wrote: > > > On 2014/06/04 20:28:27, scroggo wrote: > > > > Brian, > > > > > > > > I've cc'ed you mainly to make sure I'm doing the right thing in this case. > > > > > > I suppose... I can't think of any better way to handle it. > > > > Is this because pixelref::readPixels is poorly defined, or is it defined that > it > > must return 32bit-premul? > > I suppose the former. readPixels was written before we had any notion of > unpremul, and does not take it into account. The implementation specifically > creates a premultiplied bitmap. ok. I'm fine with the change then, with appropriate dox/apologies. Lets next investigate who all the callers of readPixels are, and see if we can upgrade this sucker so we don't need to have this post-check.
On 2014/06/05 16:01:33, reed1 wrote: > On 2014/06/05 15:33:28, scroggo wrote: > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp > > File src/core/SkBitmap.cpp (right): > > > > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > > src/core/SkBitmap.cpp:416: bool SkBitmap::allocPixels(const SkImageInfo& info, > > SkPixelRefFactory* factory, > > On 2014/06/05 12:38:45, reed1 wrote: > > > perhaps rename this to origInfo or requestedInfo, to make your point even > > > clearer. > > > > Done. > > > > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > > src/core/SkBitmap.cpp:447: bool SkBitmap::installPixels(const SkImageInfo& > info, > > void* pixels, size_t rb, SkColorTable* ct, > > On 2014/06/05 12:38:45, reed1 wrote: > > > ditto about renaming this too. > > > > Done. > > > > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > > src/core/SkBitmap.cpp:972: if (fPixelRef->info().alphaType() == > > kUnpremul_SkAlphaType) { > > On 2014/06/05 12:38:45, reed1 wrote: > > > On 2014/06/04 20:39:32, bsalomon wrote: > > > > On 2014/06/04 20:28:27, scroggo wrote: > > > > > Brian, > > > > > > > > > > I've cc'ed you mainly to make sure I'm doing the right thing in this > case. > > > > > > > > I suppose... I can't think of any better way to handle it. > > > > > > Is this because pixelref::readPixels is poorly defined, or is it defined > that > > it > > > must return 32bit-premul? > > > > I suppose the former. readPixels was written before we had any notion of > > unpremul, and does not take it into account. The implementation specifically > > creates a premultiplied bitmap. > > ok. I'm fine with the change then, with appropriate dox/apologies. Lets next > investigate who all the callers of readPixels are, and see if we can upgrade > this sucker so we don't need to have this post-check. What's the appropriate dox update? Something like "copyTo will fail to copy a premultiplied pixelref that implements readPixels"? Or call out GrPixelRef specifically? Brian, does GrPixelRef support unpremul? If so, would it be hard to add support to readPixels?
On 2014/06/05 17:14:00, scroggo wrote: > On 2014/06/05 16:01:33, reed1 wrote: > > On 2014/06/05 15:33:28, scroggo wrote: > > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp > > > File src/core/SkBitmap.cpp (right): > > > > > > > > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > > > src/core/SkBitmap.cpp:416: bool SkBitmap::allocPixels(const SkImageInfo& > info, > > > SkPixelRefFactory* factory, > > > On 2014/06/05 12:38:45, reed1 wrote: > > > > perhaps rename this to origInfo or requestedInfo, to make your point even > > > > clearer. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > > > src/core/SkBitmap.cpp:447: bool SkBitmap::installPixels(const SkImageInfo& > > info, > > > void* pixels, size_t rb, SkColorTable* ct, > > > On 2014/06/05 12:38:45, reed1 wrote: > > > > ditto about renaming this too. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/317053003/diff/1/src/core/SkBitmap.cpp#newcod... > > > src/core/SkBitmap.cpp:972: if (fPixelRef->info().alphaType() == > > > kUnpremul_SkAlphaType) { > > > On 2014/06/05 12:38:45, reed1 wrote: > > > > On 2014/06/04 20:39:32, bsalomon wrote: > > > > > On 2014/06/04 20:28:27, scroggo wrote: > > > > > > Brian, > > > > > > > > > > > > I've cc'ed you mainly to make sure I'm doing the right thing in this > > case. > > > > > > > > > > I suppose... I can't think of any better way to handle it. > > > > > > > > Is this because pixelref::readPixels is poorly defined, or is it defined > > that > > > it > > > > must return 32bit-premul? > > > > > > I suppose the former. readPixels was written before we had any notion of > > > unpremul, and does not take it into account. The implementation specifically > > > creates a premultiplied bitmap. > > > > ok. I'm fine with the change then, with appropriate dox/apologies. Lets next > > investigate who all the callers of readPixels are, and see if we can upgrade > > this sucker so we don't need to have this post-check. > > What's the appropriate dox update? Something like "copyTo will fail to copy a > premultiplied pixelref that implements readPixels"? Or call out GrPixelRef > specifically? Brian, does GrPixelRef support unpremul? If so, would it be hard > to add support to readPixels? Sorry, I was referring to (now that I see it) the FIXME you've already added. I think we're covered. lgtm
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/scroggo@google.com/317053003/20001
Message was sent while issue was closed.
Change committed as 0187dc2155830c1ae3390d97463d5dd3971eca41 |
