Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1258863008:
Split SkBmpCodec into three separate classes (Closed)
DescriptionSplit SkBmpCodec into three separate classes
Will regress behavior on gold on test32bfv4.bmp, where we
will no longer fix transparent decodes.
TODO: Start fixing transparent decodes again, or decide
that we don't want to fix them and remove isTransparent
from SkSwizzler. I think this may become more clear when I
start implementing the scanline decoder.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/4ab9d5f1bc6d05c49dc765c3de5ade816f4c968e
Patch Set 1 : #
Total comments: 47
Patch Set 2 : SkBmpCodec is a parent class of the bmp codecs #
Total comments: 41
Patch Set 3 : Responses to comments #Patch Set 4 : Remove code to fix transparent decodes #
Total comments: 12
Patch Set 5 : #
Total comments: 1
Patch Set 6 : Rebase #Patch Set 7 : Fix rebase #Patch Set 8 : #Patch Set 9 : #
Depends on Patchset: Messages
Total messages: 37 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
msarett@google.com changed reviewers: + emmaleer@google.com, scroggo@google.com
One relevant response to a comment from 1249973003: "I believe these are subclasses? It seems weird to make a subclass a friend. Can we move stuff the subclass needs to access to protected, so they do not have to be friends?" Actually SkBmpMaskCodec, SkBmpStandardCodec, and SkBmpRLECodec are subclasses of SkCodec. SkBmpCodec has no parent class or subclasses, it just provides NewFromStream and other useful functions. I'm not against subclassing SkBmpCodec, but I didn't really see a compelling reason to do so. Instead I make shared methods private in SkBmpCodec and rely on friend classes. I'm not opposed to redesigning if you think there is a better option. Since SkBmpCodec has no real relationship with the three real codecs, we could move shared methods to SkCodecPriv.h?
Is any of this code new? I think FixTransparentDecode is a new method, but I suppose it's just moved functionality? It would be helpful if you could comment (in the code review, not in the code) where code moved vs had to be changed. On 2015/07/30 19:24:46, msarett wrote: > One relevant response to a comment from 1249973003: > > "I believe these are subclasses? It seems weird to make a subclass a friend. Can > we move stuff the subclass needs to access to protected, so they do not have to > be friends?" > > Actually SkBmpMaskCodec, SkBmpStandardCodec, and SkBmpRLECodec are subclasses of > SkCodec. SkBmpCodec has no parent class or subclasses, it just provides > NewFromStream and other useful functions. > > I'm not against subclassing SkBmpCodec, but I didn't really see a compelling > reason to do so. Instead I make shared methods private in SkBmpCodec and rely > on friend classes. I'm not opposed to redesigning if you think there is a > better option. Since SkBmpCodec has no real relationship with the three real > codecs, we could move shared methods to SkCodecPriv.h? When you decide whether to subclass, you ask whether the class "is-a" the other one. I would argue that all three of these classes are BmpCodecs. Why do you *not* want to make them subclasses? They do have real relationships - they share methods and variables and represent specific types of BmpCodec. I think this is a big step in the right direction, but we have more code that can be shared, and making them subclass SkBmpCodec will help you do that. It can have a function for handling the rewind, which will be more clear how to do once it is in the hierarchy. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpCodec.cpp File src/codec/SkBmpCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpCodec.cp... src/codec/SkBmpCodec.cpp:470: *codecOut = SkNEW_ARGS(SkBmpStandardCodec, ( nit: I believe we typically do not end a line with an open parens. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpCodec.cp... src/codec/SkBmpCodec.cpp:473: break; nit: Why not just return true; (I do not know if there are guidelines for this, but it seems like there is no code we need to execute after the break?) https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpCodec.cp... src/codec/SkBmpCodec.cpp:476: // We can do this here because there if no color table to read there is* ? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... File src/codec/SkBmpMaskCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.cpp:11: #include "SkScanlineDecoder.h" This is not needed, right? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCodec.h File src/codec/SkBmpMaskCodec.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:10: #include "SkColorTable.h" I think this is not needed? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:13: #include "SkStream.h" This can be forward declared. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:14: #include "SkSwizzler.h" Is this necessary? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:18: * This class implements the decoding for bmp images For a specific type, right? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:30: SkEncodedFormat onGetEncodedFormat() const override { return kBMP_SkEncodedFormat; } If these all inherited from SkBmpCodec, that class could implement this, and the others would not have to. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:37: enum BitmapInputFormat { Isn't this the same as in SkBmpCodec? Why the duplication? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:45: * Initialize swizzler These comments don't really add much, do they? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:57: * Called only by NewFromStream nit: SkBmpCodec::NewFromStream. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec... File src/codec/SkBmpRLECodec.cpp (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.cpp:71: // Check for proper input and output formats This comment seems to not apply to the code next to it. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.cpp:72: SkCodec::RewindState rewindState = this->rewindIfNeeded(); This code should be shared between the codecs. Again, if they are all subclasses of SkBmpCodec, this can be a protected function. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec.h File src/codec/SkBmpRLECodec.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:43: bool initializeStreamBuffer(); The comment does not match the name? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:18: static bool conversion_possible(const SkImageInfo& dst, This function looks like it can be shared? (Possibly a part of SkBmpCodec) https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:12: #include "SkMaskSwizzler.h" This is not used by this class. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:13: #include "SkStream.h" I think this can be forward declared. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:18: * This class implements the decoding for bmp images Which kind? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:69: // Fields comment not needed. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:70: const uint16_t fBitsPerPixel; This looks to be used by all three codecs. You can put it in a base class, and then add a protected accessor. Then you won't have to duplicate this field. (Maybe some of the others can be shared?) https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkCodecPriv.h... src/codec/SkCodecPriv.h:34: static inline bool valid_alpha(SkAlphaType dstAlpha, SkAlphaType srcAlpha) { Is this used by all codecs? Or just BMP? Nvm, it looks like it is also used by libpng. +1 for code sharing. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkCodecPriv.h... src/codec/SkCodecPriv.h:99: * Get the destination row to start filling from This probably belongs in SkBmpCodec.
Patchset #2 (id:60001) has been deleted
"Is any of this code new? I think FixTransparentDecode is a new method, but I suppose it's just moved functionality? It would be helpful if you could comment (in the code review, not in the code) where code moved vs had to be changed." Sorry about this. I'm sure that this CL is massively difficult to review even when it's been split up from the other components. Especially because I left in a lot of the refactoring that will be useful for scanline decoding. I have added comments throughout the latest patch set that explain FixTransparentDecode (which is new btw) and hopefully help explain what changes have been made and why. "When you decide whether to subclass, you ask whether the class "is-a" the other one. I would argue that all three of these classes are BmpCodecs. Why do you *not* want to make them subclasses? They do have real relationships - they share methods and variables and represent specific types of BmpCodec." I agree 100% that they should be subclasses. Subclassing has allowed me to: 1) Share code more cleanly. 2) Create a more logical relationship between bmp codec classes. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpCodec.cpp File src/codec/SkBmpCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpCodec.cp... src/codec/SkBmpCodec.cpp:470: *codecOut = SkNEW_ARGS(SkBmpStandardCodec, ( On 2015/07/31 15:05:43, scroggo wrote: > nit: I believe we typically do not end a line with an open parens. Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpCodec.cp... src/codec/SkBmpCodec.cpp:473: break; On 2015/07/31 15:05:43, scroggo wrote: > nit: Why not just > > return true; > > (I do not know if there are guidelines for this, but it seems like there is no > code we need to execute after the break?) Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpCodec.cp... src/codec/SkBmpCodec.cpp:476: // We can do this here because there if no color table to read On 2015/07/31 15:05:43, scroggo wrote: > there is* ? Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... File src/codec/SkBmpMaskCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.cpp:11: #include "SkScanlineDecoder.h" On 2015/07/31 15:05:43, scroggo wrote: > This is not needed, right? Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCodec.h File src/codec/SkBmpMaskCodec.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:10: #include "SkColorTable.h" On 2015/07/31 15:05:43, scroggo wrote: > I think this is not needed? Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:13: #include "SkStream.h" On 2015/07/31 15:05:43, scroggo wrote: > This can be forward declared. We actually don't need to include it at all. Out of curiosity, what is the advantage of a forward declaration vs an include? https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:14: #include "SkSwizzler.h" On 2015/07/31 15:05:43, scroggo wrote: > Is this necessary? Nope. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:18: * This class implements the decoding for bmp images On 2015/07/31 15:05:43, scroggo wrote: > For a specific type, right? Yes https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:30: SkEncodedFormat onGetEncodedFormat() const override { return kBMP_SkEncodedFormat; } On 2015/07/31 15:05:43, scroggo wrote: > If these all inherited from SkBmpCodec, that class could implement this, and the > others would not have to. Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:37: enum BitmapInputFormat { On 2015/07/31 15:05:43, scroggo wrote: > Isn't this the same as in SkBmpCodec? Why the duplication? This was accidental. Sorry. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:45: * Initialize swizzler On 2015/07/31 15:05:43, scroggo wrote: > These comments don't really add much, do they? No they don't. Removing them. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:57: * Called only by NewFromStream On 2015/07/31 15:05:43, scroggo wrote: > nit: SkBmpCodec::NewFromStream. Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec... File src/codec/SkBmpRLECodec.cpp (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.cpp:71: // Check for proper input and output formats On 2015/07/31 15:05:43, scroggo wrote: > This comment seems to not apply to the code next to it. I'll eliminate it. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.cpp:72: SkCodec::RewindState rewindState = this->rewindIfNeeded(); On 2015/07/31 15:05:43, scroggo wrote: > This code should be shared between the codecs. Again, if they are all subclasses > of SkBmpCodec, this can be a protected function. It's not quite so simple because of isIco, but I agree. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec.h File src/codec/SkBmpRLECodec.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:43: bool initializeStreamBuffer(); On 2015/07/31 15:05:44, scroggo wrote: > The comment does not match the name? Removed this comment https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:18: static bool conversion_possible(const SkImageInfo& dst, On 2015/07/31 15:05:44, scroggo wrote: > This function looks like it can be shared? (Possibly a part of SkBmpCodec) Umm not quite. I tried to share it. The best I could do was the valid_alpha function. Standard and Mask share the alpha block but RLE does not. Standard and RLE share the color type block but Mask does not. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:12: #include "SkMaskSwizzler.h" On 2015/07/31 15:05:44, scroggo wrote: > This is not used by this class. Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:13: #include "SkStream.h" On 2015/07/31 15:05:44, scroggo wrote: > I think this can be forward declared. Acknowledged. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:18: * This class implements the decoding for bmp images On 2015/07/31 15:05:44, scroggo wrote: > Which kind? Improved this description https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:69: // Fields On 2015/07/31 15:05:44, scroggo wrote: > comment not needed. Done. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:70: const uint16_t fBitsPerPixel; On 2015/07/31 15:05:44, scroggo wrote: > This looks to be used by all three codecs. > > You can put it in a base class, and then add a protected accessor. Then you > won't have to duplicate this field. > > (Maybe some of the others can be shared?) Done. fRowOrder can also be shared. https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkCodecPriv.h... src/codec/SkCodecPriv.h:34: static inline bool valid_alpha(SkAlphaType dstAlpha, SkAlphaType srcAlpha) { On 2015/07/31 15:05:44, scroggo wrote: > Is this used by all codecs? Or just BMP? > > Nvm, it looks like it is also used by libpng. +1 for code sharing. Acknowledged :) https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkCodecPriv.h... src/codec/SkCodecPriv.h:99: * Get the destination row to start filling from On 2015/07/31 15:05:44, scroggo wrote: > This probably belongs in SkBmpCodec. Done. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h File src/codec/SkBmpCodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:16: #include "SkTypes.h" Changes: Deleted functions that relate to specific SkBmp<Name>Codecs. Moved helper functions to protected to be shared by SkBmp<Name>Codecs. Create new protected helper functions. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:70: static SkCodec::Result FixTransparentDecode(void* dst, size_t dstRowBytes, FixTransparentDecode() is new. It is an improvement over the old method of fixing transparent decodes in several ways. 1) It is used by both SkBmpMaskCodec and SkBmpStandardCodec. This is a behavior change in that we used to not fix transparent decodes in Standard mode. We do not have any test images for Standard mode that require this fix. In fact, we don't even have any images in Standard mode that use alpha. The chromium bmp decoder claims that these images exist and that we need to correct them. Having this function makes it easy, so why not go ahead and correct them (and get rid of a giant FIXME). 2) We now decode bmps in Mask mode line by line instead of allocating a whole image sized block of memory. We can do this because this version of the fix restarts the entire decode rather reusing an image buffer sized block of memory. This will be significant when we implement scanline decoders. This also makes fixing transparent decodes worse one way: 1) We must allow callers to ask for kOpaque when we recommend kUnpremul. This is because, in FixTransparentDecode, we repeat the decode with kOpaque passed in as the alpha type. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpMaskCodec.h File src/codec/SkBmpMaskCodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:40: bool initializeSwizzler(const SkImageInfo& dstInfo); Creation of the swizzler has been factored out of decodeMask(). This will be useful when I implement the scanline decoder. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:42: Result decode(const SkImageInfo& dstInfo, void* dst, size_t dstRowBytes, This used to be decodeMask() in SkBmpCodec. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec.h File src/codec/SkBmpRLECodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:54: bool createColorTable(SkAlphaType alphaType, int* colorCount); Used to be in SkBmpCodec. This has been simplified given that we know we are creating a color table for RLE mode (fIsIco is always false, alpha is always 0xFF). https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:56: bool initializeStreamBuffer(); Factored out of decodeRLE(). Will make scanline decoding easier. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:74: Result decode(const SkImageInfo& dstInfo, void* dst, Used to be decodeRLE() in SkBmpCodec. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:52: bool createColorTable(SkAlphaType alphaType, int* colorCount); Used to be in SkBmpCodec. Simplified given that we know that we are not in RLE mode. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:54: bool initializeSwizzler(const SkImageInfo& dstInfo, const Options& opts); Factored out of decode() in SkBmpCodec. Will be useful for scanline decoding. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:56: Result decode(const SkImageInfo& dstInfo, void* dst, size_t dstRowBytes, const Options& opts); Used to be decode() in SkBmpCodec.
https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCodec.h File src/codec/SkBmpMaskCodec.h (right): https://codereview.chromium.org/1258863008/diff/40001/src/codec/SkBmpMaskCode... src/codec/SkBmpMaskCodec.h:13: #include "SkStream.h" On 2015/08/03 22:52:35, msarett wrote: > On 2015/07/31 15:05:43, scroggo wrote: > > This can be forward declared. > > We actually don't need to include it at all. > > Out of curiosity, what is the advantage of a forward declaration vs an include? Google's style guide has a nice succinct section on the advantages and disadvantages of forward declarations: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla... https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.cpp File src/codec/SkBmpCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.cp... src/codec/SkBmpCodec.cpp:550: * Used to fill the remainder of the image on incomplete input for bmps Maybe explain why this is necessary? (And maybe that should be in the header.) The reason we need it is because for top down, we start where we left off, but for bottom up, we start at the beginning, right? https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h File src/codec/SkBmpCodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:19: * This class enables code sharing between its bmp codec subclasses. It is the nit: This second sentence seems unnecessarily wordy. How about: The subclasses actually do the work. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:70: static SkCodec::Result FixTransparentDecode(void* dst, size_t dstRowBytes, On 2015/08/03 22:52:36, msarett wrote: > FixTransparentDecode() is new. It is an improvement over the old method of > fixing transparent decodes in several ways. > > 1) It is used by both SkBmpMaskCodec and SkBmpStandardCodec. This is a behavior > change in that we used to not fix transparent decodes in Standard mode. We do > not have any test images for Standard mode that require this fix. In fact, we > don't even have any images in Standard mode that use alpha. The chromium bmp > decoder claims that these images exist and that we need to correct them. Having > this function makes it easy, so why not go ahead and correct them (and get rid > of a giant FIXME). We should find those images. Have you checked out src-internal in Chromium? (See https://wiki.corp.google.com/twiki/bin/view/Main/ChromeBuildInstructions) It contains some test images you could look at. > > 2) We now decode bmps in Mask mode line by line instead of allocating a whole > image sized block of memory. We can do this because this version of the fix > restarts the entire decode rather reusing an image buffer sized block of memory. > This will be significant when we implement scanline decoders. > > This also makes fixing transparent decodes worse one way: > > 1) We must allow callers to ask for kOpaque when we recommend kUnpremul. This > is because, in FixTransparentDecode, we repeat the decode with kOpaque passed in > as the alpha type. I think the problem is larger than that. For a PNG image, for example, we may recommend unpremul because the image uses a format that supports alpha, but we may find out during decode that none of the pixels actually have a non-opaque alpha. So we recommend unpremul, and we reject a request for opaque, but the image actually *is* opaque. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:71: SkPMColor* colorPtr, int* colorCountPtr, SkStream* stream); Can you update the comment that this requires a call to stream::duplicate? That said, I do not know that that is such a good idea. We need to be able to use this class when duplicating the stream is not necessarily doable (unless, of course, we copy the stream into an SkData, like I believe we do in ICO). https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:87: const uint16_t bitsPerPixel() const { return fBitsPerPixel; } It seems a little weird to return a const basic data type. Did you mean to return a const reference? https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:88: const RowOrder rowOrder() const {return fRowOrder; } nit: space between "{" and "return" https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:95: enum BmpInputFormat { It looks like this is no longer used by the header file. How do you feel about moving it into the implementation with the other enums. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... File src/codec/SkBmpRLECodec.cpp (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.cpp:70: SkCodecPrintf("Error: could not rewind image stream.\n"); Do you think this is necessary? It seems like returning kCouldNotRewind tells the caller this. I'm not opposed to it, just curious. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec.h File src/codec/SkBmpRLECodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:21: * Called only by SkBmpCodec::NewFromStream Interesting that you say that, because it is a public method. It is in src/ so maybe that plus the comment is enough to dissuade someone from trying to call it directly. I suppose you could make SkBmpCodec a friend of SkBmpRLECodec, so this constructor could be private, but that seems weird? https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:24: * @param stream the stream of image data nit: encoded (image data) https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:32: * @param RLEBytes used only for RLE decodes, as we must decode all It seems redundant to say this is used only for RLE decodes, since this class is only for decoding RLE https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:32: SkCodecPrintf("Warning: The client should not request an opaque " Oh, wait, so we are allowing this only because we'll call it in this mode? https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:126: bool SkBmpStandardCodec::createColorTable(SkAlphaType alphaType, int* numColors) { IIUC, there are now two versions of createColorTable? Can they share code (I haven't looked through them to see how similar they are)? https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:34: * @param rowOrder indicates whether rows are ordered top-down or bottom-up @param isIco Whether this is part of an ICO image. (Maybe inIco is more appropriate?) https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:64: const bool fIsIco; So, if I understand correctly, only standard BMPs can be in ICO?
I want to talk in person about the issues with FixTransparentDecode, but here's the latest responses. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.cpp File src/codec/SkBmpCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.cp... src/codec/SkBmpCodec.cpp:550: * Used to fill the remainder of the image on incomplete input for bmps On 2015/08/04 16:16:46, scroggo wrote: > Maybe explain why this is necessary? (And maybe that should be in the header.) > The reason we need it is because for top down, we start where we left off, but > for bottom up, we start at the beginning, right? Yes that's correct, adding a comment. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h File src/codec/SkBmpCodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:19: * This class enables code sharing between its bmp codec subclasses. It is the On 2015/08/04 16:16:46, scroggo wrote: > nit: This second sentence seems unnecessarily wordy. How about: > > The subclasses actually do the work. Yep agreed https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:19: * This class enables code sharing between its bmp codec subclasses. It is the On 2015/08/04 16:16:46, scroggo wrote: > nit: This second sentence seems unnecessarily wordy. How about: > > The subclasses actually do the work. Done. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:70: static SkCodec::Result FixTransparentDecode(void* dst, size_t dstRowBytes, On 2015/08/04 16:16:46, scroggo wrote: > On 2015/08/03 22:52:36, msarett wrote: > > FixTransparentDecode() is new. It is an improvement over the old method of > > fixing transparent decodes in several ways. > > > > 1) It is used by both SkBmpMaskCodec and SkBmpStandardCodec. This is a > behavior > > change in that we used to not fix transparent decodes in Standard mode. We do > > not have any test images for Standard mode that require this fix. In fact, we > > don't even have any images in Standard mode that use alpha. The chromium bmp > > decoder claims that these images exist and that we need to correct them. > Having > > this function makes it easy, so why not go ahead and correct them (and get rid > > of a giant FIXME). > > We should find those images. Have you checked out src-internal in Chromium? (See > https://wiki.corp.google.com/twiki/bin/view/Main/ChromeBuildInstructions) It > contains some test images you could look at. > > > > > 2) We now decode bmps in Mask mode line by line instead of allocating a whole > > image sized block of memory. We can do this because this version of the fix > > restarts the entire decode rather reusing an image buffer sized block of > memory. > > This will be significant when we implement scanline decoders. > > > > This also makes fixing transparent decodes worse one way: > > > > 1) We must allow callers to ask for kOpaque when we recommend kUnpremul. This > > is because, in FixTransparentDecode, we repeat the decode with kOpaque passed > in > > as the alpha type. > > I think the problem is larger than that. For a PNG image, for example, we may > recommend unpremul because the image uses a format that supports alpha, but we > may find out during decode that none of the pixels actually have a non-opaque > alpha. So we recommend unpremul, and we reject a request for opaque, but the > image actually *is* opaque. I'll chat with you tomorrow, but I checked out src-internal and I'm not finding extra test images. I think we went through not being able to find a test image for FixTransparentDecode in standard mode, which is ultimately why we decided to disable it with a FIXME. I reenabled it because I thought this new design makes it easy to fix and doesn't require an image sized buffer. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:71: SkPMColor* colorPtr, int* colorCountPtr, SkStream* stream); On 2015/08/04 16:16:46, scroggo wrote: > Can you update the comment that this requires a call to stream::duplicate? > > That said, I do not know that that is such a good idea. We need to be able to > use this class when duplicating the stream is not necessarily doable (unless, of > course, we copy the stream into an SkData, like I believe we do in ICO). Yeah it's unfortunate. I see the trade off as: Rare images decode incorrectly when we cannot duplicate the stream OR We cannot scanline decode Mask bmps (and maybe not Standard bmps either if we can find a test case where FixTransparentDecode is necessary). https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:87: const uint16_t bitsPerPixel() const { return fBitsPerPixel; } On 2015/08/04 16:16:46, scroggo wrote: > It seems a little weird to return a const basic data type. Did you mean to > return a const reference? Nope let's just make it a basic data type. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:88: const RowOrder rowOrder() const {return fRowOrder; } On 2015/08/04 16:16:46, scroggo wrote: > nit: space between "{" and "return" Done. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpCodec.h#... src/codec/SkBmpCodec.h:95: enum BmpInputFormat { On 2015/08/04 16:16:46, scroggo wrote: > It looks like this is no longer used by the header file. How do you feel about > moving it into the implementation with the other enums. Done. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... File src/codec/SkBmpRLECodec.cpp (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.cpp:70: SkCodecPrintf("Error: could not rewind image stream.\n"); On 2015/08/04 16:16:46, scroggo wrote: > Do you think this is necessary? It seems like returning kCouldNotRewind tells > the caller this. I'm not opposed to it, just curious. Not necessary https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec.h File src/codec/SkBmpRLECodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:21: * Called only by SkBmpCodec::NewFromStream On 2015/08/04 16:16:46, scroggo wrote: > Interesting that you say that, because it is a public method. > > It is in src/ so maybe that plus the comment is enough to dissuade someone from > trying to call it directly. > > I suppose you could make SkBmpCodec a friend of SkBmpRLECodec, so this > constructor could be private, but that seems weird? Yeah I'm pretty happy with the subclasses instead of friend classes design. Public constructors on the subclasses is the main drawback. I will make the comment a little stronger to hopefully dissuade anyone from calling it. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:24: * @param stream the stream of image data On 2015/08/04 16:16:46, scroggo wrote: > nit: encoded (image data) Done. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpRLECodec... src/codec/SkBmpRLECodec.h:32: * @param RLEBytes used only for RLE decodes, as we must decode all On 2015/08/04 16:16:46, scroggo wrote: > It seems redundant to say this is used only for RLE decodes, since this class is > only for decoding RLE Done. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:32: SkCodecPrintf("Warning: The client should not request an opaque " On 2015/08/04 16:16:47, scroggo wrote: > Oh, wait, so we are allowing this only because we'll call it in this mode? Yes. Maybe there is a better way to bypass this check? https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.cpp:126: bool SkBmpStandardCodec::createColorTable(SkAlphaType alphaType, int* numColors) { On 2015/08/04 16:16:46, scroggo wrote: > IIUC, there are now two versions of createColorTable? Can they share code (I > haven't looked through them to see how similar they are)? I made a comment about this in the old upload. I didn't want to share them because of differences with regard to alpha and inIco and because they both depend on a lot of fields. These are fields that they share, but SkBmpCodec does not have access to these fields because SkBmpMaskCodec does not share them. I guess we could subclass SkBmpCodec with SkBmpMightHaveColorTableCodec and then subclass that with Standard and RLE, but IMO sharing this code is more trouble than it's worth. https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... File src/codec/SkBmpStandardCodec.h (right): https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:34: * @param rowOrder indicates whether rows are ordered top-down or bottom-up On 2015/08/04 16:16:47, scroggo wrote: > @param isIco Whether this is part of an ICO image. (Maybe inIco is more > appropriate?) Agreed https://codereview.chromium.org/1258863008/diff/80001/src/codec/SkBmpStandard... src/codec/SkBmpStandardCodec.h:64: const bool fIsIco; On 2015/08/04 16:16:47, scroggo wrote: > So, if I understand correctly, only standard BMPs can be in ICO? You understand correctly. I will add checks for this and comments in ReadHeader.
Removing all the FixTransparentDecode stuff. I think it makes more sense to break up these bmp CLs even further.
lgtm https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpCodec.cpp File src/codec/SkBmpCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpCodec.c... src/codec/SkBmpCodec.cpp:481: return false; Is it possible to reach here? If not, this can be SkASSERT(!inIco) https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpCodec.c... src/codec/SkBmpCodec.cpp:496: if (inIco) { Same. https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpMaskCod... File src/codec/SkBmpMaskCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpMaskCod... src/codec/SkBmpMaskCodec.cpp:83: SkCodec::Result result = decode(dstInfo, dst, dstRowBytes, opts); nit: This can be simplified (also it should have a this pointer): return this->decode(...); https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpMaskCod... src/codec/SkBmpMaskCodec.cpp:119: if (this->stream()->read(srcRow, rowBytes) != rowBytes) { Doesn't need to be a part of this patch set, but I think this can become a readRow function, which can be shared with the scanline decoder, like wbmp. https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpRLECodec.h File src/codec/SkBmpRLECodec.h (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpRLECode... src/codec/SkBmpRLECodec.h:78: uint32_t fNumColors; This gets updated, but it is updated based on fBitsPerPixel, which is const. Maybe compute this in the constructor, and make it const? https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkCodecPriv.... src/codec/SkCodecPriv.h:11: #include "SkBmpCodec.h" Why does this need to include SkBmpCodec?
https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpCodec.cpp File src/codec/SkBmpCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpCodec.c... src/codec/SkBmpCodec.cpp:481: return false; On 2015/08/06 21:12:41, scroggo wrote: > Is it possible to reach here? If not, this can be SkASSERT(!inIco) I do think these are reachable. Not having these checks before was a bug, I believe. https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpMaskCod... File src/codec/SkBmpMaskCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpMaskCod... src/codec/SkBmpMaskCodec.cpp:83: SkCodec::Result result = decode(dstInfo, dst, dstRowBytes, opts); On 2015/08/06 21:12:41, scroggo wrote: > nit: This can be simplified (also it should have a this pointer): > > return this->decode(...); Done. https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpMaskCod... src/codec/SkBmpMaskCodec.cpp:119: if (this->stream()->read(srcRow, rowBytes) != rowBytes) { On 2015/08/06 21:12:41, scroggo wrote: > Doesn't need to be a part of this patch set, but I think this can become a > readRow function, which can be shared with the scanline decoder, like wbmp. Acknowledged. https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpRLECodec.h File src/codec/SkBmpRLECodec.h (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpRLECode... src/codec/SkBmpRLECodec.h:78: uint32_t fNumColors; On 2015/08/06 21:12:41, scroggo wrote: > This gets updated, but it is updated based on fBitsPerPixel, which is const. > Maybe compute this in the constructor, and make it const? Done. This can be a shared function in SkBmpCodec. https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkCodecPriv.... src/codec/SkCodecPriv.h:11: #include "SkBmpCodec.h" On 2015/08/06 21:12:41, scroggo wrote: > Why does this need to include SkBmpCodec? It doesn't, I have fixed this.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1258863008/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258863008/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258863008/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...) skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1258863008/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258863008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258863008/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Mac10.8-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.8-Clang-...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-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 Link to the patchset: https://codereview.chromium.org/1258863008/#ps170001 (title: "Fix rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258863008/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258863008/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1258863008/#ps190001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258863008/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258863008/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1258863008/#ps210001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258863008/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258863008/210001
Message was sent while issue was closed.
Committed patchset #9 (id:210001) as https://skia.googlesource.com/skia/+/4ab9d5f1bc6d05c49dc765c3de5ade816f4c968e
Message was sent while issue was closed.
https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpCodec.cpp File src/codec/SkBmpCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/120001/src/codec/SkBmpCodec.c... src/codec/SkBmpCodec.cpp:481: return false; On 2015/08/06 21:41:42, msarett wrote: > On 2015/08/06 21:12:41, scroggo wrote: > > Is it possible to reach here? If not, this can be SkASSERT(!inIco) > > I do think these are reachable. Not having these checks before was a bug, I > believe. It would be nice to verify that (bonus points for a test!) https://codereview.chromium.org/1258863008/diff/140001/src/codec/SkBmpCodec.cpp File src/codec/SkBmpCodec.cpp (right): https://codereview.chromium.org/1258863008/diff/140001/src/codec/SkBmpCodec.c... src/codec/SkBmpCodec.cpp:565: // Also set fNumColors to maxColors when it is too large nit: fNumColors -> numColors. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
