Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22329003:
Unpremultiply SkBitmaps for PDF output (Closed)
|
Created:
7 years, 4 months ago by ducky Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionUnpremultiply SkBitmaps for PDF output
BUG=skia:236, chromium:175548
Committed: http://code.google.com/p/skia/source/detail?r=10725
Patch Set 1 #Patch Set 2 : Add GM #
Total comments: 17
Patch Set 3 : Rename GM #
Total comments: 2
Patch Set 4 : Optimizations and cleanup #Patch Set 5 : More cleanup #
Total comments: 6
Patch Set 6 : More style fixes #Patch Set 7 : 80 cols #
Total comments: 9
Patch Set 8 : Style fixes; add stripe slide to GM #
Total comments: 6
Patch Set 9 : Style fix #Patch Set 10 : More style fixes #Patch Set 11 : Fix Windows build #
Total comments: 2
Patch Set 12 : fixes #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (left): https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#old... src/pdf/SkPDFImage.cpp:62: hasAlpha = true; Your unpremultiply call could drop in right here. https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#old... src/pdf/SkPDFImage.cpp:76: hasAlpha = true; and here https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#old... src/pdf/SkPDFImage.cpp:117: hasAlpha = true; and here https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:25: static uint32_t unpremultiply_argb8888(uint32_t src) { Instead of needing two unpremultiply routines to deal with the multiple formats, why not have one that deals with the output format: unpremultiple_pixel(uint8_t alpha, uint8_t[3] rgb)
https://codereview.chromium.org/22329003/diff/3001/gm/bitmappremul.cpp File gm/bitmappremul.cpp (right): https://codereview.chromium.org/22329003/diff/3001/gm/bitmappremul.cpp#newcode71 gm/bitmappremul.cpp:71: return SkString("bitmap-premul"); short names tend to use _ and not -
Fixed GM naming, not sure if restructuring unpremultiply according to your suggestion will work. https://codereview.chromium.org/22329003/diff/3001/gm/bitmappremul.cpp File gm/bitmappremul.cpp (right): https://codereview.chromium.org/22329003/diff/3001/gm/bitmappremul.cpp#newcode71 gm/bitmappremul.cpp:71: return SkString("bitmap-premul"); On 2013/08/07 18:11:07, vandebo wrote: > short names tend to use _ and not - Done. https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:25: static uint32_t unpremultiply_argb8888(uint32_t src) { On 2013/08/07 18:02:47, vandebo wrote: > Instead of needing two unpremultiply routines to deal with the multiple formats, > why not have one that deals with the output format: > > unpremultiple_pixel(uint8_t alpha, uint8_t[3] rgb) I'm not sure that will work - argb4444 is also stored in PDF as a 4-bit value. The 4-bit alpha also needs to be scaled up to the 8 bits expected by unpremultiply. Right now, this is all handled by stuff already in SkColorPriv, so I assume it will be consistent with the way premultiply is done. Furthermore, this might turn into 3 functions: unpremul_argb888, unpremul_argb4444_left, and unpremul_argb444_right (depending on whether the rgb starts on the left or right half of the byte). Though it's more complex, it does eliminate the extra packing and unpacking, but only in the argb8888 case (since you still need to extract and scale up the 4-bit values in argb4444 into 8-bit values expected by unpremultiply). What do you think?
Adding Mike I think we have plans to deprecate 4444, as I have seen in the mails circulating on the skia mailing list
https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:25: static uint32_t unpremultiply_argb8888(uint32_t src) { On 2013/08/08 03:35:11, ducky wrote: > On 2013/08/07 18:02:47, vandebo wrote: > > Instead of needing two unpremultiply routines to deal with the multiple > formats, > > why not have one that deals with the output format: > > > > unpremultiple_pixel(uint8_t alpha, uint8_t[3] rgb) > > I'm not sure that will work - argb4444 is also stored in PDF as a 4-bit value. > The 4-bit alpha also needs to be scaled up to the 8 bits expected by > unpremultiply. Right now, this is all handled by stuff already in SkColorPriv, > so I assume it will be consistent with the way premultiply is done. > > Furthermore, this might turn into 3 functions: unpremul_argb888, > unpremul_argb4444_left, and unpremul_argb444_right (depending on whether the rgb > starts on the left or right half of the byte). Though it's more complex, it does > eliminate the extra packing and unpacking, but only in the argb8888 case (since > you still need to extract and scale up the 4-bit values in argb4444 into 8-bit > values expected by unpremultiply). > > What do you think? Sorry I didn't look at it closely enough. You are right. I still don't like repacking the data. How about combining extraction and unpremultiply into one (per format) helper function that is always done. void get_argb_values(uint32_t src, uint8_t* a, uint8_t* r, uint8_t* g, uint8_t* b) With this, the data doesn't need to be copied again. https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:26: uint8_t a = SkGetPackedA32(src); Is this currently just the same as PMColorToColor() ? https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:35: static uint16_t unpremultiply_argb4444(uint16_t src) { The 4444 case is more of a pain. Maybe this should do two 4444 values at once. The edge case where we only have one can still use this function, just passing in 0 for the second pixel. https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:36: uint32_t src32 = SkPixel4444ToPixel32(src); Do you need to convert to 8888 space to unpremultiply? I suspect it will work just as well to use the 4 bit values directly. Can you check? If not, this should probably be implemented in terms of the other method. https://codereview.chromium.org/22329003/diff/10001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/10001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:78: uint16_t pixel0 = src[x], pixel1 = src[x + 1]; two lines.
Optimizations and cleanup https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:26: uint8_t a = SkGetPackedA32(src); On 2013/08/09 16:56:49, vandebo wrote: > Is this currently just the same as PMColorToColor() ? Not exactly. PMColorToColor turns the output into a SkColor, which may be different from the ARGB 8888 format. This one is guaranteed to keep the formatting consistent. https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:35: static uint16_t unpremultiply_argb4444(uint16_t src) { On 2013/08/09 16:56:49, vandebo wrote: > The 4444 case is more of a pain. Maybe this should do two 4444 values at once. > The edge case where we only have one can still use this function, just passing > in 0 for the second pixel. Also may be problematic. On the edge case, it only increments the pointer by 2 bytes, so if we packed 3 bytes (2x 444) into it, it will run off the edge of the array. https://codereview.chromium.org/22329003/diff/10001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/10001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:78: uint16_t pixel0 = src[x], pixel1 = src[x + 1]; On 2013/08/09 16:56:49, vandebo wrote: > two lines. Done.
https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:26: uint8_t a = SkGetPackedA32(src); On 2013/08/09 21:45:14, ducky wrote: > On 2013/08/09 16:56:49, vandebo wrote: > > Is this currently just the same as PMColorToColor() ? > > Not exactly. PMColorToColor turns the output into a SkColor, which may be > different from the ARGB 8888 format. This one is guaranteed to keep the > formatting consistent. SkColor.h says that the order of components in SkColor are fixed. https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:35: static uint16_t unpremultiply_argb4444(uint16_t src) { On 2013/08/09 21:45:14, ducky wrote: > On 2013/08/09 16:56:49, vandebo wrote: > > The 4444 case is more of a pain. Maybe this should do two 4444 values at > once. > > The edge case where we only have one can still use this function, just passing > > in 0 for the second pixel. > > Also may be problematic. On the edge case, it only increments the pointer by 2 > bytes, so if we packed 3 bytes (2x 444) into it, it will run off the edge of the > array. Yes, for the edge case, you wouldn't pass in the final destination, pass in a local variable and copy over the values you need. https://codereview.chromium.org/22329003/diff/16002/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/16002/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:97: uint16_t pixel0 = src[x]; I was imagining the following code - push the conditional down into the function. alphaDst[0] = (SkGetPackedA4444(src[x]) << 4) || SkGetPackedA4444(src[x + 1]); if (alphaDst[0] != 0xFF) { hasAlpha = true; } unpremultiply_and_pack_argb4444(alphaDst[0], src[x], src[x + 1], dst); https://codereview.chromium.org/22329003/diff/16002/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:102: if (alphaDst[0] != 0xFF) { SK_AlphaOPAQUE https://codereview.chromium.org/22329003/diff/16002/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:160: case SkBitmap::kARGB_8888_Config: { I think you can just use PMColorToColor in this case.
More style fixes! https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:26: uint8_t a = SkGetPackedA32(src); On 2013/08/12 15:56:05, vandebo wrote: > On 2013/08/09 21:45:14, ducky wrote: > > On 2013/08/09 16:56:49, vandebo wrote: > > > Is this currently just the same as PMColorToColor() ? > > > > Not exactly. PMColorToColor turns the output into a SkColor, which may be > > different from the ARGB 8888 format. This one is guaranteed to keep the > > formatting consistent. > > SkColor.h says that the order of components in SkColor are fixed. Ok. I think this was back from when I actually needed to keep it in the ARGB888 format, before the repacking was moved in here. Changed to just call PMColorToColor now. https://codereview.chromium.org/22329003/diff/3001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:35: static uint16_t unpremultiply_argb4444(uint16_t src) { On 2013/08/12 15:56:05, vandebo wrote: > On 2013/08/09 21:45:14, ducky wrote: > > On 2013/08/09 16:56:49, vandebo wrote: > > > The 4444 case is more of a pain. Maybe this should do two 4444 values at > > once. > > > The edge case where we only have one can still use this function, just > passing > > > in 0 for the second pixel. > > > > Also may be problematic. On the edge case, it only increments the pointer by 2 > > bytes, so if we packed 3 bytes (2x 444) into it, it will run off the edge of > the > > array. > > Yes, for the edge case, you wouldn't pass in the final destination, pass in a > local variable and copy over the values you need. Done. https://codereview.chromium.org/22329003/diff/16002/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/16002/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:97: uint16_t pixel0 = src[x]; On 2013/08/12 15:56:05, vandebo wrote: > I was imagining the following code - push the conditional down into the > function. > > alphaDst[0] = (SkGetPackedA4444(src[x]) << 4) || > SkGetPackedA4444(src[x + 1]); > if (alphaDst[0] != 0xFF) { > hasAlpha = true; > } > unpremultiply_and_pack_argb4444(alphaDst[0], src[x], src[x + 1], dst); Done. https://codereview.chromium.org/22329003/diff/16002/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:102: if (alphaDst[0] != 0xFF) { On 2013/08/12 15:56:05, vandebo wrote: > SK_AlphaOPAQUE Done. https://codereview.chromium.org/22329003/diff/16002/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:160: case SkBitmap::kARGB_8888_Config: { On 2013/08/12 15:56:05, vandebo wrote: > I think you can just use PMColorToColor in this case. Done and moved up into the unpremul_and_pack, for consistency with the 4444 case.
nits https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:35: static void unpremultiply_and_pack_argb4444(uint8_t alpha0, uint8_t alpha1, Don't pass in alpha, just extract it from source again. https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:37: uint8_t* dst) { uint8_t dst[3] https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:38: if (alpha0 != 0x0F) { SK_AlphaOPAQUE https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:42: dst[0] = dst[0] << 4; Breaking out the shift into a different assignment makes the code harder to understand. Do the shift in the previous line. https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:97: uint16_t pixel0 = src[x]; Inline these four variables.
More style fixes. Also add striped test case to GM. https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:35: static void unpremultiply_and_pack_argb4444(uint8_t alpha0, uint8_t alpha1, On 2013/08/12 22:20:48, vandebo wrote: > Don't pass in alpha, just extract it from source again. Done. https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:37: uint8_t* dst) { On 2013/08/12 22:20:48, vandebo wrote: > uint8_t dst[3] Done. https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:38: if (alpha0 != 0x0F) { On 2013/08/12 22:20:48, vandebo wrote: > SK_AlphaOPAQUE Done. https://codereview.chromium.org/22329003/diff/35001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:42: dst[0] = dst[0] << 4; On 2013/08/12 22:20:48, vandebo wrote: > Breaking out the shift into a different assignment makes the code harder to > understand. Do the shift in the previous line. Fixed.
LGTM after nits https://codereview.chromium.org/22329003/diff/47001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/47001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:44: uint8_t alpha1 = SkGetPackedA4444(src1); nit move alpha1 calculation to line 46 https://codereview.chromium.org/22329003/diff/47001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:117: uint16_t pixel = src[x]; nit: inline pixel and alpha https://codereview.chromium.org/22329003/diff/47001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:119: alphaDst[0] = (alpha << 4); ()'s are not needed
Fix nits and changes to GM to not break the Windows build. https://codereview.chromium.org/22329003/diff/47001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/47001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:44: uint8_t alpha1 = SkGetPackedA4444(src1); On 2013/08/13 16:04:47, vandebo wrote: > nit move alpha1 calculation to line 46 Done. https://codereview.chromium.org/22329003/diff/47001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:117: uint16_t pixel = src[x]; On 2013/08/13 16:04:47, vandebo wrote: > nit: inline pixel and alpha Done. https://codereview.chromium.org/22329003/diff/47001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:119: alphaDst[0] = (alpha << 4); On 2013/08/13 16:04:47, vandebo wrote: > ()'s are not needed Done.
LGTM with nits https://codereview.chromium.org/22329003/diff/62001/gm/bitmappremul.cpp File gm/bitmappremul.cpp (right): https://codereview.chromium.org/22329003/diff/62001/gm/bitmappremul.cpp#newco... gm/bitmappremul.cpp:17: * sets of greyscale gradients. Does this need to be updated? https://codereview.chromium.org/22329003/diff/62001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22329003/diff/62001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:118: // in 12 bytes to the single 4444 value in 2 bytes. nit: 12 bytes -> 3 bytes.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/22329003/71001
Message was sent while issue was closed.
Change committed as 10725 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
