Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
947283002:
Bmp Image Decoding (Closed)
DescriptionImplementation of image decoding for bmp files, in accordance with the new API.
Currently decodes to opaque and unpremul.
Tested on local test suite.
BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/3675874468de7228944ce21922e6ec863f5f2310
Patch Set 1 #Patch Set 2 : Tested bmp and swizzler design #
Total comments: 45
Patch Set 3 : Fixing merge issues hopefully #
Total comments: 1
Patch Set 4 : Fixes from last iteration and RLE #
Total comments: 34
Patch Set 5 : Minor fixes #
Total comments: 5
Patch Set 6 : Fixed latest comments and tested further #
Total comments: 8
Patch Set 7 : Minor dm testing changes #Patch Set 8 : Improved clarity of swizzler with regard to weird alpha #
Total comments: 34
Patch Set 9 : Moved bmp specific code out of the swizzler #
Total comments: 33
Patch Set 10 : separate swizzler for masks #
Total comments: 22
Patch Set 11 : Rebase #Patch Set 12 : Minor fixes #Patch Set 13 : Creation of SkMasks #
Total comments: 13
Patch Set 14 : clarified ownership of SkMasks* #
Total comments: 28
Patch Set 15 : Various fixes from Leon's comments #
Total comments: 22
Patch Set 16 : Improved efficiency of ResultAlpha and code sharing #
Total comments: 8
Patch Set 17 : More code sharing #
Total comments: 8
Patch Set 18 : Commented out the rare case until we demonstrate that we need it #
Total comments: 4
Patch Set 19 : Keep the correction step for decodeMask, it is used #Patch Set 20 : Premul marked as unsupported #
Total comments: 1
Patch Set 21 : Fixed include #Patch Set 22 : Fixed relase mode failure - inline function not defined #Patch Set 23 : Removed implicit conversions between size_t and uint32_t #Patch Set 24 : One more casting error #Patch Set 25 : Build bot failure on for loop syntax #Patch Set 26 : Bmp Decoder - Undoing the reverts #Patch Set 27 : Original change plus fix #
Messages
Total messages: 65 (15 generated)
Overall, I am pretty excited about this set of changes. Moving complexity to the swizzler has resulted in the SkBmpCodec class becoming much clearer and better designed. Additionally, with the exception RLE decoding (still unimplemented), the decoder properly decodes all of the reasonable bmp images I have collected. One of the drawbacks is that the swizzler has become a little complicated/messy. I have done my best to avoid adding fields/parameters, but I don't think I did too well. I am planning to look at it again when my mind is fresh. Next Steps: RLE Decoding Testing on Edge Cases Integration with Leon's Newest Code Moving from my Laptop to the Desktop https://codereview.chromium.org/947283002/diff/20001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/947283002/diff/20001/include/codec/SkCodec.h#... include/codec/SkCodec.h:72: SkAutoTDelete<SkStream> fStream; I needed to make this field protected to use in SkBmpCodec. Another option is to keep a separate, unowned copy of the field. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:109: if (stream->read(hBuffer.get(), kBmpHeaderBytes + 4) != The size of the second header is technically the first field of the second header. By reading this field along with the first header, I can read the second header in one call to stream->read instead of two. I am hesitant to change the constants in the header file to avoid this extra +4/-4 because the values of the constant will then fail to match up with the bitmap spec. Also I think that changing the constant would fix this addition in some places but then make it necessary to add in others. Let me know what you think. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp... src/codec/SkSwizzler.cpp:502: fFixAlpha, &fSeenNonZeroAlpha, &fZeroPrevRows); The row procedure may need to let the caller know that it has seen a non-zero value of alpha and that the previous rows should be zeroed. Surely, there is a better way to do this than passing in the address of the fields - I just haven't figured it out yet.
scroggo@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/947283002/diff/20001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/947283002/diff/20001/include/codec/SkCodec.h#... include/codec/SkCodec.h:72: SkAutoTDelete<SkStream> fStream; On 2015/02/24 21:56:06, msarett wrote: > I needed to make this field protected to use in SkBmpCodec. Another option is > to keep a separate, unowned copy of the field. I'd prefer a protected accessor: protected: SkStream* stream() { return fStream.get(); } private: SkAutoTDelete<SkStream> fStream; https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:31: #ifdef SK_CPU_BENDIAN This should not be indented. Same with other #ifdef etc. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:108: SkAutoTDeleteArray<uint8_t> hBuffer(new uint8_t[kBmpHeaderBytes + 4]); You should use SkNEW_ARRAY (it can be found in SkPostConfig.h). SkAutoTDeleteArray uses SkDELETE_ARRAY to correspond to this. (Typically our clients convert directly to what you expect, in which case it doesn't make a difference, but if someone were redefining SkNEW* and SkDELETE* we want to be consistent.) https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:109: if (stream->read(hBuffer.get(), kBmpHeaderBytes + 4) != On 2015/02/24 21:56:06, msarett wrote: > The size of the second header is technically the first field of the second > header. By reading this field along with the first header, I can read the > second header in one call to stream->read instead of two. I am hesitant to > change the constants in the header file to avoid this extra +4/-4 because the > values of the constant will then fail to match up with the bitmap spec. Also I > think that changing the constant would fix this addition in some places but then > make it necessary to add in others. Let me know what you think. Why not have something like: const int kBmpHeaderBytesPlusFour = kBmpHeaderBytes + 4; https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:117: uint32_t offset = get_int(hBuffer.get(), 10); Can these values be const? More importantly, keep in mind that we're reading arbitrary data, so we need to make sure that it's sensible. (Otherwise we end up with bugs like skbug.com/3401 and skbug.com/3426.) On that note, maybe add a comment and/or assert that these calls to get_int are safe. (I know I discouraged you from using asserts in the general case, but in this case the numbers being compared are all compile time constants, so you could even use SK_COMPILE_ASSERT. We'll be able to catch that in tests/compilation without having to test every possible BMP file out there.) It also might be nice to have comments explaining what offset and infoBytes mean. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:123: SkAutoTDeleteArray<uint8_t> iBuffer(new uint8_t[infoBytes - 4]); Again, if you're using SkAutoTDeleteArray, which calls SkDELETE_ARRAY, you should use SkNEW_ARRAY. Also, I think it would be good to create a new variable and set it to infoBytes - 4. You can give it a descriptive name, too, which will help me understand what's going on here. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:179: width = (short) get_short(iBuffer.get(), 0); Why did you cast to a short? It looks like width and height are ints? https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:193: SkDebugf("Warning: bitmap header type may not be supported.\n"); What does this mean? (Also, it looks like it could be in the default case above - is there any other way it could be unknown?) https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:210: // Represent standard 16-bit format as bit masks (555) nit: This would be clearer to me if it said something like "RRRRRGGGGGBBBBB". Maybe this is clear enough for other readers. (Does the comment belong inside the case statement though?) https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:216: default: Do you plan to support other bitsPerPixel? If so, maybe add a TODO? Otherwise, could you just use an if statement? https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:229: alphaMask = get_int(iBuffer.get(), 48); compile assert that 48 is safe. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:260: mBuffer.free(); This call is unnecessary. mBuffer will go out of scope on the next line, calling free automatically (that's the benefit of using SkAutoTDeleteArray!) https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:261: } else if (headerType == kInfoV2_BitmapHeaderType || Any reason not to make this a switch statement? - Oh, maybe you wanted to avoid a switch within a switch? Yeah, that's ugly ... https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:272: } else if (headerType == kOS2VX_BitmapHeaderType) { Is this a TODO? It seems like we could have returned much earlier. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:297: uint32_t* masks = new uint32_t[4]; Instead of an array, what do you think of using a struct? struct ColorMasks { uint32_t redMask; uint32_t greenMask; uint32_t blueMask; uint32_t alphaMask; }; That will make all the code dealing with these masks much clearer. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:307: return NULL; This will leak masks. If you used SkAutoTDeleteArray, you can call detach() later if masks needs to live on. Alternatively, you could create masks much later, since it does not appear to be used until the end. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:312: int maxColors = 1 << bitsPerPixel; nit: could be const. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:323: // list of colors intended for optimization. What does this mean? https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:356: cBuffer.free(); Again, this is unnecessary. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:403: SkBmpCodec::~SkBmpCodec() {} This may just be a matter of preference, but I feel like declaring and defining an empty destructor just adds clutter. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:486: SkDebugf("Error: invalid number of bits per pixel.\n"); We already knew this when creating the SkBmpCodec. Why not just return NULL from NewFromStream? https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:506: uint8_t* srcBuffer = new uint8_t[paddedRowBytes]; Use some form of auto deleter to avoid the memory leak below. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.h:19: * This class implements the decoding for bitmap images nit: "bitmap" is unfortunately overloaded. I'd prefer saying "BMP" or "bmp", so as not to confuse with "SkBitmap". Maybe others won't find this confusing though. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.h:78: SkBmpCodec(const SkImageInfo&, SkStream*, const uint16_t, Can you add parameter names? Also, please document which parameters the SkBmpCodec takes ownership of. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.h:79: const BitmapInputFormat, uint32_t*, SkPMColor*, const bool); Typically we do not mark primitive parameters as const. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.h:89: static const int kBmpHeaderBytes = 14; These are all details of the implementation. I think they belong in the cpp file. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.h:103: const SkAutoTDelete<SkPMColor> fColorTable; I believe this is an array. SkAutoTDelete calls "delete", not "delete[]". I'd lean towards using an SkColorTable (with SkAutoTUnref), but you could also use SkAutoTDeleteArray if you'd rather stick with the array. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp... src/codec/SkSwizzler.cpp:186: switch (deltaSrc) { these static functions are intended to be specific to one situation, so they can skip conditional statements and be fast. Is there a way we can do that here? https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp... src/codec/SkSwizzler.cpp:267: return false; Sorry, I thought I had documented this, but the return value is intended to return whether there was alpha. Maybe we don't always know though. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:22: kIndex1, Alternatively, we could make the caller specify how big a pixel is, and just have kIndex. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:43: return 0; // Flag used when pixels are less than a byte I don't like having both BitsPerPixel and BytesPerPixel. Can we convert all callers to use BitsPerPixel? (Or maybe there's another solution?) https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:85: * @param bitMasks Custom bit masks used to interpret pixels What does custom mean? https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:85: * @param bitMasks Custom bit masks used to interpret pixels What does this mean? Does the swizzler always do the same thing with it? If so, it should be described here https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:95: const bool inverted = false); Can you add a comment explaining "inverted"? I shouldn't have made skipZeroes a boolean. We tend to encourage using enums so the call site is clearer. With three booleans, now, it's hard to tell what's what. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:115: const SkPMColor ctable[], const uint32_t* masks, please add comments for all of these. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:116: const bool fixAlpha, bool* fSeenNonZeroAlphaPtr, fCamelCase is reserved for member fields. Parameter names should be camelCase (e.g. seenNonZeroAlphaPtr).
https://codereview.chromium.org/947283002/diff/40001/src/codec/SkCodec_libbmp... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/40001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:367: // Use of image info for input format does not make sense given That's a good point. reed@ and I discussed this, but did not come up with a solution. SkImageInfo does encapsulate much information we want to convey: width height alpha linear/srgb For PNG, I considered whether to return kIndex8_SkColorType for PNG_COLOR_TYPE_PALETTE images, or kAlpha8_SkColorType for PNG_COLOR_TYPE_GRAY. But I think in some sense, the info is the *default* info. The codec should be able to decode to that colortype so long as the data is valid.
I've added RLE and I think it's a lot better this time. I still the swizzler could be improved - I'm planning to spend more time on that. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:216: default: On 2015/02/25 17:22:42, scroggo wrote: > Do you plan to support other bitsPerPixel? If so, maybe add a TODO? Otherwise, > could you just use an if statement? Other bits per pixel are supported. We just don't need default masks for the others. bPP < 16 uses indices. 24 and 32 bit are captured using RGB and RGBA types (we don't need to use bit masks unless the compression type is explicitly set as "bit masks", in which case the masks are set below). However, this is a good place to check for a valid bPP input, rather than in the decode function. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:272: } else if (headerType == kOS2VX_BitmapHeaderType) { On 2015/02/25 17:22:42, scroggo wrote: > Is this a TODO? It seems like we could have returned much earlier. It is a TODO in the chromium code. It is pretty low on my list of things to do. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.h:103: const SkAutoTDelete<SkPMColor> fColorTable; On 2015/02/25 17:22:43, scroggo wrote: > I believe this is an array. SkAutoTDelete calls "delete", not "delete[]". > > I'd lean towards using an SkColorTable (with SkAutoTUnref), but you could also > use SkAutoTDeleteArray if you'd rather stick with the array. Oops I meant to make these SkAutoTDeleteArray. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp... src/codec/SkSwizzler.cpp:186: switch (deltaSrc) { On 2015/02/25 17:22:43, scroggo wrote: > these static functions are intended to be specific to one situation, so they can > skip conditional statements and be fast. Is there a way we can do that here? Yes we can. My concern is an explosion in the number of input types and swizzle functions. I think the same tradeoff occurs with Index1, 2, 4, 8. My preference is actually to split them up, but I was worried about duplicated code. https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:116: const bool fixAlpha, bool* fSeenNonZeroAlphaPtr, On 2015/02/25 17:22:43, scroggo wrote: > fCamelCase is reserved for member fields. Parameter names should be camelCase > (e.g. seenNonZeroAlphaPtr). I was trying to draw attention to the fact that I was using ptrs to fields, which I think is pretty hacky. Any thoughts on this?
Minor fixes
https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/20001/src/codec/SkSwizzler.cpp... src/codec/SkSwizzler.cpp:186: switch (deltaSrc) { On 2015/02/26 23:58:18, msarett wrote: > On 2015/02/25 17:22:43, scroggo wrote: > > these static functions are intended to be specific to one situation, so they > can > > skip conditional statements and be fast. Is there a way we can do that here? > > Yes we can. My concern is an explosion in the number of input types and swizzle > functions. I think the same tradeoff occurs with Index1, 2, 4, 8. My > preference is actually to split them up, but I was worried about duplicated > code. In general, my preference is to avoid duplicated code. As I said, these functions are intended to avoid conditionals, but maybe it would be better to time/profile to see if I am correct that it's worth the duplicated code. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:133: SkASSERT(totalBytes > kBmpHeaderBytes + kBmpOS2V1Bytes); This is not a valid assert. Someone could create an invalid BMP file which stores a value here that is too small. (I can't speak to BMP or this particular case, but in some cases we deliberately allow certain kinds of bad data, since lots of images have some mistakes.) If totalBytes is a number that we depend on, we need to handle it gracefully (i.e. not crash). The assert I was looking for is something like SK_COMPILE_ASSERT(2 + 4 <= kBmpHeaderBytes+4, invalid_read); i.e. that that read was safe to do. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:145: const uint32_t infoBytesRemaining = infoBytes - 4; Since infoBytes is read from the buffer, it could be invalid. e.g. it could be < 4, making infoBytesRemaining wrap to a large number. We should probably check for that rather than attempting to read a large amount from the stream down below. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:150: SkNEW_ARRAY(uint8_t, infoBytesRemaining)); Nit: Does this fit on one line? (We don't have a strict rule about line length: We wrap lines at 100 columns unless it is excessively ugly (use your judgement). but that looks like it fits in 80 columns?) https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:178: headerType = kOS2VX_BitmapHeaderType; Since this type is unsupported, can you return here? Or is it possible that one of the statements below will correct it? (If the latter, can we rearrange things so that we don't reach this check if it would've been corrected anyway?) https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:207: SkASSERT(infoBytesRemaining >= 12); Again, we need to handle the case where infoBytesRemaining was not large enough. It looks like if a file reported infoBytes < 36 (in this case), we can return kInvalidInput. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:250: if (width <= 0 || width > kBmpMaxDim || !height || height > kBmpMaxDim) { Is it possible for width or height to be greater than kBmpMaxDim? Even though they are ints, you created them with get_short, which returns a uint16_t. Isn't kBmpMaxDim 17 bits? https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:256: uint32_t redMask = 0, greenMask = 0, blueMask = 0, alphaMask = 0; Why not use an SkSwizzler::ColorMask here? https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:265: SkASSERT(infoBytesRemaining > 52); My mistake. I suggested using a compile assert (which is SK_COMPILE_ASSERT; SkASSERT is a runtime assert), but you actually need to do a check for invalid data here, since infoBytesRemaining is determined by the input. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:357: } Nit: It was hard for me to notice this fall through. Typically we document fall throughs (except in "trivial" cases, whatever that means), although it seems like we could just put a break here (since it's what we fall through to). https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:371: SkSwizzler::ColorMasks* masks = SkNEW(SkSwizzler::ColorMasks); I'd recommend stack allocating this, and copy constructing it to the SkBmpCodec. (This will prevent leaks down below. Plus it's only four ints.) https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:388: uint32_t colorBytes = numColors * bytesPerColor; Can be const? https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:389: SkPMColor* colorTable = SkNEW_ARRAY(SkPMColor, numColors); Any reason you're not using an SkColorTable? Also, this gets leaked if we return NULL a few lines below. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:392: SkNEW_ARRAY(uint8_t, colorBytes)); nit: I think this could fit on one line. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:511: const uint32_t alphaMask = fBitMasks.get()->alphaMask; FYI: SkAutoTDelete defines operator->, so this can be written as fBitMasks->alphaMask https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:548: SkDebugf("Error: default case should be unreachable.\n"); I'd put an SkASSERT(false), if it should never be reached. That will alert us to the fact that we were wrong - it was reachable. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:565: fInverted ? SkSwizzler::kBottomUp : SkSwizzler::kTopDown); I'd recommend storing fInverted as this enum. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:590: void SkBmpCodec::setPixel(SkPMColor* dst, uint32_t dstRowBytes, int height, Maybe this should be called setRLEPixel. Also, not a big deal, but I would assert that fInputType is something that expects RLE. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:591: uint32_t x, uint32_t y, uint8_t index) { nit: should line up with SkPMColor. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:597: return; Not needed. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:633: // If the code hits EOL or EOF early, remaining pixels are transparent To be fair, they're only transparent in the case of a type with transparency. As an aside, Android often allocates their memory from the Java heap, meaning that it was already zeroed. I haven't written an API for that yet, but maybe add a TODO here to skip this if the memory was already zeroed. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:675: x += dx; Nit: this is probably premature optimization on my part, but why not instead do the following: x += dx; y += dy; if (x > width || y > height) { return kInvalidInput; } https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:695: val >> 4); nit: This should line up with dstPtr https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:723: const int endX = std::min(x + count, (uint32_t) width); Nit: We typically use our own min functions, defined in SkTypes.h https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:728: uint8_t indexes[2] = { code, code }; nit: indices? https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.h:31: static bool IsBmp(SkStream* stream); Nit: typically I think we leave out the parameter name from a declaration if it does not provide any information. (Sorry, I could've been more clear when I requested you use names. Names are useful when it's something like size_t dstRowBytes but not needed when it's something like ClassName className.) https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.h:99: SkSwizzler::ColorMasks* bitMasks, SkPMColor* colorTable, Could you add some comments describing which of these parameters pass ownership to the SkBmpCodec? Also some comments describing what they mean? In particular, inverted, which I think means in y (and maybe it should be an enum, in which case the names should serve as documentation), and remainingBytes (in the stream? How do we know?). https://codereview.chromium.org/947283002/diff/60001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/60001/src/codec/SkSwizzler.h#n... src/codec/SkSwizzler.h:52: kTopDown, Style nit: In Skia, we append the enum type name to each enum. So this should be: kTopDown_RowOrder etc. https://codereview.chromium.org/947283002/diff/80001/src/codec/SkCodec_libbmp... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/80001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:431: // color table with black. How did you decide on black? Maybe it's correct, but I know that in PNG, we decided to duplicate the last color. (I don't know whether that is generally correct either, but it'd be nice to know why we picked black.) https://codereview.chromium.org/947283002/diff/80001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:433: colorTable[i] = SkPreMultiplyColor(SkColorSetARGBInline(0xFF, FWIW, SkColorSetARGBInline(0xFF,0,0,0) can be replaced with SK_ColorBLACK. That said, in this case, the result of premultiplication is obvious, so you could just say: colorTable[i] = SkPackARGB32(0xFF, 0, 0, 0) https://codereview.chromium.org/947283002/diff/80001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:439: colorBytes = numColors * bytesPerColor; If bitsPerPixel >= 16, we don't do a safety check for numColors? What if it is max int? Is that valid? (Or negative?) https://codereview.chromium.org/947283002/diff/80001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:505: case kStandard_BitmapInputFormat: { Why are the braces necessary here? https://codereview.chromium.org/947283002/diff/80001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:771: SkColorSetARGBInline(0xFF, red, green, blue)); No need to use SkColorSet + SkPremultiply. This can be done in one call: SkPreMultiplyARGB
Testing Comments: Tried to test on file with no extension: DmSrcSink won't read it. On incomplete inputs (RLE without EOF marker or normal input that is missing rows), we do not return kSuccess and DmSrcSink does not draw the partial output. We should be able to check that partial outputs are correct. Current version does not crash on corrupt inputs and generally does a good job of correcting questionable inputs. In my test suite, it works for every image that chromium works for plus 1 or 2 others. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:133: SkASSERT(totalBytes > kBmpHeaderBytes + kBmpOS2V1Bytes); On 2015/02/27 17:04:01, scroggo wrote: > This is not a valid assert. > > Someone could create an invalid BMP file which stores a value here that is too > small. (I can't speak to BMP or this particular case, but in some cases we > deliberately allow certain kinds of bad data, since lots of images have some > mistakes.) If totalBytes is a number that we depend on, we need to handle it > gracefully (i.e. not crash). > > The assert I was looking for is something like > > SK_COMPILE_ASSERT(2 + 4 <= kBmpHeaderBytes+4, invalid_read); > > i.e. that that read was safe to do. Agreed this has already started causing a problem. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:178: headerType = kOS2VX_BitmapHeaderType; On 2015/02/27 17:04:03, scroggo wrote: > Since this type is unsupported, can you return here? Or is it possible that one > of the statements below will correct it? (If the latter, can we rearrange things > so that we don't reach this check if it would've been corrected anyway?) This type is supported. You have caught a bug. Right now, it will get set to unknown type below, but it should not. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:207: SkASSERT(infoBytesRemaining >= 12); On 2015/02/27 17:04:01, scroggo wrote: > Again, we need to handle the case where infoBytesRemaining was not large enough. > It looks like if a file reported infoBytes < 36 (in this case), we can return > kInvalidInput. infoBytesRemaining is guaranteed to be large enough. We do not enter this if statement unless infoBytes >= 16 which indicates infoBytesRemaining >= 12. Not sure if the assert is necessary though. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:250: if (width <= 0 || width > kBmpMaxDim || !height || height > kBmpMaxDim) { On 2015/02/27 17:04:01, scroggo wrote: > Is it possible for width or height to be greater than kBmpMaxDim? Even though > they are ints, you created them with get_short, which returns a uint16_t. Isn't > kBmpMaxDim 17 bits? Yes. One version of the header reads them as int16. Later versions provide them as int32. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:265: SkASSERT(infoBytesRemaining > 52); On 2015/02/27 17:04:03, scroggo wrote: > My mistake. I suggested using a compile assert (which is SK_COMPILE_ASSERT; > SkASSERT is a runtime assert), but you actually need to do a check for invalid > data here, since infoBytesRemaining is determined by the input. In this situation, we are guaranteed to have enough infoBytesRemaining because infoBytes matches this header size. Still not sure if that means there should be an assert, check, or neither.
On 2015/02/27 21:17:29, msarett wrote: > Testing Comments: > > Tried to test on file with no extension: DmSrcSink won't read it. This is actually part of DM.cpp, so we can have a single folder with images and other files, but non image files will be ignored. If we want to test images with no extension, we can always add extensions. > On incomplete inputs (RLE without EOF marker or normal input that is missing > rows), we do not return kSuccess and DmSrcSink does not draw the partial output. > We should be able to check that partial outputs are correct. That's an easy fix: just change ImageSrc to consider kIncompleteInput as well. > Current version does not crash on corrupt inputs and generally does a good job > of correcting questionable inputs. Great! It might be worth putting corrupt inputs in resources/invalid_images for testing. > In my test suite, it works for every image > that chromium works for plus 1 or 2 others. Also great! Where did you find them? If you're looking for more, you might be able to find some with render_pictures. I can show you how to do that. Keep track of your test suite so we can run them on our bots. https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:178: headerType = kOS2VX_BitmapHeaderType; On 2015/02/27 21:17:28, msarett wrote: > On 2015/02/27 17:04:03, scroggo wrote: > > Since this type is unsupported, can you return here? Or is it possible that > one > > of the statements below will correct it? (If the latter, can we rearrange > things > > so that we don't reach this check if it would've been corrected anyway?) > > This type is supported. You have caught a bug. Right now, it will get set to > unknown type below, but it should not. I'm confused, in patch set 6, it looks like we return NULL on line 346 for kOS2VX_BitmapHeaderType, or is that only for certain compression methods? https://codereview.chromium.org/947283002/diff/60001/src/codec/SkCodec_libbmp... src/codec/SkCodec_libbmp.cpp:250: if (width <= 0 || width > kBmpMaxDim || !height || height > kBmpMaxDim) { On 2015/02/27 21:17:28, msarett wrote: > On 2015/02/27 17:04:01, scroggo wrote: > > Is it possible for width or height to be greater than kBmpMaxDim? Even though > > they are ints, you created them with get_short, which returns a uint16_t. > Isn't > > kBmpMaxDim 17 bits? > > Yes. One version of the header reads them as int16. Later versions provide > them as int32. Ah, my bad - I missed the other assignment - using get_int. https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:225: SkASSERT(infoBytesRemaining >= 12); Could you add a comment explaining why you know this to be true? I had to search to figure it out. https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:233: if (infoBytesRemaining >= 16) { If this is not true, can we go ahead and return? https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:290: SkASSERT(infoBytesRemaining > 52); (I think this comment corresponds to this code:) On 2015/02/27 21:17:28, msarett wrote: > In this situation, we are guaranteed to have enough infoBytesRemaining because > infoBytes matches this header size. Still not sure if that means there should > be an assert, check, or neither. If it's guaranteed, and it must be true in order to do the read, I say assert. That way if someone else (including future you) modifies this file later, they know the call to get_int is safe (and if they change things so it's not, the assert can help us discover that). Also, please add a comment with your statement about infoBytes matching the header size. That's not easy to see from looking at this code. https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:455: if (numColors <= 1 << 8) { Nit: I find this more legible with parentheses: (1 << 8) Also, maybe a note explaining where this number comes from? https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:466: if (bytesRead <= offset) { nit: This is more readable if you reverse the if: if (bytesRead > offset) { return NULL; } if (stream->skip... https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:761: SkPMColor color = SkPackARGB32( I think this one should be SkPreMultiplyARGB (Pack doesn't multiply the alpha). https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:782: const uint32_t endX = SkTMin<uint32_t>(x + count, (uint32_t) width); Do you still need algorithm? (I assume you had it for min?)
Minor dm testing changes https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/100001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:233: if (infoBytesRemaining >= 16) { On 2015/02/28 00:25:13, scroggo wrote: > If this is not true, can we go ahead and return? We can't. There are valid images without this header field. It will default to uncompressed.
Improved clarity of swizzler with regard to weird alpha. We could possibly improve performance if necessary by increasing the number of row procs, but I think this a good compromise with code that is somewhat understandable.
https://codereview.chromium.org/947283002/diff/140001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/947283002/diff/140001/dm/DMSrcSink.cpp#newcode70 dm/DMSrcSink.cpp:70: if (result == SkImageGenerator::kIncompleteInput) { I think this would read better as a switch statement. That said, I'm not sure what the result of drawing to the bitmap and reporting an error is (as opposed to one or the other). https://codereview.chromium.org/947283002/diff/140001/dm/DMSrcSink.cpp#newcode71 dm/DMSrcSink.cpp:71: error = SkStringPrintf("Warning incomplete getPixels %s.", fPath.c_str()); I'm not sure what the right behavior is here. Note that DM does not distinguish between errors and warnings. So these will be reported as failures. If we expect to run our tests on incomplete images, this will mean we'll turn the bot red (and make people think there is a problem with the code). On the other hand, these are potentially worth investigating, since the decoder *could* have a bug leading to only partially decoding. It may just be enough to look at the results in Gold, and if the image is incorrectly incomplete, that tells us we have a problem. https://codereview.chromium.org/947283002/diff/140001/dm/DMSrcSink.cpp#newcode75 dm/DMSrcSink.cpp:75: error = ""; Maybe set error to "" initially? https://codereview.chromium.org/947283002/diff/140001/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/947283002/diff/140001/gyp/codec.gyp#newcode23 gyp/codec.gyp:23: #'../src/codec/SkCodec_libpng.cpp', Note: This should no longer be necessary. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:10: #include "SkCodec_libbmp.h" nit on the file name: SkCodec_libpng was named that because it uses a library called libpng. Here, you are not using libbmp, so this name seems like a misnomer. (To be fair, I actually wrote SkImageDecoder_libico.cpp, which is similarly misnamed.) Maybe I shouldn't have followed the old convention; it might be better to put these classes in files whose names match the classes (SkPngCodec.h, SkBmpCodec.h etc). FWIW, I don't think you should change it right now (it'll make it hard to follow the diff from version to version), but maybe add a TODO. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:130: //uint16_t signature = get_short(hBuffer, 0); Can these be removed? https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:263: static const uint32_t kBmpMaxDim = 1 << 16; nit: This could be after the next if statement (it's not used until then). https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:277: masks.redMask = 0; Maybe ColorMasks should have a constructor? (Or we can use memset here?) https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:447: i = -1; don't you want a 'continue' here? Otherwise, won't you be modifying colorTable[-1] incorrectly? https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:450: colorTable[i] = SkPreMultiplyColor(SkColorSetARGBInline(alpha, As stated in an earlier patch set, you don't need this two step (create an SkColor, convert to SkPMColor). You can just create an SkPMColor. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:511: kN32_SkColorType, kPremul_SkAlphaType); At this point, isn't it possible to know (at least for some formats) that the image is opaque? If so, you should use kOpaque. Also, FWIW, the plan is to make unpremul the default for Android. That doesn't mean we have to make it the default on our end, although we should be consistent. For SkPngCodec, I made unpremul the default, since that actually matches the data better. The downside to the default of unpremul is that Skia currently doesn't support drawing unpremul (but we plan to). https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:546: if (dstInfo.dimensions() != getOriginalInfo().dimensions()) { You'll need to do other checks, for example whether the alpha type is correct, and check for a rewind. See SkPngCodec for examples (and maybe that should be moved into a common place). https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:558: SkDebugf("Error: unknown bitmap input format.\n"); When does this happen? Can we skip creating an SkBmpCodec in this case? https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:577: const uint32_t paddedRowBytes = (unpaddedRowBytes + 3) & (~3); You're probably not familiar with it, but I believe this is the same as SkAlign4 (in SkTypes.h)? https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:691: uint32_t i = 0; It looks like in this case, "i" means "bytesRead"? I think the rest of this method would be clearer with that name instead of i. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:705: // TODO: Skip this is memory was already zeroed. if* https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:717: const uint8_t count = buffer.get()[i++]; Can you elaborate what these variables mean? IIUC, "count" tells us something like how many pixels or bytes to read, with RLE_ESCAPE being a sentinel. And code means some kind of code? Anywhere understanding what's going on requires having studied the bmp spec would be a good place to add comments. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:719: if ((count || (code != RLE_EOF)) && y >= height) { I'm having trouble following this line. Is it basically saying "if count/code says we're not done, but y says we are, the input is malformed?" Also, I think by treating count as a boolean you'll get a warning (which we turn into an error) on Windows. I also think it's clearer to use a comparison. (You use this pattern in a number of other places, e.g. when checking the alpha, where I think it would be clearer and warning free to say what you mean.) https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:757: (unpaddedBytes + 1) & (~1); I think you can use SkAlign2 here. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:758: if (x + code > width || totalBytes - i < paddedBytes) { So in this case, does code mean a number of pixels? https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:14: #ifdef SK_SUPPORT_LEGACY_IMAGE_GENERATOR_RETURN No longer needed https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:100: passes ownership to SkBmpCodec no longer true. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:101: * @param colorTable color table for index-based bmp formats nit: Maybe it's obvious, but this doesn't mention that it's an array? https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:225: uint32_t redMask = masks.redMask; Would it be possible to make a function that looks something like the following?: static inline void mask_color(uint8_t* a, uint8_t* r, uint8_t* g, uint8_t* b, const ColorMasks& masks) { ... } This seems like it would simplify your swizzle functions. Maybe this would mean recalculating a bunch of things. If so, maybe we could modify the ColorMask to have some member functions that do things ahead of time (or even do the calculations in the constructor). I'm guessing that the mask info will always be the same for a single ColorMask? https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:251: return *seenNonZeroAlphaPtr; I think this is not want you want to return. The functions are supposed to return the following: true : If anything on this line had some alpha less than 100% (e.g. 255 for 8 bit alpha) false : Everything on this line has 100% alpha IIUC, seenNonZeroAlpha returns: true: Something had an alpha > 0 false : Everything had an alpha of 0 (In addition, it seems to include prior lines, which is also different from the intended return value.) The general decoders only care about the former. Maybe there's a way to get both? What if we used a bit field, where part of it tells us if there was a non-zero alpha, and part tells whether there was a non-100% alpha? Then could we get rid of the in/out parameter seenNonZeroAlphaPtr? (Or maybe there's a better way?) https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:263: bool* zeroPrevRowsPtr) { I'm trying to figure out how we can cut down on the parameters passed to this function. Let me see if I understand how zeroPrevRowsPtr: It is only an out parameter. We set it to true if 1) all previous rows had an alpha of zero. and 2) this row had an alpha that was not zero. Then, next will set all the previous rows to zero. Does that sound correct? Is it redundant with seenNonZeroAlphaPtr? It seems like the value for one can tell us the meaningful value for the other? https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:633: if (0 == fCurrY && kBottomUp_RowOrder == fRowOrder) { I think the bottom up row order is only going to be used by BMP. (Though I could be wrong.) That said, GIF also has a mode that decodes in an order besides top down - it is interlaced. Maybe instead of providing the option to go in reverse order, add a function that can be called instead of next, where the caller supplies the y value. SkBmpCodec can take care of the loop going from height down to zero, and then we can reuse the same function for GIF. Take a look at SkScaledBitmapSampler::sampleInterlaced. This function should be simpler (no need to worry about sampling), but work somewhat similarly. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:646: if (fZeroPrevRows) { Some thoughts: 1) I think this behavior is exclusive to BMP. Can we move it into SkBmpCodec? 2) It looks like you zero out every single value on previous rows. This assumes premultiplied, and we need to support unpremul for Android. For normal drawing, premultiplying the alpha, when 0, is the same as unpremul, but if an SkXfermode is used, then I believe it will be different. 3) I wonder if it would be better to make SkBmpCodec do the correction after the fact (simplifying this function and the row procs). Or, we could switch row procs once we've seen a non zero alpha (though that may make things much more complicated). https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:58: * TODO: should we consider merging this SkAlphaType? I don't think so. (I don't think we'll want to include kTransparentAsOpaque - it would essentially be kIgnore, which Mike just removed.) Maybe we should just use SkAlphaType here instead of AlphaOption though. In that case, we would use kUnknown instead of kTransparentAsOpaque. We'd then set the true SkAlphaType at the end. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:60: * kNormal_AlphaOption corresponds to kPremul_SkAlphaType Keep in mind we want to support unpremul as well. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:118: const ColorMasks bitMasks = { 0, 0, 0, 0 }, I'm glad you're using the Swizzler, but: A lot of this stuff looks to be very specific to BMP. Might it be possible to pull the BMP specific stuff into SkBmpCodec? It seems like SkSwizzler has become much more complex, without necessarily adding things that will be shared by other decoders.
Oh, and one meta comment: NewFromStream is 400 lines decodeRLE is around 200 lines Would it be possible to break these functions into smaller functions? (Don't do it just because I said so though - if there are places where you can pull out common functionality so that it helps readability/isolates variables, it may be worth it.)
I think this iteration is a better balance between SkBmpCodec and the Swizzler. https://codereview.chromium.org/947283002/diff/140001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/947283002/diff/140001/dm/DMSrcSink.cpp#newcode70 dm/DMSrcSink.cpp:70: if (result == SkImageGenerator::kIncompleteInput) { On 2015/03/04 17:10:29, scroggo wrote: > I think this would read better as a switch statement. > > That said, I'm not sure what the result of drawing to the bitmap and reporting > an error is (as opposed to one or the other). Doesn't feel to me like a switch statement is necessary here now that we've revised the behavior to be simpler again. Let me know if you feel otherwise. https://codereview.chromium.org/947283002/diff/140001/dm/DMSrcSink.cpp#newcode71 dm/DMSrcSink.cpp:71: error = SkStringPrintf("Warning incomplete getPixels %s.", fPath.c_str()); On 2015/03/04 17:10:29, scroggo wrote: > I'm not sure what the right behavior is here. Note that DM does not distinguish > between errors and warnings. So these will be reported as failures. If we expect > to run our tests on incomplete images, this will mean we'll turn the bot red > (and make people think there is a problem with the code). > > On the other hand, these are potentially worth investigating, since the decoder > *could* have a bug leading to only partially decoding. It may just be enough to > look at the results in Gold, and if the image is incorrectly incomplete, that > tells us we have a problem. The decision here is to accept kSuccess or kIncompleteInput and draw without signaling an error. The decoder should print a warning before returning kIncompleteInput. Other return values will signal an error. It might be worth noting that a few paths in the bmp decoder are able to decode partial images before hitting an invalid input that forces the decode to halt (ex: invalid RLE code). I still return kIncompleteInput here (instead of kInvalidInput) because we want to signal that there is a valid partially decoded image. Let me know if this is confusing. https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/140001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:450: colorTable[i] = SkPreMultiplyColor(SkColorSetARGBInline(alpha, On 2015/03/04 17:10:30, scroggo wrote: > As stated in an earlier patch set, you don't need this two step (create an > SkColor, convert to SkPMColor). You can just create an SkPMColor. Sorry I missed this. Had the wrong code in so many places. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:56: fNeedsRewind = true; Why does this method set fNeedsRewind to true. It doesn't affect bmp, I'm just curious. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:361: SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) { This function is very long. There is not really repetitive code, but I could try to break it up: processHeader() processMasks() processColorTable() etc. My concern is that I would have to pass a ton of parameters to each call because most of the processing depends on the previous step. I plan to look into this tomorrow, but also feel free to let me know your thoughts. As far as the really long decodeRLE(), I think that function is best left alone. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:820: void SkBmpCodec::processMasks() { I can make this function less repetitive (but harder to understand) if I cast the structs to arrays and use a loop. Do you think this would be beneficial?
https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:361: SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) { On 2015/03/05 23:13:17, msarett wrote: > This function is very long. There is not really repetitive code, but I could > try to break it up: > processHeader() > processMasks() > processColorTable() etc. > My concern is that I would have to pass a ton of parameters to each call because > most of the processing depends on the previous step. > > I plan to look into this tomorrow, but also feel free to let me know your > thoughts. > I'm going to look at the whole of the code more in depth tomorrow, but I did want to stop you from digging down this hole. If you have to pass a ton of parameters to each function, that's a good sign you shouldn't break it up in that way. (At least to me; YMMV.) (As a side note, if the parameters are constants (e.g. kBmpHeaderBytes etc), those can just be made global. I'm guessing you're talking about more than passing constants, in which case my earlier statement holds.) > As far as the really long decodeRLE(), I think that function is best left alone.
On 2015/03/05 23:13:17, msarett wrote: > I think this iteration is a better balance between > SkBmpCodec and the Swizzler. Agreed. We may decide to break up SkCodec_libbmp (maybe put the functions that look like their own swizzler into a separate file? even on an SkBmpSwizzler?), but I'd hold off on that so it'll be easier to follow the diffs. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:27: const bool isPng = SkPngCodec::IsPng(stream); nit: PNG should be first, since we'll see more PNG images. That said, This function is going to quickly get out of hand. I think we should take some advice from the old method in SkImageDecoder_Factory. Something like: struct DecoderProc { bool (*IsFormat)(SkStream*); SkCodec* (*NewFromStream)(SkStream*); }; static const DecoderProc gDecoderProcs[] = { { SkPngCodec::IsPng, SkPngCodec::NewFromStream }, { SkBmpCodec::IsBmp, SkBmpCodec::NewFromStream } }; // Inside NewFromStream for (proc in gDecoderProcs) { const bool correctFormat = proc.IsFormat(stream); if (!stream->rewind()) { return NULL; } if (correctFormat) { return proc.NewFromStream(stream); } } return NULL; https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:56: fNeedsRewind = true; On 2015/03/05 23:13:17, msarett wrote: > Why does this method set fNeedsRewind to true. It doesn't affect bmp, I'm just > curious. I'm trying to make it so that a caller (for example a discardable pixel ref) can decode multiple times (assuming the stream supports that). This gets called in onGetPixels. If you call getPixels again, we will need to rewind the stream. Setting it to true means the next call will require a rewind. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:95: } else if (8 == n) { I would change this to: } else { SkASSERT(8 == n); return component; } (Maybe the compiler is smart enough to know the last else case will never be reached, so it doesn't need to do the if check in release, but I also think this is more clear.) https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:120: (p & masks.redMask) >> shifts.redShift, sizes.redSize); It looks like this same code is repeated a few times. Can we make it a function? Can the three BitMask objects be unified? Or do they mix and match? i.e. can we have a single BitMask object that contains all the information and does something like the following: uint8_t BitMask::getRed(uint16_t p) { return convert_n_to_8((p & fRedMask) >> fRedShift, fRedSize); } https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:125: dstPtr[i] = SkColorSetARGBInline(0xFF, red, green, blue); This doesn't do what we want. We want either premultiplied colors, or unpremultiplied colors (masquerading as SkPMColor, but treating the values as unpremultiplied), not SkColors. In this case, we know it's opaque, so premultiplication is irrelevant. So you can use SkPackARGB32NoCheck https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:144: uint8_t zAlpha = 0; What does zAlpha mean? zeroAlpha? https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:145: uint8_t mAlpha = 0xFF; I think this is max alpha? Refrain from using mVariable for local variables. It looks like a member variable (at least to someone who sometimes programs in Java - others probably use this pattern for members, too). https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:160: return (mAlpha == 0xFF) ? SkSwizzler::kOpaque_ResultAlpha : Can you make a helper function for this? Something like: ResultAlpha getResult(uint8_t zeroAlpha, uint8_t maxAlpha); (I'd like to do some tricks to get rid of the if checks if we can, but making it a function is a good start - then we can only change it once.) https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:295: // TODO: Support all conversions nit: we won't support *all* conversions, but perhaps to 565. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:299: if (kIgnore_SkAlphaType == dst.alphaType()) { Can we also check for opaque vs non-opaque? For PNG, I decided that if the image reports that it is opaque, we consider it opaque, and if the caller requests premul/unpremul, we return false. To be fair, in that case, we could have just said "Sure, you can treat that as unpremul/premul!" - they'll pay a penalty when it's time to draw, but maybe that's on them (the current implementation hopefully means they'll catch their error earlier, and never suffer the penalty). More interestingly, if the image reports that it's not opaque, we don't allow the caller to request decoding to opaque. This is because we want them to use an alpha type that supports the actual data. This of course has its own downside - an image may report that it's not opaque, but then later have only opaque pixels. I think we should be consistent across decoders, but I'm starting to wonder if I made the wrong decision. It was partly influenced by SkImageDecoder, where the client requested a particular Config, and we didn't use that if it didn't make sense. But SkCodec has a different model; it tells them the correct "Config" (color type and alpha type) to use. If they choose a different one, maybe we should honor that, even if they're wrong? (And maybe they know better than us - they could know the image has no alpha, even though it reported that it did? Of course, with such control, maybe they should have written the encoded image better?) This model does make testing trickier though. We don't want to compare drawing an image with alpha as opaque to drawing it with alpha... https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:612: // Most versions of bmps should to be rendered as opaque. Either they do should be* https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:623: // V4+, we are guaranteed to be able at least this size. able... to read? https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:661: SkColor* colorTable = NULL; SkPMColor. Same down below. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:789: SkDebugf("Error: rewind should not be necessary.\n"); This comment isn't necessary. couldRewindIfNeeded should never return true the first time; this is just for attempting to decode again. In that case, rewinding is absolutely necessary. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:820: void SkBmpCodec::processMasks() { On 2015/03/05 23:13:17, msarett wrote: > I can make this function less repetitive (but harder to understand) if I cast > the structs to arrays and use a loop. Do you think this would be beneficial? Hard to say... My initial thought was to let timing/profiling tell us where we need to optimize, but this code appears to do the same thing four times in a row. If there was a copy/paste error it seems like it would be hard to find it... I'll be curious what the array function looks like. Another thing that might be interesting is using Sk4x, although maybe we can't use it here since we make different decisions for each component. At the very least, it seems like we should use a macro/function for each component. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:822: if (fBitsPerPixel < 32) { If you merged all the masks together, as suggested elsewhere, could this be a function on BitMasks? It looks like the only reason you need the SkBmpCodec to get fBitsPerPixel, which could be passed as a parameter. (You also currently need fBitMasks etc, but after a merge you have all that.) https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:888: return; not needed. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:902: const uint32_t bytesPerPixel = fBitsPerPixel / 8; what happens if fBitsPerPixel is not an even multiple of 8? Does that ever happen? https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:903: const uint32_t unpaddedRowBytes = fBitsPerPixel < 16 ? I think all you want here is the paddedRowBytes? And you have some similar calculations down below? I think this is a perfect place for a function: size_t compute_row_bytes(uint32_t bitsPerPixel, int width) { size_t unpaddedRowBytes; if (fBitsPerPixel < 16) { // compute unpaddedRowBytes using pixelsPerByte } else { // compute unpaddedRowBytes using bytesPerPixel } return SkAlign4(unpaddedRowBytes); } (Typically we use size_t to a represent number of bytes, hence the return value of size_t.) https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:972: transparent &= SkSwizzler::kTransparent_ResultAlpha == r; nit: parens around the == check https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:1053: // Read the next two bytes. These bytes have different meanings Thanks for the comments! It makes this code a lot easier to follow :) https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:94: size_t dstRowBytes, SkColor*, I think this happens to compile because SkColor and SkPMColor are defined to the same thing. This is incorrect though; SkColor implies a particular order no matter what, whereas SkPMColor implies an ordering that can be modified with compile flags. Originally, SkPMColor also meant PreMultiplied. Currently we still use SkPMColor when we're talking about unpremultiplied colors (but it still uses the compile flag specified ordering). https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:132: void setRLEPixel(SkColor* dst, uint32_t dstRowBytes, int height, SkPMColor (I think) https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:448: const int bpp = SkSwizzler::BitsPerPixel(sc) / 8; This makes me nervous that we've messed up if BitsPerPixel was *not* divisible by 8. Can we make this a helper function in SkSwizzler?: int BytesPerPixel(SrcConfig sc) { SkASSERT(SkIsAlign4(BitsPerPixel(sc))); return BitsPerPixel >> 3; } https://codereview.chromium.org/947283002/diff/160001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:89: ResultAlpha next(const uint8_t* SK_RESTRICT src); I think the PNG decoder needs to update the way it handles the result of next? https://codereview.chromium.org/947283002/diff/160001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:97: ResultAlpha next(const uint8_t* SK_RESTRICT src, int32_t delta); This interface is going to be hard for our SkGifCodec to use. Why not pass a y instead of delta? https://codereview.chromium.org/947283002/diff/160001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:104: * @param bpp bytes per pixel of the source. No longer bytes. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:121: SkSwizzler(RowProc proc, const SkPMColor* ctable, int srcBPP, Why did this change?
I think this version is looking pretty good. No worries if you can't get to the code review until later next weekend. I have plenty to do. https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/160001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:902: const uint32_t bytesPerPixel = fBitsPerPixel / 8; On 2015/03/06 18:56:13, scroggo wrote: > what happens if fBitsPerPixel is not an even multiple of 8? Does that ever > happen? It would an error if it happened. We should probably assert that it won't happen. https://codereview.chromium.org/947283002/diff/180001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/947283002/diff/180001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:118: if (FLAGS_codec) { I couldn't figure out how to get rid of these changes since they are already committed.
msarett@google.com changed reviewers: + djsollen@google.com
Adding Derek
High level comment: When you've made a change in response to one of my comments (or if I'm wrong, so no change was needed), please reply to the comment. It helps me know what's changed and what are the important bits to look at. This is especially important in a really large change like this one, as well as when the code moves. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:82: SkASSERT(0 == 8 % bitsPerPixel); This could be done once outside the if statement. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:347: masks.red = get_int(mBuffer.get(), 0); Could any of these values be invalid? Or are all values of int acceptable? (Or do we check later?) https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:409: // V4+, we are guaranteed to be able ro read at least this size. to* read https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:607: const size_t paddedRowBytes = SkAlign4( nit: Now that unpaddedRowBytes is no longer in this function, you can just call this "rowBytes" (we typically call this rowBytes, and it is padded if necessary). Also, I think this can go on one line (esp with the new name). https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:652: dstPtr[y * dstRowBytes + x] |= 0xFF000000; nit: We could probably do this faster without doing a multiply on each iteration (and instead do something like the Swizzler does - add the row bytes only to jump rows and add the width of a pixel for each x value). https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:765: const size_t unpaddedBytes = compute_row_bytes(numPixels, How about "rowBytes"? https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:769: totalBytes - currByte < paddedBytes) { Can you explain this check? I'm having trouble following it. Also, it seems like you could remove the variable "paddedBytes" and inline it here. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:875: const size_t paddedRowBytes = SkAlign4( nit: rowBytes https://codereview.chromium.org/947283002/diff/180001/src/codec/SkMaskSwizzle... File src/codec/SkMaskSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/180001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.cpp:84: uint8_t red = get_comp(p, masks.red.mask, masks.red.shift, get_comp is a step in the right direction. What if Masks had some member functions: Masks { ... uint8_t getRed(uint16_t p) { return getComp(red.mask, red.shift, red.size); } private: static uint8_t getComp(...); } https://codereview.chromium.org/947283002/diff/180001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/180001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:106: * It is very important to only use one version of next. Since the other In SkScaledBitmapSampler, we use an enum in debug mode to assert that we are only using one version. It might be good to do that here as well.
Thanks for the feedback! I just rebased everything and it caused a huge mess. I'm not sure the new version will be particularly soon, but I will respond to Leon's most recent comments and upload a resolved version.
Here is the new, improved, and rebased version! Sorry about not responding to comments in the past. Hope this makes it a little easier to see my corrections. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:82: SkASSERT(0 == 8 % bitsPerPixel); On 2015/03/11 14:23:10, scroggo wrote: > This could be done once outside the if statement. The assert is actually slightly different. 8 % bpp versus bpp % 8 https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:347: masks.red = get_int(mBuffer.get(), 0); On 2015/03/11 14:23:10, scroggo wrote: > Could any of these values be invalid? Or are all values of int acceptable? (Or > do we check later?) In this version, minimal checking was performed. We now trim masks to bitsPerPixel size (ex: in 24 bpp, we discard the top 8 bits), check that masks do not overlap, and issue a warning if masks are continuous (I don't think this is technically illegal). This is all done in a call to CreateMasks on line 444. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:409: // V4+, we are guaranteed to be able ro read at least this size. On 2015/03/11 14:23:10, scroggo wrote: > to* read Fixed https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:607: const size_t paddedRowBytes = SkAlign4( On 2015/03/11 14:23:09, scroggo wrote: > nit: Now that unpaddedRowBytes is no longer in this function, you can just call > this "rowBytes" (we typically call this rowBytes, and it is padded if > necessary). > > Also, I think this can go on one line (esp with the new name). You are correct. Fixed. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:652: dstPtr[y * dstRowBytes + x] |= 0xFF000000; On 2015/03/11 14:23:09, scroggo wrote: > nit: We could probably do this faster without doing a multiply on each iteration > (and instead do something like the Swizzler does - add the row bytes only to > jump rows and add the width of a pixel for each x value). Agreed. Done. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:765: const size_t unpaddedBytes = compute_row_bytes(numPixels, On 2015/03/11 14:23:10, scroggo wrote: > How about "rowBytes"? Done. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:769: totalBytes - currByte < paddedBytes) { On 2015/03/11 14:23:10, scroggo wrote: > Can you explain this check? I'm having trouble following it. > > Also, it seems like you could remove the variable "paddedBytes" and inline it > here. I added a comment. // Abort if setting numPixels moves us off the edge of the // image. Also abort if there are not enough bytes // remaining in the stream to set numPixels. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:875: const size_t paddedRowBytes = SkAlign4( On 2015/03/11 14:23:10, scroggo wrote: > nit: rowBytes Done. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkMaskSwizzle... File src/codec/SkMaskSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/180001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.cpp:84: uint8_t red = get_comp(p, masks.red.mask, masks.red.shift, On 2015/03/11 14:23:10, scroggo wrote: > get_comp is a step in the right direction. > > What if Masks had some member functions: > > Masks { > ... > uint8_t getRed(uint16_t p) { > return getComp(red.mask, red.shift, red.size); > } > private: > static uint8_t getComp(...); > } Agreed this is done. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/180001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:106: * It is very important to only use one version of next. Since the other On 2015/03/11 14:23:10, scroggo wrote: > In SkScaledBitmapSampler, we use an enum in debug mode to assert that we are > only using one version. It might be good to do that here as well. That seems like a good solution. I added that here.
Agreed that this is the right direction. I have a few more comments, and then the code is good - at least the code I can keep all in my head at once. After this stuff is fixed, I plan to look over the whole CL again and see if anything jumps out at me that we've missed. https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/180001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:82: SkASSERT(0 == 8 % bitsPerPixel); On 2015/03/11 18:53:05, msarett wrote: > On 2015/03/11 14:23:10, scroggo wrote: > > This could be done once outside the if statement. > > The assert is actually slightly different. > 8 % bpp versus bpp % 8 D'oh! Nvm. https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:446: SkMasks* masks = SkMasks::CreateMasks(inputMasks, bitsPerPixel); This can be leaked from one of our debug statements. https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:818: if (rowBytes & 1) { SkIsAlign2? https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:116: * @param bitMasks optional color masks for certain bmp formats masks* Also, can you add a comment about ownership? It looks like we currently leak masks. https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMaskSwizzler.h File src/codec/SkMaskSwizzler.h (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.h:27: SkMasks* masks, Who owns masks? https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMasks.cpp File src/codec/SkMasks.cpp (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMasks.cpp#n... src/codec/SkMasks.cpp:88: const SkMasks::MaskInfo SkMasks::ProcessMask(uint32_t mask, uint32_t bpp) { This can be a static function local to this file. https://codereview.chromium.org/947283002/diff/240001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:124: enum SwizzleMode { I think this name is not very informative. How about something like NextMode or RowMode? (And I just realized this is based off the similarly poorly named SkScaledBitmapSampler::SampleMode...)
I added fixes from your latest comments. Hopefully I'm getting better at using AutoTDelete and thinking about who owns pointers - thanks for catching this for the millionth time. I agree that this needs one final look through from me as well as you. Feel free to hold off until tomorrow because I think I will (I need a break from this code today!). https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:446: SkMasks* masks = SkMasks::CreateMasks(inputMasks, bitsPerPixel); On 2015/03/11 19:22:45, scroggo wrote: > This can be leaked from one of our debug statements. Got it. I will try to fix this. https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:818: if (rowBytes & 1) { On 2015/03/11 19:22:45, scroggo wrote: > SkIsAlign2? Yes that would be the better choice! https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:116: * @param bitMasks optional color masks for certain bmp formats On 2015/03/11 19:22:45, scroggo wrote: > masks* > > Also, can you add a comment about ownership? It looks like we currently leak > masks. You are correct. I made fMasks an AutoTDelete and designed the pointer as owned by SkBmpCodec. https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMaskSwizzler.h File src/codec/SkMaskSwizzler.h (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.h:27: SkMasks* masks, On 2015/03/11 19:22:45, scroggo wrote: > Who owns masks? SkBmpCodec owns masks. I will make that clear! https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMasks.cpp File src/codec/SkMasks.cpp (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMasks.cpp#n... src/codec/SkMasks.cpp:88: const SkMasks::MaskInfo SkMasks::ProcessMask(uint32_t mask, uint32_t bpp) { On 2015/03/11 19:22:45, scroggo wrote: > This can be a static function local to this file. Done. Might be worth noting that this change required making struct MaskInfo into a public type. https://codereview.chromium.org/947283002/diff/240001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:124: enum SwizzleMode { On 2015/03/11 19:22:45, scroggo wrote: > I think this name is not very informative. How about something like NextMode or > RowMode? (And I just realized this is based off the similarly poorly named > SkScaledBitmapSampler::SampleMode...) Almost went with NextMode, and then I tried to be consistent with the old code (bad decision). Happy to change this.
I still need to look over SkSwizzler one last time in its entirety, but the rest looks good modulo the below comments. https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMasks.cpp File src/codec/SkMasks.cpp (right): https://codereview.chromium.org/947283002/diff/240001/src/codec/SkMasks.cpp#n... src/codec/SkMasks.cpp:88: const SkMasks::MaskInfo SkMasks::ProcessMask(uint32_t mask, uint32_t bpp) { On 2015/03/11 20:03:11, msarett wrote: > On 2015/03/11 19:22:45, scroggo wrote: > > This can be a static function local to this file. > > Done. Might be worth noting that this change required making struct MaskInfo > into a public type. Ah, that might be a good enough reason to make it a member static function... Sorry I overlooked that :( FWIW, it may be "public", but we consider anything in src/ to not be a part of our API, which I believe this still is. I'm fine with it being that type of public. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:15: * Get a byte from the buffer Maybe add a comment that these are unsafe, and assume the caller did the real check? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:52: static bool premul_and_unpremul(SkAlphaType dst, SkAlphaType src) { // TODO: Share with other codecs (unless this is different in some way I've missed?) https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:265: //uint16_t planes = get_short(iBuffer, 8); Any reason to leave this here? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:276: if (infoBytesRemaining >= 32) { Maybe this should go inside the previous if statement? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:357: // is V2+, we are guaranteed to be able at least this size. able to read* https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:485: colorTable[i] = SkPackARGB32NoCheck(alpha, red, green, blue); So these colors are unpremultiplied? Can you add a comment (maybe by the declaration of colorTable)? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:503: // value of numColors greater than this maximum is invalid. If numColors is greater than the maximum, is the rest of the image likely to be valid? Maybe there's not a good way to determine that... https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:786: uint8_t val = buffer.get()[currByte++]; Do we need to do a check to make sure currByte is still in range? (Or did some previous check account for this one?) Same questions for below. Oh, wait, we're guaranteed that we'll only read rowBytes bytes in this while loop? Maybe add an SkASSERT that we know these reads are safe. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:809: dstPtr, y * dstRowBytes); y is unchanged in the loop, correct? Can we do this multiply only once? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:888: switch (fBitsPerPixel) { It's ironic that here we use fBitsPerPixel to determine the SrcConfig, and SkSwizzler has a function that takes the SrcConfig and returns the bitsPerPixel. Would it make sense to make the two independent? Not completely, obviously, but I wonder if it would make sense to just have kIndex, and let the caller separately specify the bitsPerPixel. (If so, we can land that as a separate CL.) https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:940: transparent &= (SkSwizzler::kTransparent_ResultAlpha == r); Maybe the parenthetical bit should be a helper function? If/when we change the way ResultAlpha is stored we can then continue using that function without modifying callers. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkMaskSwizzle... File src/codec/SkMaskSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/260001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.cpp:53: dstPtr[i] = SkPackARGB32NoCheck(alpha, red, green, blue); I think this is wrong. Is the intention to premultiply or not? If not, maybe make the name specify that it does not? (Is there a different version for premultiplied?) https://codereview.chromium.org/947283002/diff/260001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.cpp:98: dstPtr[i/3] = SkPackARGB32NoCheck(alpha, red, green, blue); Again, this is only valid for unpremultiplied. (or opaque, if alpha is FF)
Biggest Changes: Added shared code to SkCodec.h Added methods to SkSwizzler.h to help callers interpret ResultAlpha Modified libpng to correctly interpret the result of a call to next. (This change had already existed but seems to have been lost in the rebase.) https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:15: * Get a byte from the buffer On 2015/03/11 21:14:26, scroggo wrote: > Maybe add a comment that these are unsafe, and assume the caller did the real > check? Yes I agree, I made this more clear. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:52: static bool premul_and_unpremul(SkAlphaType dst, SkAlphaType src) { On 2015/03/11 21:14:25, scroggo wrote: > // TODO: Share with other codecs > > (unless this is different in some way I've missed?) I have added this method to SkCodec.h (unfortunately it's public because it's called by conversion_possible, which is not a member function). I also added get_byte, get_short, and get_int to SkCodec.h because they will be shared by the Ico decoder that is coming next. Let me know if this is this follows the design you were looking for. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:265: //uint16_t planes = get_short(iBuffer, 8); On 2015/03/11 21:14:26, scroggo wrote: > Any reason to leave this here? Sorry I must have forgotten to delete this. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:276: if (infoBytesRemaining >= 32) { On 2015/03/11 21:14:25, scroggo wrote: > Maybe this should go inside the previous if statement? Yes that makes sense. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:357: // is V2+, we are guaranteed to be able at least this size. On 2015/03/11 21:14:26, scroggo wrote: > able to read* Nice catch. How does this error keep coming back? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:485: colorTable[i] = SkPackARGB32NoCheck(alpha, red, green, blue); On 2015/03/11 21:14:25, scroggo wrote: > So these colors are unpremultiplied? Can you add a comment (maybe by the > declaration of colorTable)? Yes. I hope that I am following your convention by always using Opaque or Unpremultiplied alpha types. I should try to make it more clear that I default to Unpremul. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:503: // value of numColors greater than this maximum is invalid. On 2015/03/11 21:14:25, scroggo wrote: > If numColors is greater than the maximum, is the rest of the image likely to be > valid? Maybe there's not a good way to determine that... No idea. Your guess is as good as mine. I think it's reasonable to continue because the next step is to try to move the stream to the pixel data. Hopefully, this will fail if the rest of the image is invalid and succeed if there is actually pixel data to decode. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:786: uint8_t val = buffer.get()[currByte++]; On 2015/03/11 21:14:25, scroggo wrote: > Do we need to do a check to make sure currByte is still in range? (Or did some > previous check account for this one?) Same questions for below. > > Oh, wait, we're guaranteed that we'll only read rowBytes bytes in this while > loop? Maybe add an SkASSERT that we know these reads are safe. I agree that this is confusing. Hopefully adding asserts will make it more clear. https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:809: dstPtr, y * dstRowBytes); On 2015/03/11 21:14:26, scroggo wrote: > y is unchanged in the loop, correct? Can we do this multiply only once? Yes you are correct! https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:888: switch (fBitsPerPixel) { On 2015/03/11 21:14:25, scroggo wrote: > It's ironic that here we use fBitsPerPixel to determine the SrcConfig, and > SkSwizzler has a function that takes the SrcConfig and returns the bitsPerPixel. > > Would it make sense to make the two independent? Not completely, obviously, but > I wonder if it would make sense to just have kIndex, and let the caller > separately specify the bitsPerPixel. (If so, we can land that as a separate CL.) Yeah I completely agree with you, it doesn't make much sense. I will make myself a note to make that change. I'm glad to do it as a separate CL. I'm thinking that we could provide another call to CreateSwizzler where bpp is provided, otherwise it is calculated? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:940: transparent &= (SkSwizzler::kTransparent_ResultAlpha == r); On 2015/03/11 21:14:25, scroggo wrote: > Maybe the parenthetical bit should be a helper function? > > If/when we change the way ResultAlpha is stored we can then continue using that > function without modifying callers. Agreed, I will add a helper function to the Swizzler.
https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:357: // is V2+, we are guaranteed to be able at least this size. On 2015/03/12 14:06:46, msarett wrote: > On 2015/03/11 21:14:26, scroggo wrote: > > able to read* > > Nice catch. How does this error keep coming back? Haha, I don't know, but I know when I rebase I sometimes get confused about which version is which... https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:485: colorTable[i] = SkPackARGB32NoCheck(alpha, red, green, blue); On 2015/03/12 14:06:47, msarett wrote: > On 2015/03/11 21:14:25, scroggo wrote: > > So these colors are unpremultiplied? Can you add a comment (maybe by the > > declaration of colorTable)? > > Yes. I hope that I am following your convention by always using Opaque or > Unpremultiplied alpha types. I should try to make it more clear that I default > to Unpremul. But it appears there is no way to get premul. Does onGetPixels later premultiply the colors if the caller requested premul? Come to think of it, I just realized that this is different from SkPngCodec. That Codec reads the color table inside onGetPixels, whereas this one reads it in NewFromStream. I can think of advantages to either one: - In NewFromStream: - if the color table is invalid, we can early exit without creating a codec - if the caller chooses to rewind and decode again, the color table is already decoded - In onGetPixels: - when reading the color table, we know whether we want premultiplied data or not - the caller can get the dimensions sooner (Maybe there are others.) I thought maybe all codecs should behave the same, but it may be that this model fits the BMP codec better, while the other fits the PNG model better. Matt, what are your thoughts? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:503: // value of numColors greater than this maximum is invalid. On 2015/03/12 14:06:46, msarett wrote: > On 2015/03/11 21:14:25, scroggo wrote: > > If numColors is greater than the maximum, is the rest of the image likely to > be > > valid? Maybe there's not a good way to determine that... > > No idea. Your guess is as good as mine. I think it's reasonable to continue > because the next step is to try to move the stream to the pixel data. > Hopefully, this will fail if the rest of the image is invalid and succeed if > there is actually pixel data to decode. Come to think of it, what do we accomplish with this check? All we do is skip some bytes. We would skip those bytes anyway down below when we skip to the offset. It seems like we potentially eliminate images which have invalid color tables even though the offset is correct. What if instead we just set colorBytes to 0 in the else case? https://codereview.chromium.org/947283002/diff/260001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:888: switch (fBitsPerPixel) { On 2015/03/12 14:06:46, msarett wrote: > On 2015/03/11 21:14:25, scroggo wrote: > > It's ironic that here we use fBitsPerPixel to determine the SrcConfig, and > > SkSwizzler has a function that takes the SrcConfig and returns the > bitsPerPixel. > > > > Would it make sense to make the two independent? Not completely, obviously, > but > > I wonder if it would make sense to just have kIndex, and let the caller > > separately specify the bitsPerPixel. (If so, we can land that as a separate > CL.) > > Yeah I completely agree with you, it doesn't make much sense. I will make > myself a note to make that change. I'm glad to do it as a separate CL. I'm > thinking that we could provide another call to CreateSwizzler where bpp is > provided, otherwise it is calculated? I'd lean towards going all one direction or the other, rather than having a separate CreateSwizzler for each. https://codereview.chromium.org/947283002/diff/280001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/947283002/diff/280001/include/codec/SkCodec.h... include/codec/SkCodec.h:57: static bool premul_and_unpremul(SkAlphaType dst, SkAlphaType src) { I think it'd be better if we moved all these static helper functions to a new file, in src/codec, perhaps named SkCodecPriv.h. They're not really a part of the API so much as part of the implementation. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/280001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:435: // Fill in the color table (colors are stored unpremultiplied) We should also have this comment in the header file regarding the parameters passed to the SkBmpCodec constructor. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:912: // Modifying alpha is safe because colors are stored unpremultiplied. Oh, interesting. How can we do this if the caller requested premultiplied? Keep in mind that I although I think it makes sense to make unpremul the default, we have to support premul (esp since we currently don't support drawing unpremul...). https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:24: const uint32_t rowBytes = (width + pixelsPerByte - 1) / pixelsPerByte; Can this use compute_row_bytes? (Maybe moved to SkCodecPriv?) (maybe not because we also need pixelsPerByte here?) Oh, yeah, and rowBytes should be size_t. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:122: int deltaSrc = bitsPerPixel / 8; It's a shame we've added this divide to each row (especially since it's the same each time!). What if we still passed this as deltaSrc, and each function decided how to use it (bitsPerPixel for some, deltaSrc e.g. bytesPerPixel for others) or is that too confusing? https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:142: maxAlpha &= alpha; I wonder if we can do some more code sharing here, since we do a similar thing each time. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:215: return (maxAlpha == 0xFF) ? SkSwizzler::kOpaque_ResultAlpha : Alright, I think all callers are comparing using the functions now. If I understand correctly, each caller only cares about one of: is it transparent is it opaque Is that correct? Or does anyone actually care about the tri state? I think we can stop using an enum, and pack the result into a single int (or even a uint16, technically). Then we can avoid this if check. (Maybe I'm prematurely optimizing though...) https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:49: * transprent. transparent* https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:110: SrcConfig sc, const SkPMColor* ctable, const SkImageInfo& info, Why did this change? It looks like you just changed the formatting? https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:123: * @return Whether the row had non-opaque alpha. Please update this comment.
Improved efficiency of ResultAlpha and code sharing https://codereview.chromium.org/947283002/diff/280001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/947283002/diff/280001/include/codec/SkCodec.h... include/codec/SkCodec.h:57: static bool premul_and_unpremul(SkAlphaType dst, SkAlphaType src) { On 2015/03/12 15:19:57, scroggo wrote: > I think it'd be better if we moved all these static helper functions to a new > file, in src/codec, perhaps named SkCodecPriv.h. They're not really a part of > the API so much as part of the implementation. Agreed. I didn't really think they belonged here, but wasn't sure where to put them. That makes sense. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/280001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:435: // Fill in the color table (colors are stored unpremultiplied) On 2015/03/12 15:19:57, scroggo wrote: > We should also have this comment in the header file regarding the parameters > passed to the SkBmpCodec constructor. Of course. Will do. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:912: // Modifying alpha is safe because colors are stored unpremultiplied. On 2015/03/12 15:19:57, scroggo wrote: > Oh, interesting. How can we do this if the caller requested premultiplied? > > Keep in mind that I although I think it makes sense to make unpremul the > default, we have to support premul (esp since we currently don't support drawing > unpremul...). I think it's best to keep it out of the swizzler (as we do here) to avoid cluttering the swizzler with weird bmp alpha stuff. My strategy would probably be to use the BGRX swizzler and then correct and premultiply afterwards (right here). But I haven't even come close to starting that, so we can discuss alternatives. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:24: const uint32_t rowBytes = (width + pixelsPerByte - 1) / pixelsPerByte; On 2015/03/12 15:19:57, scroggo wrote: > Can this use compute_row_bytes? (Maybe moved to SkCodecPriv?) (maybe not because > we also need pixelsPerByte here?) > > Oh, yeah, and rowBytes should be size_t. Yes! I agree though that it doesn't make sense to share the helper with SkBmpCodec. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:122: int deltaSrc = bitsPerPixel / 8; On 2015/03/12 15:19:57, scroggo wrote: > It's a shame we've added this divide to each row (especially since it's the same > each time!). What if we still passed this as deltaSrc, and each function decided > how to use it (bitsPerPixel for some, deltaSrc e.g. bytesPerPixel for others) or > is that too confusing? I think it's worth pursuing given that we are trying to minimize the complexity of the row swizzles. I will give it a try. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:142: maxAlpha &= alpha; On 2015/03/12 15:19:57, scroggo wrote: > I wonder if we can do some more code sharing here, since we do a similar thing > each time. The only thing I can think of that is efficient is a #define, which is not ideal. Still it might be better than nothing. The really unfortunate part is that we now have the same #define in SkSwizzler and SkMaskSwizzler. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:215: return (maxAlpha == 0xFF) ? SkSwizzler::kOpaque_ResultAlpha : On 2015/03/12 15:19:57, scroggo wrote: > Alright, I think all callers are comparing using the functions now. If I > understand correctly, each caller only cares about one of: > > is it transparent > is it opaque > > Is that correct? Or does anyone actually care about the tri state? I think we > can stop using an enum, and pack the result into a single int (or even a uint16, > technically). Then we can avoid this if check. (Maybe I'm prematurely optimizing > though...) I think this is a good place to go now that the behavior is defined. I will add something. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:49: * transprent. On 2015/03/12 15:19:57, scroggo wrote: > transparent* Done. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:110: SrcConfig sc, const SkPMColor* ctable, const SkImageInfo& info, On 2015/03/12 15:19:58, scroggo wrote: > Why did this change? It looks like you just changed the formatting? I believe I added parameters, which prompted me to change the formatting, then removed my new parameters and did not restore the formatting. There is no reason for the change, I will try to restore the old formatting. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:123: * @return Whether the row had non-opaque alpha. On 2015/03/12 15:19:58, scroggo wrote: > Please update this comment. Sorry I'm not too good at remembering to do that.
https://codereview.chromium.org/947283002/diff/280001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/280001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:912: // Modifying alpha is safe because colors are stored unpremultiplied. On 2015/03/12 18:37:46, msarett wrote: > On 2015/03/12 15:19:57, scroggo wrote: > > Oh, interesting. How can we do this if the caller requested premultiplied? > > > > Keep in mind that I although I think it makes sense to make unpremul the > > default, we have to support premul (esp since we currently don't support > drawing > > unpremul...). > > I think it's best to keep it out of the swizzler (as we do here) to avoid > cluttering the swizzler with weird bmp alpha stuff. Agreed. > My strategy would probably > be to use the BGRX swizzler and then correct and premultiply afterwards (right > here). But I haven't even come close to starting that, so we can discuss > alternatives. There are two situations we're correcting here, and I'm not sure I understand both. For the second one, where the image was stored with an alpha of 0, and it should be 100%, we could *not* have premultiplied along the way, because then we would have lost the R,G,B values already. (I know we can't know 100% ahead of time, but do we ever know that this is not the case, so it is safe to premultiply during the swizzle?) In the first case, we read the alpha as is, and now we're following that by applying the alpha mask - correct? Could we have applied the alpha mask during swizzling? Or do we need to get here to know that we need to apply the alpha mask? If the latter, it seems like we also cannot premultiply during swizzling for this case either. https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/280001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:142: maxAlpha &= alpha; On 2015/03/12 18:37:47, msarett wrote: > On 2015/03/12 15:19:57, scroggo wrote: > > I wonder if we can do some more code sharing here, since we do a similar thing > > each time. > > The only thing I can think of that is efficient is a #define, which is not > ideal. Still it might be better than nothing. The really unfortunate part is > that we now have the same #define in SkSwizzler and SkMaskSwizzler. Sounds fine to me. We can put it in SkCodecPriv. https://codereview.chromium.org/947283002/diff/300001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/947283002/diff/300001/include/codec/SkCodec.h... include/codec/SkCodec.h:11: #include "SkCodecPriv.h" Does this file depend on anything in SkCodecPriv? We don't want files in include/ to depend on files in src/ (And now I see that SkCodecPriv is in include. Can it be moved to src/?) https://codereview.chromium.org/947283002/diff/300001/src/codec/SkMaskSwizzle... File src/codec/SkMaskSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/300001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.cpp:11: #define UPDATE_ALPHA(alpha, zeroAlpha, maxAlpha) \ As stated elsewhere, since we share it, I think it should go in SkCodecPriv. I also think the name would be better if it reflected that it is for ResultAlpha. What if we took this further: #define INIT_RESULT_ALPHA \ uint8_t zeroAlpha = 0; \ uint8_t maxAlpha = 0xFF; #define UPDATE_RESULT_ALPHA(alpha) \ zeroAlpha |= alpha; \ maxAlpha &= alpha; #define COMPUTE_RESULT_ALPHA \ SkSwizzler::GetResult(zeroAlpha, maxAlpha) https://codereview.chromium.org/947283002/diff/300001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/300001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:16: static inline size_t compute_row_bytes(int width, int pixelsPerByte) { It seems like this is part of the if case of the other compute_row_bytes. Can we move both into SkCodecPriv and implement the other in terms of this one? (We'd need to change the names so it's clearer which one you're calling.) https://codereview.chromium.org/947283002/diff/300001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/300001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:75: * Returns bytes per pixel if bits per pixel >= 8 I'm ambivalent about this. The tricky part is that the caller needs to know whether it's bytes or bits. Just to clarify: I *do* think it's good to store the one we care about down below, and pass it to each call to fRowProc. It just seems a little weird that this function doesn't specify whether its answer is in bits or bytes.
So here are my thoughts on bmp alpha corrections: Of my bmp test suite of 210 "valid" images only 6 of them contain any kind of alpha information at all. My test suite is about 1/3 "random" images from the web and 2/3 images taken from online bmp image test suites. All six of the images with alpha information are contrived examples from bmp test suites (the creators of the suites often consider these "questionable"). None of them are from "the wild". One of them fails to display properly in chrome and 2-3 fail to display properly on ubuntu. All of them use the decodeMask() mode and SkMaskSwizzler, so the special case in decode() is never exercised. Only one of them uses the correction step (for fully transparent images) after the SkMaskSwizzle and this image fails to display on ubuntu. The reason for the special cases is that the documentation of the chromium decoder said it was necessary, not because I have come across any need for it. The good news is that current code is probably fast because the correction code is guarded by a check for an alpha mask or a fully transparent image, and therefore not executed. My recommendation for premul is to not worry about the alpha mask and transparent results. We can swizzle premul in the swizzler and include a slow path for images with alpha in SkBmpCodec just like we are now. It looks like that code will be only exercised one in a million times (or never if I can't manage to find a test image). Ico is slightly more interesting because these images tend to actually have alpha, but I don't think it changes things much. https://codereview.chromium.org/947283002/diff/300001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/947283002/diff/300001/include/codec/SkCodec.h... include/codec/SkCodec.h:11: #include "SkCodecPriv.h" On 2015/03/12 19:58:40, scroggo wrote: > Does this file depend on anything in SkCodecPriv? We don't want files in > include/ to depend on files in src/ > > (And now I see that SkCodecPriv is in include. Can it be moved to src/?) Done. https://codereview.chromium.org/947283002/diff/300001/src/codec/SkMaskSwizzle... File src/codec/SkMaskSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/300001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.cpp:11: #define UPDATE_ALPHA(alpha, zeroAlpha, maxAlpha) \ On 2015/03/12 19:58:40, scroggo wrote: > As stated elsewhere, since we share it, I think it should go in SkCodecPriv. > > I also think the name would be better if it reflected that it is for > ResultAlpha. > > What if we took this further: > > #define INIT_RESULT_ALPHA \ > uint8_t zeroAlpha = 0; \ > uint8_t maxAlpha = 0xFF; > > #define UPDATE_RESULT_ALPHA(alpha) \ > zeroAlpha |= alpha; \ > maxAlpha &= alpha; > > #define COMPUTE_RESULT_ALPHA \ > SkSwizzler::GetResult(zeroAlpha, maxAlpha) Done. https://codereview.chromium.org/947283002/diff/300001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/300001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:16: static inline size_t compute_row_bytes(int width, int pixelsPerByte) { On 2015/03/12 19:58:40, scroggo wrote: > It seems like this is part of the if case of the other compute_row_bytes. Can we > move both into SkCodecPriv and implement the other in terms of this one? (We'd > need to change the names so it's clearer which one you're calling.) Done. https://codereview.chromium.org/947283002/diff/300001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/300001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:75: * Returns bytes per pixel if bits per pixel >= 8 On 2015/03/12 19:58:40, scroggo wrote: > I'm ambivalent about this. The tricky part is that the caller needs to know > whether it's bytes or bits. > > Just to clarify: I *do* think it's good to store the one we care about down > below, and pass it to each call to fRowProc. It just seems a little weird that > this function doesn't specify whether its answer is in bits or bytes. Done.
On 2015/03/12 21:59:41, msarett wrote: > So here are my thoughts on bmp alpha corrections: > > Of my bmp test suite of 210 "valid" images only 6 of them contain any kind of > alpha information at all. My test suite is about 1/3 "random" images from the > web and 2/3 images taken from online bmp image test suites. > > All six of the images with alpha information are contrived examples from bmp > test suites (the creators of the suites often consider these "questionable"). > None of them are from "the wild". One of them fails to display properly in > chrome and 2-3 fail to display properly on ubuntu. > > All of them use the decodeMask() mode and SkMaskSwizzler, so the special case in > decode() is never exercised. Only one of them uses the correction step (for > fully transparent images) after the SkMaskSwizzle and this image fails to > display on ubuntu. > > The reason for the special cases is that the documentation of the chromium > decoder said it was necessary, not because I have come across any need for it. > > The good news is that current code is probably fast because the correction code > is guarded by a check for an alpha mask or a fully transparent image, and > therefore not executed. > > My recommendation for premul is to not worry about the alpha mask and > transparent results. We can swizzle premul in the swizzler and include a slow > path for images with alpha in SkBmpCodec just like we are now. It looks like > that code will be only exercised one in a million times (or never if I can't > manage to find a test image). > > Ico is slightly more interesting because these images tend to actually have > alpha, but I don't think it changes things much. Sgtm. For the case that never happens except in the test suite, maybe just leave handling that out for now with a FIXME pointing to a bug (with the test image attached) that describes what the issue is, and how we would fix it. https://codereview.chromium.org/947283002/diff/320001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/320001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:9: #include "SkCodecPriv.h" nit: Is anything in SkCodecPriv used by this header? If not, this should go in the cpp file. https://codereview.chromium.org/947283002/diff/320001/src/codec/SkCodec_libpng.h File src/codec/SkCodec_libpng.h (right): https://codereview.chromium.org/947283002/diff/320001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.h:9: #include "SkCodecPriv.h" I think this can move to SkCodec_libpng.cpp https://codereview.chromium.org/947283002/diff/320001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/320001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:303: int deltaSrc = SkIsAlign8(BitsPerPixel(sc)) ? BytesPerPixel(sc) : Again, I think you want SkIsAlign4. https://codereview.chromium.org/947283002/diff/320001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/320001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:110: SkASSERT(SkIsAlign8(BitsPerPixel(sc))); I think this should be SkIsAlign4? (I think this is what you had in one of your earlier patch sets.) It needs to be divisible by 8, not aligned to 8 bits.
Commented out the rare case until we demonstrate that we need it https://codereview.chromium.org/947283002/diff/320001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/947283002/diff/320001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:9: #include "SkCodecPriv.h" On 2015/03/13 14:06:59, scroggo wrote: > nit: Is anything in SkCodecPriv used by this header? If not, this should go in > the cpp file. Done. https://codereview.chromium.org/947283002/diff/320001/src/codec/SkCodec_libpng.h File src/codec/SkCodec_libpng.h (right): https://codereview.chromium.org/947283002/diff/320001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.h:9: #include "SkCodecPriv.h" On 2015/03/13 14:06:59, scroggo wrote: > I think this can move to SkCodec_libpng.cpp Done.
https://codereview.chromium.org/947283002/diff/340001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/340001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:577: // FIXME: transparent &= SkSwizzler::IsTransparent(r); Are we still using IsTransparent anywhere? https://codereview.chromium.org/947283002/diff/340001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:588: // http://pxd.me/dompdf/www/test/image_bmp.html. nit: I'm wary of posting links to the websites we're not in control of. Maybe this one has been there a long time and isn't likely to disappear, but it could. I think it would be better to file a bug and attach the image to the bug (and link to the bug here).
Keep the correction step for decodeMask, it is used https://codereview.chromium.org/947283002/diff/340001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/947283002/diff/340001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:577: // FIXME: transparent &= SkSwizzler::IsTransparent(r); On 2015/03/13 15:04:46, scroggo wrote: > Are we still using IsTransparent anywhere? It works and it's necessary for one of the test images, so let's use it! https://codereview.chromium.org/947283002/diff/340001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:588: // http://pxd.me/dompdf/www/test/image_bmp.html. On 2015/03/13 15:04:46, scroggo wrote: > nit: I'm wary of posting links to the websites we're not in control of. Maybe > this one has been there a long time and isn't likely to disappear, but it could. > I think it would be better to file a bug and attach the image to the bug (and > link to the bug here). Good to know in the future. Since we are in fact enabling this feature, the link is gone.
Premul marked as unsupported
lgtm nit: Can you update the description to be more informative about the changes here? (You can do this from the link that says "Edit Issue" on the left.) Also, please add BUG=skia:3257 to the description. https://codereview.chromium.org/947283002/diff/320001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/947283002/diff/320001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:303: int deltaSrc = SkIsAlign8(BitsPerPixel(sc)) ? BytesPerPixel(sc) : On 2015/03/13 14:06:59, scroggo wrote: > Again, I think you want SkIsAlign4. Just for posterity, this was my misunderstanding. https://codereview.chromium.org/947283002/diff/320001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/947283002/diff/320001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:110: SkASSERT(SkIsAlign8(BitsPerPixel(sc))); On 2015/03/13 14:06:59, scroggo wrote: > I think this should be SkIsAlign4? (I think this is what you had in one of your > earlier patch sets.) > > It needs to be divisible by 8, not aligned to 8 bits. My mistake. SkIsAlign8 *does* mean divisible by 8.
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/947283002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 947283002-380001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com') Was the presubmit check useful? If not, run "git cl presubmit -v" to figure out which PRESUBMIT.py was run, then run git blame on the file to figure out who to ask for help.
https://codereview.chromium.org/947283002/diff/380001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/947283002/diff/380001/include/codec/SkCodec.h... include/codec/SkCodec.h:11: #include "SkEndian.h" drop this include.
API lgtm when the include is dropped.
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, djsollen@google.com Link to the patchset: https://codereview.chromium.org/947283002/#ps400001 (title: "Fixed include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/947283002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
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, djsollen@google.com Link to the patchset: https://codereview.chromium.org/947283002/#ps420001 (title: "Fixed relase mode failure - inline function not defined")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/947283002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)
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, djsollen@google.com Link to the patchset: https://codereview.chromium.org/947283002/#ps440001 (title: "Removed implicit conversions between size_t and uint32_t")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/947283002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)
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, djsollen@google.com Link to the patchset: https://codereview.chromium.org/947283002/#ps460001 (title: "One more casting error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/947283002/460001
Message was sent while issue was closed.
Committed patchset #24 (id:460001) as https://skia.googlesource.com/skia/+/3675874468de7228944ce21922e6ec863f5f2310
Message was sent while issue was closed.
A revert of this CL (patchset #25 id:480001) has been created in https://codereview.chromium.org/1010813003/ by egdaniel@google.com. The reason for reverting is: breaking webkit image decoding tests.
Message was sent while issue was closed.
On 2015/03/16 15:27:57, I haz the power (commit-bot) wrote: > Committed patchset #24 (id:460001) as > https://skia.googlesource.com/skia/+/3675874468de7228944ce21922e6ec863f5f2310 Test crashes have appeared in Blink unit tests. This change may be to blame. DeferredImageDecoder tests are crashing on all platform. For example: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/27...
Message was sent while issue was closed.
Thanks Greg.
Message was sent while issue was closed.
On 2015/03/16 17:54:48, junov wrote: > On 2015/03/16 15:27:57, I haz the power (commit-bot) wrote: > > Committed patchset #24 (id:460001) as > > https://skia.googlesource.com/skia/+/3675874468de7228944ce21922e6ec863f5f2310 > > Test crashes have appeared in Blink unit tests. This change may be to blame. > DeferredImageDecoder tests are crashing on all platform. For example: > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/27... I don't understand how this change could be responsible for that. All these changes apply to SkCodec, which is not used by Chromium/Blink. (There's also a change to DM, which I believe is also not used by the tests.) That said, none of the other CLs in the Skia roll look like they could be responsible either: https://chromium.googlesource.com/skia/+log/1e23738..3675874 Since these are mostly related to deferred decoding, I'd guess the problem is in https://codereview.chromium.org/1004043002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
