Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22889020:
Refactor SkPDFImage (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. |
DescriptionRefactor SkPDFImage
Committed: http://code.google.com/p/skia/source/detail?r=10896
Patch Set 1 #Patch Set 2 : Fixes #Patch Set 3 : Make JPEG mode not crash #
Total comments: 2
Patch Set 4 : Cleanup transparency tests #Patch Set 5 : Simplify comparison in 4-bit case #
Total comments: 26
Patch Set 6 : Merge back image and alpha extraction, other minor fixes #Patch Set 7 : More style changes #Patch Set 8 : Refactor to delay alpha stream generation #Patch Set 9 : A slightly different style #
Total comments: 34
Patch Set 10 : Style improvements from review comments #Patch Set 11 : Further style improvements #
Total comments: 10
Patch Set 12 : More style fixes #
Total comments: 2
Patch Set 13 : Fix some nits #
Messages
Total messages: 17 (0 generated)
This refactors SkPDFImage to: - Combine SkPDFImage and SkPDFImageStream. The artificial separation layer between those two seem to have little purpose, as SkPDFImageStream is not re-used elsewhere. - Avoid extracting image data when SkPDFImage is created - wait until populate, so compression can be attempted first. The previous scheme would extract image data, then compress it and toss out the previously extracted data. This is part 1 in a 3-CL series which will: 1. Refactor the GM to make further changes cleaner 2. Add unpremultiply support (even in JPEG compression mode) 3. Avoid re-encoding the JPEG when a JPEG backing store already exists.
https://codereview.chromium.org/22889020/diff/8001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/8001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:167: if (alphaDst[0] != 0xFF) { alphaDst[0] != 0xFF if (alphaDst[0]) { alphaDst[0] != 0xF0 should be macros with some names that can be read easily
Fixed! https://codereview.chromium.org/22889020/diff/8001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/8001/src/pdf/SkPDFImage.cpp#new... src/pdf/SkPDFImage.cpp:167: if (alphaDst[0] != 0xFF) { On 2013/08/21 18:40:51, edisonn wrote: > alphaDst[0] != 0xFF > if (alphaDst[0]) { > alphaDst[0] != 0xF0 > > should be macros with some names that can be read easily Done. I think the old code was written before things like SK_AlphaOPAQUE and SK_AlphaTRANSPARENT were defined.
https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (left): https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ol... src/pdf/SkPDFImage.cpp:22: SkStream** imageData, SkStream** alphaData) { We talked about the different cases the other day: 1) No alpha 1a) Current jpeg compression/ DCT compression, use passed Bitmap 1b) zlib compression, 2) Alpha in image (need to extra alpha channel) 2a) Put unpremultiplied pixels into a bitmap for DCT compression. 2b) zlib compression, extract color channels and unpremultiply We should be able to use SkBitmap.isOpaque to determine if we're in case 1 or 2. In case 1, we don't need to traverse the image until we find out that DCT compression didn't work out. Can we make an unpremultiplied SkBitmap first, see if the DCT compression works out to decided if we're in case 2a or 2b, and then extract the alpha or alpha and image bytes in one pass? https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:21: namespace { Looks like you can probably remove the anonymous namespace (since everything in it is converted to static) - Skia convention. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:23: #define kNoColorTransform 0 const int kNoColorTransform = 0; https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:88: dst[0] = (SkGetPackedR4444(src[x]) << 4) | UnPreMultiply? https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:133: ((uint8_t*)image->getMemoryBase())[0] = 0; Sk_ColorBLACK? https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:318: if (image->isEmpty()) { isEmpty just checks srcRect. 1) I think that's a change in behavior, and 2) it could just be done here and doesn't need to be a member of SkPDFImage. Actually if you change this to use one of the InitFrom methods, you can return a boolean. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:380: // Nothing to do here - the image params are already copied in SkPDFStream's Remove comment. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:444: if (fEncoder && fEncoder(&dctCompressedWStream, fBitmap, fSrcRect)) { This doesn't handle the case where the image has alpha. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:447: get_uncompressed_size(fBitmap, fSrcRect)) { if get_uncompressed_size is 1, you probably shouldn't even bother calling fEncoder https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:482: // But a new Catalog would want it compressed. ...created, but the new catalog wants it compressed. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.h File src/pdf/SkPDFImage.h (right): https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.h#newc... src/pdf/SkPDFImage.h:74: SkPDFImage(const SkBitmap& bitmap, const SkIRect& srcRect, Maybe there should be one constructor that makes an "empty" image with various InitFromFoo methods. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.h#newc... src/pdf/SkPDFImage.h:78: * for the image data, while the bitmap contains contains parameters. contains contains https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.h#newc... src/pdf/SkPDFImage.h:106: SkStream* getCompressedStream(); nit: getDCTCompressedStream()
Addresses most of the stylistic issues, and adds some more fixes. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (left): https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ol... src/pdf/SkPDFImage.cpp:22: SkStream** imageData, SkStream** alphaData) { On 2013/08/21 22:37:17, vandebo wrote: > We talked about the different cases the other day: > > 1) No alpha > 1a) Current jpeg compression/ DCT compression, use passed Bitmap > 1b) zlib compression, > 2) Alpha in image (need to extra alpha channel) > 2a) Put unpremultiplied pixels into a bitmap for DCT compression. > 2b) zlib compression, extract color channels and unpremultiply > > We should be able to use SkBitmap.isOpaque to determine if we're in case 1 or 2. > > In case 1, we don't need to traverse the image until we find out that DCT > compression didn't work out. Can we make an unpremultiplied SkBitmap first, see > if the DCT compression works out to decided if we're in case 2a or 2b, and then > extract the alpha or alpha and image bytes in one pass? Done. I think the merged one is less elegant, but I do see the benefits in maintainability... For the sake of other reviewers: to avoid extracting the image twice (once in extract_image_data, again in DCT encoding if requested), an attempt at DCT encoding has to happen first. However, we don't know whether compression is requested until populate(), and by then it's too late to create new objects. Therefore, extracting alpha and image data happen at different times - image in populate() and alpha when the SkPDFImage is first created. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:21: namespace { On 2013/08/21 22:37:17, vandebo wrote: > Looks like you can probably remove the anonymous namespace (since everything in > it is converted to static) - Skia convention. Done. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:23: #define kNoColorTransform 0 On 2013/08/21 22:37:17, vandebo wrote: > const int kNoColorTransform = 0; Done. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:88: dst[0] = (SkGetPackedR4444(src[x]) << 4) | On 2013/08/21 22:37:17, vandebo wrote: > UnPreMultiply? I'm going to do unpremul on the bitmap instead of here. A future optimization could do unpremul here so you don't have to create a new bitmap, but that case only happens when DCT encoding is not requested. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:133: ((uint8_t*)image->getMemoryBase())[0] = 0; On 2013/08/21 22:37:17, vandebo wrote: > Sk_ColorBLACK? No can do... SK_ColorBLACK is a 4-byte value containing alpha. This is just one byte. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:318: if (image->isEmpty()) { Good point, it probably does change behavior. What do you think about just skipping the isTransparent optimization? It seems like a very edge case - lots of machinery for little gain. Additionally, if SkBitmap::isOpaque is reliable (I hope it is), we can also remove the alpha stream generation from the main image constructor (since we can use isOpaque() to check if we need an SMask, rather than generate the alpha data and not want to chuck it). This means we can also unify all stream generation in populate, so the code is a bit more elegant. On 2013/08/21 22:37:17, vandebo wrote: > isEmpty just checks srcRect. 1) I think that's a change in behavior, and 2) it > could just be done here and doesn't need to be a member of SkPDFImage. > > Actually if you change this to use one of the InitFrom methods, you can return a > boolean. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:380: // Nothing to do here - the image params are already copied in SkPDFStream's On 2013/08/21 22:37:17, vandebo wrote: > Remove comment. I think the comment about SkPDFStream is helpful - it exposes non-obvious behavior (that the stuff created in initImageParams is copied by SkPDFStream's copy constructor). https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:444: if (fEncoder && fEncoder(&dctCompressedWStream, fBitmap, fSrcRect)) { Yeah, this regressed a quite a few tests... fixed. I've added a fStreamValid flag to indicate whether there's already an alpha stream, so it doesn't attempt to extract the image data and clobber the alpha. It's kind of ugly how image and alpha extraction needs to happen in two different places... On 2013/08/21 22:37:17, vandebo wrote: > This doesn't handle the case where the image has alpha. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:447: get_uncompressed_size(fBitmap, fSrcRect)) { On 2013/08/21 22:37:17, vandebo wrote: > if get_uncompressed_size is 1, you probably shouldn't even bother calling > fEncoder Done. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:482: // But a new Catalog would want it compressed. On 2013/08/21 22:37:17, vandebo wrote: > ...created, but the new catalog wants it compressed. Done. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.h File src/pdf/SkPDFImage.h (right): https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.h#newc... src/pdf/SkPDFImage.h:74: SkPDFImage(const SkBitmap& bitmap, const SkIRect& srcRect, On 2013/08/21 22:37:17, vandebo wrote: > Maybe there should be one constructor that makes an "empty" image with various > InitFromFoo methods. I kind of like having a self-contained constructor, instead of having two parts (and having to remember to do both parts)... I also think the isValid (isEmpty in this case) pattern for checking if construction succeeded is used elsewhere in the PDF code as well. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.h#newc... src/pdf/SkPDFImage.h:78: * for the image data, while the bitmap contains contains parameters. On 2013/08/21 22:37:17, vandebo wrote: > contains contains Done. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.h#newc... src/pdf/SkPDFImage.h:106: SkStream* getCompressedStream(); On 2013/08/21 22:37:17, vandebo wrote: > nit: getDCTCompressedStream() Removed, other style changes rendered this obsolete.
Here's a different style, using SkBitmap::isOpaque to determine whether or not to add a soft mask. I'm not sure how well (or tightly) isOpaque is implemented, but all the GMs seem to be still visually passing. Stylistically, this is much more elegant (as well as integrating well with the upcoming unpremultiply CL). This incurs a slight performance penalty - the bitmap has to be deep-copied, or alpha corruption may occur (this shows up on the GMs). However, it may be a bug that deep-copying wasn't happening before, we were just lucky that the alpha data was eagerly saved to a stream and the color data came out fine. Anyways, this still will require space for both the alpha stream and original bitmap - note that because of unpremul, the original bitmap (for bitmaps with alpha) will not be referenced by the main SkPDFImage.
Ok, this looks prettier. I think.
https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:55: return get_row_bytes(bitmap, srcRect) * srcRect.height(); Here and extract_index8_image are the only place that get_row_bytes is used. It doesn't appear to be a useful abstraction. Inline it here and at the other call site. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:82: bool* hasAlpha, If it doesn't have alpha, or is completely transparent, and we are trying to extract alpha, then it should just return NULL. I don't think you need this args. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:103: if (dst[0] != SK_AlphaOPAQUE) { This can probably made more streamlined.... hasAlpha |= dst[0] != SK_AlphaOPAQUE; isTransparent &= dst[0] == SK_AlphaTRANSPARENT; https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:124: *hasAlpha = true; A better name for hasAlpha is probably the opposite notion - isOpaque https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:277: * Extract image data to a SkStream. Interlaced alpha is separated into alpha Interlaced usually refers to lines/horizontal stripe interlacing... "Interlaced alpha is..." => "Either the alpha component or the color data is extracted into a pdf image stream." or something similar. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:287: * transparent or opaque. If Null is returned when the image is transparent, then you don't need the isTransparent flag. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:295: Probably easier to have an early return for the alpha request with a non-alpha config case... if (extractAlpha && (bitmap.getConfig() == SkBitmap::kIndex8_Config || bitmap.getConfig() == SkBitmap::kRGB_565_Config) { return NULL; } https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:300: stream = extract_index8_image(bitmap, srcRect); Use of functions makes this one easier to follow. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:348: static SkPDFArray* makeIndexedColorSpace(SkColorTable* table) { static functions should have hacker case: make_indexed_color_space https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:421: SkPDFImage::SkPDFImage(SkStream* stream, const SkBitmap& bitmap, This constructor is effectively the same as the previous one. How about a constructor that takes a possibly NULL stream and a bool alpha flag? That would also get rid of initImageParams which is kind of awkward as a method. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:438: // constructor, and the bitmap will be regenerated and re-encoded in "...regenerated and re-encoded..." => encoded https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:519: if (!fStreamValid) { Should this be a debug check? You always expect this to be true, correct? https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:520: bool dummy; dummy -> no_used, but generally I don't think you need this parameter. If you do need, it, the method should all passing in a NULL one.
https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:379: SkAutoTUnref<SkStream> alphaData( nit: if bitmap.isOpaque() is set, then you can skip extracting the alpha.
Fixed! https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:55: return get_row_bytes(bitmap, srcRect) * srcRect.height(); On 2013/08/23 05:25:20, vandebo wrote: > Here and extract_index8_image are the only place that get_row_bytes is used. It > doesn't appear to be a useful abstraction. Inline it here and at the other call > site. Done. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:82: bool* hasAlpha, On 2013/08/23 05:25:20, vandebo wrote: > If it doesn't have alpha, or is completely transparent, and we are trying to > extract alpha, then it should just return NULL. I don't think you need this > args. But they're necessary for the optimizations: you need to differentiate hasAlpha from isTransparent - if its's transparent, you can skip creating the image at all. However, if it just has no alpha, you only skip the SMask. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:103: if (dst[0] != SK_AlphaOPAQUE) { On 2013/08/23 05:25:20, vandebo wrote: > This can probably made more streamlined.... > > hasAlpha |= dst[0] != SK_AlphaOPAQUE; > isTransparent &= dst[0] == SK_AlphaTRANSPARENT; Nice. Will do. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:124: *hasAlpha = true; On 2013/08/23 05:25:20, vandebo wrote: > A better name for hasAlpha is probably the opposite notion - isOpaque Ok. It does become more consistent with SkBitmap too. And it's cleaner with the isTransparent code. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:277: * Extract image data to a SkStream. Interlaced alpha is separated into alpha On 2013/08/23 05:25:20, vandebo wrote: > Interlaced usually refers to lines/horizontal stripe interlacing... > > "Interlaced alpha is..." => "Either the alpha component or the color data is > extracted into a pdf image stream." or something similar. Done. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:287: * transparent or opaque. On 2013/08/23 05:25:20, vandebo wrote: > If Null is returned when the image is transparent, then you don't need the > isTransparent flag. Necessary to differentiate from opaque case (where we don't need the alpha, but can't not create the color case). I think the code is clean enough (or at least not intrusive enough) to keep that optimization. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:295: On 2013/08/23 05:25:20, vandebo wrote: > Probably easier to have an early return for the alpha request with a non-alpha > config case... > > if (extractAlpha && (bitmap.getConfig() == SkBitmap::kIndex8_Config || > bitmap.getConfig() == SkBitmap::kRGB_565_Config) { > return NULL; > } Ehhhhh - the current style uses consistent mechanisms for everything , which I think is nice. There is a bit of overhead incurred by bitmap.lockPixels(), though we eventually do need the pixels when we have to generate the color channel (assuming that SkBitmap does some kind of caching, though). https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:300: stream = extract_index8_image(bitmap, srcRect); On 2013/08/23 05:25:20, vandebo wrote: > Use of functions makes this one easier to follow. Yeah, the old one was getting super bloated... https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:348: static SkPDFArray* makeIndexedColorSpace(SkColorTable* table) { On 2013/08/23 05:25:20, vandebo wrote: > static functions should have hacker case: make_indexed_color_space Missed updating this one... done. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:379: SkAutoTUnref<SkStream> alphaData( On 2013/08/23 05:45:10, vandebo wrote: > nit: if bitmap.isOpaque() is set, then you can skip extracting the alpha. Done, along with cautionary comment. Are there any actual guarantees on isOpaque()? By the interface, it seems quite haphazard - especially since setIsOpaque is a public function... https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:421: SkPDFImage::SkPDFImage(SkStream* stream, const SkBitmap& bitmap, On 2013/08/23 05:25:20, vandebo wrote: > This constructor is effectively the same as the previous one. How about a > constructor that takes a possibly NULL stream and a bool alpha flag? > > That would also get rid of initImageParams which is kind of awkward as a method. Done - it is cleaner in many regards. I've clarified the stream and bitmap behavior in the header comments as well. However, I'm a bit worried about parameter bloat. I also don't think initImageParams was that bad - perhaps it might have been cleaner to pass in a bitmap and srcRect. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:438: // constructor, and the bitmap will be regenerated and re-encoded in On 2013/08/23 05:25:20, vandebo wrote: > "...regenerated and re-encoded..." => encoded Done. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:519: if (!fStreamValid) { On 2013/08/23 05:25:20, vandebo wrote: > Should this be a debug check? You always expect this to be true, correct? No - when the alpha channel is generated, the stream is saved and fStreamValid is set to true. In that case, we don't want to extract the color data. This is more useful when a substitute is created, which relies on going through the populate process again to create a compressed stream. In that case, it will skip creating a stream, and go straight to SkPDFStream::populate, which does flate-encode and adds the length field. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:520: bool dummy; On 2013/08/23 05:25:20, vandebo wrote: > dummy -> no_used, but generally I don't think you need this parameter. If you > do need, it, the method should all passing in a NULL one. Done. Updated code and comments to reflect that this field may be NULL when alpha data is not requested.
https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:82: bool* hasAlpha, On 2013/08/23 06:59:08, ducky wrote: > On 2013/08/23 05:25:20, vandebo wrote: > > If it doesn't have alpha, or is completely transparent, and we are trying to > > extract alpha, then it should just return NULL. I don't think you need this > > args. > > But they're necessary for the optimizations: you need to differentiate hasAlpha > from isTransparent - if its's transparent, you can skip creating the image at > all. However, if it just has no alpha, you only skip the SMask. Ok, but you don't need both of them. I would suggest keeping isOpaque https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:295: On 2013/08/23 06:59:08, ducky wrote: > On 2013/08/23 05:25:20, vandebo wrote: > > Probably easier to have an early return for the alpha request with a non-alpha > > config case... > > > > if (extractAlpha && (bitmap.getConfig() == SkBitmap::kIndex8_Config || > > bitmap.getConfig() == SkBitmap::kRGB_565_Config) { > > return NULL; > > } > > Ehhhhh - the current style uses consistent mechanisms for everything , which I > think is nice. There is a bit of overhead incurred by bitmap.lockPixels(), > though we eventually do need the pixels when we have to generate the color > channel (assuming that SkBitmap does some kind of caching, though). Disagree. Not sure what you mean by consistent mechanisms, I assume you mean the code look symmetrical in some way, but it's not - the 8888 and 4444 case are already different. If you are reading the code and interested in one particular case, an early return makes it easier to understand - if it's one of those cases, then you're done. If it's not, then there's less code to read below. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:520: bool dummy; On 2013/08/23 06:59:08, ducky wrote: > On 2013/08/23 05:25:20, vandebo wrote: > > dummy -> no_used, but generally I don't think you need this parameter. If > you > > do need, it, the method should all passing in a NULL one. > > Done. Updated code and comments to reflect that this field may be NULL when > alpha data is not requested. It should alway be able to be NULL. https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:86: *isOpaque &= dst[0] == SK_AlphaOPAQUE; Has I mentioned, this function should tolerate NULL values for these parameters. An easy way to accomplish that is to compute the value anyway in a local variable and then at the end of the method set the parameter if it's not NULL. https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:195: if (x + 7 < srcRect.fRight) { This is the same condition, put the two bodies together. https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:207: if (srcRect.width() % 8) { Same here https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:494: extract_image_data(fBitmap, fSrcRect, false, NULL)); If we use isAlpha from the constructor here instead of false, we could remove the the warning in the constructor about it not working with alpha. It would be nice to clean up that API wart.
https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.h File src/pdf/SkPDFImage.h (right): https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.h#newc... src/pdf/SkPDFImage.h:96: void initImageParams(bool isAlpha); Remove.
Another style fix patchset. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:82: bool* hasAlpha, On 2013/08/23 15:45:35, vandebo wrote: > On 2013/08/23 06:59:08, ducky wrote: > > On 2013/08/23 05:25:20, vandebo wrote: > > > If it doesn't have alpha, or is completely transparent, and we are trying to > > > extract alpha, then it should just return NULL. I don't think you need this > > > args. > > > > But they're necessary for the optimizations: you need to differentiate > hasAlpha > > from isTransparent - if its's transparent, you can skip creating the image at > > all. However, if it just has no alpha, you only skip the SMask. > > Ok, but you don't need both of them. I would suggest keeping isOpaque Both are here so that the final check if transparent or opaque and free the stream can all be put into extract_image_data, instead of copies in all functions with alpha. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:295: I see - done. On 2013/08/23 15:45:35, vandebo wrote: > On 2013/08/23 06:59:08, ducky wrote: > > On 2013/08/23 05:25:20, vandebo wrote: > > > Probably easier to have an early return for the alpha request with a > non-alpha > > > config case... > > > > > > if (extractAlpha && (bitmap.getConfig() == SkBitmap::kIndex8_Config || > > > bitmap.getConfig() == SkBitmap::kRGB_565_Config) { > > > return NULL; > > > } > > > > Ehhhhh - the current style uses consistent mechanisms for everything , which I > > think is nice. There is a bit of overhead incurred by bitmap.lockPixels(), > > though we eventually do need the pixels when we have to generate the color > > channel (assuming that SkBitmap does some kind of caching, though). > > Disagree. Not sure what you mean by consistent mechanisms, I assume you mean > the code look symmetrical in some way, but it's not - the 8888 and 4444 case are > already different. If you are reading the code and interested in one particular > case, an early return makes it easier to understand - if it's one of those > cases, then you're done. If it's not, then there's less code to read below. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:520: bool dummy; On 2013/08/23 15:45:35, vandebo wrote: > On 2013/08/23 06:59:08, ducky wrote: > > On 2013/08/23 05:25:20, vandebo wrote: > > > dummy -> no_used, but generally I don't think you need this parameter. If > > you > > > do need, it, the method should all passing in a NULL one. > > > > Done. Updated code and comments to reflect that this field may be NULL when > > alpha data is not requested. > > It should alway be able to be NULL. Done. https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:86: *isOpaque &= dst[0] == SK_AlphaOPAQUE; On 2013/08/23 15:45:36, vandebo wrote: > Has I mentioned, this function should tolerate NULL values for these parameters. > An easy way to accomplish that is to compute the value anyway in a local > variable and then at the end of the method set the parameter if it's not NULL. Done. Added at a higher level to avoid duplication. https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:195: if (x + 7 < srcRect.fRight) { On 2013/08/23 15:45:36, vandebo wrote: > This is the same condition, put the two bodies together. D'oh. https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:207: if (srcRect.width() % 8) { On 2013/08/23 15:45:36, vandebo wrote: > Same here Done. https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:494: extract_image_data(fBitmap, fSrcRect, false, NULL)); Good point. Done. On 2013/08/23 15:45:36, vandebo wrote: > If we use isAlpha from the constructor here instead of false, we could remove > the the warning in the constructor about it not working with alpha. It would be > nice to clean up that API wart. https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.h File src/pdf/SkPDFImage.h (right): https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.h#newc... src/pdf/SkPDFImage.h:96: void initImageParams(bool isAlpha); Oops. Not sure how that got left in there... On 2013/08/23 15:50:19, vandebo wrote: > Remove.
LGTM https://codereview.chromium.org/22889020/diff/24012/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/24012/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:263: bool localTransparent = true; nit: localTransparent -> transparent https://codereview.chromium.org/22889020/diff/24012/src/pdf/SkPDFImage.cpp#ne... src/pdf/SkPDFImage.cpp:263: bool localTransparent = true; init to extractAlpha
Fixed. Will commit when trybots go green.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/22889020/57001
Message was sent while issue was closed.
Change committed as 10896 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
