Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1212373006:
Swizzling, except for premultiplying, done using libpng transforms (Closed)
DescriptionSwizzling, except for premultiplying, done using libpng transforms
As libpng transforms allow you to choose the format of the data,
these transforms are replacing the swizzling, except for premultiplying
The swizzler is still needed to premultiply as libpng does not have
that ability
BUG=skia:
Patch Set 1 #
Total comments: 54
Patch Set 2 : fixing size of row allocation, member variable initialization, and synax issues #
Total comments: 8
Patch Set 3 : Removing changes from SkMallocPixelRef #Patch Set 4 : Removing changes from SkMallocPixelRef #
Messages
Total messages: 10 (2 generated)
emmaleer@google.com changed reviewers: + msarett@google.com, scroggo@google.com
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:410: // Allow output to RGBA from any type of input not sure either of these comments are needed (also confusing since the component order is reversed from the enum)
What is the effect of this? - faster, slower? - different quality?
Please add BUG=skia:3954 to the description. > The swizzler is still needed to premultiply as libpng does not have > that ability nit: IIUC, libpng is able to premultiply, but only if you handle gamma? On 2015/06/30 13:53:17, reed1 wrote: > What is the effect of this? > - faster, slower? We originally thought we could avoid the swizzler altogether (assuming we could let libpng handle premultiplication), which would mean we could skip a read and write on our end (although we should also look to see how libpng does its transforms - maybe it just means an extra copy on their end?). (In fact, if the caller requests unpremul, I believe we can skip the swizzler, since it is now just needed for premultiplication.) That said, we do need to run the benchmarks to see the difference. > - different quality? Also a good question. My guess is that it will be similar quality, but we can verify with DM. (It just occurred to me that it would be great if we could run a trybot to view the difference before submitting a CL. I filed skbug.com/3994 to track that feature.) FWIW, in addition to letting libpng do more of our transformations, it also fixes some bugs around alternative configs (e.g. Gray) and rewinding (and requesting a different config). (We should also add tests, if these bugs are not covered by existing tests.) So even if it is slower to let libpng do the swizzling, part of this CL should land. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:217: png_infop* info_ptrp, SkImageInfo* imageInfo, int bitDepth) { I believe you want this to be an int* rather than an int. (You could make it an int&, but by convention we use a pointer if we are going to modify an input parameter.) https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:290: if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) { In other places, we also check to see whether the number of transparent colors in the tRNS chunk is greater than zero. Should we make that check here? (Or is it unnecessary where we already do the check?) https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:299: skColorType = kRGBA_8888_SkColorType; Interesting. I assume you chose RGBA because that is the order PNG has it stored. I *think* we settled on reporting the info that best represents the data (which is what you've done), but it leaves an open question in my mind - if a client uses the info reported by the SkCodec, it may not be a good choice. For example, we report that PNGs are unpremul, but we currently do not support drawing unpremul sources. This means that in DM, we need to check for a reported unpremul and switch to premul. (This also means various clients of SkImageGenerator that use the info blindly do not work properly.) Mike, do we support drawing the version of 8888 that is not N32? If not, we will need to convert in this case as well. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:303: //convert to RGBA if there is transparency information in the tRNS chunk Maybe this should be a FIXME; if we supported gray with alpha, we wouldn't need to convert to RGBA. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:314: //convert to RGBA as GreyAlpha is not a Skia color type Maybe add a similar FIXME here. (Also nit: I don't feel strongly about "Gray" vs "Grey", but libpng and Skia seem to have chosen "Gray", so this should say "Gray", too.) https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:315: png_set_gray_to_rgb(png_ptr); Do we need to also call png_set_tRNS_to_alpha here? https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:320: //colorType = PNG_COLOR_TYPE_RGBA Is this the only colorType left? (If so, I think it would be better to add another case, and then assert false for the default.) https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:347: if (read_header(stream, &png_ptr, &info_ptr, &imageInfo, bitDepth)) { Since the function takes an int, this does not modify the local variable. You will be passing an uninitialized value to the SkPngCodec constructor. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:437: int srcColorType = this->getInfo().colorType(); Why do you store these as ints instead of SkColorType? (Also, I think they can be const) https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:439: switch (dstColorType) { nit: this should line up with the line before it. Also, I think you want to switch on the srcColorType? https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:478: // FIXME: Here is where we should likely insert some of the modifications I think this FIXME has been resolved? https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:486: SkCodecPrintf("setjmp long jump!\n"); I know this is the same message that is printed elsewhere (blame me...), but this should perhaps have a more informative error message (e.g. the function where it was called). https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:493: int srcColorType = this->getInfo().colorType(); Again, I think these should be SkColorType instead of int (and can be const). https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:496: //dstColorType either equals kRGBA_8888_SkColorType or kBGRA_8888_SkColorType This seems to just add nothing new to the line above? https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:497: if (kIndex_8_SkColorType == srcColorType) { nit: This should only be indented 4 spaces from the if statement. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:523: if (kSuccess != setDataFormat(requestedInfo)) { nit: this->setDataFormat https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:524: return false; Interestingly, we've changed the nature of the error. setDataFormat will fail with kInvalidInput if we hit a longjmp, but we'll ultimately return kCouldNotRewind. That's probably okay though. (If we do think it's important, we could make handleRewind return a Result, and if it's kSuccess the caller can continue, otherwise return the Result returned by handleRewind.) https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:542: if (kSuccess != setDataFormat(requestedInfo)) { this->setDataFormat https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:558: if (!this->handleRewind(requestedInfo)) { I notice that setDataFormat never cares if it sees a color type that we do not support (e.g. 4444, which will never support). We'll fail down below in conversion_possible though. Not sure whether the order matters. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:574: It looks like you added some whitespace? https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:640: , fSrcRowBytes(dstInfo.minRowBytes()) Why do we need to store this? It seems like we only use it in the constructor? https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:642: fStorage.reset(dstInfo.width() * fSrcRowBytes); I don't think this is what you want. minRowBytes() is width * bytesPerPixel. So now this is width * width * bytesPerPixel. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:710: SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override { This should be indented 4 spaces. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:715: if(false == fCodec->handleRewind(fDstInfo)) { Alternatively, you could use this->dstInfo(), which might be better. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:757: SkImageGenerator::Result onSkipScanlines(int count) override { More added whitespace? https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:799: // Note: We set dst to NULL since we do not know it yet. rowBytes is not needed, nit: 4 space indent.
Mike, the idea is to utilize libpng's transforms to do the swizzling, rather than swizzling after decoding. We might as well not do the work if libpng can do it for us, unless our method of swizzling is faster than libpng's. I am about to do a performance test to see which method is faster, and then we will decide if using libpng for swizzling is better. Quality should be the same for both methods, but still needs to be tested.
https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:217: png_infop* info_ptrp, SkImageInfo* imageInfo, int bitDepth) { On 2015/06/30 15:38:37, scroggo wrote: > I believe you want this to be an int* rather than an int. (You could make it an > int&, but by convention we use a pointer if we are going to modify an input > parameter.) Done. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:290: if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) { On 2015/06/30 15:38:38, scroggo wrote: > In other places, we also check to see whether the number of transparent colors > in the tRNS chunk is greater than zero. Should we make that check here? (Or is > it unnecessary where we already do the check?) Yes, I think it's important to make that check. I've added it in. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:299: skColorType = kRGBA_8888_SkColorType; On 2015/06/30 15:38:38, scroggo wrote: > Interesting. I assume you chose RGBA because that is the order PNG has it > stored. I *think* we settled on reporting the info that best represents the data > (which is what you've done), but it leaves an open question in my mind - if a > client uses the info reported by the SkCodec, it may not be a good choice. For > example, we report that PNGs are unpremul, but we currently do not support > drawing unpremul sources. This means that in DM, we need to check for a reported > unpremul and switch to premul. (This also means various clients of > SkImageGenerator that use the info blindly do not work properly.) > > Mike, do we support drawing the version of 8888 that is not N32? If not, we will > need to convert in this case as well. Changed to N32 https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:303: //convert to RGBA if there is transparency information in the tRNS chunk On 2015/06/30 15:38:37, scroggo wrote: > Maybe this should be a FIXME; if we supported gray with alpha, we wouldn't need > to convert to RGBA. Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:314: //convert to RGBA as GreyAlpha is not a Skia color type On 2015/06/30 15:38:37, scroggo wrote: > Maybe add a similar FIXME here. > > (Also nit: I don't feel strongly about "Gray" vs "Grey", but libpng and Skia > seem to have chosen "Gray", so this should say "Gray", too.) Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:315: png_set_gray_to_rgb(png_ptr); On 2015/06/30 15:38:38, scroggo wrote: > Do we need to also call png_set_tRNS_to_alpha here? No, as PNG_COLOR_TYPE_GRAY_ALPHA has an alpha channel the tRNS chunk is used for images without an alpha channel, and gives an alpha value for all the pixels in the image So, the tRNS chunk is only converted to the alpha channel for images without an alpha channel (gray, index, RGB) https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:320: //colorType = PNG_COLOR_TYPE_RGBA On 2015/06/30 15:38:37, scroggo wrote: > Is this the only colorType left? (If so, I think it would be better to add > another case, and then assert false for the default.) Yes, this is the last color type. Okay, I have done that. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:347: if (read_header(stream, &png_ptr, &info_ptr, &imageInfo, bitDepth)) { On 2015/06/30 15:38:38, scroggo wrote: > Since the function takes an int, this does not modify the local variable. You > will be passing an uninitialized value to the SkPngCodec constructor. Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:410: // Allow output to RGBA from any type of input On 2015/06/30 13:51:01, reed1 wrote: > not sure either of these comments are needed (also confusing since the component > order is reversed from the enum) I removed the unneeded comments In the enum in SkImageInfo.h the order is: kUnknown_SkColorType, kAlpha_8_SkColorType, kRGB_565_SkColorType, kARGB_4444_SkColorType, kRGBA_8888_SkColorType, kBGRA_8888_SkColorType, kIndex_8_SkColorType, kGray_8_SkColorType, So, I think RGBA and then BGRA is okay. Let me know if this is still the wrong ordering. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:437: int srcColorType = this->getInfo().colorType(); On 2015/06/30 15:38:37, scroggo wrote: > Why do you store these as ints instead of SkColorType? > > (Also, I think they can be const) Changed to const SkColorType https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:439: switch (dstColorType) { On 2015/06/30 15:38:37, scroggo wrote: > nit: this should line up with the line before it. > > Also, I think you want to switch on the srcColorType? I switch on dstColorType, since libpng will convert the srcColorType to the dstColorType (except for premultiplying) Then the swizzler gets the data in the dstColorType format This may be confusing for the reader. I've added a comment about this. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:478: // FIXME: Here is where we should likely insert some of the modifications On 2015/06/30 15:38:37, scroggo wrote: > I think this FIXME has been resolved? Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:486: SkCodecPrintf("setjmp long jump!\n"); On 2015/06/30 15:38:37, scroggo wrote: > I know this is the same message that is printed elsewhere (blame me...), but > this should perhaps have a more informative error message (e.g. the function > where it was called). Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:493: int srcColorType = this->getInfo().colorType(); On 2015/06/30 15:38:37, scroggo wrote: > Again, I think these should be SkColorType instead of int (and can be const). Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:496: //dstColorType either equals kRGBA_8888_SkColorType or kBGRA_8888_SkColorType On 2015/06/30 15:38:37, scroggo wrote: > This seems to just add nothing new to the line above? Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:497: if (kIndex_8_SkColorType == srcColorType) { On 2015/06/30 15:38:37, scroggo wrote: > nit: This should only be indented 4 spaces from the if statement. Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:523: if (kSuccess != setDataFormat(requestedInfo)) { On 2015/06/30 15:38:38, scroggo wrote: > nit: this->setDataFormat Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:524: return false; On 2015/06/30 15:38:37, scroggo wrote: > Interestingly, we've changed the nature of the error. setDataFormat will fail > with kInvalidInput if we hit a longjmp, but we'll ultimately return > kCouldNotRewind. That's probably okay though. > > (If we do think it's important, we could make handleRewind return a Result, and > if it's kSuccess the caller can continue, otherwise return the Result returned > by handleRewind.) I think it's important to return the correct result, otherwise it is misleading to the user I think I will change handleRewind to return a Result, but I'll do it in a separate CL, since this one is already quite large https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:542: if (kSuccess != setDataFormat(requestedInfo)) { On 2015/06/30 15:38:37, scroggo wrote: > this->setDataFormat Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:558: if (!this->handleRewind(requestedInfo)) { On 2015/06/30 15:38:38, scroggo wrote: > I notice that setDataFormat never cares if it sees a color type that we do not > support (e.g. 4444, which will never support). We'll fail down below in > conversion_possible though. Not sure whether the order matters. I changed it so that conversion_possible is called first, then the scaling is checked, and then handleRewind is called The order doesn't matter for the implementation I think it's better calling handleRewind last, as it avoids rewinding the entire stream on the cases we will fail one step later anyways https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:574: On 2015/06/30 15:38:38, scroggo wrote: > It looks like you added some whitespace? Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:640: , fSrcRowBytes(dstInfo.minRowBytes()) On 2015/06/30 15:38:37, scroggo wrote: > Why do we need to store this? It seems like we only use it in the constructor? I removed this https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:642: fStorage.reset(dstInfo.width() * fSrcRowBytes); On 2015/06/30 15:38:37, scroggo wrote: > I don't think this is what you want. minRowBytes() is width * bytesPerPixel. So > now this is width * width * bytesPerPixel. Yes, this is wrong. I changed this back to using width * SkSwizzler::BytesPerPixel, since we are still using the swizzler https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:710: SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override { On 2015/06/30 15:38:38, scroggo wrote: > This should be indented 4 spaces. Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:715: if(false == fCodec->handleRewind(fDstInfo)) { On 2015/06/30 15:38:38, scroggo wrote: > Alternatively, you could use this->dstInfo(), which might be better. Acknowledged. https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:799: // Note: We set dst to NULL since we do not know it yet. rowBytes is not needed, On 2015/06/30 15:38:37, scroggo wrote: > nit: 4 space indent. Acknowledged.
https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1212373006/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:410: // Allow output to RGBA from any type of input On 2015/06/30 20:13:40, emmaleer wrote: > On 2015/06/30 13:51:01, reed1 wrote: > > not sure either of these comments are needed (also confusing since the > component > > order is reversed from the enum) > > I removed the unneeded comments > In the enum in SkImageInfo.h the order is: > > kUnknown_SkColorType, > kAlpha_8_SkColorType, > kRGB_565_SkColorType, > kARGB_4444_SkColorType, > kRGBA_8888_SkColorType, > kBGRA_8888_SkColorType, > kIndex_8_SkColorType, > kGray_8_SkColorType, > > So, I think RGBA and then BGRA is okay. > Let me know if this is still the wrong ordering. I think Mike's comment about ordering is actually for the next comment - you mention "GBRA", when you mean "BGRA". https://codereview.chromium.org/1212373006/diff/20001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1212373006/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:251: int bitDepth, colorType; No need to create this variable (bitDepth). Instead, you can pass bitDepthPtr directly to png_get_IHDR. (Come to think of it though, I think you pass NULL for bitDepthPtr in one of the calls to this method. In that case, this might be helpful, if png_get_IHDR does not handle NULL gracefully.) https://codereview.chromium.org/1212373006/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:255: bitDepthPtr = &bitDepth; This is going to do the wrong thing. I believe you want to assign the value of bitDepth to the variable pointed to by bitDepthPtr. In order to do that you would do the following: if (bitDepthPtr) { // don't forget to check for NULL *bitDepthPtr = bitDepth; } What you've done here is made bitDepthPtr point to the memory address of bitDepth. The variable it used to point to remains unchanged. Let me know if this does not make sense and we can talk about it more in person. https://codereview.chromium.org/1212373006/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:443: //set swizzler fSrcConfig to dstColorType, as libbpng will convert from nit: Maybe specify that libpng will do the conversion thanks to the transforms we have specified? https://codereview.chromium.org/1212373006/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:444: //srcColorType to dstColorType. Swizzler still needed for premultiplying The swizzler is only needed in one case: the client requested kPremul. If the image is opaque (and known to be) or if the client requested unpremul, we should skip the swizzler entirely. https://codereview.chromium.org/1212373006/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:445: switch (dstColorType) { nit: This (and the comments above) should line up with the code above it (4 spaces, rather than 8) https://codereview.chromium.org/1212373006/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:464: //dstColorType == kBGRA_8888_SkColorType Again, I think it would be better to make this a case, and then handle the rest in default. (It seems like we have not covered all the cases though. Maybe the default should be SkASSERT(false) with a comment that we would have exited before now if the colortype is one of the remaining cases.) https://codereview.chromium.org/1212373006/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:501: if (png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS)) { Should this call has_transparency_in_palette, too? https://codereview.chromium.org/1212373006/diff/20001/src/core/SkMallocPixelR... File src/core/SkMallocPixelRef.cpp (right): https://codereview.chromium.org/1212373006/diff/20001/src/core/SkMallocPixelR... src/core/SkMallocPixelRef.cpp:56: SkDebugf("failing 1\n"); I think you didn't mean to include the changes to SkMallocPixelRef?
Closing this issue for now. The idea is still tracked in skbug.com/3954, which has a link to this CL for reference. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
