Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1011343003:
Enabling ico decoding with use of png and bmp decoders (Closed)
DescriptionEnabling ico decoding with use of png and bmp decoders
BUG=skia:3257
NOPRESUBMIT=true
Committed: https://skia.googlesource.com/skia/+/9bde918754bc292469d801f156f3b626eb3db780
Patch Set 1 #Patch Set 2 : Clean up before public review #
Total comments: 26
Patch Set 3 : Provide the user with an option of which ico to decode #
Total comments: 25
Patch Set 4 : onGetScaled dimensions, improved choice of proper ico to decode #
Total comments: 22
Patch Set 5 : Using appropriate Sk data types for array of codecs, removing getOriginalInfo #
Total comments: 16
Patch Set 6 : Minor fixes from last set #Patch Set 7 : Updated blacklist #
Total comments: 6
Patch Set 8 : Included subset on blacklist #Patch Set 9 : Return kInvalidConversion for 565, Added cast #Patch Set 10 : Use 1.0f instead of 1.0 or windows thinks its a double #Patch Set 11 : Removed additional blacklist items #
Messages
Total messages: 52 (22 generated)
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1011343003/diff/20001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1011343003/diff/20001/include/codec/SkCodec.h... include/codec/SkCodec.h:54: const SkImageInfo& getOriginalInfo() { return fInfo; } I made this public in order to check the dimensions of the embedded codec (bmp-in-ico or png-in-ico). The use case is that sometimes the dimensions of the embedded codec will not match that of the ico codec, which can be a problem given that the memory allocated for the destination is based on the ico codec. This allows us to correct the error in the ico dimensions and proceed with the decode.
https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:203: return fEmbeddedCodec->getPixels(dstInfo, dst, dstRowBytes); Forgot to mention: I'm not sure about the implications of calling getPixels vs onGetPixels here (onGetPixels is protected so we can't call it). getPixels does not allow us to pass the options object, which I feel may be an issue.
https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:1113: case kRGB_565_SkColorType: { Forgot to mention part 2: This case can be removed and this code can be simplified quite a bit when I remove the swizzles to 565. As discussed, we do not need to enable decoding to 565 when the input has 24 or 32 bpp. All inputs that use this function are 24 or 32 bpp.
https://codereview.chromium.org/1011343003/diff/20001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1011343003/diff/20001/include/codec/SkCodec.h... include/codec/SkCodec.h:54: const SkImageInfo& getOriginalInfo() { return fInfo; } On 2015/03/18 19:53:22, msarett wrote: > I made this public in order to check the dimensions of the embedded codec > (bmp-in-ico or png-in-ico). > > The use case is that sometimes the dimensions of the embedded codec will not > match that of the ico codec, which can be a problem given that the memory > allocated for the destination is based on the ico codec. This allows us to > correct the error in the ico dimensions and proceed with the decode. FWIW, I added this function because I didn't want to go through the trouble of doing the following: SkImageInfo info; this->getInfo(&info); Mike is working on changing getInfo to just return the info, like you'd want/expect, and then you can just call that. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:137: // read the first four infoBytes. No longer true. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:171: // Bmps in Ico cannot specify an offset. We will always assume that Really? SkImageDecoder_libico reads an offset field. Was that a mistake? https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:622: if (!fIsIco) { I wonder if it would be possible/better to split into a few classes, possibly subclassing from SkBmpCodec. Looking at the list of member variables we have a few that are only valid for different types: fRLEBytes - only used for RLE fMasks - possibly its own set (or used by RLE and others?) fOffset - not used by ico fIsIco - not needed if we used different classes We'd still need to make sure we share code when we can, and maybe it wouldn't split up as cleanly as I imagine. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:1107: (srcBuffer.get()[x / 8] >> shift) & 0x1; nit: Avoiding divides in a loop is generally a good thing. Can we switch to x >> 3? Ooh, even better: You are also doing a mod up above, and we have a function to do both faster in SkMath.h: SkTDivMod https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:68: // decode the "best" of these embedded images. The largest image in size In getPixels, the client will specify a desired SkImageInfo. I think the "best" image is the one that most closely matches the desired info (possibly choosing a step bigger if there isn't a size that matches exactly). We should also implement onGetScaledDimensions, so the client can find an image which matches their desired scale. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:70: // more bits per pixel is the "best". Note that bitsPerPixel is often left If the client wanted 565, maybe an 8 bit image is not the best? https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:137: if (!stream->move(-PNG_BYTES_TO_CHECK)) { Unfortunately, we cannot depend on this call on Android. Also, this depends on internal knowledge of how IsPng is implemented, which is dangerous if PNG decides to do something different. If SkPngCodec gave us some more info/methods, we could know how much it read, and tell it to skip that much (libpng actually has a method to tell it how many you have read from the start). That doesn't help us in the case where it was BMP though. We've talked about adding a peek(size_t bytes) to SkStream which would allow something like this. (FWIW, SkMemoryStream already has a method named peek, which does something different, but I don't see any callers, at least internally...) I'm not sure what the best solution is for now :( If nothing else, we can put a giant FIXME here, but I'd like to at least get a band-aid in before submitting. I was thinking you could wrap the stream in an SkFrontBufferedStream (which we use elsewhere on Android). Unfortunately, it expects to own the passed in stream, as does the SkCodec. (I'm to blame here - SkStream used to be refcounted, in which case this wouldn't be a problem...) https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:163: const SkImageInfo& imageInfo = SkImageInfo::Make(bestWidth, bestHeight, This should not be a reference. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:179: : INHERITED(info, stream) If stream is always NULL, you can remove it from the parameter list for this constructor, and just pass NULL here. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:192: if (!this->rewindIfNeeded()) { How does this work? rewindIfNeeded will attempt to dereference fStream, which is NULL, IIUC. Also, it seems like PNG or BMP will next call rewindIfNeeded, which would always rewind (assuming the first one *had* worked). Would it be possible to make NewFromStream just return an SkBmpCodec or an SkPngCodec, skipping the wrapper altogether? (Does giving the caller an option of which image to pick make that impossible?) https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:203: return fEmbeddedCodec->getPixels(dstInfo, dst, dstRowBytes); On 2015/03/18 19:59:25, msarett wrote: > Forgot to mention: > I'm not sure about the implications of calling getPixels vs onGetPixels here > (onGetPixels is protected so we can't call it). getPixels does not allow us to > pass the options object, which I feel may be an issue. There are two versions of getPixels. You should call the first one, which takes all the parameters.
This patch provides the user with the option of the best ico to decode to. Let me know if this is along the lines of what we envisioned. If so, I still need to: Add a peek() method to SkMemoryStream Figure out if/how to use rewindIfNeeded() Modify onGetPixels to check for matching color type/alpha type (what do we do if there is not a perfect match?) https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:137: // read the first four infoBytes. On 2015/03/18 21:39:18, scroggo wrote: > No longer true. Done. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:171: // Bmps in Ico cannot specify an offset. We will always assume that On 2015/03/18 21:39:18, scroggo wrote: > Really? SkImageDecoder_libico reads an offset field. Was that a mistake? Ico files have multiple directory entries. Each stores an icoOffset which indicates where the data for that directory entry begins within the Ico file. If it is a bmp-in-ico, this icoOffset will indicate the start of the second bmp header (since bmp-in-ico omit the first header). Since bmp-in-ico omit the first header, there is no bmpOffset specified. The bmp pixel data begins immediately after the 2nd header or color table if it exists. This is clearly confusing in the code. I hope that adding comments to SkCodec_libico.cpp will help? https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:622: if (!fIsIco) { On 2015/03/18 21:39:18, scroggo wrote: > I wonder if it would be possible/better to split into a few classes, possibly > subclassing from SkBmpCodec. > > Looking at the list of member variables we have a few that are only valid for > different types: > fRLEBytes - only used for RLE > fMasks - possibly its own set (or used by RLE and others?) > fOffset - not used by ico > fIsIco - not needed if we used different classes > > We'd still need to make sure we share code when we can, and maybe it wouldn't > split up as cleanly as I imagine. It wouldn't be a clean split, but it's possible that some version of this would be superior to the current design. Still thinking about this. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:1107: (srcBuffer.get()[x / 8] >> shift) & 0x1; On 2015/03/18 21:39:18, scroggo wrote: > nit: Avoiding divides in a loop is generally a good thing. Can we switch to x >> > 3? > > Ooh, even better: You are also doing a mod up above, and we have a function to > do both faster in SkMath.h: SkTDivMod That's clever! Done. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:68: // decode the "best" of these embedded images. The largest image in size On 2015/03/18 21:39:19, scroggo wrote: > In getPixels, the client will specify a desired SkImageInfo. I think the "best" > image is the one that most closely matches the desired info (possibly choosing a > step bigger if there isn't a size that matches exactly). We should also > implement onGetScaledDimensions, so the client can find an image which matches > their desired scale. Done. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:70: // more bits per pixel is the "best". Note that bitsPerPixel is often left On 2015/03/18 21:39:18, scroggo wrote: > If the client wanted 565, maybe an 8 bit image is not the best? Acknowledged. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:137: if (!stream->move(-PNG_BYTES_TO_CHECK)) { On 2015/03/18 21:39:18, scroggo wrote: > Unfortunately, we cannot depend on this call on Android. Also, this depends on > internal knowledge of how IsPng is implemented, which is dangerous if PNG > decides to do something different. > > If SkPngCodec gave us some more info/methods, we could know how much it read, > and tell it to skip that much (libpng actually has a method to tell it how many > you have read from the start). That doesn't help us in the case where it was BMP > though. > > We've talked about adding a peek(size_t bytes) to SkStream which would allow > something like this. (FWIW, SkMemoryStream already has a method named peek, > which does something different, but I don't see any callers, at least > internally...) > > I'm not sure what the best solution is for now :( > > If nothing else, we can put a giant FIXME here, but I'd like to at least get a > band-aid in before submitting. > > I was thinking you could wrap the stream in an SkFrontBufferedStream (which we > use elsewhere on Android). Unfortunately, it expects to own the passed in > stream, as does the SkCodec. (I'm to blame here - SkStream used to be > refcounted, in which case this wouldn't be a problem...) I have added a FIXME. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:163: const SkImageInfo& imageInfo = SkImageInfo::Make(bestWidth, bestHeight, On 2015/03/18 21:39:18, scroggo wrote: > This should not be a reference. Done. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:179: : INHERITED(info, stream) On 2015/03/18 21:39:19, scroggo wrote: > If stream is always NULL, you can remove it from the parameter list for this > constructor, and just pass NULL here. Done. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:192: if (!this->rewindIfNeeded()) { On 2015/03/18 21:39:18, scroggo wrote: > How does this work? rewindIfNeeded will attempt to dereference fStream, which is > NULL, IIUC. > > Also, it seems like PNG or BMP will next call rewindIfNeeded, which would always > rewind (assuming the first one *had* worked). > > Would it be possible to make NewFromStream just return an SkBmpCodec or an > SkPngCodec, skipping the wrapper altogether? (Does giving the caller an option > of which image to pick make that impossible?) Giving the caller an option of which image to pick does in fact prevent us from skipping the wrapper. https://codereview.chromium.org/1011343003/diff/20001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:203: return fEmbeddedCodec->getPixels(dstInfo, dst, dstRowBytes); On 2015/03/18 21:39:18, scroggo wrote: > On 2015/03/18 19:59:25, msarett wrote: > > Forgot to mention: > > I'm not sure about the implications of calling getPixels vs onGetPixels here > > (onGetPixels is protected so we can't call it). getPixels does not allow us > to > > pass the options object, which I feel may be an issue. > > There are two versions of getPixels. You should call the first one, which takes > all the parameters. Done. https://codereview.chromium.org/1011343003/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1011343003/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:144: SkDebugf("\n%s\n", fPath.c_str()); Ignore this. I will remove. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:130: Will remove this line.
https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:131: SkData* data = SkData::NewFromStream(stream, size); SkMemoryStream's constructor will call data->ref(), and data starts with a refcount of 1, so you need to unref data (preferably by storing it in an SkAutoTUnref). https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:134: if (NULL == embeddedStream.get()) { You called the constructor for SkMemoryStream, so it will not be NULL. data may be NULL, on the other hand, so you should check for that. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:136: break; The only way for NewFromStream to fail is if the stream had an error or was truncated. I think in this case we need to treat the rest of the images as missing. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:141: // FIXME: Implement peek() for SkMemoryStream and use it to determine Actually, in this case, since you've created an SkMemoryStream, we don't need to worry about peeking. SkMemoryStream can be rewound (cheaply!), and it will rewind to its own beginning. I think you should go ahead and behave similarly to SkCodec::NewFromStream: call IsPng, rewind, return SkPngCodec::NewFromStrea if IsPng call IsBMP, rewind, return SkBmpCodec::NewFromIco https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:164: codecs.get()[index++] = codec; I think you might be better served here by using an SkTDArray<SkCodec*> or SkTArray<SkCodec*>. These arrays are similar to vector, allowing you to append new items and keeping track of the count. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:175: int width = codecs.get()[i]->getOriginalInfo().width(); If you're up to date, you can call getInfo() instead of getOriginalInfo(). https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:180: maxColorType = codecs.get()[i]->getOriginalInfo().colorType(); I think it might be worth it to set up a local variable for the info so we can simplify these. Actually, why not just store the index of the largest one? (You need to store the maxWidth/maxHeight, but the other ones seem unnecessary.) https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:186: if (0 == maxWidth) { I would argue that if a codec reported its width or height as 0 (I think all the subimages would have to, in this case), NewFromStream should have returned NULL anyway (the embedded ones, keeping consistent with this one). https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:218: // TODO: Should we also try to match color and alpha type? Good question. If we have multiple images with the same size but different color types, we could check the result of getPixels, and if it's a color conversion failure, try others. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:223: fEmbeddedCodecs.get()[i]->getOriginalInfo().dimensions()) { Is it possible that two embedded images have the same dimensions? https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:229: SkDebugf("Error: No ico candidate image with matching dimensions.\n"); Please implement onGetScaledDimensions, so a caller can find the dimensions to use.
onGetScaled dimensions implemented Improved process for choosing an embedded image decode https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:131: SkData* data = SkData::NewFromStream(stream, size); On 2015/03/20 19:36:01, scroggo wrote: > SkMemoryStream's constructor will call data->ref(), and data starts with a > refcount of 1, so you need to unref data (preferably by storing it in an > SkAutoTUnref). Done. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:134: if (NULL == embeddedStream.get()) { On 2015/03/20 19:36:00, scroggo wrote: > You called the constructor for SkMemoryStream, so it will not be NULL. data may > be NULL, on the other hand, so you should check for that. Done. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:136: break; On 2015/03/20 19:36:00, scroggo wrote: > The only way for NewFromStream to fail is if the stream had an error or was > truncated. I think in this case we need to treat the rest of the images as > missing. Agreed. That is why we break out of the loop here. We may still have a chance to decode any images that we identified before the stream was truncated. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:141: // FIXME: Implement peek() for SkMemoryStream and use it to determine On 2015/03/20 19:36:00, scroggo wrote: > Actually, in this case, since you've created an SkMemoryStream, we don't need to > worry about peeking. SkMemoryStream can be rewound (cheaply!), and it will > rewind to its own beginning. > > I think you should go ahead and behave similarly to SkCodec::NewFromStream: > > call IsPng, rewind, return SkPngCodec::NewFromStrea if IsPng > call IsBMP, rewind, return SkBmpCodec::NewFromIco Gotcha, will do. We won't call isBmp because the first bmp header (including the signature) is omitted. Instead, if it isn't a png, we assume it's a bmp. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:164: codecs.get()[index++] = codec; On 2015/03/20 19:36:01, scroggo wrote: > I think you might be better served here by using an SkTDArray<SkCodec*> or > SkTArray<SkCodec*>. These arrays are similar to vector, allowing you to append > new items and keeping track of the count. Yeah of course. I was trying to use SkTArray but I couldn't figure out to take away owership. Glad to know there is SkTDArray. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:175: int width = codecs.get()[i]->getOriginalInfo().width(); On 2015/03/20 19:36:01, scroggo wrote: > If you're up to date, you can call getInfo() instead of getOriginalInfo(). Done. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:180: maxColorType = codecs.get()[i]->getOriginalInfo().colorType(); On 2015/03/20 19:36:00, scroggo wrote: > I think it might be worth it to set up a local variable for the info so we can > simplify these. > > Actually, why not just store the index of the largest one? (You need to store > the maxWidth/maxHeight, but the other ones seem unnecessary.) Yes this is better. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:186: if (0 == maxWidth) { On 2015/03/20 19:36:00, scroggo wrote: > I would argue that if a codec reported its width or height as 0 (I think all the > subimages would have to, in this case), NewFromStream should have returned NULL > anyway (the embedded ones, keeping consistent with this one). Agreed. What I am actually checking is if there are no valid images to loop over. This check has been rewritten in a way that actually makes sense. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:218: // TODO: Should we also try to match color and alpha type? On 2015/03/20 19:36:01, scroggo wrote: > Good question. If we have multiple images with the same size but different color > types, we could check the result of getPixels, and if it's a color conversion > failure, try others. Done. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:223: fEmbeddedCodecs.get()[i]->getOriginalInfo().dimensions()) { On 2015/03/20 19:36:00, scroggo wrote: > Is it possible that two embedded images have the same dimensions? Yes and it's not that rare. The new version will try again if an image with matching dimensions fails to decode. https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:229: SkDebugf("Error: No ico candidate image with matching dimensions.\n"); On 2015/03/20 19:36:01, scroggo wrote: > Please implement onGetScaledDimensions, so a caller can find the dimensions to > use. Done.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/40001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:136: break; On 2015/03/23 12:20:58, msarett wrote: > On 2015/03/20 19:36:00, scroggo wrote: > > The only way for NewFromStream to fail is if the stream had an error or was > > truncated. I think in this case we need to treat the rest of the images as > > missing. > > Agreed. That is why we break out of the loop here. We may still have a chance > to decode any images that we identified before the stream was truncated. Oh, of course. nvm https://codereview.chromium.org/1011343003/diff/60001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1011343003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:54: const SkImageInfo& getOriginalInfo() const { return fInfo; } Can you always use getInfo(), so this is never needed? I think Mike added a version which returns an SkImageInfo. (The only reason I had this to begin with was to simplify the call of: SkImageInfo info; codec->getInfo(&info); info.dimensions(); // (or whatever) to codec->getInfo().dimensions() https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:167: const bool isIco); nit: We typically do not use const for primitive parameters. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:110: SkTDArray<SkCodec*>* codecs = SkNEW(SkTDArray<SkCodec*>); This can leak. SkTDArray has a copy constructor. If you use that, you don't need to use SkNEW here. Note that it will be using malloc under the covers, so we'll still be allocating memory somewhere. And here's the ugly part: SkTDArray won't take care of calling SkDELETE on your SkCodecs. You need to call SkTDArray::deleteAll() to take care of that. You could put it in an SkAutoTCallVProc (in SkTemplates.h, along with SkAutoTDelete), which lets you call a function when it goes out of scope. Your function would call both deleteAll and delete the SkTDArray. As an alternative, you could use an SkTArray<SkAutoTDelete<SkCodec>>. SkTArray *does* call destructors, so this would delete each SkCodec when it goes out of scope. The tricky part for both is that when you pass ownership to the new codec you need to make sure the original holder doesn't delete the entries. Maybe you use an SkAutoTDelete<SkTArray<SkAutoTDelete<SkCodec>>>, although that's pretty ugly... https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:140: SkAutoTDelete<SkCodec*> codec(SkNEW(SkCodec*)); I think this is rarely something you want to do. SkNewFromStream will return an SkCodec*. You'll immediately put that into your list which takes care of deleting (and it's okay to copy the raw pointer, in this case, since you know you're passing it to something that will take care of deleting it). https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:142: if (!embeddedStream->rewind()) { SkMemoryStream::rewind will always return true. (Maybe use an assert: SkAssertResult(embeddedStream->rewind()); Unlike SkASSERT, this is called in release mode.) https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:158: uint32_t numValidImages = codecs->count(); I think this can be const. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:169: codecs->operator[](i)->getInfo(&info); I think there's a version of getInfo which just returns an SkImageInfo, so you don't need to call this, more complicated version. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:180: return SkNEW_ARGS(SkIcoCodec, (info, codecs->detach(), numValidImages)); If you stored codecs as an SkTDArray or SkTArray in SkIcoCodec, you don't need numValidImages. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:202: return this->getOriginalInfo().dimensions(); getInfo() https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:212: int width = fEmbeddedCodecs.get()[i]->getOriginalInfo().width();; no need for double ; https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:213: int height = fEmbeddedCodecs.get()[i]->getOriginalInfo().width();; height()*
Patchset #6 (id:140001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:160001) has been deleted
Improved data type for array of codecs Removed getOriginalInfo https://codereview.chromium.org/1011343003/diff/60001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1011343003/diff/60001/include/codec/SkCodec.h... include/codec/SkCodec.h:54: const SkImageInfo& getOriginalInfo() const { return fInfo; } On 2015/03/23 13:41:24, scroggo wrote: > Can you always use getInfo(), so this is never needed? I think Mike added a > version which returns an SkImageInfo. (The only reason I had this to begin with > was to simplify the call of: > > SkImageInfo info; > codec->getInfo(&info); > info.dimensions(); // (or whatever) > > to > > codec->getInfo().dimensions() Done. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:167: const bool isIco); On 2015/03/23 13:41:24, scroggo wrote: > nit: We typically do not use const for primitive parameters. Sorry I feel like I keep reintroducing this. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:110: SkTDArray<SkCodec*>* codecs = SkNEW(SkTDArray<SkCodec*>); On 2015/03/23 13:41:24, scroggo wrote: > This can leak. > > SkTDArray has a copy constructor. If you use that, you don't need to use SkNEW > here. Note that it will be using malloc under the covers, so we'll still be > allocating memory somewhere. > > And here's the ugly part: SkTDArray won't take care of calling SkDELETE on your > SkCodecs. You need to call SkTDArray::deleteAll() to take care of that. You > could put it in an SkAutoTCallVProc (in SkTemplates.h, along with > SkAutoTDelete), which lets you call a function when it goes out of scope. Your > function would call both deleteAll and delete the SkTDArray. > > As an alternative, you could use an SkTArray<SkAutoTDelete<SkCodec>>. SkTArray > *does* call destructors, so this would delete each SkCodec when it goes out of > scope. > > The tricky part for both is that when you pass ownership to the new codec you > need to make sure the original holder doesn't delete the entries. Maybe you use > an SkAutoTDelete<SkTArray<SkAutoTDelete<SkCodec>>>, although that's pretty > ugly... Went with the SkAutoTDelete<SkTArray<SkAutoTDelete<SkCodec>>> https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:140: SkAutoTDelete<SkCodec*> codec(SkNEW(SkCodec*)); On 2015/03/23 13:41:25, scroggo wrote: > I think this is rarely something you want to do. > > SkNewFromStream will return an SkCodec*. You'll immediately put that into your > list which takes care of deleting (and it's okay to copy the raw pointer, in > this case, since you know you're passing it to something that will take care of > deleting it). Acknowledged. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:142: if (!embeddedStream->rewind()) { On 2015/03/23 13:41:24, scroggo wrote: > SkMemoryStream::rewind will always return true. (Maybe use an assert: > > SkAssertResult(embeddedStream->rewind()); > > Unlike SkASSERT, this is called in release mode.) Done. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:158: uint32_t numValidImages = codecs->count(); On 2015/03/23 13:41:24, scroggo wrote: > I think this can be const. You are correct. We are no longer using this variable, so there is no need to fix. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:169: codecs->operator[](i)->getInfo(&info); On 2015/03/23 13:41:25, scroggo wrote: > I think there's a version of getInfo which just returns an SkImageInfo, so you > don't need to call this, more complicated version. Done. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:180: return SkNEW_ARGS(SkIcoCodec, (info, codecs->detach(), numValidImages)); On 2015/03/23 13:41:25, scroggo wrote: > If you stored codecs as an SkTDArray or SkTArray in SkIcoCodec, you don't need > numValidImages. Acknowledged. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:202: return this->getOriginalInfo().dimensions(); On 2015/03/23 13:41:25, scroggo wrote: > getInfo() Done. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:212: int width = fEmbeddedCodecs.get()[i]->getOriginalInfo().width();; On 2015/03/23 13:41:25, scroggo wrote: > no need for double ; Done. https://codereview.chromium.org/1011343003/diff/60001/src/codec/SkCodec_libic... src/codec/SkCodec_libico.cpp:213: int height = fEmbeddedCodecs.get()[i]->getOriginalInfo().width();; On 2015/03/23 13:41:25, scroggo wrote: > height()* Done. https://codereview.chromium.org/1011343003/diff/180001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1011343003/diff/180001/dm/DM.cpp#newcode149 dm/DM.cpp:149: return true;//strcmp(ext, "png") == 0 || strcmp(ext, "PNG") == 0; Not planning to commit this.
https://codereview.chromium.org/1011343003/diff/180001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1011343003/diff/180001/dm/DM.cpp#newcode149 dm/DM.cpp:149: return true;//strcmp(ext, "png") == 0 || strcmp(ext, "PNG") == 0; On 2015/03/23 19:40:02, msarett wrote: > Not planning to commit this. Even better would be to add "bmp", "BMP", "ico", "ICO". (And of course, make sure all images we have that are being tested on the bots pass using your Codecs. https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:1010: if (kOpaque_SkAlphaType == dstInfo.alphaType() && !fIsIco) { If the caller requested opaque, but the image actually has alpha, we should be returning false. https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:1111: dstRow = SkTAddOffset<SkPMColor>(dstRow, delta); I commented on this before, and I don't remember if you responded: Does this work if delta is negative? It seems like it would cast it to a size_t, which is unsigned? https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.h:181: bool fIsIco; Can't this be const? https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... src/codec/SkCodec_libico.cpp:153: codecs->operator[](codecs->count() - 1).reset(codec); push_back returns a T&, so you can combine these two lines: codecs->push_back().reset(codec); https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... src/codec/SkCodec_libico.cpp:239: if (kInvalidConversion == result || kInvalidInput == result) { Why did you pick these two? I would think if we got kInvalidScale we'd want to try again, too. https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... File src/codec/SkCodec_libico.h (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... src/codec/SkCodec_libico.h:51: * @param embeddedCodec codec for the embedded image Plural. Takes ownership
Minor fixes from the last iteration. Also, I verified that the new decoders work properly on the current test images and I plan to upload new images after I have blacklisted the ones that fail for the old version. https://codereview.chromium.org/1011343003/diff/180001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1011343003/diff/180001/dm/DM.cpp#newcode149 dm/DM.cpp:149: return true;//strcmp(ext, "png") == 0 || strcmp(ext, "PNG") == 0; On 2015/03/23 20:34:50, scroggo wrote: > On 2015/03/23 19:40:02, msarett wrote: > > Not planning to commit this. > > Even better would be to add "bmp", "BMP", "ico", "ICO". (And of course, make > sure all images we have that are being tested on the bots pass using your > Codecs. Done. https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:1010: if (kOpaque_SkAlphaType == dstInfo.alphaType() && !fIsIco) { On 2015/03/23 20:34:50, scroggo wrote: > If the caller requested opaque, but the image actually has alpha, we should be > returning false. Thanks, I made a mistake when I merged. https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:1111: dstRow = SkTAddOffset<SkPMColor>(dstRow, delta); On 2015/03/23 20:34:51, scroggo wrote: > I commented on this before, and I don't remember if you responded: > > Does this work if delta is negative? It seems like it would cast it to a size_t, > which is unsigned? You're right my fault. I swear I fixed this, but it's back. https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.h:181: bool fIsIco; On 2015/03/23 20:34:51, scroggo wrote: > Can't this be const? Done. https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... src/codec/SkCodec_libico.cpp:153: codecs->operator[](codecs->count() - 1).reset(codec); On 2015/03/23 20:34:51, scroggo wrote: > push_back returns a T&, so you can combine these two lines: > > codecs->push_back().reset(codec); Nice, that's convenient! https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... src/codec/SkCodec_libico.cpp:239: if (kInvalidConversion == result || kInvalidInput == result) { On 2015/03/23 20:34:51, scroggo wrote: > Why did you pick these two? I would think if we got kInvalidScale we'd want to > try again, too. I absolutely agree. I assumed that since we were checking dimensions before-hand in this function, we would never get an invalid scale error. But the other implementations may change, so it can't hurt to check. https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... File src/codec/SkCodec_libico.h (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... src/codec/SkCodec_libico.h:51: * @param embeddedCodec codec for the embedded image On 2015/03/23 20:34:51, scroggo wrote: > Plural. > > Takes ownership Done.
Updated blacklist
lgtm https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... File src/codec/SkCodec_libico.cpp (right): https://codereview.chromium.org/1011343003/diff/180001/src/codec/SkCodec_libi... src/codec/SkCodec_libico.cpp:239: if (kInvalidConversion == result || kInvalidInput == result) { On 2015/03/24 13:08:37, msarett wrote: > On 2015/03/23 20:34:51, scroggo wrote: > > Why did you pick these two? I would think if we got kInvalidScale we'd want to > > try again, too. > > I absolutely agree. I assumed that since we were checking dimensions > before-hand in this function, we would never get an invalid scale error. But > the other implementations may change, so it can't hurt to check. Oh right, I wasn't thinking. I'm fine with or without the extra check. https://codereview.chromium.org/1011343003/diff/220001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1011343003/diff/220001/dm/DM.cpp#newcode149 dm/DM.cpp:149: return strcmp(ext, "png") == 0 || strcmp(ext, "PNG") == 0 || nit: As we add more, perhaps we should follow the example of gather_srcs down below: static const char* const exts[] = { // all the ones we support so far }; for (supportedExt : exts) { if (strcmp(supportedExt, ext) == 0) { return true; } } return false; (I think we discovered the above foreach syntax didn't pass all our bots. Not sure if that's changed with dropping the NaCl bot). I'd be fine with leaving it as is though, especially since the plan is to remove this code once we support all of these. https://codereview.chromium.org/1011343003/diff/220001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1011343003/diff/220001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:22: { SkIcoCodec::IsIco, SkIcoCodec::NewFromStream }, nit: We should put these in order of frequency, so we attempt the most common ones first. I'm guessing BMP is more common than ICO, but that is a completely uneducated guess, so I could certainly be wrong. https://codereview.chromium.org/1011343003/diff/220001/tools/dm_flags.py File tools/dm_flags.py (right): https://codereview.chromium.org/1011343003/diff/220001/tools/dm_flags.py#newc... tools/dm_flags.py:55: blacklist.extend('_ image pal8os2v2.bmp'.split(' ')) Do we also need to blacklist "subset" for these images? (If they don't fail earlier, they'll fail in buildTileIndex, which we don't treat as a failure since not all formats support it. But if they fail earlier, in SkImageDecoder::Factory, DM will treat that as a failure.)
Adding subset to blacklist https://codereview.chromium.org/1011343003/diff/220001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1011343003/diff/220001/dm/DM.cpp#newcode149 dm/DM.cpp:149: return strcmp(ext, "png") == 0 || strcmp(ext, "PNG") == 0 || On 2015/03/24 15:24:42, scroggo wrote: > nit: As we add more, perhaps we should follow the example of gather_srcs down > below: > > static const char* const exts[] = { > // all the ones we support so far > }; > > for (supportedExt : exts) { > if (strcmp(supportedExt, ext) == 0) { > return true; > } > } > > return false; > > > (I think we discovered the above foreach syntax didn't pass all our bots. Not > sure if that's changed with dropping the NaCl bot). > > I'd be fine with leaving it as is though, especially since the plan is to remove > this code once we support all of these. I'll make this change with the addition of gif. https://codereview.chromium.org/1011343003/diff/220001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1011343003/diff/220001/src/codec/SkCodec.cpp#... src/codec/SkCodec.cpp:22: { SkIcoCodec::IsIco, SkIcoCodec::NewFromStream }, On 2015/03/24 15:24:42, scroggo wrote: > nit: We should put these in order of frequency, so we attempt the most common > ones first. > > I'm guessing BMP is more common than ICO, but that is a completely uneducated > guess, so I could certainly be wrong. Will be changed to: Png, JPEG, Gif, Bmp, Ico with a few other formats mixed in as well. https://codereview.chromium.org/1011343003/diff/220001/tools/dm_flags.py File tools/dm_flags.py (right): https://codereview.chromium.org/1011343003/diff/220001/tools/dm_flags.py#newc... tools/dm_flags.py:55: blacklist.extend('_ image pal8os2v2.bmp'.split(' ')) On 2015/03/24 15:24:42, scroggo wrote: > Do we also need to blacklist "subset" for these images? > > (If they don't fail earlier, they'll fail in buildTileIndex, which we don't > treat as a failure since not all formats support it. But if they fail earlier, > in SkImageDecoder::Factory, DM will treat that as a failure.) Most of them "fail" by creating an image that doesn't look right. A few fail in the factory, and I need to add subset for these.
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/1011343003/#ps240001 (title: "Included subset on blacklist")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011343003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 1011343003-240001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com') Presubmit checks took 2.0s to calculate.
msarett@google.com changed reviewers: + djsollen@google.com
Adding Derek. Looks like I need someone to check out the API.
scroggo@google.com changed reviewers: + reed@google.com
Mike can you review the API changes? It only modifies a header file to remove a protected API you wanted to get rid of anyway (we can now call getInfo directly).
api lgtm
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011343003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCE-NoGPU-x86_64...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, djsollen@google.com Link to the patchset: https://codereview.chromium.org/1011343003/#ps260001 (title: "Return kInvalidConversion for 565, Added cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011343003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, djsollen@google.com Link to the patchset: https://codereview.chromium.org/1011343003/#ps280001 (title: "Use 1.0f instead of 1.0 or windows thinks its a double")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011343003/280001
Message was sent while issue was closed.
Committed patchset #10 (id:280001) as https://skia.googlesource.com/skia/+/15bfd075d38e4422a477e22940d06a137f66cc97
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:280001) has been created in https://codereview.chromium.org/1022843005/ by tomhudson@google.com. The reason for reverting is: Reverting on suspicion of massive bot failures - possible command line too long?.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, djsollen@google.com Link to the patchset: https://codereview.chromium.org/1011343003/#ps300001 (title: "Removed additional blacklist items")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011343003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 1011343003-300001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** dm/DM.cpp is missing a correct copyright header. dm/DMSrcSink.cpp is missing a correct copyright header. gyp/codec.gyp is missing a correct copyright header.
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011343003/300001
Message was sent while issue was closed.
Committed patchset #11 (id:300001) as https://skia.googlesource.com/skia/+/9bde918754bc292469d801f156f3b626eb3db780 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
