Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1022673011:
Creating a new wrapper for gif decoder (Closed)
DescriptionCreating a new wrapper for gif decoder
BUG=skia:3257
BUG=skia:3534
Committed: https://skia.googlesource.com/skia/+/8c8f22a3bba18edaad41d6ac8451a77aa093a958
Patch Set 1 : Adding gif functionality to SkCodec #
Total comments: 90
Patch Set 2 : First round of gif improvements #
Total comments: 32
Patch Set 3 : Second set of fixes to gif codec #
Total comments: 4
Patch Set 4 : Ready to land after giflib lands #Patch Set 5 : Merge with Hal's wbmp changes #Patch Set 6 : Trybot fix - unitialized variable #Patch Set 7 : giflib for Windows #Patch Set 8 : Windows - added more defines #Patch Set 9 : Trying cflags approach #Patch Set 10 : Removing original define #Patch Set 11 : Last attempt #Patch Set 12 : Flags within giflib #Patch Set 13 : More flags #Patch Set 14 : Using a dummy file for windows #Patch Set 15 : Dummy giflib include for windows #Patch Set 16 : Dummy giflib include for windows 2 #Patch Set 17 : Disable warnings on windows #Patch Set 18 : Final changes #Patch Set 19 : Last time #
Total comments: 3
Patch Set 20 : Warnings mostly disabled #
Total comments: 30
Patch Set 21 : Only include extra file on windows #Patch Set 22 : Fixes from last round #
Total comments: 6
Patch Set 23 : Added comments and do while loop #
Messages
Total messages: 64 (36 generated)
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
Enabling Gif Decoding for SkCodec Gif TODOs Not Addressed by this CL: ***There is a test case for a very large image that takes about 5 minutes to run. Chromium does not allow images that are this large. I have not taken any steps to deal with this issue. SkCodec may want to create a uniform maximum size for all decoders? ***Animated Gifs are unsupported. We plan to come back to this after implementing the basic functionality. ***Multiple Gifs in the same image frame are unsupported. It is unclear whether there are any actual gifs that expect this behavior. ***Is there a way to update the gif library so we no longer need all of the #if GIF_LIB_MAJOR statements?
On 2015/03/25 15:30:46, msarett wrote: > Enabling Gif Decoding for SkCodec > > Gif TODOs Not Addressed by this CL: > > ***There is a test case for a very large image that takes about 5 minutes to > run. Chromium does not allow images that are this large. I have not taken any > steps to deal with this issue. SkCodec may want to create a uniform maximum > size for all decoders? That seems reasonable. We can make that a part of NewFromStream. > > ***Is there a way to update the gif library so we no longer need all of the #if > GIF_LIB_MAJOR statements? We could update the gif library we use by default, but Android potentially uses a different one, and maybe someone else wants to use a different one. This reminds me though: we don't expect Windows and Mac to have gif installed, so we may want to include the library, like we did with libpng. See https://codereview.chromium.org/930283002 - the DEPS file allows us to download the library with gclient sync. Then we'll need to build it with a gyp file. https://codereview.chromium.org/1022673011/diff/20001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/dm/DM.cpp#newcode157 dm/DM.cpp:157: "bmp", "gif", "png", "webp", "ico", We don't support webp yet. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:44: int32_t SkGifCodec::close_gif(GifFileType* gif) { It looks like this returns an int in the header? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:81: const ExtensionBlock* eb = image.ExtensionBlocks + i; Is this an array? i.e. Can this be const ExtensionBlock* eb = image.ExtensionBlocks[i]; ? Also, I don't find "eb" to be a very informative name. Maybe extBlock? What is an extension block, after all? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:82: if (GRAPHICS_EXT_FUNC_CODE == eb->Function && 4 == eb->ByteCount) { Can you add some comments to this function? I'm having trouble following what's going on. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:85: if (transIndex >= (signed) colorCount) { I'm confused: transIndex is declared as an int. When you assign to it, you cast to an unsigned type. When you compare it to another unsigned type, you cast that one to signed. Can we stick with a type and avoid (at least some of) this casting? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:88: break; This might be a matter of preference, but what if we returned transIndex here (and declared it in this block), and then returned -1 at the end? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:117: // FIXME: Gifs are naturally stored as kIndex. We should default to this Go ahead and return kIndex. I do think it's a little weird that we don't support the color type that we report, but this better matches the data. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:120: // that they are stored as kUnpremul. Gifs without this index are Refresh my memory: the colors besides the transparent color are guaranteed to be opaque, right? This comment seems a little misleading - I think you mean we're guessing that it's not opaque, since it *might* have a transparent color, as opposed to whether the colors are premultiplied or not. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:166: SkDebugf("Error: scaling not supported.\n"); gif_error? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:170: SkDebugf("Error: cannot convert input type to output type.\n"); gif_error? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:176: SavedImage saveExt; It looks like this is only needed if GIFLIB_MAJOR < 5? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:185: uint32_t fillIndex = fGif->SBackGroundColor; Can you move this variable closer to where it's used? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:190: int width = dstInfo.width(); Can these be const? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:226: gif_error("Inner image too wide, shrinking.\n"); Should this be a gif_warning? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:230: // What are the implications of this decision? I think this is the right decision. At this point, the caller has already allocated their memory, so we don't have a way to correct that. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:234: gif_error("Shifting inner image to left to fit.\n"); gif_warning? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:241: gif_error("Inner image too tall, shrinking.\n"); gif_warning? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:282: int transIndex = find_trans_index(saveExt, colorCount); Maybe add a scope here, since transIndex is only used here? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:287: fillIndex = 0; How did you choose 0? Can you add a comment explaining this decision? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:297: SkNEW_ARGS(SkColorTable, (colorPtr, colorCount))); I don't think you need an SkColorTable here. It would come in handy if you wanted to keep it outside this function, but since you only use it here (I think), it seems like you can just use colorPtr directly. (That said, if we *do* decide to support kIndex8, we will need an SkColorTable, but we can add this if/when we add that support.) https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:323: colorTable->operator[](fillIndex), It's possible that the color you're filling with is zero. If so, and Options.fYes_ZeroInitialized, you can skip this. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:326: // case kIndex_SkColorType: I'm fine with supporting kIndex. Android *might* decide they want it after all. One downside is that we don't have a good way to test - DM checks the destination color type in order to decide which type to try for, and it will never be kIndex, since we cannot draw to kIndex. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:345: SkImageGenerator::kNo_ZeroInitialized)); Why did you use this instead of the value in Options? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:367: gif_error("Could not decode line.\n"); Maybe specify what line you're on? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:370: for (; y < innerHeight; y++) { This isn't quite right, is it? You have switched the version of next that is called. This image is interlaced, so you could have failed on any arbitrary line. Maybe you want to call next with iter.nextY()? Also, in both cases, if the fillIndex is 0, this is unnecessary if kYes_ZeroInitialized. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:385: swizzler->next(buffer.get()); Is the swizzler overkill here? Could we just use memset/memset32? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:459: case TERMINATE_RECORD_TYPE: Handle skbug.com/3534? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:468: free_extension(&saveExt); Can you put this in an SkAutoCallVProc? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.h File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:48: static int close_gif(GifFileType* gif); nit: If part of the header, this should follow a different convention: static int CloseGif(GifFileType*); (Also no need for "gif", since it doesn't add any information here.) https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:62: SkAutoTCallIProc<GifFileType, close_gif> fAutoClose; Is it possible to just store fGif here, and only have one variable? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... File src/codec/SkGifInterlaceIter.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.cpp:10: static const uint8_t kStartingInterlaceYValue[] = { 0, 4, 2, 1 }; nit: Values, plural. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.cpp:24: // We went from an if statement to a while loop so that we iterate Are you saying that the old decoder trashed memory? I would reword this comment to explain why we're doing it this way, without referencing the other way. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... File src/codec/SkGifInterlaceIter.h (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:13: class SkGifInterlaceIter { Unless you want this to be copyable, make it inherit from SkNoncopyable (in SkTypes.h) https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:17: * Constructor This comment does not provide any useful information, so we don't need it. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:19: SkGifInterlaceIter(int height); We typically declare constructors that take one parameter explicit, unless they are intended to be called implicitly. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:24: uint32_t nextY(); It's weird that this returns a uint32_t while we create one with an int. (Is this because the caller expects something unsigned?) https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:33: // Fields Again, this comment is unnecessary.
https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:470: return kSuccess; This function is over 300 lines. Can it be broken up?
Patchset #2 (id:40001) has been deleted
First round of gif improvements Note that CL 1038863003 will include the gif library on all platforms and needs to be landed first. https://codereview.chromium.org/1022673011/diff/20001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/dm/DM.cpp#newcode157 dm/DM.cpp:157: "bmp", "gif", "png", "webp", "ico", On 2015/03/25 19:44:48, scroggo wrote: > We don't support webp yet. Yeah you're right. Copy paste error. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:44: int32_t SkGifCodec::close_gif(GifFileType* gif) { On 2015/03/25 19:44:48, scroggo wrote: > It looks like this returns an int in the header? I'll make it uniform. Is there a difference between using int and int32_t? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:81: const ExtensionBlock* eb = image.ExtensionBlocks + i; On 2015/03/25 19:44:49, scroggo wrote: > Is this an array? i.e. Can this be > > const ExtensionBlock* eb = image.ExtensionBlocks[i]; > > ? > > Also, I don't find "eb" to be a very informative name. Maybe extBlock? What is > an extension block, after all? Yes this is an array. Sorry will fix up this helper function. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:82: if (GRAPHICS_EXT_FUNC_CODE == eb->Function && 4 == eb->ByteCount) { On 2015/03/25 19:44:48, scroggo wrote: > Can you add some comments to this function? I'm having trouble following what's > going on. Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:85: if (transIndex >= (signed) colorCount) { On 2015/03/25 19:44:49, scroggo wrote: > I'm confused: > > transIndex is declared as an int. > When you assign to it, you cast to an unsigned type. > When you compare it to another unsigned type, you cast that one to signed. > > Can we stick with a type and avoid (at least some of) this casting? Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:88: break; On 2015/03/25 19:44:48, scroggo wrote: > This might be a matter of preference, but what if we returned transIndex here > (and declared it in this block), and then returned -1 at the end? Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:117: // FIXME: Gifs are naturally stored as kIndex. We should default to this On 2015/03/25 19:44:49, scroggo wrote: > Go ahead and return kIndex. I do think it's a little weird that we don't support > the color type that we report, but this better matches the data. Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:120: // that they are stored as kUnpremul. Gifs without this index are On 2015/03/25 19:44:49, scroggo wrote: > Refresh my memory: the colors besides the transparent color are guaranteed to be > opaque, right? > > This comment seems a little misleading - I think you mean we're guessing that > it's not opaque, since it *might* have a transparent color, as opposed to > whether the colors are premultiplied or not. Yes, colors that are not the transparent color are guaranteed to be opaque. And yes, the comment is unclear, I meant that we are guessing that it is not opaque. Thinking through it again, it makes the most sense to mark kPremul, since pixels will either be completely opaque or all components will be zeroed. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:166: SkDebugf("Error: scaling not supported.\n"); On 2015/03/25 19:44:48, scroggo wrote: > gif_error? Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:170: SkDebugf("Error: cannot convert input type to output type.\n"); On 2015/03/25 19:44:48, scroggo wrote: > gif_error? Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:176: SavedImage saveExt; On 2015/03/25 19:44:48, scroggo wrote: > It looks like this is only needed if GIFLIB_MAJOR < 5? It's actually needed for both. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:185: uint32_t fillIndex = fGif->SBackGroundColor; On 2015/03/25 19:44:48, scroggo wrote: > Can you move this variable closer to where it's used? Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:190: int width = dstInfo.width(); On 2015/03/25 19:44:48, scroggo wrote: > Can these be const? Yes. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:226: gif_error("Inner image too wide, shrinking.\n"); On 2015/03/25 19:44:48, scroggo wrote: > Should this be a gif_warning? gif_error, does not return it only prints a message. I should probably change the name to make that clear. I use it for errors and warnings. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:230: // What are the implications of this decision? On 2015/03/25 19:44:48, scroggo wrote: > I think this is the right decision. At this point, the caller has already > allocated their memory, so we don't have a way to correct that. Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:234: gif_error("Shifting inner image to left to fit.\n"); On 2015/03/25 19:44:49, scroggo wrote: > gif_warning? Acknowledged. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:241: gif_error("Inner image too tall, shrinking.\n"); On 2015/03/25 19:44:48, scroggo wrote: > gif_warning? Acknowledged. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:282: int transIndex = find_trans_index(saveExt, colorCount); On 2015/03/25 19:44:49, scroggo wrote: > Maybe add a scope here, since transIndex is only used here? Done. What is the benefit of adding this type of scope? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:287: fillIndex = 0; On 2015/03/25 19:44:49, scroggo wrote: > How did you choose 0? Can you add a comment explaining this decision? I don't really have a great reason. This only occurs on invalid inputs. Basically it is the same way ImageDecoder deals with this case. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:297: SkNEW_ARGS(SkColorTable, (colorPtr, colorCount))); On 2015/03/25 19:44:48, scroggo wrote: > I don't think you need an SkColorTable here. It would come in handy if you > wanted to keep it outside this function, but since you only use it here (I > think), it seems like you can just use colorPtr directly. > > (That said, if we *do* decide to support kIndex8, we will need an SkColorTable, > but we can add this if/when we add that support.) Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:323: colorTable->operator[](fillIndex), On 2015/03/25 19:44:48, scroggo wrote: > It's possible that the color you're filling with is zero. If so, and > Options.fYes_ZeroInitialized, you can skip this. Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:326: // case kIndex_SkColorType: On 2015/03/25 19:44:48, scroggo wrote: > I'm fine with supporting kIndex. Android *might* decide they want it after all. > > One downside is that we don't have a good way to test - DM checks the > destination color type in order to decide which type to try for, and it will > never be kIndex, since we cannot draw to kIndex. That's interesting, I wasn't aware of that. My thinking is to put this on hold until we are finished supporting kN32. Then hopefully if we want to support other color types the changes will almost all be in the swizzler. I mainly want to draw attention to color type specific code outside of the swizzler. Does that seem reasonable? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:345: SkImageGenerator::kNo_ZeroInitialized)); On 2015/03/25 19:44:49, scroggo wrote: > Why did you use this instead of the value in Options? No idea. This is clearly wrong, thanks. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:367: gif_error("Could not decode line.\n"); On 2015/03/25 19:44:49, scroggo wrote: > Maybe specify what line you're on? Good idea. I know that information is useful because I printed it out when I was debugging. Seems to makes sense to make it permanent. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:370: for (; y < innerHeight; y++) { On 2015/03/25 19:44:49, scroggo wrote: > This isn't quite right, is it? You have switched the version of next that is > called. > > This image is interlaced, so you could have failed on any arbitrary line. Maybe > you want to call next with iter.nextY()? > > Also, in both cases, if the fillIndex is 0, this is unnecessary if > kYes_ZeroInitialized. Nice catch. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:385: swizzler->next(buffer.get()); On 2015/03/25 19:44:48, scroggo wrote: > Is the swizzler overkill here? Could we just use memset/memset32? Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:459: case TERMINATE_RECORD_TYPE: On 2015/03/25 19:44:49, scroggo wrote: > Handle skbug.com/3534? I added a check for 0x0 images in NewFromStream. Additionally, if we break out of the loop, it means that we have not decoded an image. I now return failure in this case. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:468: free_extension(&saveExt); On 2015/03/25 19:44:48, scroggo wrote: > Can you put this in an SkAutoCallVProc? Yes definitely. The way it is set up now seems to leak every time. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:470: return kSuccess; On 2015/03/25 19:56:58, scroggo wrote: > This function is over 300 lines. Can it be broken up? This is unfortunate. I can't find a good way to break it up. I thought about refactoring all of the innerWidth/Height vs width/height checks but we may make modifications to these values and use them later. I actually wrote a SkPMColor* createColorTable() function, but it caused issues. We end up needing to pass around fill/transIndex information that needs access to the colorCount and also is used later. I think it's best to keep it together, but I'm sure it is actually possible to break it up, and I'm happy to do it if you feel strongly. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.h File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:48: static int close_gif(GifFileType* gif); On 2015/03/25 19:44:49, scroggo wrote: > nit: If part of the header, this should follow a different convention: > > static int CloseGif(GifFileType*); > > (Also no need for "gif", since it doesn't add any information here.) Per another improvement, this is no longer part of the header. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:62: SkAutoTCallIProc<GifFileType, close_gif> fAutoClose; On 2015/03/25 19:44:49, scroggo wrote: > Is it possible to just store fGif here, and only have one variable? Yes it is! https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... File src/codec/SkGifInterlaceIter.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.cpp:10: static const uint8_t kStartingInterlaceYValue[] = { 0, 4, 2, 1 }; On 2015/03/25 19:44:49, scroggo wrote: > nit: Values, plural. Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.cpp:24: // We went from an if statement to a while loop so that we iterate On 2015/03/25 19:44:49, scroggo wrote: > Are you saying that the old decoder trashed memory? > > I would reword this comment to explain why we're doing it this way, without > referencing the other way. This comment is straight from the old decoder. I assume it was added after a bug fix. I thought it might be important and worth preserving. I added a new comment that hopefully makes more sense. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... File src/codec/SkGifInterlaceIter.h (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:13: class SkGifInterlaceIter { On 2015/03/25 19:44:49, scroggo wrote: > Unless you want this to be copyable, make it inherit from SkNoncopyable (in > SkTypes.h) Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:17: * Constructor On 2015/03/25 19:44:49, scroggo wrote: > This comment does not provide any useful information, so we don't need it. Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:19: SkGifInterlaceIter(int height); On 2015/03/25 19:44:49, scroggo wrote: > We typically declare constructors that take one parameter explicit, unless they > are intended to be called implicitly. Done. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:24: uint32_t nextY(); On 2015/03/25 19:44:49, scroggo wrote: > It's weird that this returns a uint32_t while we create one with an int. (Is > this because the caller expects something unsigned?) This code now deals with int32_t uniformly. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:33: // Fields On 2015/03/25 19:44:49, scroggo wrote: > Again, this comment is unnecessary. Done.
https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:44: int32_t SkGifCodec::close_gif(GifFileType* gif) { On 2015/03/26 19:15:57, msarett wrote: > On 2015/03/25 19:44:48, scroggo wrote: > > It looks like this returns an int in the header? > > I'll make it uniform. Is there a difference between using int and int32_t? int is platform dependent, and may be 2 or 4 bytes (possibly other sizes?). int32_t is guaranteed 4 bytes. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:82: if (GRAPHICS_EXT_FUNC_CODE == eb->Function && 4 == eb->ByteCount) { On 2015/03/26 19:15:56, msarett wrote: > On 2015/03/25 19:44:48, scroggo wrote: > > Can you add some comments to this function? I'm having trouble following > what's > > going on. > > Done. This is helpful. Thanks! https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:117: // FIXME: Gifs are naturally stored as kIndex. We should default to this On 2015/03/26 19:15:56, msarett wrote: > On 2015/03/25 19:44:49, scroggo wrote: > > Go ahead and return kIndex. I do think it's a little weird that we don't > support > > the color type that we report, but this better matches the data. > > Done. So I've realized that in order to return kIndex8, we need to support that as an output option, in order for clients of SkImageGenerator to work properly. So we need to implement the swizzler for index 8, which should not be hard. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:120: // that they are stored as kUnpremul. Gifs without this index are On 2015/03/26 19:15:57, msarett wrote: > On 2015/03/25 19:44:49, scroggo wrote: > > Refresh my memory: the colors besides the transparent color are guaranteed to > be > > opaque, right? > > > > This comment seems a little misleading - I think you mean we're guessing that > > it's not opaque, since it *might* have a transparent color, as opposed to > > whether the colors are premultiplied or not. > > Yes, colors that are not the transparent color are guaranteed to be opaque. And > yes, the comment is unclear, I meant that we are guessing that it is not opaque. > > Thinking through it again, it makes the most sense to mark kPremul, since pixels > will either be completely opaque or all components will be zeroed. To be fair, ARGB 0 0 0 0 is a valid premul AND unpremul color. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:176: SavedImage saveExt; On 2015/03/26 19:15:56, msarett wrote: > On 2015/03/25 19:44:48, scroggo wrote: > > It looks like this is only needed if GIFLIB_MAJOR < 5? > > It's actually needed for both. But it looks like you've deleted it? Oh, you've moved it up to the top. Why? Its comment is still down here though. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:226: gif_error("Inner image too wide, shrinking.\n"); On 2015/03/26 19:15:57, msarett wrote: > On 2015/03/25 19:44:48, scroggo wrote: > > Should this be a gif_warning? > > gif_error, does not return it only prints a message. I should probably change > the name to make that clear. I use it for errors and warnings. We could also have a separate one for errors vs warnings. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:326: // case kIndex_SkColorType: On 2015/03/26 19:15:56, msarett wrote: > On 2015/03/25 19:44:48, scroggo wrote: > > I'm fine with supporting kIndex. Android *might* decide they want it after > all. > > > > One downside is that we don't have a good way to test - DM checks the > > destination color type in order to decide which type to try for, and it will > > never be kIndex, since we cannot draw to kIndex. > > That's interesting, I wasn't aware of that. My thinking is to put this on hold > until we are finished supporting kN32. Then hopefully if we want to support > other color types the changes will almost all be in the swizzler. I mainly want > to draw attention to color type specific code outside of the swizzler. > > Does that seem reasonable? Sure. Go ahead and ignore my other comments about supporting kIndex8 for now. But change the default to kN32 with a TODO that it should be kIndex8 once we support that. (Sorry for flip flopping...) https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:468: free_extension(&saveExt); On 2015/03/26 19:15:56, msarett wrote: > On 2015/03/25 19:44:48, scroggo wrote: > > Can you put this in an SkAutoCallVProc? > > Yes definitely. The way it is set up now seems to leak every time. Oh, you mean before updating? And now it's fixed? Good. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:470: return kSuccess; On 2015/03/26 19:15:56, msarett wrote: > On 2015/03/25 19:56:58, scroggo wrote: > > This function is over 300 lines. Can it be broken up? > > This is unfortunate. > > I can't find a good way to break it up. I thought about refactoring all of the > innerWidth/Height vs width/height checks but we may make modifications to these > values and use them later. > > I actually wrote a SkPMColor* createColorTable() function, but it caused issues. > We end up needing to pass around fill/transIndex information that needs access > to the colorCount and also is used later. > > I think it's best to keep it together, but I'm sure it is actually possible to > break it up, and I'm happy to do it if you feel strongly. Only break it up if there's a clear way to do it. I'm fine with it as is. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.h File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:62: SkAutoTCallIProc<GifFileType, close_gif> fAutoClose; On 2015/03/26 19:15:57, msarett wrote: > On 2015/03/25 19:44:49, scroggo wrote: > > Is it possible to just store fGif here, and only have one variable? > > Yes it is! Sorry, I think I was unclear. I definitely want you to use a deleter. I mean have the following: SkAutoTCallIProc<GifFileType, close_gif> fGif; Keep referring to fGif as usual. (I think SkAutoTCallIProc implements the methods you need to treat it as if it were a raw pointer.) https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:46: // TODO: Is this the correct maxSize? This is 512 MB for kN32. Maybe explain why it was chosen? https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:48: if (codec->getInfo().width() * codec->getInfo().height() > maxSize) { We need to check codec for NULL. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:49: SkDebugf("Error: Image size too large, cannot decode.\n"); Use SkCodecPrintf. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:34: * TODO: Add option to turn off printing of error This is done by using SkCodecPrintf https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:37: SkCodecPrintf("Gif Warning: %s\n", msg); We might want two different functions: one for errors and one for warnings. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:69: static int32_t read_bytes_callback(GifFileType* fileType, GifByteType* out, What does gif_lib expect? size_t seems most appropriate here. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:85: // Most of the extension blocks contained in gif images are useless. Really? Any idea why they are there? Or are they just useless in finding this index? https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:94: // Check the "transparent color flag" which indicates whether a Is there a constant/macro for "transparent color flag"? https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:119: SkAutoTCallIProc<GifFileType, close_gif> autoCloseGif( Just like when we call an SkAutoTDeleteArray "buffer", I think it's clearer to just call this "gif". https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:123: DGifOpen(stream, read_bytes_callback, NULL)); Can we factor out the two different versions like we did with close_gif? https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:125: if (NULL == (GifFileType*) autoCloseGif) { Do you need to explicitly cast here for it to work? https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:277: SkAutoTDeleteArray<SkPMColor> colorTable( This is better as SkPMColor colorPtr[maxColors]; (Which is what you had in the first patch set.) We don't need to worry about deleting since it's on the stack. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:314: fillBackground = (kYes_ZeroInitialized != zeroInit); Is it possible that the fillIndex, either as specified by fGif->sBackgroundColor or arbitrarily zero, also points to zero? If so, it seems like we could set fillBackground to false in those cases too. (Also, once we support kIndex8, we care about whether the fillIndex is 0.) https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:410: memset(dstPtr, fillIndex, For N32, we need to memset32 with the colorPtr[fillIndex]
Second set of improvements to gif codec https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:120: // that they are stored as kUnpremul. Gifs without this index are On 2015/03/26 20:03:53, scroggo wrote: > On 2015/03/26 19:15:57, msarett wrote: > > On 2015/03/25 19:44:49, scroggo wrote: > > > Refresh my memory: the colors besides the transparent color are guaranteed > to > > be > > > opaque, right? > > > > > > This comment seems a little misleading - I think you mean we're guessing > that > > > it's not opaque, since it *might* have a transparent color, as opposed to > > > whether the colors are premultiplied or not. > > > > Yes, colors that are not the transparent color are guaranteed to be opaque. > And > > yes, the comment is unclear, I meant that we are guessing that it is not > opaque. > > > > Thinking through it again, it makes the most sense to mark kPremul, since > pixels > > will either be completely opaque or all components will be zeroed. > > To be fair, ARGB 0 0 0 0 is a valid premul AND unpremul color. Agreed. I was just thinking that if it could be considered premul or unpremul, it's best to consider it premul because it saves us work later? https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:176: SavedImage saveExt; On 2015/03/26 20:03:52, scroggo wrote: > On 2015/03/26 19:15:56, msarett wrote: > > On 2015/03/25 19:44:48, scroggo wrote: > > > It looks like this is only needed if GIFLIB_MAJOR < 5? > > > > It's actually needed for both. > > But it looks like you've deleted it? > > Oh, you've moved it up to the top. Why? Its comment is still down here though. I was grouping auto calls together. It's better down here, you're right. Especially because I refactored the auto calls again anyway. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:46: // TODO: Is this the correct maxSize? This is 512 MB for kN32. On 2015/03/26 20:03:53, scroggo wrote: > Maybe explain why it was chosen? Done. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:48: if (codec->getInfo().width() * codec->getInfo().height() > maxSize) { On 2015/03/26 20:03:53, scroggo wrote: > We need to check codec for NULL. Done. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:49: SkDebugf("Error: Image size too large, cannot decode.\n"); On 2015/03/26 20:03:53, scroggo wrote: > Use SkCodecPrintf. Done. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:34: * TODO: Add option to turn off printing of error On 2015/03/26 20:03:54, scroggo wrote: > This is done by using SkCodecPrintf Done. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:37: SkCodecPrintf("Gif Warning: %s\n", msg); On 2015/03/26 20:03:53, scroggo wrote: > We might want two different functions: one for errors and one for warnings. Agreed. This is better. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:69: static int32_t read_bytes_callback(GifFileType* fileType, GifByteType* out, On 2015/03/26 20:03:53, scroggo wrote: > What does gif_lib expect? size_t seems most appropriate here. Agreed, but it expects an int return type. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:85: // Most of the extension blocks contained in gif images are useless. On 2015/03/26 20:03:53, scroggo wrote: > Really? Any idea why they are there? Or are they just useless in finding this > index? They are really in fact useless. But I don't think this comment is necessary. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:94: // Check the "transparent color flag" which indicates whether a On 2015/03/26 20:03:53, scroggo wrote: > Is there a constant/macro for "transparent color flag"? The library actually doesn't have one. It makes a very similar check. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:119: SkAutoTCallIProc<GifFileType, close_gif> autoCloseGif( On 2015/03/26 20:03:53, scroggo wrote: > Just like when we call an SkAutoTDeleteArray "buffer", I think it's clearer to > just call this "gif". Done. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:123: DGifOpen(stream, read_bytes_callback, NULL)); On 2015/03/26 20:03:53, scroggo wrote: > Can we factor out the two different versions like we did with close_gif? Done. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:125: if (NULL == (GifFileType*) autoCloseGif) { On 2015/03/26 20:03:53, scroggo wrote: > Do you need to explicitly cast here for it to work? Apparently not :) https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:277: SkAutoTDeleteArray<SkPMColor> colorTable( On 2015/03/26 20:03:53, scroggo wrote: > This is better as > > SkPMColor colorPtr[maxColors]; > > (Which is what you had in the first patch set.) We don't need to worry about > deleting since it's on the stack. Done. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:314: fillBackground = (kYes_ZeroInitialized != zeroInit); On 2015/03/26 20:03:53, scroggo wrote: > Is it possible that the fillIndex, either as specified by fGif->sBackgroundColor > or arbitrarily zero, also points to zero? If so, it seems like we could set > fillBackground to false in those cases too. (Also, once we support kIndex8, we > care about whether the fillIndex is 0.) Hmm yeah it will add complexity when we care about whether the fillIndex is 0 vs whether the pixel stored at fillIndex is 0. That will make the eventual swizzle to Index8 more interesting. For now, however, the fillIndex can never point to a 0 pixel unless it is the same as transIndex. We are guaranteed that all other pixels in the color table are opaque, meaning that they have 0xFF alpha components. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:410: memset(dstPtr, fillIndex, On 2015/03/26 20:03:54, scroggo wrote: > For N32, we need to memset32 with the colorPtr[fillIndex] Wow yeah, I messed that up. This makes me wonder a little bit if we might want to find a way to use the swizzler here, given that this code will be a little bit different for each dst color type.
LGTM. If you go ahead and land it with it reporting kIndex8, please follow up with a CL to actually support kIndex8. It should be fairly straightforward, and we'll need it to work so we can add it to Hal's test. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:120: // that they are stored as kUnpremul. Gifs without this index are On 2015/03/26 22:26:36, msarett wrote: > On 2015/03/26 20:03:53, scroggo wrote: > > On 2015/03/26 19:15:57, msarett wrote: > > > On 2015/03/25 19:44:49, scroggo wrote: > > > > Refresh my memory: the colors besides the transparent color are guaranteed > > to > > > be > > > > opaque, right? > > > > > > > > This comment seems a little misleading - I think you mean we're guessing > > that > > > > it's not opaque, since it *might* have a transparent color, as opposed to > > > > whether the colors are premultiplied or not. > > > > > > Yes, colors that are not the transparent color are guaranteed to be opaque. > > And > > > yes, the comment is unclear, I meant that we are guessing that it is not > > opaque. > > > > > > Thinking through it again, it makes the most sense to mark kPremul, since > > pixels > > > will either be completely opaque or all components will be zeroed. > > > > To be fair, ARGB 0 0 0 0 is a valid premul AND unpremul color. > > Agreed. I was just thinking that if it could be considered premul or unpremul, > it's best to consider it premul because it saves us work later? Good thinking. https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:176: SavedImage saveExt; On 2015/03/26 22:26:36, msarett wrote: > On 2015/03/26 20:03:52, scroggo wrote: > > On 2015/03/26 19:15:56, msarett wrote: > > > On 2015/03/25 19:44:48, scroggo wrote: > > > > It looks like this is only needed if GIFLIB_MAJOR < 5? > > > > > > It's actually needed for both. > > > > But it looks like you've deleted it? > > > > Oh, you've moved it up to the top. Why? Its comment is still down here though. > > I was grouping auto calls together. It's better down here, you're right. > Especially because I refactored the auto calls again anyway. In general, I would say to declare variables as late as you can (i.e. right before they're used). https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:314: fillBackground = (kYes_ZeroInitialized != zeroInit); On 2015/03/26 22:26:37, msarett wrote: > On 2015/03/26 20:03:53, scroggo wrote: > > Is it possible that the fillIndex, either as specified by > fGif->sBackgroundColor > > or arbitrarily zero, also points to zero? If so, it seems like we could set > > fillBackground to false in those cases too. (Also, once we support kIndex8, we > > care about whether the fillIndex is 0.) > > Hmm yeah it will add complexity when we care about whether the fillIndex is 0 vs > whether the pixel stored at fillIndex is 0. That will make the eventual swizzle > to Index8 more interesting. > > For now, however, the fillIndex can never point to a 0 pixel unless it is the > same as transIndex. We are guaranteed that all other pixels in the color table > are opaque, meaning that they have 0xFF alpha components. Really? There aren't any images with invalid entries like we've seen with BMP etc? https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:410: memset(dstPtr, fillIndex, On 2015/03/26 22:26:37, msarett wrote: > On 2015/03/26 20:03:54, scroggo wrote: > > For N32, we need to memset32 with the colorPtr[fillIndex] > > Wow yeah, I messed that up. > > This makes me wonder a little bit if we might want to find a way to use the > swizzler here, given that this code will be a little bit different for each dst > color type. Possibly. It would make this code much simpler, for sure. https://codereview.chromium.org/1022673011/diff/80001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1022673011/diff/80001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:47: // Set the max size at 128 MP (512 MB for kN32). mega pixel? https://codereview.chromium.org/1022673011/diff/80001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:49: // draw. decode?
Just waiting to get the giflib dependencies sorted out. Will implement kIndex8 swizzles after this lands. https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:314: fillBackground = (kYes_ZeroInitialized != zeroInit); On 2015/03/27 13:09:36, scroggo wrote: > On 2015/03/26 22:26:37, msarett wrote: > > On 2015/03/26 20:03:53, scroggo wrote: > > > Is it possible that the fillIndex, either as specified by > > fGif->sBackgroundColor > > > or arbitrarily zero, also points to zero? If so, it seems like we could set > > > fillBackground to false in those cases too. (Also, once we support kIndex8, > we > > > care about whether the fillIndex is 0.) > > > > Hmm yeah it will add complexity when we care about whether the fillIndex is 0 > vs > > whether the pixel stored at fillIndex is 0. That will make the eventual > swizzle > > to Index8 more interesting. > > > > For now, however, the fillIndex can never point to a 0 pixel unless it is the > > same as transIndex. We are guaranteed that all other pixels in the color > table > > are opaque, meaning that they have 0xFF alpha components. > > Really? There aren't any images with invalid entries like we've seen with BMP > etc? Yeah this is an actual guarantee. The gif color table is supplied in RGB format, so the alpha component is always supplied by us and is always (correctly) supplied as 0xFF. However, if we decide to store a color table in RGB format (not sure if this would ever happen), your point is valid. https://codereview.chromium.org/1022673011/diff/80001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1022673011/diff/80001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:47: // Set the max size at 128 MP (512 MB for kN32). On 2015/03/27 13:09:36, scroggo wrote: > mega pixel? Haha yeah. I actually googled "megapixel" to find this abbreviation. If you've never seen it either, it's probably not widely used. https://codereview.chromium.org/1022673011/diff/80001/src/codec/SkCodec.cpp#n... src/codec/SkCodec.cpp:49: // draw. On 2015/03/27 13:09:36, scroggo wrote: > decode? This is the test with the very, very large frame size and the small subset image to decode. I added print statements because I was trying to figure out if it was hanging, and it didn't look like the decode was taking much time compared to all of the set up and drawing in dm. Of course, if we were actually decoding an image of this size, the decode may take several minutes, I just don't know for sure.
lgtm https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:314: fillBackground = (kYes_ZeroInitialized != zeroInit); On 2015/03/27 14:23:19, msarett wrote: > On 2015/03/27 13:09:36, scroggo wrote: > > On 2015/03/26 22:26:37, msarett wrote: > > > On 2015/03/26 20:03:53, scroggo wrote: > > > > Is it possible that the fillIndex, either as specified by > > > fGif->sBackgroundColor > > > > or arbitrarily zero, also points to zero? If so, it seems like we could > set > > > > fillBackground to false in those cases too. (Also, once we support > kIndex8, > > we > > > > care about whether the fillIndex is 0.) > > > > > > Hmm yeah it will add complexity when we care about whether the fillIndex is > 0 > > vs > > > whether the pixel stored at fillIndex is 0. That will make the eventual > > swizzle > > > to Index8 more interesting. > > > > > > For now, however, the fillIndex can never point to a 0 pixel unless it is > the > > > same as transIndex. We are guaranteed that all other pixels in the color > > table > > > are opaque, meaning that they have 0xFF alpha components. > > > > Really? There aren't any images with invalid entries like we've seen with BMP > > etc? > > Yeah this is an actual guarantee. The gif color table is supplied in RGB > format, so the alpha component is always supplied by us and is always > (correctly) supplied as 0xFF. However, if we decide to store a color table in > RGB format (not sure if this would ever happen), your point is valid. Oh yeah, of course!
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/1022673011/#ps110001 (title: "Merge with Hal's wbmp changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1022673011/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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...)
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/1022673011/#ps130001 (title: "Trybot fix - unitialized variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1022673011/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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...)
msarett@google.com changed reviewers: + djsollen@google.com
We are failing to include giflib on Windows bots. "Cannot open include file: unistd.h from gif_hash.h" Apparently, this doesn't exist in Windows environment. Our version of giflib may not be Windows compatible. We are researching this.
Waiting on the trybots to verify a potential fix. Will revisit on Monday.
Patchset #12 (id:250001) has been deleted
Patchset #11 (id:230001) has been deleted
Patchset #12 (id:290001) has been deleted
Patchset #11 (id:270001) has been deleted
Patchset #11 (id:310001) has been deleted
Patchset #10 (id:210001) has been deleted
Patchset #9 (id:190001) has been deleted
Patchset #12 (id:390001) has been deleted
I still can't get giflib to compile on Windows. I feel like I have started to become unproductive trying to enable this. Maybe a fresh start tomorrow will help, but do you think there are any options to put this issue on hold? I can work on the jpeg decoder independent of this, but I also am working on code for swizzling to Index8 that depends on this CL. I don't think the solution is to give up - it makes sense to have consistent decoders across platforms, and I feel like it is clearly possible to make this work (giflib on SourceForge has a windows Makefile that I have been trying to replicate). I just didn't accomplish much today and want to find a way to get back on track.
On 2015/03/30 22:25:10, msarett wrote: > I still can't get giflib to compile on Windows. I feel like I have started to > become unproductive trying to enable this. > > Maybe a fresh start tomorrow will help, but do you think there are any options > to put this issue on hold? I can work on the jpeg decoder independent of this, > but I also am working on code for swizzling to Index8 that depends on this CL. > > I don't think the solution is to give up - it makes sense to have consistent > decoders across platforms, and I feel like it is clearly possible to make this > work (giflib on SourceForge has a windows Makefile that I have been trying to > replicate). I just didn't accomplish much today and want to find a way to get > back on track. I would investigate what Chromium does. If they are using giflib, we can do the same thing they are. You could also try GnuWin's version here: http://gnuwin32.sourceforge.net/packages/giflib.htm (I don't know much about it, but it seems like it might work?) The error from the CQ looks like the problem is unistd.h missing on windows. Can we check in a version of that file? Searching for unistd.h windows led me to this proposed replacement: http://stackoverflow.com/questions/341817/is-there-a-replacement-for-unistd-h... (Not saying we should take that one, but maybe something like this is the answer?)
Patchset #14 (id:450001) has been deleted
Patchset #14 (id:470001) has been deleted
Patchset #14 (id:490001) has been deleted
Patchset #31 (id:820001) has been deleted
Patchset #14 (id:510001) has been deleted
Patchset #14 (id:530001) has been deleted
Patchset #21 (id:680001) has been deleted
Patchset #26 (id:780001) has been deleted
Patchset #25 (id:770010) has been deleted
Patchset #14 (id:250002) has been deleted
Patchset #14 (id:560001) has been deleted
Patchset #14 (id:580001) has been deleted
Patchset #14 (id:600001) has been deleted
Patchset #14 (id:620001) has been deleted
Patchset #14 (id:640001) has been deleted
Patchset #14 (id:660001) has been deleted
I think I've finally solved the windows problem. I haven't been able to get rid of a final warning on Windows: cl : Command line warning D9025 : overriding '/W3' with '/w' But I think this is ready to go.
https://codereview.chromium.org/1022673011/diff/860001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/860001/gyp/giflib.gyp#newcode20 gyp/giflib.gyp:20: '../third_party/giflib', only add this include directory on windows.
Only include dummy file on windows https://codereview.chromium.org/1022673011/diff/860001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/860001/gyp/giflib.gyp#newcode20 gyp/giflib.gyp:20: '../third_party/giflib', On 2015/03/31 17:33:02, djsollen wrote: > only add this include directory on windows. Done.
lgtm
https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp#newcode44 gyp/giflib.gyp:44: '/W3', I guess this did not work? https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:54: codec->getInfo().width() * codec->getInfo().height() > maxSize) { This may never/rarely come up in practice, but I'm wondering whether this is the right approach. It seems like it's possible that the image's default info is too large, but the client uses a smaller info which is not too big, and that is supported by the codec. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:123: } nit: Could there be multiple graphics control extensions? Maybe we should break after the first one if there was no valid transIndex? I'm guessing there won't be large numbers of ExtensionBlocks, so it's probably not a big deal. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:159: // opaque. However, we can at least mark it as kPremul, since pixels will Maybe a note that it could be treated as unpremul, but premul is better since that's what we support (and it would be more efficient even when unpremul is supported)? https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:188: kUnpremul_SkAlphaType == src.alphaType()); The source will always be premul, right? And it could easily be treated as unpremul. I think we really want to say: return dst.alphaType() == kPremul_SkAlphaType || dst.alphaType() == kUnpremul_SkAlphaType; It seems like it would be nice to support kOpaque, if there is no transparent color, but it seems like we cannot know that yet? https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:229: while (TERMINATE_RECORD_TYPE != recordType) { Is it possible that a file never contains TERMINATE_RECORD_TYPE? I guess in that case we'll fail on some call to DGifGetRecordType down below? Unless, of course, it's a valid image, in which case we return as soon as we have the image. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:246: SavedImage* image = &fGif->SavedImages[fGif->ImageCount - 1]; It looks like we're using the last image? Why not the first? (Or are they stored backwards, so this *is* the first?) https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:294: colorCount = colorMap->ColorCount; Is ColorCount created by giflib (therefore reliable) or is it directly taken from the encoded data (and therefore could be wrong)? https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:300: colorMap->Colors[i].Red, nit: This should line up with 0xFF or be indented 8 spaces. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:314: int32_t transIndex = find_trans_index(saveExt, colorCount); nit: What if we compared against colorCount here, instead of passing it into find_trans_index? I'm a little torn on which is better; maybe it's fine as is. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:454: case EXTENSION_RECORD_TYPE: Does this part add the extension that provides the transparent index? https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:494: case TERMINATE_RECORD_TYPE: Any reason to single this out from default? The behavior is the same. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.h:66: * @param gif pointer to library type that manages gif decode takes ownership. https://codereview.chromium.org/1022673011/diff/860001/third_party/giflib/uni... File third_party/giflib/unistd.h (right): https://codereview.chromium.org/1022673011/diff/860001/third_party/giflib/uni... third_party/giflib/unistd.h:7: // <unistd.h>. This file exists so that the include does fail on Windows. not* fail.
For me, the remaining outstanding question is what to do about the size check in SkCodec.cpp. I'm happy to do what you think is best. https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp#newcode44 gyp/giflib.gyp:44: '/W3', On 2015/03/31 18:39:56, scroggo wrote: > I guess this did not work? Unfortunately not. I did a grep for "W3" and it doesn't appear that we suggest that flag in any of our gyp files. This makes me think that somehow we automatically use this flag on windows bots? Which maybe explains why I was unable to turn it off? Do you think this is worth exploring further? https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:54: codec->getInfo().width() * codec->getInfo().height() > maxSize) { On 2015/03/31 18:39:56, scroggo wrote: > This may never/rarely come up in practice, but I'm wondering whether this is the > right approach. It seems like it's possible that the image's default info is too > large, but the client uses a smaller info which is not too big, and that is > supported by the codec. Yeah that is certainly possible. Maybe this isn't the right approach. I'm not exactly sure what the best approach is. Another option is to make the check in onGetPixels based on the requestedInfo? Maybe we only fail if the requested dimensions and the original dimensions are too large? Currently we fail in most decoders when the requested dimensions don't match the original dimensions, but I'm guessing this may not be the permanent strategy. Also, we could simply not check size at all? Excessive app memory use and long wait times could simply be our deterrent to users decoding large images. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:123: } On 2015/03/31 18:39:56, scroggo wrote: > nit: Could there be multiple graphics control extensions? Maybe we should break > after the first one if there was no valid transIndex? > > I'm guessing there won't be large numbers of ExtensionBlocks, so it's probably > not a big deal. There can be multiple GCEs because there can be multiple image frames in a gif. The spec states that the scope of the GCE is the next image frame in the file, and there can only be one GCE preceding an image frame. Since the current gif decoder only ever decodes one image frame, you are correct that we should only have to look for one GCE. So to finally answer the question, yes I think there should be a break. I am also going to reverse the loop so it starts by checking the most recent extension it has encountered (This shouldn't impact valid images since there should only be one GCE, but it's my guess that in weird cases it may be best to go with the GCE closest to the image data). I imagine that neither of these changes will cause a change in behavior of any of the test images (but it's worth monitoring). Also, it's worth noting that both of these changes may cause the new decoder to have different behavior than SkImageDecoder. This function was adapted from SkImageDecoder, and this is the first time that I have thought through it properly. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:159: // opaque. However, we can at least mark it as kPremul, since pixels will On 2015/03/31 18:39:56, scroggo wrote: > Maybe a note that it could be treated as unpremul, but premul is better since > that's what we support (and it would be more efficient even when unpremul is > supported)? Agreed https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:188: kUnpremul_SkAlphaType == src.alphaType()); On 2015/03/31 18:39:57, scroggo wrote: > The source will always be premul, right? And it could easily be treated as > unpremul. I think we really want to say: > > return dst.alphaType() == kPremul_SkAlphaType > || dst.alphaType() == kUnpremul_SkAlphaType; > > It seems like it would be nice to support kOpaque, if there is no transparent > color, but it seems like we cannot know that yet? I agree and fixed the return statement. We discussed supporting kOpaque in person. It's doable, but low priority. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:229: while (TERMINATE_RECORD_TYPE != recordType) { On 2015/03/31 18:39:56, scroggo wrote: > Is it possible that a file never contains TERMINATE_RECORD_TYPE? I guess in that > case we'll fail on some call to DGifGetRecordType down below? > > Unless, of course, it's a valid image, in which case we return as soon as we > have the image. Yes this will always find a way to fail inside or outside the loop. If we hit a: terminate record type - we exit the loop and fail extension record type - we process it and keep looping image record type - we decode it and return invalid record type - dgiflib returns an error (which we catch) The default case should never be reached (which I have just made clear). https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:246: SavedImage* image = &fGif->SavedImages[fGif->ImageCount - 1]; On 2015/03/31 18:39:56, scroggo wrote: > It looks like we're using the last image? Why not the first? (Or are they stored > backwards, so this *is* the first?) We are always using and returning the first image. So every time this code is executed ImageCount will be 1 and we will be accessing SavedImages[0]. We need to access SavedImages[0] because giflib uses it to store information from the image descriptor such as width, height, etc. I think the confusion is from unnecessary future proofing. If we were ever to want to decode the next image in the file, ImageCount would be 2 and we would use SavedImages[1]. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:294: colorCount = colorMap->ColorCount; On 2015/03/31 18:39:56, scroggo wrote: > Is ColorCount created by giflib (therefore reliable) or is it directly taken > from the encoded data (and therefore could be wrong)? colorCount is reliable. giflib calculates it and returns a NULL colorMap if it is invalid. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:300: colorMap->Colors[i].Red, On 2015/03/31 18:39:56, scroggo wrote: > nit: This should line up with 0xFF or be indented 8 spaces. Done. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:314: int32_t transIndex = find_trans_index(saveExt, colorCount); On 2015/03/31 18:39:56, scroggo wrote: > nit: What if we compared against colorCount here, instead of passing it into > find_trans_index? I'm a little torn on which is better; maybe it's fine as is. I think this is much better, and it is now possible, since we are only checking one GCE for a valid index. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:454: case EXTENSION_RECORD_TYPE: On 2015/03/31 18:39:57, scroggo wrote: > Does this part add the extension that provides the transparent index? Yes. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:494: case TERMINATE_RECORD_TYPE: On 2015/03/31 18:39:56, scroggo wrote: > Any reason to single this out from default? The behavior is the same. The default case should never be reached. I will make this clear. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.h:66: * @param gif pointer to library type that manages gif decode On 2015/03/31 18:39:57, scroggo wrote: > takes ownership. Done. https://codereview.chromium.org/1022673011/diff/860001/third_party/giflib/uni... File third_party/giflib/unistd.h (right): https://codereview.chromium.org/1022673011/diff/860001/third_party/giflib/uni... third_party/giflib/unistd.h:7: // <unistd.h>. This file exists so that the include does fail on Windows. On 2015/03/31 18:39:57, scroggo wrote: > not* fail. Done.
https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp#newcode44 gyp/giflib.gyp:44: '/W3', On 2015/03/31 20:11:40, msarett wrote: > On 2015/03/31 18:39:56, scroggo wrote: > > I guess this did not work? > > Unfortunately not. I did a grep for "W3" and it doesn't appear that we suggest > that flag in any of our gyp files. This makes me think that somehow we > automatically use this flag on windows bots? Which maybe explains why I was > unable to turn it off? Do you think this is worth exploring further? I would say no, until someone complains about it. https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:54: codec->getInfo().width() * codec->getInfo().height() > maxSize) { On 2015/03/31 20:11:40, msarett wrote: > On 2015/03/31 18:39:56, scroggo wrote: > > This may never/rarely come up in practice, but I'm wondering whether this is > the > > right approach. It seems like it's possible that the image's default info is > too > > large, but the client uses a smaller info which is not too big, and that is > > supported by the codec. > > Yeah that is certainly possible. Maybe this isn't the right approach. I'm not > exactly sure what the best approach is. > > Another option is to make the check in onGetPixels based on the requestedInfo? > Maybe we only fail if the requested dimensions and the original dimensions are > too large? > Currently we fail in most decoders when the requested dimensions > don't match the original dimensions, but I'm guessing this may not be the > permanent strategy. You are correct. JPEG supports downscaling by powers of two. And Webp supports arbitrary scales (see https://codereview.chromium.org/1044433002/). > > Also, we could simply not check size at all? Excessive app memory use and long > wait times could simply be our deterrent to users decoding large images. That's certainly an option. The nasty part about *not* doing something special is that a client may write the code but not be in control of the images they end up decoding. On the other hand, different clients may make different decisions about how big is too big, and it would be a shame if we set the limit smaller than they need. That said, some of our SkImageDecoders do their own size checks, which I have included in their corresponding SkCodecs (those may be or may have been real limits for us, rather than "this is too slow", though). I forgot, what does Chromium do? https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:123: } On 2015/03/31 20:11:40, msarett wrote: > On 2015/03/31 18:39:56, scroggo wrote: > > nit: Could there be multiple graphics control extensions? Maybe we should > break > > after the first one if there was no valid transIndex? > > > > I'm guessing there won't be large numbers of ExtensionBlocks, so it's probably > > not a big deal. > > There can be multiple GCEs because there can be multiple image frames in a gif. > > The spec states that the scope of the GCE is the next image frame in the file, > and there can only be one GCE preceding an image frame. Since the current gif > decoder only ever decodes one image frame, you are correct that we should only > have to look for one GCE. > > So to finally answer the question, yes I think there should be a break. I am > also going to reverse the loop so it starts by checking the most recent > extension it has encountered (This shouldn't impact valid images since there > should only be one GCE, but it's my guess that in weird cases it may be best to > go with the GCE closest to the image data). I imagine that neither of these > changes will cause a change in behavior of any of the test images (but it's > worth monitoring). I wouldn't worry about reversing this. Since you're just guessing, we may be better off as is. That said, since you've already changed it, this way seems just as well to me. > > Also, it's worth noting that both of these changes may cause the new decoder to > have different behavior than SkImageDecoder. This function was adapted from > SkImageDecoder, and this is the first time that I have thought through it > properly. Lots of our SkCodec behavior is different from SkImageDecoder, and hopefully better! https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:129: return (uint32_t) -1; Why not return a signed type? https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:232: while (TERMINATE_RECORD_TYPE != recordType) { nit: It seems like this could be a do while loop. Then you don't need to initialize recordType (it will be set by DGifGetRecordType before we ever read it). https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:321: if (transIndex < colorCount) { This seems a little sneaky: It seems that find_trans_index returns max uint32_t on failure, so we can use one if statement to take care of both failure cases (i.e. not found and out of range). I think this could use a comment (plus one by find_trans_index).
Added comments about transIndex and a do-while loop https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:129: return (uint32_t) -1; On 2015/03/31 20:39:11, scroggo wrote: > Why not return a signed type? It's my personal preference for indices to be unsigned when possible. Hopefully using max int as the invalid flag is better than this ridiculous cast. https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:232: while (TERMINATE_RECORD_TYPE != recordType) { On 2015/03/31 20:39:11, scroggo wrote: > nit: It seems like this could be a do while loop. Then you don't need to > initialize recordType (it will be set by DGifGetRecordType before we ever read > it). Done. https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:321: if (transIndex < colorCount) { On 2015/03/31 20:39:11, scroggo wrote: > This seems a little sneaky: > > It seems that find_trans_index returns max uint32_t on failure, so we can use > one if statement to take care of both failure cases (i.e. not found and out of > range). I think this could use a comment (plus one by find_trans_index). Done.
LGTM at patch set 23
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com Link to the patchset: https://codereview.chromium.org/1022673011/#ps910001 (title: "Added comments and do while loop")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1022673011/910001
Message was sent while issue was closed.
Committed patchset #23 (id:910001) as https://skia.googlesource.com/skia/+/8c8f22a3bba18edaad41d6ac8451a77aa093a958 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
