Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1055743003:
Swizzler changes Index8 and 565 (Closed)
Description***Disables swizzles to 565.
We may want to enable swizzles to 565
for images that are encoded in a format
similar to 565, however, we do not want
to take images that decode naturally to
kN32 and then convert them to 565.
***Enable swizzles to kIndex_8. For images
encoded in a color table format, we suggest
that they be decoded to kIndex_8. When we
decode, we only allow conversion to kIndex_8
if it matches the suggested color type (except
wbmp which seems good as is).
***Modify dm to test images that decode to
kIndex_8.
BUG=skia:3257
BUG=skia:3440
Committed: https://skia.googlesource.com/skia/+/438b2adefb9e9213e0ddaf0609405d3087a1cf0a
Patch Set 1 #
Total comments: 31
Patch Set 2 : Avoid setting a field to a local variable #
Total comments: 28
Patch Set 3 : Safe fill for unlikely values of row bytes and other minor fixes #
Total comments: 13
Patch Set 4 : Minor fixes - Need to clarify testing plan for dm and Fill() #
Total comments: 6
Patch Set 5 : Enabled kIndex8 testing in dm, Created a test for SkSwizzler::Fill() #
Total comments: 16
Patch Set 6 : Create codec before pushing srcs #
Total comments: 29
Patch Set 7 : Cleaner testing #
Total comments: 18
Patch Set 8 : Improved SwizzlerTest #
Total comments: 12
Patch Set 9 : Gif rewinding that actually works #
Total comments: 8
Patch Set 10 : Using AutoTCallVProc #Patch Set 11 : Fix windows errors #
Messages
Total messages: 37 (11 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp#newcode79 dm/DMSrcSink.cpp:79: SkAutoTUnref<SkColorTable> colorTable(NULL); I saw what you did for nanobench, but I decided to continue to use SkColorTable and SkBitmap because of the convenience of drawing an SkBitmap at the end of the function. Let me know if I am missing a better alternative, I'm not too familiar with canvas->draw. I don't think this is too bad as is, but one way to get rid of some complexity would be to always pass in a color table. But I think it's a better idea to pass NULL (when it might be NULL) to catch bugs early. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:394: switch (dstColorType) { I've been looking at factoring this into the swizzler with some sort of fill() method (in my jpeg cl that is in progress). I think it could be useful to gif, bmp, and jpeg, but it is tricky because we still have to explain whether the input parameter is an index or a color.
https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (left): https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp#oldcode75 dm/DMSrcSink.cpp:75: SkImageInfo decodeInfo = codec->getInfo().makeColorType(canvasInfo.colorType()); By removing this, we no longer have a way to test 565. It also means that if an image decodes to Index8 by default, we no longer test to 8888. https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp#newcode79 dm/DMSrcSink.cpp:79: SkAutoTUnref<SkColorTable> colorTable(NULL); On 2015/04/02 17:26:29, msarett wrote: > I saw what you did for nanobench, but I decided to continue to use SkColorTable > and SkBitmap because of the convenience of drawing an SkBitmap at the end of the > function. Let me know if I am missing a better alternative, I'm not too > familiar with canvas->draw. This may be as good as anything. If you took my approach in nanobench (allocate a buffer based on the SkImageInfo - for posterity, it's here: https://codereview.chromium.org/1051973002/, patch set 3), you could create an SkBitmap later. It would be ugly, but you could create an SkMallocPixelRef using NewDirect (maybe by using a custom SkBitmap::Allocator, which I also use in that CL...). I was looking to see how you could use SkImage. In theory, you could use SkImage::NewFromGenerator, but that does not allow us to specify a different SkImageInfo or use the scanline decoder, which we want to do. > > I don't think this is too bad as is, but one way to get rid of some complexity > would be to always pass in a color table. But I think it's a better idea to > pass NULL (when it might be NULL) to catch bugs early. sgtm https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:462: // However, we cannot in RLE format since we may need to leave some It seems a shame that we cannot support this. It seems like SkColorTable should be able to support more than 256 colors, except that we make assumptions all over the place that it only supports 256 (and it would require more than 8 bits to index...). Oh well :( https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:582: fColorTable = get_color_table_ptr(dstInfo.colorType(), inputColorPtr, This seems fragile. You are setting a pointer field to a local variable, which will no longer exist when the function returns. As it happens, we *only* access fColorTable before this function ends (from decodeMask, decodeRLE, or decode), but if anything were to change, we might start dereferencing a pointer to arbitrary data. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:863: if (kYes_ZeroInitialized == opts.fZeroInitialized) { kNo_ZeroInitialized. If it's already zero initialized, you do not need to do this, but if it is, then you do. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:864: memset(dst, 0, dstRowBytes * height); Ooh, I missed this on an earlier review. We allow clients to provide smaller allocations than dstRowBytes * height - technically, for the last row, you only need width * bytesPerPixel for the last line. This can allow the client to save a little bit of memory. We already have a function that calculates this: SkImageInfo::getSafeSize https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:123: void setRLEPixel(void* dst, size_t dstRowBytes, Why did these change? https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:394: switch (dstColorType) { On 2015/04/02 17:26:29, msarett wrote: > I've been looking at factoring this into the swizzler with some sort of fill() > method (in my jpeg cl that is in progress). I think it could be useful to gif, > bmp, and jpeg, but it is tricky because we still have to explain whether the > input parameter is an index or a color. That should be determined by the SrcConfig, correct? https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:404: ((int) dstRowBytes) * height); Why did you cast to an int? (Sorry if this came up last time for the N32 case...) Also, as mentioned in BMP, this is more than we can safely set. It would probably be good to write a test for this. The test would *not* use a tight row packing (i.e. it would use something larger than minRowBytes), but it would allocate getSafeSize(largeRowBytes) bytes. ASAN would catch us writing beyond the bounds of allocated memory. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:466: (height - y) * ((int) dstRowBytes) Again, for both of these, we need to be careful about not going beyond the end of the allocated memory. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (left): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:137: const int colorCount = numPalette + (numPalette < 256); Part of the contract of onGetPixels is that we will set the passed in color pointer to the actual number of colors. We need to support that. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:170: /* BUGGY IMAGE WORKAROUND As long as you're touching this code, you might as well add the fix from https://codereview.chromium.org/948163002, to SkImageDecoder. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:488: fColorTable = get_color_table_ptr(requestedInfo.colorType(), ctable, ctableCount, Once again, we're potentially setting the member pointer to something that will disappear when this function ends. This is dangerous. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:78: UPDATE_RESULT_ALPHA(ctable[src[x]] >> SK_A32_SHIFT); Interestingly, looking at the old SkScaledBitmapSampler, the analogue (Sample_Index_DI) just returns false. I'd be curious to know why that is. The goal of learning whether the image had alpha is to help us take the fast (opaque) case. It seems like we would still want to know that when drawing with Index?
*** Avoid setting a field to a local variable *** Added bug fix for indexed png overflow *** Added a SkSwizzler::Fill() function (although I'm undecided if it's actually useful) *** Other minor fixes https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (left): https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp#oldcode75 dm/DMSrcSink.cpp:75: SkImageInfo decodeInfo = codec->getInfo().makeColorType(canvasInfo.colorType()); On 2015/04/02 19:20:30, scroggo wrote: > By removing this, we no longer have a way to test 565. > > It also means that if an image decodes to Index8 by default, we no longer test > to 8888. Oh ok, I think I misunderstood this line. So if we keep it, we will try to decode to the config passed on the command line? Is it valid to pass Index8 there? https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:462: // However, we cannot in RLE format since we may need to leave some On 2015/04/02 19:20:30, scroggo wrote: > It seems a shame that we cannot support this. It seems like SkColorTable should > be able to support more than 256 colors, except that we make assumptions all > over the place that it only supports 256 (and it would require more than 8 bits > to index...). Oh well :( Agreed. Wish there was a good way. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:582: fColorTable = get_color_table_ptr(dstInfo.colorType(), inputColorPtr, On 2015/04/02 19:20:31, scroggo wrote: > This seems fragile. You are setting a pointer field to a local variable, which > will no longer exist when the function returns. > > As it happens, we *only* access fColorTable before this function ends (from > decodeMask, decodeRLE, or decode), but if anything were to change, we might > start dereferencing a pointer to arbitrary data. Yeah that concerned me as well. I think the new approach of using SkColorTable and copying is better. I have made this change here and in png. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:863: if (kYes_ZeroInitialized == opts.fZeroInitialized) { On 2015/04/02 19:20:30, scroggo wrote: > kNo_ZeroInitialized. > > If it's already zero initialized, you do not need to do this, but if it is, then > you do. Of course! https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:864: memset(dst, 0, dstRowBytes * height); On 2015/04/02 19:20:30, scroggo wrote: > Ooh, I missed this on an earlier review. > > We allow clients to provide smaller allocations than dstRowBytes * height - > technically, for the last row, you only need width * bytesPerPixel for the last > line. This can allow the client to save a little bit of memory. > > We already have a function that calculates this: SkImageInfo::getSafeSize Good to know! I don't think this is the only place that I have made this mistake. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:123: void setRLEPixel(void* dst, size_t dstRowBytes, On 2015/04/02 19:20:31, scroggo wrote: > Why did these change? I wanted to setRLEPixels for kN32 and for kIndex8, so it made more sense to pass a void* than a SkPMColor*. After the fact, I removed the capability to setRLEPixels for kIndex8 because of the fact that some pixels in a decoded RLE may be left transparent, and we do not have a transparent index to create a valid kIndex8 representation. However, I left the change to void* because I think it is cleaner to pass a generic destination to setRLEPixels and then to cast the pointer in setRLEPixels to the color type indicated by dstInfo. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:394: switch (dstColorType) { On 2015/04/02 19:20:31, scroggo wrote: > On 2015/04/02 17:26:29, msarett wrote: > > I've been looking at factoring this into the swizzler with some sort of fill() > > method (in my jpeg cl that is in progress). I think it could be useful to > gif, > > bmp, and jpeg, but it is tricky because we still have to explain whether the > > input parameter is an index or a color. > > That should be determined by the SrcConfig, correct? I added a SkSwizzler::Fill() function. It turns out that using SrcConfig as a field does not solve the difficulties with this routine. I needed to make it static so that: (1) It was usable by decodeRLE() in SkBmpCodec which does not use a swizzler object. (2) It was usable in SkGifCodec where we create a swizzler for only a subset of the image (but want to fill the entire image). I thought neither of these cases were compelling on their own, but because they are 2 of the 3 use cases for this function (although there are more coming with jpeg), it should be static. The result is that the Fill() function needs a ton of parameters. I still think it is better to have it than not because it consolidates complexity to a single place (which will only get worse if we support more color types). https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:404: ((int) dstRowBytes) * height); On 2015/04/02 19:20:31, scroggo wrote: > Why did you cast to an int? (Sorry if this came up last time for the N32 > case...) > > Also, as mentioned in BMP, this is more than we can safely set. > > It would probably be good to write a test for this. The test would *not* use a > tight row packing (i.e. it would use something larger than minRowBytes), but it > would allocate getSafeSize(largeRowBytes) bytes. ASAN would catch us writing > beyond the bounds of allocated memory. The cast was added for sk_memset32 which takes an int (Windows bots were failing). Nice catch, I believe it is unnecessary for memset. I will add the test you described in a subsequent CL if that seems like a reasonable approach. I think I fixed this bug everywhere, but I agree we need to verify. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:466: (height - y) * ((int) dstRowBytes) On 2015/04/02 19:20:31, scroggo wrote: > Again, for both of these, we need to be careful about not going beyond the end > of the allocated memory. Acknowledged. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (left): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:137: const int colorCount = numPalette + (numPalette < 256); On 2015/04/02 19:20:31, scroggo wrote: > Part of the contract of onGetPixels is that we will set the passed in color > pointer to the actual number of colors. We need to support that. Didn't know this. I think I now handle this properly in bmp, gif, and png. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:170: /* BUGGY IMAGE WORKAROUND On 2015/04/02 19:20:31, scroggo wrote: > As long as you're touching this code, you might as well add the fix from > https://codereview.chromium.org/948163002, to SkImageDecoder. Done. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:488: fColorTable = get_color_table_ptr(requestedInfo.colorType(), ctable, ctableCount, On 2015/04/02 19:20:31, scroggo wrote: > Once again, we're potentially setting the member pointer to something that will > disappear when this function ends. This is dangerous. This has been fixed. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:78: UPDATE_RESULT_ALPHA(ctable[src[x]] >> SK_A32_SHIFT); On 2015/04/02 19:20:31, scroggo wrote: > Interestingly, looking at the old SkScaledBitmapSampler, the analogue > (Sample_Index_DI) just returns false. > > I'd be curious to know why that is. The goal of learning whether the image had > alpha is to help us take the fast (opaque) case. It seems like we would still > want to know that when drawing with Index? Hmmm seems like an interesting tradeoff. It does seem like a shame to loop over the entire row when all we really need is a memcpy. However, I wonder if anyone has actually explored whether it is better to improve performance here in the decode or when drawing. Or which performance improvement is more significant. I think I'll make it a low priority TODO. Let me know if you want me to imitate the behavior of SkScaledBitmapSampler for now. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:511: sk_memcpy32(ctable, fColorTable->readColors(), *ctableCount); Is this better than using regular memcpy? https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:440: if (NULL != colorTable) { My original version passed a bool parameter indicating whether the input was an index into a color table or a color. Instead, if the color table is non-NULL, we assume it's an index. I can't decide if this is confusing or clever. FWIW: This function looks a lot cleaner if I implement it as non-static (doesn't need a ton of parameters). This version would be useful in one place in SkGifCodec and one place in SkJpegCodec.
Can you add to the description: BUG=skia:3440 https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (left): https://codereview.chromium.org/1055743003/diff/60001/dm/DMSrcSink.cpp#oldcode75 dm/DMSrcSink.cpp:75: SkImageInfo decodeInfo = codec->getInfo().makeColorType(canvasInfo.colorType()); On 2015/04/03 18:01:31, msarett wrote: > On 2015/04/02 19:20:30, scroggo wrote: > > By removing this, we no longer have a way to test 565. > > > > It also means that if an image decodes to Index8 by default, we no longer test > > to 8888. > > Oh ok, I think I misunderstood this line. So if we keep it, we will try to > decode to the config passed on the command line? Essentially. This means to decode to the config of the Sink. We determine which Sinks to use based on --config on the command line. (By default, one run will include a few different Sinks.) > Is it valid to pass Index8 there? No. None of our Sinks are Index8, since Skia does not allow you to create an SkCanvas with Index8 color type - Skia does not have a way to draw to Index8. However, if an SkBitmap (or SkImage) is Index8, Skia has code to draw that to multiple color types. https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1055743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:123: void setRLEPixel(void* dst, size_t dstRowBytes, On 2015/04/03 18:01:32, msarett wrote: > On 2015/04/02 19:20:31, scroggo wrote: > > Why did these change? > > I wanted to setRLEPixels for kN32 and for kIndex8, so it made more sense to pass > a void* than a SkPMColor*. After the fact, I removed the capability to > setRLEPixels for kIndex8 because of the fact that some pixels in a decoded RLE > may be left transparent, and we do not have a transparent index to create a > valid kIndex8 representation. > > However, I left the change to void* because I think it is cleaner to pass a > generic destination to setRLEPixels and then to cast the pointer in setRLEPixels > to the color type indicated by dstInfo. sgtm https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:30: kUnpremul_SkAlphaType == src.alphaType())) { I think this is not quite the ! of the original statement, but it is intended to be, correct? The prior patch looks like it says: A || (B && C) and the new patch says !A && (!B || C) I think that !(A || (B && C)) should be !A && (!B || !C) That said, given that we have to think so hard about this multi-conditional statement, I think we can make this easier to read: if (src.alphaType() != dst.alphaType()) { if (kOpaque_SkAlpha == src.alphaType()) { // If the source is opaque, we cannot decode to anything but opaque return false; } // src is opaque. switch (dst.alphaType()) { case kPremul: case kUnpremul: // The source is not opaque, so either of these is okay. break; default: // We cannot decode a non-opaque image to opaque (or unknown) return false; } } The switch statement could be an if, and technically you could only check for premul, since we know the source must be unpremul if it's not opaque. If we decide to make all our codecs claim their non-opaque images are premul (which is not correct, but maybe more useful for callers that try to use the default info), we'd have to go back and change this code if you only check the dst for unpremul. To catch that, we could assert that src.alphaType() is unpremul. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:631: *numColors = maxColors; Maybe add a note that we set it to maxColors in case the image file has errors (i.e. fNumColors was wrong). https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:382: (kIndex_8_SkColorType == dstColorType && fillIndex == 0); Don't we want to also check zeroInit? https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:511: sk_memcpy32(ctable, fColorTable->readColors(), *ctableCount); On 2015/04/03 18:01:32, msarett wrote: > Is this better than using regular memcpy? We have MemoryBench which compares the two, and it looks like we use SSE2 if we have it. I'm guessing that if MemoryBench did not show it to be faster, we'd just make it call memcpy. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:651: if (this->initializeSwizzler(dstInfo, NULL, dstInfo.minRowBytes(), opts, NULL) != kSuccess) { Maybe add a FIXME acknowledging that getScanlineDecoder currently does not have a way to get the color table information for a kIndex8 image. (It seems like the caller cannot reasonably use a kIndex8 image from scanline decoding, since they won't know what the colors are.) https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:433: size_t totalBytes = dstInfo.getSafeSize(dstRowBytes); const https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:434: size_t bytes = totalBytes - y * dstRowBytes; const. Also, Maybe this is better described as remainingBytes? https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:440: if (NULL != colorTable) { On 2015/04/03 18:01:32, msarett wrote: > My original version passed a bool parameter indicating whether the input was an > index into a color table or a color. Instead, if the color table is non-NULL, > we assume it's an index. I can't decide if this is confusing or clever. It is a little bit confusing, although I think that's better than having redundant parameters. A comment in the header will help with any confusion though. > > FWIW: This function looks a lot cleaner if I implement it as non-static (doesn't > need a ton of parameters). This version would be useful in one place in > SkGifCodec and one place in SkJpegCodec. What is the advantage to doing it this way? https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:441: sk_memset32((uint32_t*) dstRow, colorTable[(uint8_t) input], Is this cast just in case? I think it might be interesting to assert that (uint8_t) input == input. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:442: bytes / sizeof(SkPMColor)); This seems potentially dangerous. In general, I would expect rowBytes to be divisible by 4, in which case this is safe. That said, we allow the caller to specify, so they could supply some weird number (maybe they have their reasons?). If the padding on the end was not a multiple of 4, we would end up not setting our colors properly. That said, this is likely faster than doing a loop over each row. Maybe do a check that this is safe based on the rowBytes, and if not, have a backup option which handles each row, with a comment that (and maybe even an SkCodecPrintf) that this is a weird rowBytes and an unlikely case. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:453: SkCodecPrintf("Warning: Unsupported dst color type for fill(). Doing nothing.\n"); Maybe SkASSERT(false); https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:135: * @param input the value to fill with - may be a color or an index Maybe expand this comment to explain why it might be a color vs an index, and when it will be treated as a uint8_t. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:140: uint32_t input, SkPMColor* colorTable); I find it a little surprising that we do not need to know the SrcConfig. I guess we always convert our colors to 32bit before here (unless it's an index)?
Safe fill for unlikely values of row bytes and other minor fixes https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:30: kUnpremul_SkAlphaType == src.alphaType())) { On 2015/04/06 14:55:11, scroggo wrote: > I think this is not quite the ! of the original statement, but it is intended to > be, correct? > > The prior patch looks like it says: > > A || (B && C) > > and the new patch says > > !A && (!B || C) > > I think that > > !(A || (B && C)) > > should be > > !A && (!B || !C) > > That said, given that we have to think so hard about this multi-conditional > statement, I think we can make this easier to read: > > if (src.alphaType() != dst.alphaType()) { > if (kOpaque_SkAlpha == src.alphaType()) { > // If the source is opaque, we cannot decode to anything but opaque > return false; > } > // src is opaque. > switch (dst.alphaType()) { > case kPremul: > case kUnpremul: > // The source is not opaque, so either of these is okay. > break; > default: > // We cannot decode a non-opaque image to opaque (or unknown) > return false; > } > } > > The switch statement could be an if, and technically you could only check for > premul, since we know the source must be unpremul if it's not opaque. If we > decide to make all our codecs claim their non-opaque images are premul (which is > not correct, but maybe more useful for callers that try to use the default > info), we'd have to go back and change this code if you only check the dst for > unpremul. To catch that, we could assert that src.alphaType() is unpremul. Yeah that statement was confusing and wrong. Your suggestion is much better. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:631: *numColors = maxColors; On 2015/04/06 14:55:11, scroggo wrote: > Maybe add a note that we set it to maxColors in case the image file has errors > (i.e. fNumColors was wrong). Done. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:382: (kIndex_8_SkColorType == dstColorType && fillIndex == 0); On 2015/04/06 14:55:11, scroggo wrote: > Don't we want to also check zeroInit? Yes of course, thanks for catching this. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:511: sk_memcpy32(ctable, fColorTable->readColors(), *ctableCount); On 2015/04/06 14:55:11, scroggo wrote: > On 2015/04/03 18:01:32, msarett wrote: > > Is this better than using regular memcpy? > > We have MemoryBench which compares the two, and it looks like we use SSE2 if we > have it. I'm guessing that if MemoryBench did not show it to be faster, we'd > just make it call memcpy. Acknowledged. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:651: if (this->initializeSwizzler(dstInfo, NULL, dstInfo.minRowBytes(), opts, NULL) != kSuccess) { On 2015/04/06 14:55:11, scroggo wrote: > Maybe add a FIXME acknowledging that getScanlineDecoder currently does not have > a way to get the color table information for a kIndex8 image. (It seems like the > caller cannot reasonably use a kIndex8 image from scanline decoding, since they > won't know what the colors are.) Done. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:433: size_t totalBytes = dstInfo.getSafeSize(dstRowBytes); On 2015/04/06 14:55:11, scroggo wrote: > const Done. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:434: size_t bytes = totalBytes - y * dstRowBytes; On 2015/04/06 14:55:11, scroggo wrote: > const. > > Also, Maybe this is better described as remainingBytes? Agreed https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:440: if (NULL != colorTable) { On 2015/04/06 14:55:11, scroggo wrote: > On 2015/04/03 18:01:32, msarett wrote: > > My original version passed a bool parameter indicating whether the input was > an > > index into a color table or a color. Instead, if the color table is non-NULL, > > we assume it's an index. I can't decide if this is confusing or clever. > > It is a little bit confusing, although I think that's better than having > redundant parameters. A comment in the header will help with any confusion > though. > > > > > FWIW: This function looks a lot cleaner if I implement it as non-static > (doesn't > > need a ton of parameters). This version would be useful in one place in > > SkGifCodec and one place in SkJpegCodec. > > What is the advantage to doing it this way? Added the comment. I don't think there isn't a great advantage to a non-static version, and given that it wouldn't be usable in all cases, it probably isn't worthwhile to consider. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:441: sk_memset32((uint32_t*) dstRow, colorTable[(uint8_t) input], On 2015/04/06 14:55:11, scroggo wrote: > Is this cast just in case? I think it might be interesting to assert that > (uint8_t) input == input. Done. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:442: bytes / sizeof(SkPMColor)); On 2015/04/06 14:55:11, scroggo wrote: > This seems potentially dangerous. In general, I would expect rowBytes to be > divisible by 4, in which case this is safe. That said, we allow the caller to > specify, so they could supply some weird number (maybe they have their > reasons?). If the padding on the end was not a multiple of 4, we would end up > not setting our colors properly. > > That said, this is likely faster than doing a loop over each row. Maybe do a > check that this is safe based on the rowBytes, and if not, have a backup option > which handles each row, with a comment that (and maybe even an SkCodecPrintf) > that this is a weird rowBytes and an unlikely case. Done https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:453: SkCodecPrintf("Warning: Unsupported dst color type for fill(). Doing nothing.\n"); On 2015/04/06 14:55:12, scroggo wrote: > Maybe SkASSERT(false); Done. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:135: * @param input the value to fill with - may be a color or an index On 2015/04/06 14:55:12, scroggo wrote: > Maybe expand this comment to explain why it might be a color vs an index, and > when it will be treated as a uint8_t. Done. https://codereview.chromium.org/1055743003/diff/80001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:140: uint32_t input, SkPMColor* colorTable); On 2015/04/06 14:55:12, scroggo wrote: > I find it a little surprising that we do not need to know the SrcConfig. I guess > we always convert our colors to 32bit before here (unless it's an index)? Yeah you are right. It works because we are always expecting kN32 colors for the destination (unless its an index, indicated by the color table), so we know that is what will be passed in. We may need to add srcConfig is we end up adding 565 or something like that here.
scroggo@google.com changed reviewers: + mtklein@google.com
Adding mtklein@ to review proposal of how to include testing to kIndex8 to DM. https://codereview.chromium.org/1055743003/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:83: if (kIndex_8_SkColorType == decodeInfo.colorType()) { Since we got this colorType from the canvas, this comparison will never be true. I *do* think we should test decoding to kIndex8, but I do not know how to fit this into DM. Ideally, it would show up in Gold with its Config set to kIndex8, with the same name as the other versions. Maybe mtklein@ will have a better idea, but I think the way to do it will be to add to the source options (lucky for us source options was just added!). Here's how I would do this: In CodecSrc, add a field which determines whether to get the colortype from the canvas or to use something else always. (The reason I say "something else" is because we may also want to test drawing to GrayScale, or some other color type without a backend.) We can write a new enum inside CodecSrc; something like: enum DstColorType { kGetFromCanvas_DstColorType, kIndex8_Always_DstColorType, kGrayscale_Always_DstColorType, }; Then here: switch (fDstColorType) { case kGetFromCanvas_DstColorType: // Get from the canvas, as above case kIndex8_Always_DstColorType: dstColorType = kIndex_8_SkColorType; // Also, only draw this to 8888, since 565 will not show us anything different/ // interesting. break; case kGrayscale_Always_DstColorType: dstColorType = kGrayscale_SkColorType; // Same - do not draw to 565 break; } In DM.cpp, when we call push_src with codec, see if the image is natively kIndex8. If so, create a CodecSrc with kIndex8_Always_DstColorType, and call push_src with this special codec and append "kIndex8" to the source options (which currently just say "codec"), in addition to the existing CodecSrcs we push. It might be worth factoring out the code to call push_src with CodecSrc, to avoid duplication between individual files and folders. https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:30: // If the source is opaque, we must decoder to opaque decode* https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:442: sk_memset32((uint32_t*) dstRow, colorTable[input], This code will have the same issue with the else clause if dstRowBytes is not a multiple of four. https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:451: for (int32_t row = y; row < dstInfo.height(); row++) { It would be good to add tests for all these different cases. https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:143: * decoding to kN32 and the input color table is NULL. In this, nit:comma not needed here.
Minor fixes Still need to clarify testing plan for dm and Fill() https://codereview.chromium.org/1055743003/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:83: if (kIndex_8_SkColorType == decodeInfo.colorType()) { On 2015/04/06 21:26:30, scroggo wrote: > Since we got this colorType from the canvas, this comparison will never be true. > I *do* think we should test decoding to kIndex8, but I do not know how to fit > this into DM. > > Ideally, it would show up in Gold with its Config set to kIndex8, with the same > name as the other versions. > > Maybe mtklein@ will have a better idea, but I think the way to do it will be to > add to the source options (lucky for us source options was just added!). > > Here's how I would do this: > > In CodecSrc, add a field which determines whether to get the colortype from the > canvas or to use something else always. (The reason I say "something else" is > because we may also want to test drawing to GrayScale, or some other color type > without a backend.) > > We can write a new enum inside CodecSrc; something like: > > enum DstColorType { > kGetFromCanvas_DstColorType, > kIndex8_Always_DstColorType, > kGrayscale_Always_DstColorType, > }; > > Then here: > > switch (fDstColorType) { > case kGetFromCanvas_DstColorType: > // Get from the canvas, as above > case kIndex8_Always_DstColorType: > dstColorType = kIndex_8_SkColorType; > // Also, only draw this to 8888, since 565 will not show us anything > different/ > // interesting. > break; > case kGrayscale_Always_DstColorType: > dstColorType = kGrayscale_SkColorType; > // Same - do not draw to 565 > break; > } > > In DM.cpp, when we call push_src with codec, see if the image is natively > kIndex8. If so, create a CodecSrc with kIndex8_Always_DstColorType, and call > push_src with this special codec and append "kIndex8" to the source options > (which currently just say "codec"), in addition to the existing CodecSrcs we > push. > > It might be worth factoring out the code to call push_src with CodecSrc, to > avoid duplication between individual files and folders. This seems to be a good solution. I think it makes sense to add this pending on Mike's input. https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:30: // If the source is opaque, we must decoder to opaque On 2015/04/06 21:26:30, scroggo wrote: > decode* Done. https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:442: sk_memset32((uint32_t*) dstRow, colorTable[input], On 2015/04/06 21:26:30, scroggo wrote: > This code will have the same issue with the else clause if dstRowBytes is not a > multiple of four. Yes of course. That's an oversight on my part. https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:451: for (int32_t row = y; row < dstInfo.height(); row++) { On 2015/04/06 21:26:30, scroggo wrote: > It would be good to add tests for all these different cases. I'm planning to add a test for Fill() that exercises all of the different code paths, and checks interesting values of getSafeSize() and dstRowBytes. Do you think its ok if I work on it after libjpeg or should that be a part of this CL? https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:143: * decoding to kN32 and the input color table is NULL. In this, On 2015/04/06 21:26:30, scroggo wrote: > nit:comma not needed here. Done.
https://codereview.chromium.org/1055743003/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:83: if (kIndex_8_SkColorType == decodeInfo.colorType()) { On 2015/04/07 12:36:03, msarett wrote: > On 2015/04/06 21:26:30, scroggo wrote: > > Since we got this colorType from the canvas, this comparison will never be > true. > > I *do* think we should test decoding to kIndex8, but I do not know how to fit > > this into DM. > > > > Ideally, it would show up in Gold with its Config set to kIndex8, with the > same > > name as the other versions. > > > > Maybe mtklein@ will have a better idea, but I think the way to do it will be > to > > add to the source options (lucky for us source options was just added!). > > > > Here's how I would do this: > > > > In CodecSrc, add a field which determines whether to get the colortype from > the > > canvas or to use something else always. (The reason I say "something else" is > > because we may also want to test drawing to GrayScale, or some other color > type > > without a backend.) > > > > We can write a new enum inside CodecSrc; something like: > > > > enum DstColorType { > > kGetFromCanvas_DstColorType, > > kIndex8_Always_DstColorType, > > kGrayscale_Always_DstColorType, > > }; > > > > Then here: > > > > switch (fDstColorType) { > > case kGetFromCanvas_DstColorType: > > // Get from the canvas, as above > > case kIndex8_Always_DstColorType: > > dstColorType = kIndex_8_SkColorType; > > // Also, only draw this to 8888, since 565 will not show us anything > > different/ > > // interesting. > > break; > > case kGrayscale_Always_DstColorType: > > dstColorType = kGrayscale_SkColorType; > > // Same - do not draw to 565 > > break; > > } > > > > In DM.cpp, when we call push_src with codec, see if the image is natively > > kIndex8. If so, create a CodecSrc with kIndex8_Always_DstColorType, and call > > push_src with this special codec and append "kIndex8" to the source options > > (which currently just say "codec"), in addition to the existing CodecSrcs we > > push. > > > > It might be worth factoring out the code to call push_src with CodecSrc, to > > avoid duplication between individual files and folders. > > This seems to be a good solution. I think it makes sense to add this pending on > Mike's input. sgtm https://codereview.chromium.org/1055743003/diff/120001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/120001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:84: SkPMColor colors[maxColors]; colors needs to live outside the if, right? https://codereview.chromium.org/1055743003/diff/120001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:86: colorPtr = const_cast<SkPMColor*>(colorTable->readColors()); == &colors, right?
Added responses to Mike's comments. Will work on adding Leon's testing suggestion to dm. https://codereview.chromium.org/1055743003/diff/120001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/120001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:84: SkPMColor colors[maxColors]; On 2015/04/07 14:26:42, mtklein wrote: > colors needs to live outside the if, right? The constructor for SkColorTable copies the color table passed as an argument to its own memory, so this does not need to live outside the if statement. It is kind of odd to initialize an SkColorTable with unknown colors, but I feel this is forced on us by the weirdness of SkColorTable. We need the SkColorTable to exist in order to pass it to tryAllocPixels and we need a pointer to the SkPMColor array to pass to onGetPixels (where it will be populated with colors). I will add a comment explaining this. https://codereview.chromium.org/1055743003/diff/120001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:86: colorPtr = const_cast<SkPMColor*>(colorTable->readColors()); On 2015/04/07 14:26:41, mtklein wrote: > == &colors, right? Per the comment above, it actually does not.
https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:451: for (int32_t row = y; row < dstInfo.height(); row++) { On 2015/04/07 12:36:03, msarett wrote: > On 2015/04/06 21:26:30, scroggo wrote: > > It would be good to add tests for all these different cases. > > I'm planning to add a test for Fill() that exercises all of the different code > paths, and checks interesting values of getSafeSize() and dstRowBytes. > > Do you think its ok if I work on it after libjpeg or should that be a part of > this CL? Please include with this CL. https://codereview.chromium.org/1055743003/diff/120001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/120001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:440: uint32_t defaultValue; Maybe this should be named "color"?
Patchset #5 (id:140001) has been deleted
Enabled kIndex8 testing in dm Created a test for SkSwizzler::Fill() https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/100001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:451: for (int32_t row = y; row < dstInfo.height(); row++) { On 2015/04/07 17:09:27, scroggo wrote: > On 2015/04/07 12:36:03, msarett wrote: > > On 2015/04/06 21:26:30, scroggo wrote: > > > It would be good to add tests for all these different cases. > > > > I'm planning to add a test for Fill() that exercises all of the different code > > paths, and checks interesting values of getSafeSize() and dstRowBytes. > > > > Do you think its ok if I work on it after libjpeg or should that be a part of > > this CL? > > Please include with this CL. Done. https://codereview.chromium.org/1055743003/diff/120001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/120001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:440: uint32_t defaultValue; On 2015/04/07 17:09:27, scroggo wrote: > Maybe this should be named "color"? Done.
https://codereview.chromium.org/1055743003/diff/160001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/dm/DM.cpp#newcode199 dm/DM.cpp:199: if (path.endsWith(indexExts[i])) { Here's how I imagined this working: SkAutoTDelete<SkCodec> codec(NewFromStream(...)); switch(codec->getInfo().colorType()) { case kIndex8: push_src(...., kIndex8Always); break; case kGrayscale: push_src(...., kGrayAlways); break; default: // do nothing break; } // normal calls to push_src The advantage to my approach is that if we make another decoder (e.g. png, jpeg) support one of these types, we won't need to update this code to look for those types as well. (Case in point: looking at this code reminded me to update codec_supported in my change to implement a webp codec.) Our PNG decoder *should* actually support both Index8 and Grayscale, depending on the data (I partially kept the code to decode to grayscale - but it is disguised as Alpha8, since we didn't have grayscale - we need to update it to use grayscale and test it, although that can be a separate change). I also think it's generally simpler. One downside to my approach is that it adds an extra SkCodec creation into the setup (which is not multi-threaded), although it will remove some extraneous tasks (where we created Index8 CodecSrcs for BMPs etc which are not in fact Index8). I would lean towards my approach since I think it's more maintainable. A downside to both of our approaches is there is no way to test e.g. kIndex8 for an SkCodec that claimed to be grayscale. Our Wbmp codec supports this. (Related issue: should it? Is there any reason for a client to prefer kIndex8 over grayscale? Android may not know what to do with a grayscale bitmap; for kIndex8 their hardware renderer copies to N32 before uploading, and they may need to update to do the same thing for grayscale, at the very least.) If we do support this (I think Hal added support because it's easy), we should test it somehow. https://codereview.chromium.org/1055743003/diff/160001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:89: } else if (kIndex_8_SkColorType != codec->getInfo().colorType()) { nit: This may be a matter of preference, but I don't think there's any need for the "else" here. If the first case was true, we would have returned, so it's not functionally related, but the two checks also seem to be independent. https://codereview.chromium.org/1055743003/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:93: decodeInfo = codec->getInfo(); nit: You could move this into the declaration of decodeInfo, and in the first case say: decodeInfo = decodeInfo.makeColorType(canvasColorType); https://codereview.chromium.org/1055743003/diff/160001/gyp/tests.gypi File gyp/tests.gypi (right): https://codereview.chromium.org/1055743003/diff/160001/gyp/tests.gypi#newcode218 gyp/tests.gypi:218: '../tests/SwizzlerTest.cpp', This file appears to be missing. https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:689: if (kOpaque_SkAlphaType == alphaType || kRLE_BitmapInputFormat == fInputFormat) { This appears to fix a separate bug? https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkCodec_libp... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkCodec_libp... src/codec/SkCodec_libpng.cpp:396: if (dst.colorType() != src.colorType()) { I think we should still support N32 if src was kIndex8. https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:458: uint32_t* dstPtr = (uint32_t*) dstRow; It seems like this can be moved out of the loop?
In the first step of this change, I created an extra codec in push_codec_srcs and it made everything much clearer. As an additional step, I passed this codec to CodecSrc as a field so we don't have to create the codec again in draw(). I'm not sure if this makes things better or not, I'd welcome your input. Also added the missing test file. https://codereview.chromium.org/1055743003/diff/160001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/dm/DM.cpp#newcode199 dm/DM.cpp:199: if (path.endsWith(indexExts[i])) { On 2015/04/07 21:15:34, scroggo wrote: > Here's how I imagined this working: > > SkAutoTDelete<SkCodec> codec(NewFromStream(...)); > switch(codec->getInfo().colorType()) { > case kIndex8: > push_src(...., kIndex8Always); > break; > case kGrayscale: > push_src(...., kGrayAlways); > break; > default: > // do nothing > break; > } > > // normal calls to push_src > > The advantage to my approach is that if we make another decoder (e.g. png, jpeg) > support one of these types, we won't need to update this code to look for those > types as well. (Case in point: looking at this code reminded me to update > codec_supported in my change to implement a webp codec.) Our PNG decoder > *should* actually support both Index8 and Grayscale, depending on the data (I > partially kept the code to decode to grayscale - but it is disguised as Alpha8, > since we didn't have grayscale - we need to update it to use grayscale and test > it, although that can be a separate change). I also think it's generally > simpler. > > One downside to my approach is that it adds an extra SkCodec creation into the > setup (which is not multi-threaded), although it will remove some extraneous > tasks (where we created Index8 CodecSrcs for BMPs etc which are not in fact > Index8). > > I would lean towards my approach since I think it's more maintainable. > > A downside to both of our approaches is there is no way to test e.g. kIndex8 for > an SkCodec that claimed to be grayscale. Our Wbmp codec supports this. (Related > issue: should it? Is there any reason for a client to prefer kIndex8 over > grayscale? Android may not know what to do with a grayscale bitmap; for kIndex8 > their hardware renderer copies to N32 before uploading, and they may need to > update to do the same thing for grayscale, at the very least.) If we do support > this (I think Hal added support because it's easy), we should test it somehow. What you've described is how I originally thought you intended it. But then I ran into the problem of not having the codec and went the file type route. I have felt similar pain in adding file types to those lists, and I am noticing now that I forgot png on both lists. Pretty good evidence that creating the SkCodec earlier would serve us better. How do you feel about making the SkCodec a field in CodecSrc and only paying the price of creating it once? Maybe this is what you intended to begin with? I think its worth seeing how it looks - it would hopefully negate the cost of creating the codec if we don't have to do it again later. I was aware of the wbmp problem but decided not to handle it. FWIW, the current approach would probably be better in this case, given that we could just describe custom behavior when the file type is a wbmp. But I don't think this is a convincing argument to stick with this code. I see benefits in always supporting kN32 and also supporting the "best" native format. I'm not sure that we want to work to support all of the obscure formats just for android to convert them to kN32 anyway? But this seems like it's worth discussing. https://codereview.chromium.org/1055743003/diff/160001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:89: } else if (kIndex_8_SkColorType != codec->getInfo().colorType()) { On 2015/04/07 21:15:34, scroggo wrote: > nit: This may be a matter of preference, but I don't think there's any need for > the "else" here. If the first case was true, we would have returned, so it's not > functionally related, but the two checks also seem to be independent. Done. https://codereview.chromium.org/1055743003/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:93: decodeInfo = codec->getInfo(); On 2015/04/07 21:15:34, scroggo wrote: > nit: You could move this into the declaration of decodeInfo, and in the first > case say: > > decodeInfo = decodeInfo.makeColorType(canvasColorType); Done. https://codereview.chromium.org/1055743003/diff/160001/gyp/tests.gypi File gyp/tests.gypi (right): https://codereview.chromium.org/1055743003/diff/160001/gyp/tests.gypi#newcode218 gyp/tests.gypi:218: '../tests/SwizzlerTest.cpp', On 2015/04/07 21:15:34, scroggo wrote: > This file appears to be missing. Yes my fault. https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:689: if (kOpaque_SkAlphaType == alphaType || kRLE_BitmapInputFormat == fInputFormat) { On 2015/04/07 21:15:34, scroggo wrote: > This appears to fix a separate bug? Yes it fixes a bug that was almost introduced with this CL. I started marking RLE bmps as kUnpremul because they may have transparent (skipped) pixels. However, every color in the RLE color table table is opaque. In my head it makes more sense to have RLE bmps marked as opaque since the spec intends them to be opaque and they are almost always opaque. But it's my understanding that we are forced to mark them kUnpremul if it might have pixels that are not opaque. This fixes the bug introduced by marking RLE bmps are kUnpremul. https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkCodec_libp... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkCodec_libp... src/codec/SkCodec_libpng.cpp:396: if (dst.colorType() != src.colorType()) { On 2015/04/07 21:15:34, scroggo wrote: > I think we should still support N32 if src was kIndex8. Agreed, I missed this one. https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:458: uint32_t* dstPtr = (uint32_t*) dstRow; On 2015/04/07 21:15:34, scroggo wrote: > It seems like this can be moved out of the loop? Done. https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp#newcode204 dm/DM.cpp:204: CodecSrc::kIndex8_Always_DstColorType, create_codec(path))); Can we assume that if create_codec succeeds once, it will always succeed?
https://codereview.chromium.org/1055743003/diff/160001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/dm/DM.cpp#newcode199 dm/DM.cpp:199: if (path.endsWith(indexExts[i])) { On 2015/04/08 13:59:10, msarett wrote: > On 2015/04/07 21:15:34, scroggo wrote: > > Here's how I imagined this working: > > > > SkAutoTDelete<SkCodec> codec(NewFromStream(...)); > > switch(codec->getInfo().colorType()) { > > case kIndex8: > > push_src(...., kIndex8Always); > > break; > > case kGrayscale: > > push_src(...., kGrayAlways); > > break; > > default: > > // do nothing > > break; > > } > > > > // normal calls to push_src > > > > The advantage to my approach is that if we make another decoder (e.g. png, > jpeg) > > support one of these types, we won't need to update this code to look for > those > > types as well. (Case in point: looking at this code reminded me to update > > codec_supported in my change to implement a webp codec.) Our PNG decoder > > *should* actually support both Index8 and Grayscale, depending on the data (I > > partially kept the code to decode to grayscale - but it is disguised as > Alpha8, > > since we didn't have grayscale - we need to update it to use grayscale and > test > > it, although that can be a separate change). I also think it's generally > > simpler. > > > > One downside to my approach is that it adds an extra SkCodec creation into the > > setup (which is not multi-threaded), although it will remove some extraneous > > tasks (where we created Index8 CodecSrcs for BMPs etc which are not in fact > > Index8). > > > > I would lean towards my approach since I think it's more maintainable. > > > > A downside to both of our approaches is there is no way to test e.g. kIndex8 > for > > an SkCodec that claimed to be grayscale. Our Wbmp codec supports this. > (Related > > issue: should it? Is there any reason for a client to prefer kIndex8 over > > grayscale? Android may not know what to do with a grayscale bitmap; for > kIndex8 > > their hardware renderer copies to N32 before uploading, and they may need to > > update to do the same thing for grayscale, at the very least.) If we do > support > > this (I think Hal added support because it's easy), we should test it somehow. > > What you've described is how I originally thought you intended it. But then I > ran into the problem of not having the codec and went the file type route. > > I have felt similar pain in adding file types to those lists, and I am noticing > now that I forgot png on both lists. Pretty good evidence that creating the > SkCodec earlier would serve us better. > > How do you feel about making the SkCodec a field in CodecSrc and only paying the > price of creating it once? Maybe this is what you intended to begin with? I > think its worth seeing how it looks - it would hopefully negate the cost of > creating the codec if we don't have to do it again later. That seems okay, although it seems like you call create_codec for each creation of a CodecSrc? How is that different from letting CodecSrc create it? It seems like it *might* be nice to create the SkCodec once, but you cannot use it with multiple CodecSrcs because it is stateful. Instead, you could create the SkData once, and share it with each CodecSrc. This means we read from disk less. I'm not sure how much that will improve our testing speed overall, but it might be worth it. (I could imagine making it possible to copy an SkCodec, but that would be essentially free for one backed by an SkData, but much more difficult with an arbitrary stream.) > > I was aware of the wbmp problem but decided not to handle it. FWIW, the current > approach would probably be better in this case, given that we could just > describe custom behavior when the file type is a wbmp. But I don't think this > is a convincing argument to stick with this code. I see benefits in always > supporting kN32 and also supporting the "best" native format. I'm not sure that > we want to work to support all of the obscure formats just for android to > convert them to kN32 anyway? But this seems like it's worth discussing. Definitely worth discussing, and it can inform our decision here, but I don't know that we need to hold this up for that. If we think that a gray scale image will always be supported by index8 (it seems like it could be easily), we could make the grayscale case fall through to add index 8 as well. And even if it doesn't support index8, no harm done; we'll just return Error::Nonfatal. So I'm leaning towards this approach. https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp#newcode185 dm/DM.cpp:185: SkCodecPrintf(return SkStringPrintf("Couldn't read %s.", path.c_str())); SkCodecPrintf (which I think is defined in src/codec?), should only be used in src/codec. Also I do not think we should use a return statement inside a print statement like this. I think what you want is SkDebugf("Couldn't read %s.", path.c_str()); https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp#newcode203 dm/DM.cpp:203: push_src("image", "codec kIndex8", new CodecSrc(path, CodecSrc::kNormal_Mode, We should probably add a src for kScanline_Mode as well. https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp#newcode204 dm/DM.cpp:204: CodecSrc::kIndex8_Always_DstColorType, create_codec(path))); On 2015/04/08 13:59:10, msarett wrote: > Can we assume that if create_codec succeeds once, it will always succeed? If it does not, that is a bug (or a disk read error). https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:63: SkASSERT(NULL != fCodec.get()); Maybe put this assert in the constructor? https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:79: return Error::Nonfatal("Testing decode to 565 is uninteresting."); Maybe "Testing non-565 to 565 is uninteresting"? https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.h#newcode103 dm/DMSrcSink.h:103: CodecSrc(Path, Mode, DstColorType, SkCodec*); Comment about ownership? (Note that I'm commenting on this, but if we decide *not* to pass the SkCodec to the constructor, you can ignore them.) https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:15: uint32_t row, nit: startRow? https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:18: uint32_t input, Maybe rename this to colorOrIndex? (Same for SkSwizzler::Fill). It provides a little more context than just "input". https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:21: // Create fake image data where every bytes has a value of 0xFF nit: byte* https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:52: colorTable[0] = 0xFFFFFFFF; Why did you choose these colors? It seems like since you memset every byte to 0xFF, Filling with this color afterwards will not tell us whether the Fill did the right thing? I think an interesting test might be to use a color where all four bytes are different. That way if we accidentally Fill off by a byte, the test can catch that bug. https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:53: colorTable[1] = 0; Do we ever use values outside of the first two entries? If not, why did we create colorTable with 256 entries? https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:55: check_fill(r, kN32_SkColorType, 0, 1, 0, 0, 0, 1, colorTable); I'm having trouble following all these cases with so many parameters. One thing I could imagine doing is using for loops. This would reduce the code size, and potentially make it look a little clearer: for.... for.... check_fill(r, colorType, w, h, row, rb, totalBytes, colorOrIndex, colorTable); (Maybe we want different loops, since it looks like not all the values get crossed with all other values.) https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:56: check_fill(r, kN32_SkColorType, 0, 1, 0, 10, 0, 1, colorTable); Do we ever have a case where the dst is not four byte aligned? Maybe that is covered by using a row other than zero with an odd rowBytes? It would be nice to call out that case with a comment.
Improved SwizzlerTest and design of dm test https://codereview.chromium.org/1055743003/diff/160001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/160001/dm/DM.cpp#newcode199 dm/DM.cpp:199: if (path.endsWith(indexExts[i])) { On 2015/04/08 17:21:26, scroggo wrote: > On 2015/04/08 13:59:10, msarett wrote: > > On 2015/04/07 21:15:34, scroggo wrote: > > > Here's how I imagined this working: > > > > > > SkAutoTDelete<SkCodec> codec(NewFromStream(...)); > > > switch(codec->getInfo().colorType()) { > > > case kIndex8: > > > push_src(...., kIndex8Always); > > > break; > > > case kGrayscale: > > > push_src(...., kGrayAlways); > > > break; > > > default: > > > // do nothing > > > break; > > > } > > > > > > // normal calls to push_src > > > > > > The advantage to my approach is that if we make another decoder (e.g. png, > > jpeg) > > > support one of these types, we won't need to update this code to look for > > those > > > types as well. (Case in point: looking at this code reminded me to update > > > codec_supported in my change to implement a webp codec.) Our PNG decoder > > > *should* actually support both Index8 and Grayscale, depending on the data > (I > > > partially kept the code to decode to grayscale - but it is disguised as > > Alpha8, > > > since we didn't have grayscale - we need to update it to use grayscale and > > test > > > it, although that can be a separate change). I also think it's generally > > > simpler. > > > > > > One downside to my approach is that it adds an extra SkCodec creation into > the > > > setup (which is not multi-threaded), although it will remove some extraneous > > > tasks (where we created Index8 CodecSrcs for BMPs etc which are not in fact > > > Index8). > > > > > > I would lean towards my approach since I think it's more maintainable. > > > > > > A downside to both of our approaches is there is no way to test e.g. kIndex8 > > for > > > an SkCodec that claimed to be grayscale. Our Wbmp codec supports this. > > (Related > > > issue: should it? Is there any reason for a client to prefer kIndex8 over > > > grayscale? Android may not know what to do with a grayscale bitmap; for > > kIndex8 > > > their hardware renderer copies to N32 before uploading, and they may need to > > > update to do the same thing for grayscale, at the very least.) If we do > > support > > > this (I think Hal added support because it's easy), we should test it > somehow. > > > > What you've described is how I originally thought you intended it. But then I > > ran into the problem of not having the codec and went the file type route. > > > > I have felt similar pain in adding file types to those lists, and I am > noticing > > now that I forgot png on both lists. Pretty good evidence that creating the > > SkCodec earlier would serve us better. > > > > How do you feel about making the SkCodec a field in CodecSrc and only paying > the > > price of creating it once? Maybe this is what you intended to begin with? I > > think its worth seeing how it looks - it would hopefully negate the cost of > > creating the codec if we don't have to do it again later. > > That seems okay, although it seems like you call create_codec for each creation > of a CodecSrc? How is that different from letting CodecSrc create it? > > It seems like it *might* be nice to create the SkCodec once, but you cannot use > it with multiple CodecSrcs because it is stateful. Instead, you could create the > SkData once, and share it with each CodecSrc. This means we read from disk less. > I'm not sure how much that will improve our testing speed overall, but it might > be worth it. (I could imagine making it possible to copy an SkCodec, but that > would be essentially free for one backed by an SkData, but much more difficult > with an arbitrary stream.) > > > > > I was aware of the wbmp problem but decided not to handle it. FWIW, the > current > > approach would probably be better in this case, given that we could just > > describe custom behavior when the file type is a wbmp. But I don't think this > > is a convincing argument to stick with this code. I see benefits in always > > supporting kN32 and also supporting the "best" native format. I'm not sure > that > > we want to work to support all of the obscure formats just for android to > > convert them to kN32 anyway? But this seems like it's worth discussing. > > Definitely worth discussing, and it can inform our decision here, but I don't > know that we need to hold this up for that. > > If we think that a gray scale image will always be supported by index8 (it seems > like it could be easily), we could make the grayscale case fall through to add > index 8 as well. And even if it doesn't support index8, no harm done; we'll just > return Error::Nonfatal. So I'm leaning towards this approach. That was my disappointment with this approach. We save the creation of just one SkCodec (If we are running four tests, we create four SkCodecs here. Where before, if we ran four tests we would create one SkCodec here, and then four more in the four calls to draw.). As we discussed let's keep the code as simple as possible. https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp#newcode185 dm/DM.cpp:185: SkCodecPrintf(return SkStringPrintf("Couldn't read %s.", path.c_str())); On 2015/04/08 17:21:27, scroggo wrote: > SkCodecPrintf (which I think is defined in src/codec?), should only be used in > src/codec. > > Also I do not think we should use a return statement inside a print statement > like this. I think what you want is > > SkDebugf("Couldn't read %s.", path.c_str()); Yeah that is what I meant. https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp#newcode203 dm/DM.cpp:203: push_src("image", "codec kIndex8", new CodecSrc(path, CodecSrc::kNormal_Mode, On 2015/04/08 17:21:27, scroggo wrote: > We should probably add a src for kScanline_Mode as well. Done. https://codereview.chromium.org/1055743003/diff/180001/dm/DM.cpp#newcode204 dm/DM.cpp:204: CodecSrc::kIndex8_Always_DstColorType, create_codec(path))); On 2015/04/08 17:21:27, scroggo wrote: > On 2015/04/08 13:59:10, msarett wrote: > > Can we assume that if create_codec succeeds once, it will always succeed? > > If it does not, that is a bug (or a disk read error). Acknowledged. https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:63: SkASSERT(NULL != fCodec.get()); On 2015/04/08 17:21:27, scroggo wrote: > Maybe put this assert in the constructor? Done. https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:79: return Error::Nonfatal("Testing decode to 565 is uninteresting."); On 2015/04/08 17:21:27, scroggo wrote: > Maybe "Testing non-565 to 565 is uninteresting"? Done. https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1055743003/diff/180001/dm/DMSrcSink.h#newcode103 dm/DMSrcSink.h:103: CodecSrc(Path, Mode, DstColorType, SkCodec*); On 2015/04/08 17:21:27, scroggo wrote: > Comment about ownership? > > (Note that I'm commenting on this, but if we decide *not* to pass the SkCodec to > the constructor, you can ignore them.) Done. https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:15: uint32_t row, On 2015/04/08 17:21:27, scroggo wrote: > nit: startRow? Done. https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:18: uint32_t input, On 2015/04/08 17:21:27, scroggo wrote: > Maybe rename this to colorOrIndex? (Same for SkSwizzler::Fill). It provides a > little more context than just "input". Done. https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:21: // Create fake image data where every bytes has a value of 0xFF On 2015/04/08 17:21:27, scroggo wrote: > nit: byte* Done. https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:52: colorTable[0] = 0xFFFFFFFF; On 2015/04/08 17:21:27, scroggo wrote: > Why did you choose these colors? It seems like since you memset every byte to > 0xFF, Filling with this color afterwards will not tell us whether the Fill did > the right thing? > > I think an interesting test might be to use a color where all four bytes are > different. That way if we accidentally Fill off by a byte, the test can catch > that bug. The tests for "an index to a color" always use 1 as the input index, so we always fill with zeros, which we can detect. The other entry to the color table was created so that if we fill with the wrong entry, the test fails. However, I would agree that filling with four different bytes would make for a more interesting test. I will implement this. https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:53: colorTable[1] = 0; On 2015/04/08 17:21:27, scroggo wrote: > Do we ever use values outside of the first two entries? If not, why did we > create colorTable with 256 entries? I thought it made sense because this is the most likely size of the color table when we use Fill() in real code. But as you noted, the size does not affect the test and we might as well save space. https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:55: check_fill(r, kN32_SkColorType, 0, 1, 0, 0, 0, 1, colorTable); On 2015/04/08 17:21:27, scroggo wrote: > I'm having trouble following all these cases with so many parameters. > > One thing I could imagine doing is using for loops. This would reduce the code > size, and potentially make it look a little clearer: > > for.... > for.... > check_fill(r, colorType, w, h, row, rb, totalBytes, colorOrIndex, > colorTable); > > (Maybe we want different loops, since it looks like not all the values get > crossed with all other values.) I agree that this benefits from better organization and being easier to follow. https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:56: check_fill(r, kN32_SkColorType, 0, 1, 0, 10, 0, 1, colorTable); On 2015/04/08 17:21:27, scroggo wrote: > Do we ever have a case where the dst is not four byte aligned? Maybe that is > covered by using a row other than zero with an odd rowBytes? It would be nice to > call out that case with a comment. This is implicitly covered by odd numbers of row bytes since the next row will not be four byte aligned. Regardless of the start row, Fill() will see this issue because it iterates over rows. However, I agree it is better to specifically design test cases for this (since the implementation of Fill() may change) and to also not depend on a non-zero start row.
https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:53: colorTable[1] = 0; On 2015/04/08 19:35:40, msarett wrote: > On 2015/04/08 17:21:27, scroggo wrote: > > Do we ever use values outside of the first two entries? If not, why did we > > create colorTable with 256 entries? > > I thought it made sense because this is the most likely size of the color table > when we use Fill() in real code. But as you noted, the size does not affect the > test and we might as well save space. In this case, my concern is more about clarity than saving space. Since we are using a non-intuitive value, it makes me wonder why. e.g. Do we have a dependency on having 256 colors? https://codereview.chromium.org/1055743003/diff/200001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/200001/dm/DM.cpp#newcode201 dm/DM.cpp:201: case kGray_8_SkColorType: What do you think about my proposal to swap these, and let kGray fall through to kIndex8? I tend to think we should test it if we're going to support it (which we do currently). Maybe even with a FIXME that says that supporting kIndex8 is a weird decision and we should not do it (bonus points for creating a bug stating why we would support it vs not). https://codereview.chromium.org/1055743003/diff/200001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/200001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:139: * kN32, we will use the index to look up the color that we are Would it make more sense in the kN32 case to just pass colorTable[colorOrIndex]? Then we can leave off the colorTable parameter, correct? We would decide whether to treat it as a color or an index just based on the dstInfo. https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:40: REPORTER_ASSERT(r, 0x1 == indexPtr[x]); nit: make these constants? https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:58: memset(colorTable, 0, sizeof(SkPMColor) * (fillIndex + 1)); This seems extra complicated? Why not: colorTable[0] = 0? Is this in case we decide to use a different value for fillIndex? I also see that we never use colorTable[0]. Are we testing to make sure we didn't accidentally get it? https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:72: for (uint32_t w = 0; w < SK_ARRAY_COUNT(widths); w++) { I know I led you astray when I suggested this before, but I think now that we only support C++11 builds, we can use the range syntax: for (uint32_t w : widths) etc. Which I think will make this code a lot clearer. https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:77: uint32_t colorRowBytes = sizeof(SkPMColor) * widths[w] + paddings[p]; Same as: SkColorTypeBytesPerPixel(kN32) + paddings[p] Not sure if that's clearer. https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:81: for (uint32_t offset = p; offset <= p; offset++) { Wait, isn't this just a loop that only executes once? https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:86: uint32_t colorTotalBytes = colorRowBytes * heights[h] - paddings[p] + offset; It seems like this is SkImageInfo colorInfo = SkImageInfo::MakeN32(w, h); colorTotalBytes = colorInfo.getSafeSize(colorRowBytes) + offset; Not sure if that's better, but it puts things into Skia terms.
Improved SwizzlerTest https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/180001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:53: colorTable[1] = 0; On 2015/04/08 20:37:37, scroggo wrote: > On 2015/04/08 19:35:40, msarett wrote: > > On 2015/04/08 17:21:27, scroggo wrote: > > > Do we ever use values outside of the first two entries? If not, why did we > > > create colorTable with 256 entries? > > > > I thought it made sense because this is the most likely size of the color > table > > when we use Fill() in real code. But as you noted, the size does not affect > the > > test and we might as well save space. > > In this case, my concern is more about clarity than saving space. Since we are > using a non-intuitive value, it makes me wonder why. e.g. Do we have a > dependency on having 256 colors? Makes sense, I'm in agreement. https://codereview.chromium.org/1055743003/diff/200001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/200001/dm/DM.cpp#newcode201 dm/DM.cpp:201: case kGray_8_SkColorType: On 2015/04/08 20:37:37, scroggo wrote: > What do you think about my proposal to swap these, and let kGray fall through to > kIndex8? > > I tend to think we should test it if we're going to support it (which we do > currently). Maybe even with a FIXME that says that supporting kIndex8 is a weird > decision and we should not do it (bonus points for creating a bug stating why we > would support it vs not). I'm uncertain about the proposal because some jpegs are kGray, but these cannot be kIndex8. However, this change is still harmless because conversion possible will catch it when we try to decode jpegs to kIndex8, so I'm in. Haha I'll go for the bonus points. https://code.google.com/p/skia/issues/detail?id=3683 https://codereview.chromium.org/1055743003/diff/200001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/200001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:139: * kN32, we will use the index to look up the color that we are On 2015/04/08 20:37:37, scroggo wrote: > Would it make more sense in the kN32 case to just pass colorTable[colorOrIndex]? > Then we can leave off the colorTable parameter, correct? We would decide whether > to treat it as a color or an index just based on the dstInfo. The code for that looks something like this: switch(dstInfo.colorType()) { case kN32_SkColorType: // Fill with color break; case kIndex_8_SkColorType: // Fill with index break; // Not sure if another case might come up case kSomethingElse: ... } This code was repeated a couple of times in the gif decoder which was the first reason I thought we might want a Fill() function. Of course, the Fill() function has become more complex than this original case, in order to try to make is usable for bmp RLE and jpeg. Removing this complexity back to gif may be the right decision, especially because Fill() has become a little messier than I anticipated. But if I had to give an opinion, I guess I prefer for it be simpler in the SkCodecs and more complicated in the Fill() function. Having said that, I see your point and I don't feel particularly strongly here. Just let me know if you would prefer that I make the change and I'll be happy to. https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:40: REPORTER_ASSERT(r, 0x1 == indexPtr[x]); On 2015/04/08 20:37:38, scroggo wrote: > nit: make these constants? Done. https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:58: memset(colorTable, 0, sizeof(SkPMColor) * (fillIndex + 1)); On 2015/04/08 20:37:37, scroggo wrote: > This seems extra complicated? Why not: > > colorTable[0] = 0? > > Is this in case we decide to use a different value for fillIndex? > > I also see that we never use colorTable[0]. Are we testing to make sure we > didn't accidentally get it? Yeah, essentially it seemed weird to me to test with a color table that only has one color. And also, I didn't want to test with the fillIndex being 0 since we initialize the memory to 0. I would agree that it is overcomplicated. I think maybe its best to leave the color table uninitialized except for the fill index? The bots should catch it if we fill with uninitialized memory anyway? https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:72: for (uint32_t w = 0; w < SK_ARRAY_COUNT(widths); w++) { On 2015/04/08 20:37:37, scroggo wrote: > I know I led you astray when I suggested this before, but I think now that we > only support C++11 builds, we can use the range syntax: > > for (uint32_t w : widths) > > etc. > > Which I think will make this code a lot clearer. Excellent! https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:77: uint32_t colorRowBytes = sizeof(SkPMColor) * widths[w] + paddings[p]; On 2015/04/08 20:37:37, scroggo wrote: > Same as: > > SkColorTypeBytesPerPixel(kN32) + paddings[p] > > Not sure if that's clearer. I think it is a little clearer! https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:81: for (uint32_t offset = p; offset <= p; offset++) { On 2015/04/08 20:37:37, scroggo wrote: > Wait, isn't this just a loop that only executes once? Yes this is a bug. I intended offset to go from 0 to padding. https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:86: uint32_t colorTotalBytes = colorRowBytes * heights[h] - paddings[p] + offset; On 2015/04/08 20:37:38, scroggo wrote: > It seems like this is > > SkImageInfo colorInfo = SkImageInfo::MakeN32(w, h); > > colorTotalBytes = colorInfo.getSafeSize(colorRowBytes) + offset; > > Not sure if that's better, but it puts things into Skia terms. I think it is better. I need to be better at being aware of Skia calls and using them.
LGTM with nits nit: The description should not have so many columns. (I think git likes 72, except for the first line, which is smaller; something like 62) https://codereview.chromium.org/1055743003/diff/200001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/200001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:139: * kN32, we will use the index to look up the color that we are On 2015/04/09 13:02:47, msarett wrote: > On 2015/04/08 20:37:37, scroggo wrote: > > Would it make more sense in the kN32 case to just pass > colorTable[colorOrIndex]? > > Then we can leave off the colorTable parameter, correct? We would decide > whether > > to treat it as a color or an index just based on the dstInfo. > > The code for that looks something like this: > > switch(dstInfo.colorType()) { > case kN32_SkColorType: > // Fill with color > break; > case kIndex_8_SkColorType: > // Fill with index > break; > // Not sure if another case might come up > case kSomethingElse: > ... > } > > This code was repeated a couple of times in the gif decoder which was the first > reason I thought we might want a Fill() function. > > Of course, the Fill() function has become more complex than this original case, > in order to try to make is usable for bmp RLE and jpeg. Removing this > complexity back to gif may be the right decision, especially because Fill() has > become a little messier than I anticipated. But if I had to give an opinion, I > guess I prefer for it be simpler in the SkCodecs and more complicated in the > Fill() function. > > Having said that, I see your point and I don't feel particularly strongly here. > Just let me know if you would prefer that I make the change and I'll be happy > to. I follow now. By making the decision of whether to use the color table inside Fill, we get to share more code. That seems reasonable to me. My concern is that I find the parameters non-intuitive. I think this can be cleared up with comments. I've made a comment in the latest patch set for what I think the comment should look like. https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/200001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:58: memset(colorTable, 0, sizeof(SkPMColor) * (fillIndex + 1)); On 2015/04/09 13:02:48, msarett wrote: > On 2015/04/08 20:37:37, scroggo wrote: > > This seems extra complicated? Why not: > > > > colorTable[0] = 0? > > > > Is this in case we decide to use a different value for fillIndex? > > > > I also see that we never use colorTable[0]. Are we testing to make sure we > > didn't accidentally get it? > > Yeah, essentially it seemed weird to me to test with a color table that only has > one color. And also, I didn't want to test with the fillIndex being 0 since we > initialize the memory to 0. > > I would agree that it is overcomplicated. I think maybe its best to leave the > color table uninitialized except for the fill index? The bots should catch it > if we fill with uninitialized memory anyway? Sgtm. Please add a comment that explains why you're leaving it uninitialized. https://codereview.chromium.org/1055743003/diff/220001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/220001/dm/DM.cpp#newcode201 dm/DM.cpp:201: // FIXME: Is this a long term solution for testing wbmps decodes to kIndex8? nit: please reference skbug.com/3683 https://codereview.chromium.org/1055743003/diff/220001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1055743003/diff/220001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:234: if (!ReadHeader(this->stream(), NULL)) { Can you add a test to CodexTest.cpp which tests for rewinding GIF? https://codereview.chromium.org/1055743003/diff/220001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/220001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:136: * It will be an index in the case where the encoded image was This seems like extraneous information. What this should tell us is what this function will do with these parameters. I think what you really want to say is something like the following: If dstInfo.colorType() is kIndex8, colorOrIndex is assumed to be a uint8_t index, and colorTable is ignored. Each 8-bit pixel will be set to (uint8_t) index. If dstInfo.colorType() is kN32, colorOrIndex is treated differently depending on whether colorTable is NULL: A NULL colorTable means colorOrIndex is treated as an SkPMColor (premul or unpremul, depending on dstInfo.alphaType()). Each 4-byte pixel will be set to colorOrIndex. A non-NULL colorTable means colorOrIndex is treated as a uint8_t index into the colorTable. i.e. each 4-byte pixel will be set to colorTable[(uint8_t) colorOrIndex]. Other SkColorTypes are not supported. https://codereview.chromium.org/1055743003/diff/220001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/220001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:12: static const uint8_t fillIndex = 0x1; nit: Typically we name our constants: kFillIndex or gFillIndex (for globals) I think kFillIndex is appropriate here. https://codereview.chromium.org/1055743003/diff/220001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:23: // Calculate the total size of the image in bytes. Use the smallest possible size. nit: Might be nice to explain the offset. (We're still *using* the smallest possible size, but need to allocate more so we can skip the first offset bytes and still have enough. https://codereview.chromium.org/1055743003/diff/220001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:24: size_t totalBytes = imageInfo.getSafeSize(rowBytes) + offset; nit: could be const.
CR Still Needed: So it turns out that I never remembered to test rewinding for gif. I just added the code because I was touching gif and then I forgot about it. I apologize for uploading broken code for review. I need to be more careful in the future - thanks for catching this. The fix for gif rewinding was not trivial, so I feel this needs another review. Thanks! https://codereview.chromium.org/1055743003/diff/220001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1055743003/diff/220001/dm/DM.cpp#newcode201 dm/DM.cpp:201: // FIXME: Is this a long term solution for testing wbmps decodes to kIndex8? On 2015/04/09 14:53:48, scroggo wrote: > nit: please reference skbug.com/3683 Done. https://codereview.chromium.org/1055743003/diff/220001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1055743003/diff/220001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:234: if (!ReadHeader(this->stream(), NULL)) { On 2015/04/09 14:53:48, scroggo wrote: > Can you add a test to CodexTest.cpp which tests for rewinding GIF? Wow yes absolutely. I can't believe I never tested this - I honestly forgot that I added this code. This code is broken - I fixed it in the next CL. But I feel it requires another code review. https://codereview.chromium.org/1055743003/diff/220001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1055743003/diff/220001/src/codec/SkSwizzler.h... src/codec/SkSwizzler.h:136: * It will be an index in the case where the encoded image was On 2015/04/09 14:53:48, scroggo wrote: > This seems like extraneous information. What this should tell us is what this > function will do with these parameters. I think what you really want to say is > something like the following: > > If dstInfo.colorType() is kIndex8, colorOrIndex is assumed to be a uint8_t > index, and colorTable is ignored. Each 8-bit pixel will be set to (uint8_t) > index. > > If dstInfo.colorType() is kN32, colorOrIndex is treated differently depending on > whether colorTable is NULL: > > A NULL colorTable means colorOrIndex is treated as an SkPMColor (premul or > unpremul, depending on dstInfo.alphaType()). Each 4-byte pixel will be set to > colorOrIndex. > > A non-NULL colorTable means colorOrIndex is treated as a uint8_t index into > the colorTable. i.e. each 4-byte pixel will be set to > colorTable[(uint8_t) colorOrIndex]. > > Other SkColorTypes are not supported. Very well said. https://codereview.chromium.org/1055743003/diff/220001/tests/SwizzlerTest.cpp File tests/SwizzlerTest.cpp (right): https://codereview.chromium.org/1055743003/diff/220001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:12: static const uint8_t fillIndex = 0x1; On 2015/04/09 14:53:48, scroggo wrote: > nit: Typically we name our constants: > > kFillIndex > > or > > gFillIndex (for globals) > > I think kFillIndex is appropriate here. Done. https://codereview.chromium.org/1055743003/diff/220001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:23: // Calculate the total size of the image in bytes. Use the smallest possible size. On 2015/04/09 14:53:48, scroggo wrote: > nit: Might be nice to explain the offset. (We're still *using* the smallest > possible size, but need to allocate more so we can skip the first offset bytes > and still have enough. Yes that would be helpful. https://codereview.chromium.org/1055743003/diff/220001/tests/SwizzlerTest.cpp... tests/SwizzlerTest.cpp:24: size_t totalBytes = imageInfo.getSafeSize(rowBytes) + offset; On 2015/04/09 14:53:48, scroggo wrote: > nit: could be const. Done. https://codereview.chromium.org/1055743003/diff/240001/include/core/SkTemplat... File include/core/SkTemplates.h (right): https://codereview.chromium.org/1055743003/diff/240001/include/core/SkTemplat... include/core/SkTemplates.h:122: } I'm sure it is possible to implement gif rewinding without this API, but it seemed to me that having this API was the most straightforward way to do it. Also, it seems natural to have this API available. An identical function exists for SkAutoTCallVProc. Why not for SkAutoTCallIProc? I am a little unsure if maybe we would want it to return an int though? https://codereview.chromium.org/1055743003/diff/240001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1055743003/diff/240001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:44: SkImageInfo info = codec->getInfo().makeColorType(kN32_SkColorType); If we don't add this, gif will try to decode to kIndex8, which will fail because we don't provide a color table. If it is our goal to use this test to test all possible color type outputs, I need to go back in and implement something similar to what we have in dm. If we are simply using this test to check rewinding, then I think this change is logical.
https://codereview.chromium.org/1055743003/diff/240001/include/core/SkTemplat... File include/core/SkTemplates.h (right): https://codereview.chromium.org/1055743003/diff/240001/include/core/SkTemplat... include/core/SkTemplates.h:122: } On 2015/04/09 16:26:32, msarett wrote: > I'm sure it is possible to implement gif rewinding without this API, but it > seemed to me that having this API was the most straightforward way to do it. > > Also, it seems natural to have this API available. An identical function exists > for SkAutoTCallVProc. Why not for SkAutoTCallIProc? git blame is your friend. Using blame, I see we added SkAutoTCallVProc::reset in https://codereview.chromium.org/396143004, when it was needed. So it was probably just an oversight because we didn't need it for SkAutoTCallIProc. I think adding this is reasonable. > > I am a little unsure if maybe we would want it to return an int though? That seems reasonable to me, since P returns an int. One thing I am unsure about, though, is what to return in the following cases: fObj == obj !fObj Both of these are *not* failure cases, but we will not have called P(fObj). We do not know what return values of P are meaningful, so how can we choose what to return? This would be an argument against returning an int (maybe with a comment explaining that). An alternative (ugly) approach would be to use out parameter(s, or one out parameter + return value) to specify whether we called P and if so, what the return value is. I think a cleaner approach is to decide that if a client wants to know the return value of P, they have to do the following: T* obj = autoProc.detach(); int val = P(obj); // Do something with val autoProc.reset(newObj); You'll need API approval for such a change, so you can follow up with reed@. Looking back at your callsite, though, you do not ever care about the return value of P(), and it's a function you wrote (SkGifCodec::CloseGif). I think you should change it to return void, and then you can call SkAutoTCallVProc, which already has reset(). https://codereview.chromium.org/1055743003/diff/240001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1055743003/diff/240001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.h:37: * Read enough of the stream to initialize the SkGifCodec. Please add comments regarding ownership of SkStream. Deleted on failure. codecOut will take ownership of it in the case where we created a codec. Ownership is unchanged when we returned a gifOut. https://codereview.chromium.org/1055743003/diff/240001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1055743003/diff/240001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:44: SkImageInfo info = codec->getInfo().makeColorType(kN32_SkColorType); On 2015/04/09 16:26:32, msarett wrote: > If we don't add this, gif will try to decode to kIndex8, which will fail because > we don't provide a color table. Another option: we could check for kIndex8, and selectively use a different color type in that case. > > If it is our goal to use this test to test all possible color type outputs, I > need to go back in and implement something similar to what we have in dm. Testing all color type outputs is a slightly trickier, since we'll need to accept failure for non supported color types. > > If we are simply using this test to check rewinding, then I think this change is > logical. Agreed. Please add a comment why we only do N32, though.
Using AutoTCallVProc Added comments on ownership of SkStream Added comments explaining use of kN32 in CodexTest https://codereview.chromium.org/1055743003/diff/240001/include/core/SkTemplat... File include/core/SkTemplates.h (right): https://codereview.chromium.org/1055743003/diff/240001/include/core/SkTemplat... include/core/SkTemplates.h:122: } On 2015/04/09 17:37:17, scroggo wrote: > On 2015/04/09 16:26:32, msarett wrote: > > I'm sure it is possible to implement gif rewinding without this API, but it > > seemed to me that having this API was the most straightforward way to do it. > > > > Also, it seems natural to have this API available. An identical function > exists > > for SkAutoTCallVProc. Why not for SkAutoTCallIProc? > > git blame is your friend. Using blame, I see we added SkAutoTCallVProc::reset in > https://codereview.chromium.org/396143004, when it was needed. > > So it was probably just an oversight because we didn't need it for > SkAutoTCallIProc. I think adding this is reasonable. > > > > > I am a little unsure if maybe we would want it to return an int though? > > That seems reasonable to me, since P returns an int. One thing I am unsure > about, though, is what to return in the following cases: > > fObj == obj > !fObj > > Both of these are *not* failure cases, but we will not have called P(fObj). We > do not know what return values of P are meaningful, so how can we choose what to > return? > > This would be an argument against returning an int (maybe with a comment > explaining that). An alternative (ugly) approach would be to use out > parameter(s, or one out parameter + return value) to specify whether we called P > and if so, what the return value is. > > I think a cleaner approach is to decide that if a client wants to know the > return value of P, they have to do the following: > > T* obj = autoProc.detach(); > int val = P(obj); > // Do something with val > autoProc.reset(newObj); > > You'll need API approval for such a change, so you can follow up with reed@. > > Looking back at your callsite, though, you do not ever care about the return > value of P(), and it's a function you wrote (SkGifCodec::CloseGif). I think you > should change it to return void, and then you can call SkAutoTCallVProc, which > already has reset(). All good points. Seems like using SkAutoTCallCProc is a no brainer. We don't ever need the return value. https://codereview.chromium.org/1055743003/diff/240001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1055743003/diff/240001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.h:37: * Read enough of the stream to initialize the SkGifCodec. On 2015/04/09 17:37:17, scroggo wrote: > Please add comments regarding ownership of SkStream. > > Deleted on failure. > > codecOut will take ownership of it in the case where we created a codec. > > Ownership is unchanged when we returned a gifOut. Done. https://codereview.chromium.org/1055743003/diff/240001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1055743003/diff/240001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:44: SkImageInfo info = codec->getInfo().makeColorType(kN32_SkColorType); On 2015/04/09 17:37:17, scroggo wrote: > On 2015/04/09 16:26:32, msarett wrote: > > If we don't add this, gif will try to decode to kIndex8, which will fail > because > > we don't provide a color table. > > Another option: we could check for kIndex8, and selectively use a different > color type in that case. > > > > > If it is our goal to use this test to test all possible color type outputs, I > > need to go back in and implement something similar to what we have in dm. > > Testing all color type outputs is a slightly trickier, since we'll need to > accept failure for non supported color types. > > > > > If we are simply using this test to check rewinding, then I think this change > is > > logical. > > Agreed. Please add a comment why we only do N32, though. Done.
On 2015/04/09 19:21:31, msarett wrote: > Using AutoTCallVProc > > Added comments on ownership of SkStream > > Added comments explaining use of kN32 in CodexTest > > https://codereview.chromium.org/1055743003/diff/240001/include/core/SkTemplat... > File include/core/SkTemplates.h (right): > > https://codereview.chromium.org/1055743003/diff/240001/include/core/SkTemplat... > include/core/SkTemplates.h:122: } > On 2015/04/09 17:37:17, scroggo wrote: > > On 2015/04/09 16:26:32, msarett wrote: > > > I'm sure it is possible to implement gif rewinding without this API, but it > > > seemed to me that having this API was the most straightforward way to do it. > > > > > > Also, it seems natural to have this API available. An identical function > > exists > > > for SkAutoTCallVProc. Why not for SkAutoTCallIProc? > > > > git blame is your friend. Using blame, I see we added SkAutoTCallVProc::reset > in > > https://codereview.chromium.org/396143004, when it was needed. > > > > So it was probably just an oversight because we didn't need it for > > SkAutoTCallIProc. I think adding this is reasonable. > > > > > > > > I am a little unsure if maybe we would want it to return an int though? > > > > That seems reasonable to me, since P returns an int. One thing I am unsure > > about, though, is what to return in the following cases: > > > > fObj == obj > > !fObj > > > > Both of these are *not* failure cases, but we will not have called P(fObj). We > > do not know what return values of P are meaningful, so how can we choose what > to > > return? > > > > This would be an argument against returning an int (maybe with a comment > > explaining that). An alternative (ugly) approach would be to use out > > parameter(s, or one out parameter + return value) to specify whether we called > P > > and if so, what the return value is. > > > > I think a cleaner approach is to decide that if a client wants to know the > > return value of P, they have to do the following: > > > > T* obj = autoProc.detach(); > > int val = P(obj); > > // Do something with val > > autoProc.reset(newObj); > > > > You'll need API approval for such a change, so you can follow up with reed@. > > > > Looking back at your callsite, though, you do not ever care about the return > > value of P(), and it's a function you wrote (SkGifCodec::CloseGif). I think > you > > should change it to return void, and then you can call SkAutoTCallVProc, which > > already has reset(). > > All good points. Seems like using SkAutoTCallCProc is a no brainer. We don't > ever need the return value. > > https://codereview.chromium.org/1055743003/diff/240001/src/codec/SkCodec_libg... > File src/codec/SkCodec_libgif.h (right): > > https://codereview.chromium.org/1055743003/diff/240001/src/codec/SkCodec_libg... > src/codec/SkCodec_libgif.h:37: * Read enough of the stream to initialize the > SkGifCodec. > On 2015/04/09 17:37:17, scroggo wrote: > > Please add comments regarding ownership of SkStream. > > > > Deleted on failure. > > > > codecOut will take ownership of it in the case where we created a codec. > > > > Ownership is unchanged when we returned a gifOut. > > Done. > > https://codereview.chromium.org/1055743003/diff/240001/tests/CodexTest.cpp > File tests/CodexTest.cpp (right): > > https://codereview.chromium.org/1055743003/diff/240001/tests/CodexTest.cpp#ne... > tests/CodexTest.cpp:44: SkImageInfo info = > codec->getInfo().makeColorType(kN32_SkColorType); > On 2015/04/09 17:37:17, scroggo wrote: > > On 2015/04/09 16:26:32, msarett wrote: > > > If we don't add this, gif will try to decode to kIndex8, which will fail > > because > > > we don't provide a color table. > > > > Another option: we could check for kIndex8, and selectively use a different > > color type in that case. > > > > > > > > If it is our goal to use this test to test all possible color type outputs, > I > > > need to go back in and implement something similar to what we have in dm. > > > > Testing all color type outputs is a slightly trickier, since we'll need to > > accept failure for non supported color types. > > > > > > > > If we are simply using this test to check rewinding, then I think this > change > > is > > > logical. > > > > Agreed. Please add a comment why we only do N32, though. > > Done. LGTM
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1055743003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1055743003/#ps260002 (title: "Fix windows errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1055743003/260002
Message was sent while issue was closed.
Committed patchset #11 (id:260002) as https://skia.googlesource.com/skia/+/438b2adefb9e9213e0ddaf0609405d3087a1cf0a |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
