Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1254483004:
Scanline decoding for wbmp (Closed)
DescriptionScanline decoding for wbmp
We are also changing the wbmp to use SkSwizzler. This
will allow us to take advantage of the sampling routines
that are being implemented in SkSwizzler.
The image in this upload came from:
https://commons.wikimedia.org/wiki/File:Android_robot.png
It is licensed under the Creative Commons Attribution-Share Alike 3.0 Unported license.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/99f567e617b6c5a81e6b822c30ccb0d357db21fc
Patch Set 1 #
Total comments: 17
Patch Set 2 : Use SkSwizzler #
Total comments: 23
Patch Set 3 : Add kBit mode to SkSwizzler #
Total comments: 11
Patch Set 4 : #
Total comments: 2
Patch Set 5 : Added kIndex8 check and test #Patch Set 6 : Rebase on new design of SkScanlineDecoder #
Total comments: 18
Patch Set 7 : Response to comments #
Total comments: 28
Patch Set 8 : Various fixes #
Total comments: 9
Patch Set 9 : More minor fixes #
Total comments: 1
Depends on Patchset: Messages
Total messages: 32 (5 generated)
msarett@google.com changed reviewers: + emmaleer@google.com, scroggo@google.com
Again, no rush
https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp#... src/codec/SkCodec_wbmp.cpp:183: , fSrcRowBytes(SkAlign8(fCodec->getInfo().width()) >> 3) Maybe add a static helper function for this, to share with the codec? https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp#... src/codec/SkCodec_wbmp.cpp:192: fCodec->fProc((uint8_t*) dst, fSrcBuffer.get(), dstInfo().width()); The scanline decoder will need to store fProc, but it's not clear to me that it needs to be set on the codec. https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h File src/codec/SkCodec_wbmp.h (right): https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:19: typedef void (*ExpandProc)(uint8_t*, const uint8_t*, int); I don't see why this needs to be public. https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:25: const Options& options, SkPMColor ctable[], int* ctableCount); override? https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:27: bool setExpandProc(SkColorType colorType, SkPMColor* ctable, int* ctableCount); It is weird to me that this function is called "setExpandProc", but it does not take a parameter of an ExpandProc. Maybe a better name is updateExpandProc? It looks like it also fiddles with the color table? https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:30: ExpandProc fProc; Does fProc need to be a member? It looks like it is only used in onGetPixels, and it will be updated each time it is called. Maybe the expandProc function should be static, and return the ExpandProc. If it returns NULL, that is an indication of failure.
Is CodexTest.cpp supposed to be named CodecTest.cpp? https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp#... src/codec/SkCodec_wbmp.cpp:147: nit: added extra line https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp#... src/codec/SkCodec_wbmp.cpp:183: , fSrcRowBytes(SkAlign8(fCodec->getInfo().width()) >> 3) Could you add a comment describing why the rowBytes is width shifted right by 3? https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp#... src/codec/SkCodec_wbmp.cpp:192: fCodec->fProc((uint8_t*) dst, fSrcBuffer.get(), dstInfo().width()); Why isn't the Swizzler being used? Does fProc do something different than the swizzler? We will need to use the swizzler soon for SkScaledCodec If fProc does something different than the swizzler, let me know so I can update the swizzler to do the same Or, if the swizzler does the same thing, maybe we should just use the swizzler here so we don't have to change it in the future https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h File src/codec/SkCodec_wbmp.h (right): https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:27: bool setExpandProc(SkColorType colorType, SkPMColor* ctable, int* ctableCount); Could you add a comment about what this function does
If we're not happy with this, I'm happy to switch us back to not using the swizzler. The pro of this approach is that we can use the scaling that Emmalee is working on in the swizzler. I did not foresee the con, which is dealing with 1-bit pixels in the swizzler. My approach was to consider them as kIndex1, which required extra work to create a color table. Another approach would be to add a bunch of new routines to the swizzler (kBit_SrcConfig maybe?) that would only be useful for wbmp. https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp#... src/codec/SkCodec_wbmp.cpp:147: On 2015/07/27 14:41:37, emmaleer wrote: > nit: added extra line Acknowledged. https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp#... src/codec/SkCodec_wbmp.cpp:183: , fSrcRowBytes(SkAlign8(fCodec->getInfo().width()) >> 3) On 2015/07/27 14:41:37, emmaleer wrote: > Could you add a comment describing why the rowBytes is width shifted right by 3? Done. https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.cpp#... src/codec/SkCodec_wbmp.cpp:192: fCodec->fProc((uint8_t*) dst, fSrcBuffer.get(), dstInfo().width()); On 2015/07/27 14:41:37, emmaleer wrote: > Why isn't the Swizzler being used? Does fProc do something different than the > swizzler? > We will need to use the swizzler soon for SkScaledCodec > If fProc does something different than the swizzler, let me know so I can update > the swizzler to do the same > Or, if the swizzler does the same thing, maybe we should just use the swizzler > here so we don't have to change it in the future fSwizzler is now a field of only the scanline decoder. https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h File src/codec/SkCodec_wbmp.h (right): https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:19: typedef void (*ExpandProc)(uint8_t*, const uint8_t*, int); On 2015/07/27 14:16:45, scroggo wrote: > I don't see why this needs to be public. This has been removed. https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:25: const Options& options, SkPMColor ctable[], int* ctableCount); On 2015/07/27 14:16:45, scroggo wrote: > override? Done. https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:27: bool setExpandProc(SkColorType colorType, SkPMColor* ctable, int* ctableCount); On 2015/07/27 14:41:37, emmaleer wrote: > Could you add a comment about what this function does This has been replaced by initializeSwizzler(). https://codereview.chromium.org/1254483004/diff/1/src/codec/SkCodec_wbmp.h#ne... src/codec/SkCodec_wbmp.h:30: ExpandProc fProc; On 2015/07/27 14:16:45, scroggo wrote: > Does fProc need to be a member? It looks like it is only used in onGetPixels, > and it will be updated each time it is called. > > Maybe the expandProc function should be static, and return the ExpandProc. If it > returns NULL, that is an indication of failure. I use this pattern to create the swizzler.
Please add to the description that you also use the swizzler. (I guess that does not yet even have any payoff, since it just helps us with sampling, later. Please describe its future benefits.) On 2015/07/27 23:42:31, msarett wrote: > If we're not happy with this, I'm happy to switch us back to not using the > swizzler. > > The pro of this approach is that we can use the scaling that Emmalee is working > on in the swizzler. > > I did not foresee the con, which is dealing with 1-bit pixels in the swizzler. > My approach was to consider them as kIndex1, which required extra work to create > a color table. Another approach would be to add a bunch of new routines to the > swizzler (kBit_SrcConfig maybe?) that would only be useful for wbmp. The swizzler already deals with small indices, right? I do not see any problem with adding this instance of it. I'm not opposed to adding new routines to the swizzler, especially if it makes it faster. The main potential con I see in this CL is a possible slow down. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:14: // Each bit represents a pixel, so rowBytes can be calculated as width / 8 Almost - the code handles SkAlign8, but the comment does not acknowledge it: If the width is 7, rowBytes is 1, but 7 / 8 (in integer math) is 0. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:101: SkPMColor alternateCtable[2]; I believe in Skia we would typically name this something like "colorStorage". This name is fine, although it seems a little odd to have a capital "C" followed by lowercase "table" (although I suppose it fits with treating "ctable" as one word with no camelCase). https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:104: ctable[0] = SK_ColorBLACK; How do you feel about the following: if (kIndex_8_SkColorType == info.colorType()) { colorPtr = ctable; } else { colorPtr = alternateCtable; } colorPtr[0] = SK_ColorBLACK; colorPtr[1] = SK_ColorWHITE; https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:133: return SkCodec::kSuccess; I do not feel strongly about whether to add SkCodec:: here, but it is not needed, is it? https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:154: SkWbmpScanlineDecoder(const SkImageInfo& dstInfo, SkWbmpCodec* codec, /** * Takes ownership of all its pointer parameters. */ https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:170: this->fSwizzler->swizzle(dstRow, fSrcBuffer.get()); We use this->[methodName] so it is easy to distinguish between calling a member function and calling some other function. We label our member variables [fVariable] (for "Field") to distinguish them from other variables (i.e. local - globals get their own markers e.g. gGlobal). You do not need this-> here. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:207: // Prepare a color table Can you move this into a helper function? (I think we already have a static function which chooses between local storage and passed in pointers. The code to share here goes further, choosing its colors, but maybe it can call that function as well.) https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:220: SkAutoTDelete<SkColorTable> colorTable(SkNEW_ARGS(SkColorTable, (colorPtr, 2))); SkColorTable is refcounted, so it should be in an SkAutoTUnref, here and in the class. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:24: #define GRAYSCALE_BLACK 0 I'm kind of surprised we do not have these already defined somewhere. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:27: // This routine is currently expected to be used exclusively by wbmp. What is wbmp-specific about it? I suppose just that it's the only class which exclusively chooses between white and black? Maybe that would be a better description? https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:36: const uint32_t pixelsPerByte = 8 / bitsPerPixel; It's too bad we have to have a divide for each row, especially since it does not change for the whole image. Is there any way to factor this out and pass it to the function? Overall, do you know what the performance hit will be for switching to using the swizzler? It would be nice to minimize it.
I added a new mode to the swizzler and new swizzler routines. This should prevent a performance hit in changing wbmp to use the swizzler. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:14: // Each bit represents a pixel, so rowBytes can be calculated as width / 8 On 2015/07/28 14:03:34, scroggo wrote: > Almost - the code handles SkAlign8, but the comment does not acknowledge it: > > If the width is 7, rowBytes is 1, but 7 / 8 (in integer math) is 0. Agreed that the comment should acknowledge SkAlign8. I also think you are pointing out a bug that I have introduced. SkAlign8 should only apply to width, rounding up to the nearest 8 before dividing by 8. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:101: SkPMColor alternateCtable[2]; On 2015/07/28 14:03:34, scroggo wrote: > I believe in Skia we would typically name this something like "colorStorage". > This name is fine, although it seems a little odd to have a capital "C" followed > by lowercase "table" (although I suppose it fits with treating "ctable" as one > word with no camelCase). I agree, I prefer colorStorage :). Having said that, we don't need this anymore. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:104: ctable[0] = SK_ColorBLACK; On 2015/07/28 14:03:34, scroggo wrote: > How do you feel about the following: > > if (kIndex_8_SkColorType == info.colorType()) { > colorPtr = ctable; > } else { > colorPtr = alternateCtable; > } > > colorPtr[0] = SK_ColorBLACK; > colorPtr[1] = SK_ColorWHITE; Agreed, however, the new version only uses the ctable in kIndex_8 mode. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:133: return SkCodec::kSuccess; On 2015/07/28 14:03:34, scroggo wrote: > I do not feel strongly about whether to add SkCodec:: here, but it is not > needed, is it? Yep it's not needed. Removing it. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:154: SkWbmpScanlineDecoder(const SkImageInfo& dstInfo, SkWbmpCodec* codec, On 2015/07/28 14:03:34, scroggo wrote: > /** > * Takes ownership of all its pointer parameters. > */ Done. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:170: this->fSwizzler->swizzle(dstRow, fSrcBuffer.get()); On 2015/07/28 14:03:34, scroggo wrote: > We use this->[methodName] so it is easy to distinguish between calling a member > function and calling some other function. We label our member variables > [fVariable] (for "Field") to distinguish them from other variables (i.e. local - > globals get their own markers e.g. gGlobal). > > You do not need this-> here. Gotcha. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:207: // Prepare a color table On 2015/07/28 14:03:34, scroggo wrote: > Can you move this into a helper function? (I think we already have a static > function which chooses between local storage and passed in pointers. The code to > share here goes further, choosing its colors, but maybe it can call that > function as well.) Yes done! https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:220: SkAutoTDelete<SkColorTable> colorTable(SkNEW_ARGS(SkColorTable, (colorPtr, 2))); On 2015/07/28 14:03:34, scroggo wrote: > SkColorTable is refcounted, so it should be in an SkAutoTUnref, here and in the > class. Acknowledged. I'll try not to forget that next time :). https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:24: #define GRAYSCALE_BLACK 0 On 2015/07/28 14:03:34, scroggo wrote: > I'm kind of surprised we do not have these already defined somewhere. Me too, but I looked a little harder and I actually can't find them. They aren't with the others in SkColor.h. I could use SK_AlphaTRANSPARENT and SK_AlphaOPAQUE but I think that would be confusing. Also I could just put 0xFF and 0x00 in the code. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:27: // This routine is currently expected to be used exclusively by wbmp. On 2015/07/28 14:03:34, scroggo wrote: > What is wbmp-specific about it? I suppose just that it's the only class which > exclusively chooses between white and black? Maybe that would be a better > description? Agreed. https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:36: const uint32_t pixelsPerByte = 8 / bitsPerPixel; On 2015/07/28 14:03:34, scroggo wrote: > It's too bad we have to have a divide for each row, especially since it does not > change for the whole image. Is there any way to factor this out and pass it to > the function? > > Overall, do you know what the performance hit will be for switching to using the > swizzler? It would be nice to minimize it. Yeah I wanted wbmp to share with small_index, but that has lead to annoying color table code and a performance hit. I am adding kBit config to the swizzler and giving wbmp its own routines. These are almost identical to the original.
Sorry, I made these comments a while ago and then forgot to publish... https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1254483004/diff/20001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:24: #define GRAYSCALE_BLACK 0 On 2015/07/28 22:22:07, msarett wrote: > On 2015/07/28 14:03:34, scroggo wrote: > > I'm kind of surprised we do not have these already defined somewhere. > > Me too, but I looked a little harder and I actually can't find them. They > aren't with the others in SkColor.h. I could use SK_AlphaTRANSPARENT and > SK_AlphaOPAQUE but I think that would be confusing. Also I could just put 0xFF > and 0x00 in the code. No, I think what you've done here is perfect! https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:215: initializeSwizzler(dstInfo, ctable, options)); This seems dangerous. The swizzler will hang on to this pointer to memory provided by the caller, but we cannot guarantee that the caller's color table sticks around as long as the swizzler. It would be safer to keep a color table if it's used. https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:30: int bitsPerPixel, const SkPMColor ctable[]) { I believe these parameters are unused? I think we have some macros for documenting/enforcing that, but I believe they are used sporadically. Another approach would be to declare them like so: int /* bitsPerPixel */, const SkPMColor* /* ctable */ https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:34: U8CPU mask = *src++; I don't have an idea for a better name for this, but I feel like a "mask" is something which does masking, whereas this is the thing being masked? https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:38: dst += 8; It seems a little odd that here you index dst and then add 8, but below you do pointer arithmetic on dst. I suppose it's because here you happen to also need j for other reasons? https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:22: kBit, Should there be a comment explaining what kBit means? Maybe it's obvious? (A bit could be used to distinguish between opaque and transparent, which could actually map to the same values, so maybe that's okay? And it could also map to any arbitrary colors, but I suppose that would be kIndex1...)
On 2015/07/27 14:41:37, emmaleer wrote: > Is CodexTest.cpp supposed to be named CodecTest.cpp? You'll have to ask Hal, who wrote it. I hate that the name of the file does not match the name of the tests. I frequently try to run the following: dm --src tests --match Codex Which does not work. If anyone renames it to CodecTest, I will be very happy.
https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:215: initializeSwizzler(dstInfo, ctable, options)); On 2015/07/29 17:47:12, scroggo wrote: > This seems dangerous. The swizzler will hang on to this pointer to memory > provided by the caller, but we cannot guarantee that the caller's color table > sticks around as long as the swizzler. It would be safer to keep a color table > if it's used. Gotcha agreed. https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:30: int bitsPerPixel, const SkPMColor ctable[]) { On 2015/07/29 17:47:12, scroggo wrote: > I believe these parameters are unused? I think we have some macros for > documenting/enforcing that, but I believe they are used sporadically. > > Another approach would be to declare them like so: > > int /* bitsPerPixel */, const SkPMColor* /* ctable */ Done. https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:34: U8CPU mask = *src++; On 2015/07/29 17:47:12, scroggo wrote: > I don't have an idea for a better name for this, but I feel like a "mask" is > something which does masking, whereas this is the thing being masked? Yeah you're right. Renaming to currByte. https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:38: dst += 8; On 2015/07/29 17:47:12, scroggo wrote: > It seems a little odd that here you index dst and then add 8, but below you do > pointer arithmetic on dst. I suppose it's because here you happen to also need j > for other reasons? There is no reason for this other than that is how it was done before. Which is not a good reason. I prefer not using ptr arithmetic and have made changes. Sorry I didn't really look this routine over, I just borrowed it from SkCodec_wbmp. https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.h File src/codec/SkSwizzler.h (right): https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.h#... src/codec/SkSwizzler.h:22: kBit, On 2015/07/29 17:47:12, scroggo wrote: > Should there be a comment explaining what kBit means? Maybe it's obvious? (A bit > could be used to distinguish between opaque and transparent, which could > actually map to the same values, so maybe that's okay? And it could also map to > any arbitrary colors, but I suppose that would be kIndex1...) Adding a description.
https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1254483004/diff/40001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:38: dst += 8; On 2015/07/29 18:53:57, msarett wrote: > On 2015/07/29 17:47:12, scroggo wrote: > > It seems a little odd that here you index dst and then add 8, but below you do > > pointer arithmetic on dst. I suppose it's because here you happen to also need > j > > for other reasons? > > There is no reason for this other than that is how it was done before. Which is > not a good reason. > > I prefer not using ptr arithmetic and have made changes. Sorry I didn't really > look this routine over, I just borrowed it from SkCodec_wbmp. I do not have a problem with pointer arithmetic in general; my only confusion reading the code was that one block used it, and the other used indices. https://codereview.chromium.org/1254483004/diff/60001/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/60001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:215: setup_color_table(dstInfo.colorType(), inputColorTable, inputColorCount); Does this gracefully handle the case where the client asked for kIndex8 but passed no color ptr? (I think we otherwise try to handle that gracefully. If so, we should have a test for it.)
Patchset #5 (id:80001) has been deleted
Regardless of whether this is good or not, I'll hold off until you land "Create a scanline decoder without creating a codec (issue 1267583002)". https://codereview.chromium.org/1254483004/diff/60001/src/codec/SkCodec_wbmp.cpp File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/60001/src/codec/SkCodec_wbmp.... src/codec/SkCodec_wbmp.cpp:215: setup_color_table(dstInfo.colorType(), inputColorTable, inputColorCount); On 2015/07/29 21:49:17, scroggo wrote: > Does this gracefully handle the case where the client asked for kIndex8 but > passed no color ptr? > > (I think we otherwise try to handle that gracefully. If so, we should have a > test for it.) This does not handle it gracefully :(. I think it makes the most sense to handle this in SkCodec::getScanlineDecoder(). I have added a check there that is similar to what we have in SkCodec::getPixels(). I also added a test.
lgtm
I think this causes a merge conflict, since onGetScanlineDecoder has gone away, but is there anything else holding this up?
On 2015/08/04 17:50:22, scroggo wrote: > I think this causes a merge conflict, since onGetScanlineDecoder has gone away, > but is there anything else holding this up? Oh, yeah, of course - you'll need to switch to implementing onStart instead of onGetScanlineDecoder...
I know this already has approval, but I'd feel more comfortable if you can take another look. Thanks!
-lgtm See comments inline https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:206: SkAutoTUnref<SkColorTable> colorTable(NULL); It looks like you never initialize colorTable? (you do you initialize fColorTable, though) https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:212: fSwizzler.reset(fCodec->initializeSwizzler(dstInfo, How would you feel about storing the swizzler on the code? (that is what we do elsewhere) https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:213: get_color_ptr(colorTable.get()), options)); I think you want fColorTable.get()? colorTable is always NULL (and it would be a bad idea to use it even if it weren't NULL, since it will get deleted at the end of this function) https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkScanlineDe... File src/codec/SkScanlineDecoder.cpp (left): https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkScanlineDe... src/codec/SkScanlineDecoder.cpp:86: SkASSERT(kIndex_8_SkColorType != dstInfo.colorType()); This line is actually important - we cannot support index8 if the client did not supply a colortable, since they will not know what colors to use (they'll just have the indices). https://codereview.chromium.org/1254483004/diff/120001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1254483004/diff/120001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:303: decoder->getInfo().makeColorType(kIndex_8_SkColorType), NULL, NULL, NULL); FWIW, you could leave off the last three NULLs and call the version that just takes an SkImageInfo (or test both?) https://codereview.chromium.org/1254483004/diff/120001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:308: test_invalid_parameters(r, "mandrill_512.png"); Is this a kIndex8 image? It might be interesting to verify that this succeeds if you pass a color table, but not if you do not. (As a side note, maybe this should be kInvalidParameters, since it's not the conversion that's the problem, but the fact that you did not supply a color table.)
sorry, that's not lgtm
https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:206: SkAutoTUnref<SkColorTable> colorTable(NULL); On 2015/08/04 20:24:11, scroggo wrote: > It looks like you never initialize colorTable? (you do you initialize > fColorTable, though) Yeah my mistake. There is no need for colorTable anymore, just fColorTable. Thanks! https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:212: fSwizzler.reset(fCodec->initializeSwizzler(dstInfo, On 2015/08/04 20:24:11, scroggo wrote: > How would you feel about storing the swizzler on the code? (that is what we do > elsewhere) All the way back in Patch Set 1 when we were using fProc instead of the fSwizzler: "The scanline decoder will need to store fProc, but it's not clear to me that it needs to be set on the codec." When we switched to fSwizzler, I put it on the scanline decoder since we don't need it on the codec. But I would agree that it makes more sense to conform to the Codecs/SDs. https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:213: get_color_ptr(colorTable.get()), options)); On 2015/08/04 20:24:11, scroggo wrote: > I think you want fColorTable.get()? colorTable is always NULL (and it would be a > bad idea to use it even if it weren't NULL, since it will get deleted at the end > of this function) Yeah this is a bug. Now I'm confused about how my tests passed. Probably dumb luck. Nice catch thanks! https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkScanlineDe... File src/codec/SkScanlineDecoder.cpp (left): https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkScanlineDe... src/codec/SkScanlineDecoder.cpp:86: SkASSERT(kIndex_8_SkColorType != dstInfo.colorType()); On 2015/08/04 20:24:11, scroggo wrote: > This line is actually important - we cannot support index8 if the client did not > supply a colortable, since they will not know what colors to use (they'll just > have the indices). Happy to add this back in. I'm just unclear why it's necessary. Seems like we'll catch it in the check in the other start() implementation. https://codereview.chromium.org/1254483004/diff/120001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1254483004/diff/120001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:303: decoder->getInfo().makeColorType(kIndex_8_SkColorType), NULL, NULL, NULL); On 2015/08/04 20:24:11, scroggo wrote: > FWIW, you could leave off the last three NULLs and call the version that just > takes an SkImageInfo (or test both?) I put the SkASSERT back in. This means we will crash if try to test the other version. https://codereview.chromium.org/1254483004/diff/120001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:308: test_invalid_parameters(r, "mandrill_512.png"); On 2015/08/04 20:24:11, scroggo wrote: > Is this a kIndex8 image? It might be interesting to verify that this succeeds if > you pass a color table, but not if you do not. > > (As a side note, maybe this should be kInvalidParameters, since it's not the > conversion that's the problem, but the fact that you did not supply a color > table.) Yes kInvalidParameters is better! Yes it is better to test kSuccess on passing a ctable and kInvalidParameters when it is not passed in.
https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:212: fSwizzler.reset(fCodec->initializeSwizzler(dstInfo, On 2015/08/04 21:21:26, msarett wrote: > On 2015/08/04 20:24:11, scroggo wrote: > > How would you feel about storing the swizzler on the code? (that is what we do > > elsewhere) > > All the way back in Patch Set 1 when we were using fProc instead of the > fSwizzler: > > "The scanline decoder will need to store fProc, but it's not clear to me that it > needs to be set on the codec." The other issue with putting it on the codec was that we needed to typedef the proc in the header file, just so it could be a member variable in a class that does not use it. > > When we switched to fSwizzler, I put it on the scanline decoder since we don't > need it on the codec. But I would agree that it makes more sense to conform to > the Codecs/SDs. Well, not necessarily. Feel free to tell me I'm wrong :P If the Codec does not use it (except for scanline decoding), then maybe it belongs on the swizzler after all. (I take it the swizzler is only necessary for scanline decoding?) https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:213: get_color_ptr(colorTable.get()), options)); On 2015/08/04 21:21:26, msarett wrote: > On 2015/08/04 20:24:11, scroggo wrote: > > I think you want fColorTable.get()? colorTable is always NULL (and it would be > a > > bad idea to use it even if it weren't NULL, since it will get deleted at the > end > > of this function) > > Yeah this is a bug. Now I'm confused about how my tests passed. Probably dumb > luck. Nice catch thanks! Maybe the test didn't test what you thought it was testing? https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkScanlineDe... File src/codec/SkScanlineDecoder.cpp (left): https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkScanlineDe... src/codec/SkScanlineDecoder.cpp:86: SkASSERT(kIndex_8_SkColorType != dstInfo.colorType()); On 2015/08/04 21:21:26, msarett wrote: > On 2015/08/04 20:24:11, scroggo wrote: > > This line is actually important - we cannot support index8 if the client did > not > > supply a colortable, since they will not know what colors to use (they'll just > > have the indices). > > Happy to add this back in. I'm just unclear why it's necessary. Seems like > we'll catch it in the check in the other start() implementation. Oh, no; you were right to remove it. Say goodbye to redundant code! https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:137: if (kIndex_8_SkColorType == info.colorType()) { For consistency, I think you should also remove this check. (I see that in the full version of getPixels, we return kInvalidParameters already, as we should.) https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodecPriv.... src/codec/SkCodecPriv.h:34: * If there is a color table, get a pointer to the colors Otherwise return NULL. (/nullptr. Apparently we are updating the style guide to say to use nullptr instead of NULL. I have mixed feelings about using NULL some places and nullptr elsewhere; maybe someone will do a search and replace...) https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:127: fSwizzler.reset(initializeSwizzler(info, ctable, options)); Ah, now I understand. The swizzler *is* used by the codec, but it does not need to last beyond the call to onGetPixels. (We *could* keep it around, and in the next call check to see if we can reuse the same one, although that seems like some extra complicated code, without an obvious benefit.) The old code is probably better, where you put the swizzler in an auto deleter on the stack, and have the SD have its own swizzler. Sorry for the bad advice :-/ also, style nit: this->initializeSwizzler. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:168: SkWbmpScanlineDecoder(const SkImageInfo& dstInfo, SkWbmpCodec* codec) It is weird that you call this dstInfo here, but srcInfo when you pass it to this function. I think it's actually the srcInfo. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:172: , fSrcRowBytes(get_src_row_bytes(codec->getInfo().width())) FWIW, this->getInfo().width() is the same. (Maybe it's not ideal that internal codec and the SD have the same info value...) https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:179: if (fCodec->stream()->read(fSrcBuffer.get(), fSrcRowBytes) != fSrcRowBytes) { Maybe this should be a function on SkWbmpCodec? Result readRow(void* dst) { if (this->stream()->read(dst, fSrcRowBytes) != fSrcRowBytes) { return kIncompleteInput; } return kSuccess; } https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:236: const SkImageInfo& srcInfo = codec->getInfo(); This seems weird - you pass getInfo to the constructor, even though you could just get it from the codec, which also is passed to the constructor. https://codereview.chromium.org/1254483004/diff/140001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:307: REPORTER_ASSERT(r, SkCodec::kSuccess == result); Maybe return if the result is not success? It seems like the rest of the test is uninteresting otherwise.
https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:137: if (kIndex_8_SkColorType == info.colorType()) { Why would we need this check, if we assert above that info.colorType() != kIndex_8_SkColorType? https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:111: if (!this->handleRewind()) { I think it's best to call handleRewind after the other checks. So then if we will fail anyways we haven't spent time rewinding (I'm assuming rewinding takes time). The other checks are constant time, so I think it would be faster to check them first. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:139: return SkCodec::kIncompleteInput; Do you need SkCodec:: here? For the other return values above there isn't SkCodec:: before them. Did you change this for a specific reason?
https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:137: if (kIndex_8_SkColorType == info.colorType()) { On 2015/08/05 15:06:55, emmaleer wrote: > Why would we need this check, if we assert above that info.colorType() != > kIndex_8_SkColorType? For running in release. ASSERTs only happen in debug mode, so if we want to gracefully handle bad input, we would need to do a separate check. That said, in some cases we are willing to do the wrong thing and/or crash. When deciding whether to do both, I think you need to ask the following questions: - Is this a crucial piece of code that needs to go fast, so we should skip the check in release? - If we do not fail gracefully, is there a security risk? On a related note, I can imagine (at least) two flavors of assert: 1. Internal code - we are the only clients, so we *know* that we'll never hit the assert 2. APIs (like this case) - the assert could fire if the client called with bad parameters - in this case the assert gives the client testing their code clear feedback that they've done something wrong, and the check makes sure we handle it gracefully if the client ignores the assert - or never runs in debug mode, like on android :(. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:111: if (!this->handleRewind()) { On 2015/08/05 15:06:55, emmaleer wrote: > I think it's best to call handleRewind after the other checks. > So then if we will fail anyways we haven't spent time rewinding > (I'm assuming rewinding takes time). I'm not so sure about that assumption. For an SkMemoryStream, for example, it just means updating an offset. Various buffered streams ought to work the same way. For a file stream, I am guessing it would be the same. > The other checks are constant time, so I think it would be faster to check them > first. We might consider what errors are more likely to happen first. I do not know the answer to that question though.
https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:212: fSwizzler.reset(fCodec->initializeSwizzler(dstInfo, On 2015/08/05 14:36:20, scroggo wrote: > On 2015/08/04 21:21:26, msarett wrote: > > On 2015/08/04 20:24:11, scroggo wrote: > > > How would you feel about storing the swizzler on the code? (that is what we > do > > > elsewhere) > > > > All the way back in Patch Set 1 when we were using fProc instead of the > > fSwizzler: > > > > "The scanline decoder will need to store fProc, but it's not clear to me that > it > > needs to be set on the codec." > > The other issue with putting it on the codec was that we needed to typedef the > proc in the header file, just so it could be a member variable in a class that > does not use it. > > > > > When we switched to fSwizzler, I put it on the scanline decoder since we don't > > need it on the codec. But I would agree that it makes more sense to conform > to > > the Codecs/SDs. > > Well, not necessarily. Feel free to tell me I'm wrong :P If the Codec does not > use it (except for scanline decoding), then maybe it belongs on the swizzler > after all. (I take it the swizzler is only necessary for scanline decoding?) Ah yes I remember moving that typedef now. https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:213: get_color_ptr(colorTable.get()), options)); On 2015/08/05 14:36:20, scroggo wrote: > On 2015/08/04 21:21:26, msarett wrote: > > On 2015/08/04 20:24:11, scroggo wrote: > > > I think you want fColorTable.get()? colorTable is always NULL (and it would > be > > a > > > bad idea to use it even if it weren't NULL, since it will get deleted at the > > end > > > of this function) > > > > Yeah this is a bug. Now I'm confused about how my tests passed. Probably > dumb > > luck. Nice catch thanks! > > Maybe the test didn't test what you thought it was testing? Acknowledged. https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkScanlineDe... File src/codec/SkScanlineDecoder.cpp (left): https://codereview.chromium.org/1254483004/diff/120001/src/codec/SkScanlineDe... src/codec/SkScanlineDecoder.cpp:86: SkASSERT(kIndex_8_SkColorType != dstInfo.colorType()); On 2015/08/05 14:36:20, scroggo wrote: > On 2015/08/04 21:21:26, msarett wrote: > > On 2015/08/04 20:24:11, scroggo wrote: > > > This line is actually important - we cannot support index8 if the client did > > not > > > supply a colortable, since they will not know what colors to use (they'll > just > > > have the indices). > > > > Happy to add this back in. I'm just unclear why it's necessary. Seems like > > we'll catch it in the check in the other start() implementation. > > Oh, no; you were right to remove it. Say goodbye to redundant code! Done. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:137: if (kIndex_8_SkColorType == info.colorType()) { On 2015/08/05 14:36:20, scroggo wrote: > For consistency, I think you should also remove this check. > > (I see that in the full version of getPixels, we return kInvalidParameters > already, as we should.) Done. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:137: if (kIndex_8_SkColorType == info.colorType()) { On 2015/08/05 15:23:16, scroggo wrote: > On 2015/08/05 15:06:55, emmaleer wrote: > > Why would we need this check, if we assert above that info.colorType() != > > kIndex_8_SkColorType? > > For running in release. ASSERTs only happen in debug mode, so if we want to > gracefully handle bad input, we would need to do a separate check. > > That said, in some cases we are willing to do the wrong thing and/or crash. > > When deciding whether to do both, I think you need to ask the following > questions: > - Is this a crucial piece of code that needs to go fast, so we should skip the > check in release? > - If we do not fail gracefully, is there a security risk? > > On a related note, I can imagine (at least) two flavors of assert: > 1. Internal code - we are the only clients, so we *know* that we'll never hit > the assert > 2. APIs (like this case) - the assert could fire if the client called with bad > parameters - in this case the assert gives the client testing their code clear > feedback that they've done something wrong, and the check makes sure we handle > it gracefully if the client ignores the assert - or never runs in debug mode, > like on android :(. I'm wondering if this is an argument for me to add the assert back in... https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodecPriv.... src/codec/SkCodecPriv.h:34: * If there is a color table, get a pointer to the colors On 2015/08/05 14:36:20, scroggo wrote: > Otherwise return NULL. (/nullptr. Apparently we are updating the style guide to > say to use nullptr instead of NULL. I have mixed feelings about using NULL some > places and nullptr elsewhere; maybe someone will do a search and replace...) Updating the comment. I share your mixed feelings. Let me know if I'm wrong, but I'd like to keep using NULL for consistency until the find and replace happens. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:111: if (!this->handleRewind()) { On 2015/08/05 15:06:55, emmaleer wrote: > I think it's best to call handleRewind after the other checks. > So then if we will fail anyways we haven't spent time rewinding > (I'm assuming rewinding takes time). > The other checks are constant time, so I think it would be faster to check them > first. I think you're right to start thinking about in what order we should perform the parameter/state checks. I want to keep rewind first for consistency with the other codecs, but this is worth revisiting. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:127: fSwizzler.reset(initializeSwizzler(info, ctable, options)); On 2015/08/05 14:36:20, scroggo wrote: > Ah, now I understand. The swizzler *is* used by the codec, but it does not need > to last beyond the call to onGetPixels. (We *could* keep it around, and in the > next call check to see if we can reuse the same one, although that seems like > some extra complicated code, without an obvious benefit.) > > The old code is probably better, where you put the swizzler in an auto deleter > on the stack, and have the SD have its own swizzler. Sorry for the bad advice > :-/ > > also, style nit: this->initializeSwizzler. No worries. I'll take 99% spot on feedback :). https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:139: return SkCodec::kIncompleteInput; On 2015/08/05 15:06:55, emmaleer wrote: > Do you need SkCodec:: here? For the other return values above there isn't > SkCodec:: before them. > Did you change this for a specific reason? Nope it's not needed, nice catch! https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:168: SkWbmpScanlineDecoder(const SkImageInfo& dstInfo, SkWbmpCodec* codec) On 2015/08/05 14:36:20, scroggo wrote: > It is weird that you call this dstInfo here, but srcInfo when you pass it to > this function. I think it's actually the srcInfo. Yes I'll fix. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:172: , fSrcRowBytes(get_src_row_bytes(codec->getInfo().width())) On 2015/08/05 14:36:20, scroggo wrote: > FWIW, this->getInfo().width() is the same. (Maybe it's not ideal that internal > codec and the SD have the same info value...) Yeah that is strange...I think it makes sense to use "this" and mull over the fact that we store two infos. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:179: if (fCodec->stream()->read(fSrcBuffer.get(), fSrcRowBytes) != fSrcRowBytes) { On 2015/08/05 14:36:20, scroggo wrote: > Maybe this should be a function on SkWbmpCodec? > > Result readRow(void* dst) { > if (this->stream()->read(dst, fSrcRowBytes) != fSrcRowBytes) { > return kIncompleteInput; > } > return kSuccess; > } Sounds good. I renamed the parameter to src and added rowBytes as a param. It's awkward to call the pointer we pass in src, since we are writing to it, but I think it confuses me more to call it dst (since we have been using dst as the name of the output image memory. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:236: const SkImageInfo& srcInfo = codec->getInfo(); On 2015/08/05 14:36:20, scroggo wrote: > This seems weird - you pass getInfo to the constructor, even though you could > just get it from the codec, which also is passed to the constructor. Yes it is unnecessary, will fix. https://codereview.chromium.org/1254483004/diff/140001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:307: REPORTER_ASSERT(r, SkCodec::kSuccess == result); On 2015/08/05 14:36:20, scroggo wrote: > Maybe return if the result is not success? It seems like the rest of the test is > uninteresting otherwise. Done.
https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:137: if (kIndex_8_SkColorType == info.colorType()) { On 2015/08/05 15:29:42, msarett wrote: > On 2015/08/05 15:23:16, scroggo wrote: > > On 2015/08/05 15:06:55, emmaleer wrote: > > > Why would we need this check, if we assert above that info.colorType() != > > > kIndex_8_SkColorType? > > > > For running in release. ASSERTs only happen in debug mode, so if we want to > > gracefully handle bad input, we would need to do a separate check. > > > > That said, in some cases we are willing to do the wrong thing and/or crash. > > > > When deciding whether to do both, I think you need to ask the following > > questions: > > - Is this a crucial piece of code that needs to go fast, so we should skip the > > check in release? > > - If we do not fail gracefully, is there a security risk? > > > > On a related note, I can imagine (at least) two flavors of assert: > > 1. Internal code - we are the only clients, so we *know* that we'll never hit > > the assert > > 2. APIs (like this case) - the assert could fire if the client called with bad > > parameters - in this case the assert gives the client testing their code clear > > feedback that they've done something wrong, and the check makes sure we handle > > it gracefully if the client ignores the assert - or never runs in debug mode, > > like on android :(. > > I'm wondering if this is an argument for me to add the assert back in... Yes, I think the assert here would be good. https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkScanlineDe... File src/codec/SkScanlineDecoder.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkScanlineDe... src/codec/SkScanlineDecoder.cpp:101: SkASSERT(kIndex_8_SkColorType != dstInfo.colorType()); As I mentioned in the other location, I am in favor of the assert. https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:69: bool SkWbmpCodec::handleRewind() { I'm beginning to think this should be in the base class: bool SkCodec::rewindIfNeeded() { const bool needsRewind = fNeedsRewind; if (!needsRewind) { return true; } if (!fStream->rewind()) { return false; } return this->onRewind(); // this function is virtual } Then the subclasses can just say if (!this->rewindIfNeeded()) { return kCouldNotRewind; } bool onRewind() override { // or maybe it should return SkCodec::Result? return read_header(...) } This should be in a separate CL, but if all codecs have this function, I think this would be a cleaner, more code-shared way to do it. https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:74: (void)read_header(this->stream(), NULL); FWIW, in SkPngCodec, we check the return value of read_header. It certainly should return true the second time if it returned true the first time, so maybe this is unnecessary, but I think we should probably do the same thing everywhere? https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:96: SkCodec::Result SkWbmpCodec::readRow(uint8_t* src, size_t rowBytes) { Why do you need the rowBytes parameter? This is based on the source data, right? so it should always be the same for one image/codec/SD? ____________________________________________________________________________ I think it's even weirder to call this src. It is the destination for readRow, so I would argue that dst is better, even if the caller of readRow uses it as a source. You could also call it something like rowPointer or just row. (libpng, for example, calls it row) https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:97: if (this->stream()->read(src, rowBytes) != rowBytes) { nit: we use four space indents (Ah, I think you copied the code from my suggestion? I just use two spaces in comments because of the limited room, and the fact that "tab" does not get converted to spaces.)
Patchset #9 (id:180001) has been deleted
Patchset #9 (id:200001) has been deleted
https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:137: if (kIndex_8_SkColorType == info.colorType()) { On 2015/08/05 17:55:37, scroggo wrote: > On 2015/08/05 15:29:42, msarett wrote: > > On 2015/08/05 15:23:16, scroggo wrote: > > > On 2015/08/05 15:06:55, emmaleer wrote: > > > > Why would we need this check, if we assert above that info.colorType() != > > > > kIndex_8_SkColorType? > > > > > > For running in release. ASSERTs only happen in debug mode, so if we want to > > > gracefully handle bad input, we would need to do a separate check. > > > > > > That said, in some cases we are willing to do the wrong thing and/or crash. > > > > > > When deciding whether to do both, I think you need to ask the following > > > questions: > > > - Is this a crucial piece of code that needs to go fast, so we should skip > the > > > check in release? > > > - If we do not fail gracefully, is there a security risk? > > > > > > On a related note, I can imagine (at least) two flavors of assert: > > > 1. Internal code - we are the only clients, so we *know* that we'll never > hit > > > the assert > > > 2. APIs (like this case) - the assert could fire if the client called with > bad > > > parameters - in this case the assert gives the client testing their code > clear > > > feedback that they've done something wrong, and the check makes sure we > handle > > > it gracefully if the client ignores the assert - or never runs in debug > mode, > > > like on android :(. > > > > I'm wondering if this is an argument for me to add the assert back in... > > Yes, I think the assert here would be good. Per our discussion in person, I'm not adding an assert. This would make our kInvalidParameter test in CodexTest.cpp fail. https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:69: bool SkWbmpCodec::handleRewind() { On 2015/08/05 17:55:37, scroggo wrote: > I'm beginning to think this should be in the base class: > > bool SkCodec::rewindIfNeeded() { > const bool needsRewind = fNeedsRewind; > if (!needsRewind) { > return true; > } > if (!fStream->rewind()) { > return false; > } > return this->onRewind(); // this function is virtual > } > > Then the subclasses can just say > > if (!this->rewindIfNeeded()) { > return kCouldNotRewind; > } > > bool onRewind() override { // or maybe it should return SkCodec::Result? > return read_header(...) > } > > This should be in a separate CL, but if all codecs have this function, I think > this would be a cleaner, more code-shared way to do it. +1 for this in a separate CL https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:74: (void)read_header(this->stream(), NULL); On 2015/08/05 17:55:37, scroggo wrote: > FWIW, in SkPngCodec, we check the return value of read_header. It certainly > should return true the second time if it returned true the first time, so maybe > this is unnecessary, but I think we should probably do the same thing > everywhere? Agreed. Fixed this in wbmp. https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:96: SkCodec::Result SkWbmpCodec::readRow(uint8_t* src, size_t rowBytes) { On 2015/08/05 17:55:37, scroggo wrote: > Why do you need the rowBytes parameter? This is based on the source data, right? > so it should always be the same for one image/codec/SD? > ____________________________________________________________________________ > > I think it's even weirder to call this src. It is the destination for readRow, > so I would argue that dst is better, even if the caller of readRow uses it as a > source. > > You could also call it something like rowPointer or just row. (libpng, for > example, calls it row) Let's compromise on row as the name for the first parameter. Getting rid of the second parameter by making fSrcRowBytes a field of codec. https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:97: if (this->stream()->read(src, rowBytes) != rowBytes) { On 2015/08/05 17:55:37, scroggo wrote: > nit: we use four space indents (Ah, I think you copied the code from my > suggestion? I just use two spaces in comments because of the limited room, and > the fact that "tab" does not get converted to spaces.) Yup it's fixed now.
Is the image you're checking in open source? Make sure it's okay to check in. LGTM When you check this in, watch gold.skia.org - this should produce no new images, right? https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1254483004/diff/140001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:137: if (kIndex_8_SkColorType == info.colorType()) { On 2015/08/05 18:50:33, msarett wrote: > On 2015/08/05 17:55:37, scroggo wrote: > > On 2015/08/05 15:29:42, msarett wrote: > > > On 2015/08/05 15:23:16, scroggo wrote: > > > > On 2015/08/05 15:06:55, emmaleer wrote: > > > > > Why would we need this check, if we assert above that info.colorType() > != > > > > > kIndex_8_SkColorType? > > > > > > > > For running in release. ASSERTs only happen in debug mode, so if we want > to > > > > gracefully handle bad input, we would need to do a separate check. > > > > > > > > That said, in some cases we are willing to do the wrong thing and/or > crash. > > > > > > > > When deciding whether to do both, I think you need to ask the following > > > > questions: > > > > - Is this a crucial piece of code that needs to go fast, so we should skip > > the > > > > check in release? > > > > - If we do not fail gracefully, is there a security risk? > > > > > > > > On a related note, I can imagine (at least) two flavors of assert: > > > > 1. Internal code - we are the only clients, so we *know* that we'll never > > hit > > > > the assert > > > > 2. APIs (like this case) - the assert could fire if the client called with > > bad > > > > parameters - in this case the assert gives the client testing their code > > clear > > > > feedback that they've done something wrong, and the check makes sure we > > handle > > > > it gracefully if the client ignores the assert - or never runs in debug > > mode, > > > > like on android :(. > > > > > > I'm wondering if this is an argument for me to add the assert back in... > > > > Yes, I think the assert here would be good. > > Per our discussion in person, I'm not adding an assert. This would make our > kInvalidParameter test in CodexTest.cpp fail. An alternative I just thought of - we could have the assert and only run the test in release. I do not know if that is a good idea or a terrible one though... https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/160001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:69: bool SkWbmpCodec::handleRewind() { On 2015/08/05 18:50:33, msarett wrote: > On 2015/08/05 17:55:37, scroggo wrote: > > I'm beginning to think this should be in the base class: > > > > bool SkCodec::rewindIfNeeded() { > > const bool needsRewind = fNeedsRewind; > > if (!needsRewind) { > > return true; > > } > > if (!fStream->rewind()) { > > return false; > > } > > return this->onRewind(); // this function is virtual > > } > > > > Then the subclasses can just say > > > > if (!this->rewindIfNeeded()) { > > return kCouldNotRewind; > > } > > > > bool onRewind() override { // or maybe it should return SkCodec::Result? > > return read_header(...) > > } > > > > This should be in a separate CL, but if all codecs have this function, I think > > this would be a cleaner, more code-shared way to do it. > > +1 for this in a separate CL Filed skbug.com/4170 https://codereview.chromium.org/1254483004/diff/220001/src/codec/SkCodec_wbmp... File src/codec/SkCodec_wbmp.cpp (right): https://codereview.chromium.org/1254483004/diff/220001/src/codec/SkCodec_wbmp... src/codec/SkCodec_wbmp.cpp:83: // TODO (msarett): Reenable support for 565 if it is desired I think we do want to support 565, in order to be compatible with SkImageDecoder. (I am in fact working on a CL right now to enable it for other codecs.) It does not have to be a part of this CL (which I think is otherwise ready to submit), but I think you'll be able to write it more easily since you've thought about this more.
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/1254483004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1254483004/220001
Message was sent while issue was closed.
Committed patchset #9 (id:220001) as https://skia.googlesource.com/skia/+/99f567e617b6c5a81e6b822c30ccb0d357db21fc |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
