Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22831039:
Add unpremultiply support and GM (Closed)
|
Created:
7 years, 4 months ago by ducky Modified:
7 years, 3 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionAdd unpremultiply support and GM
Committed: http://code.google.com/p/skia/source/detail?r=10901
Patch Set 1 : #Patch Set 2 : Fix 8888 case #
Total comments: 34
Patch Set 3 : Review fixes and rebase from refactor #
Messages
Total messages: 9 (0 generated)
Unpremultiply for PDF export with filtering to remove bitmap sampling artifacts in rendering.
https://codereview.chromium.org/22831039/diff/6002/gm/bitmappremul.cpp File gm/bitmappremul.cpp (right): https://codereview.chromium.org/22831039/diff/6002/gm/bitmappremul.cpp#newcod... gm/bitmappremul.cpp:106: this->setBGColor(0xFFFFFFFF); SK_ColorWhite https://codereview.chromium.org/22831039/diff/6002/gm/bitmappremul.cpp#newcod... gm/bitmappremul.cpp:115: return SkISize::Make(512, 512); SLIDE_SIZE * 2, SLIDE_SIZE * 2 https://codereview.chromium.org/22831039/diff/6002/gm/bitmappremul.cpp#newcod... gm/bitmappremul.cpp:119: SkScalar slideSize = SLIDE_SIZE; SkIntToScalar https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:374: static uint32_t unpremultiply_argb8888(uint32_t src) { nit: src -> pm_color https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:379: SkUnPreMultiply::ApplyScale(scale, SkGetPackedR32(src)), I think it's nicer to hide the scale stuff, so on line 376, SkColor color = PMColorToColor(src); Then here SkColorGetR(color), SkColorGetG(color), SkColorGetB(color)... https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:390: uint8_t alpha = SkGetPackedA4444(src); While it might be a bit wasteful, the simplest thing here is. Sorry if I've gone back and forth on this. Since 4444 is probably going away, the simplest thing is probably best. SkPixel32ToPixel4444(unpremultiply_argb8888(SkPixel4444ToPixel32(src))); https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:399: static void argb8888_pixel_sum(uint32_t pixel, uint8_t* count, Since this is only used in one place, I think it'll be simpler to inline it. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:442: return SkPackARGB32NoCheck(SK_AlphaOPAQUE, 0, 0, 0); return SK_ColorBLACK https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:449: static uint16_t get_argb4444_neighbor_color(const SkBitmap& bitmap, nit: ...neighbor_color_avg https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:488: for (int y = srcRect.fTop; y < srcRect.fBottom; y++) { y++, dstRow++ ? https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:491: for (int x = srcRect.fLeft; x < srcRect.fRight; x++) { x++, dst++ https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:493: if ((a & 0x0F) == SK_AlphaTRANSPARENT) { if (a == (SK_AlphaTRANSPARENT & 0x0F)) https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:493: if ((a & 0x0F) == SK_AlphaTRANSPARENT) { A big comment here about why we need to do this would be good. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:502: } break; Put the break inside the braces. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:552: image = SkNEW_ARGS(SkPDFImage, (unpremulBitmap, srcRect, encoder)); You need to use newSrcRect here.
Fixed. https://codereview.chromium.org/22831039/diff/6002/gm/bitmappremul.cpp File gm/bitmappremul.cpp (right): https://codereview.chromium.org/22831039/diff/6002/gm/bitmappremul.cpp#newcod... gm/bitmappremul.cpp:106: this->setBGColor(0xFFFFFFFF); On 2013/08/23 06:16:11, vandebo wrote: > SK_ColorWhite Done. https://codereview.chromium.org/22831039/diff/6002/gm/bitmappremul.cpp#newcod... gm/bitmappremul.cpp:115: return SkISize::Make(512, 512); On 2013/08/23 06:16:11, vandebo wrote: > SLIDE_SIZE * 2, SLIDE_SIZE * 2 Done. https://codereview.chromium.org/22831039/diff/6002/gm/bitmappremul.cpp#newcod... gm/bitmappremul.cpp:119: SkScalar slideSize = SLIDE_SIZE; On 2013/08/23 06:16:11, vandebo wrote: > SkIntToScalar Done. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:374: static uint32_t unpremultiply_argb8888(uint32_t src) { On 2013/08/23 06:16:11, vandebo wrote: > nit: src -> pm_color Done. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:379: SkUnPreMultiply::ApplyScale(scale, SkGetPackedR32(src)), On 2013/08/23 06:16:11, vandebo wrote: > I think it's nicer to hide the scale stuff, so on line 376, SkColor color = > PMColorToColor(src); > > Then here SkColorGetR(color), SkColorGetG(color), SkColorGetB(color)... Done. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:390: uint8_t alpha = SkGetPackedA4444(src); On 2013/08/23 06:16:11, vandebo wrote: > While it might be a bit wasteful, the simplest thing here is. Sorry if I've > gone back and forth on this. Since 4444 is probably going away, the simplest > thing is probably best. > > SkPixel32ToPixel4444(unpremultiply_argb8888(SkPixel4444ToPixel32(src))); Ehhhhhh - it would make more sense if the code hadn't been written yet, but since it's already there, why not leave it? And is 4444 going away really soon, or is it going to be this half-dead thing for a while? https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:399: static void argb8888_pixel_sum(uint32_t pixel, uint8_t* count, On 2013/08/23 06:16:11, vandebo wrote: > Since this is only used in one place, I think it'll be simpler to inline it. Ok. Though the nested loops doesn't that look nice either. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:442: return SkPackARGB32NoCheck(SK_AlphaOPAQUE, 0, 0, 0); On 2013/08/23 06:16:11, vandebo wrote: > return SK_ColorBLACK SkColor packing doesn't necessarily match ARGB8888. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:449: static uint16_t get_argb4444_neighbor_color(const SkBitmap& bitmap, On 2013/08/23 06:16:11, vandebo wrote: > nit: ...neighbor_color_avg Done. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:488: for (int y = srcRect.fTop; y < srcRect.fBottom; y++) { On 2013/08/23 06:16:11, vandebo wrote: > y++, dstRow++ ? It's kind of nice to keep the loop self-contained and consistent, though. Also easier to understand, since I think most loops are styled to touch only one variable, so the dstRow++ would be less noticeable at a glance. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:491: for (int x = srcRect.fLeft; x < srcRect.fRight; x++) { On 2013/08/23 06:16:11, vandebo wrote: > x++, dst++ Same as above. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:493: if ((a & 0x0F) == SK_AlphaTRANSPARENT) { On 2013/08/23 06:16:11, vandebo wrote: > if (a == (SK_AlphaTRANSPARENT & 0x0F)) Done. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:493: if ((a & 0x0F) == SK_AlphaTRANSPARENT) { On 2013/08/23 06:16:11, vandebo wrote: > A big comment here about why we need to do this would be good. Done. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:502: } break; On 2013/08/23 06:16:11, vandebo wrote: > Put the break inside the braces. Done. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:552: image = SkNEW_ARGS(SkPDFImage, (unpremulBitmap, srcRect, encoder)); On 2013/08/23 06:16:11, vandebo wrote: > You need to use newSrcRect here. Done.
https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:390: uint8_t alpha = SkGetPackedA4444(src); On 2013/08/23 08:06:11, ducky wrote: > On 2013/08/23 06:16:11, vandebo wrote: > > While it might be a bit wasteful, the simplest thing here is. Sorry if I've > > gone back and forth on this. Since 4444 is probably going away, the simplest > > thing is probably best. > > > > SkPixel32ToPixel4444(unpremultiply_argb8888(SkPixel4444ToPixel32(src))); > > Ehhhhhh - it would make more sense if the code hadn't been written yet, but > since it's already there, why not leave it? > And is 4444 going away really soon, or is it going to be this half-dead thing > for a while? Just because it's already written is not a good reason to keep it. My suggestion is simpler to understand with minimal performance overhead. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:493: if ((a & 0x0F) == SK_AlphaTRANSPARENT) { On 2013/08/23 08:06:11, ducky wrote: > On 2013/08/23 06:16:11, vandebo wrote: > > A big comment here about why we need to do this would be good. > > Done. Sorry, I didn't mean a comment about masking the bottle 8-bits. I meant a comment about why we do the neighbor averaging. https://codereview.chromium.org/22831039/diff/20001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22831039/diff/20001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:343: static uint32_t unpremultiply_argb8888(uint32_t pmColor) { I just noticed that this is more than unpremultiply, you also remove the alpha... Maybe call it that - remove_alpha_argb8888 ?
Fixed. I think I also accidentally a patchset... https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:390: uint8_t alpha = SkGetPackedA4444(src); Done. Though for performance overhead, it could also be argued that this is part of a fast inner loop, but unless people are generating ridiculously large PDFs with ridiculously large images, it's probably not noticeable anyways... On 2013/08/23 16:09:17, vandebo wrote: > On 2013/08/23 08:06:11, ducky wrote: > > On 2013/08/23 06:16:11, vandebo wrote: > > > While it might be a bit wasteful, the simplest thing here is. Sorry if I've > > > gone back and forth on this. Since 4444 is probably going away, the > simplest > > > thing is probably best. > > > > > > SkPixel32ToPixel4444(unpremultiply_argb8888(SkPixel4444ToPixel32(src))); > > > > Ehhhhhh - it would make more sense if the code hadn't been written yet, but > > since it's already there, why not leave it? > > And is 4444 going away really soon, or is it going to be this half-dead thing > > for a while? > > Just because it's already written is not a good reason to keep it. My suggestion > is simpler to understand with minimal performance overhead. https://codereview.chromium.org/22831039/diff/6002/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:493: if ((a & 0x0F) == SK_AlphaTRANSPARENT) { On 2013/08/23 16:09:17, vandebo wrote: > On 2013/08/23 08:06:11, ducky wrote: > > On 2013/08/23 06:16:11, vandebo wrote: > > > A big comment here about why we need to do this would be good. > > > > Done. > > Sorry, I didn't mean a comment about masking the bottle 8-bits. I meant a > comment about why we do the neighbor averaging. Done.
LGTM
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/22831039/29001
Message was sent while issue was closed.
Change committed as 10901
Message was sent while issue was closed.
This was reverted in r10909 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
