Fix SkGifCodec to handle gifs where frameSize != imageSize
These are quite rare, causing us to miss a few bugs in how
we deal with these images.
Additionally, there is a behavior change. If the imageSize
is not large enough to contain the frame, we will "fix" the
image by increasing the image size.
SkScaledCodec is still buggy with regard to these gifs.
See skbug.com/4421. We will fix that after 1332053002
lands.
BUG=skia:
Review URL: https://codereview.chromium.org/1386973002
diff --git a/src/codec/SkCodec_libgif.cpp b/src/codec/SkCodec_libgif.cpp
index 6134e96..7b38047 100644
--- a/src/codec/SkCodec_libgif.cpp
+++ b/src/codec/SkCodec_libgif.cpp
@@ -185,13 +185,12 @@
SkASSERT(gif->ImageCount >= 1);
if (nullptr != codecOut) {
- // Get fields from header
- const int32_t width = gif->SWidth;
- const int32_t height = gif->SHeight;
- if (width <= 0 || height <= 0) {
- gif_error("Invalid dimensions.\n");
- return false;
+ SkISize size;
+ SkIRect frameRect;
+ if (!GetDimensions(gif, &size, &frameRect)) {
+ gif_error("Invalid gif size.\n");
}
+ bool frameIsSubset = (size != frameRect.size());
// Determine the recommended alpha type. The transIndex might be valid if it less
// than 256. We are not certain that the index is valid until we process the color
@@ -208,9 +207,10 @@
// Return the codec
// kIndex is the most natural color type for gifs, so we set this as
// the default.
- const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
- kIndex_8_SkColorType, alphaType);
- *codecOut = new SkGifCodec(imageInfo, streamDeleter.detach(), gif.detach(), transIndex);
+ SkImageInfo imageInfo = SkImageInfo::Make(size.width(), size.height(), kIndex_8_SkColorType,
+ alphaType);
+ *codecOut = new SkGifCodec(imageInfo, streamDeleter.detach(), gif.detach(), transIndex,
+ frameRect, frameIsSubset);
} else {
SkASSERT(nullptr != gifOut);
streamDeleter.detach();
@@ -233,7 +233,7 @@
}
SkGifCodec::SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream, GifFileType* gif,
- uint32_t transIndex)
+ uint32_t transIndex, const SkIRect& frameRect, bool frameIsSubset)
: INHERITED(srcInfo, stream)
, fGif(gif)
, fSrcBuffer(new uint8_t[this->getInfo().width()])
@@ -244,8 +244,8 @@
// Default fFillIndex is 0. We will overwrite this if fTransIndex is valid, or if
// there is a valid background color.
, fFillIndex(0)
- , fFrameDims(SkIRect::MakeEmpty())
- , fFrameIsSubset(false)
+ , fFrameRect(frameRect)
+ , fFrameIsSubset(frameIsSubset)
, fColorTable(NULL)
, fSwizzler(NULL)
{}
@@ -283,6 +283,7 @@
switch (recordType) {
case IMAGE_DESC_RECORD_TYPE: {
*transIndex = find_trans_index(saveExt);
+
// FIXME: Gif files may have multiple images stored in a single
// file. This is most commonly used to enable
// animations. Since we are leaving animated gifs as a
@@ -346,53 +347,31 @@
return gif_error("Could not find any images to decode in gif file.\n", kInvalidInput);
}
-/*
- * A gif may contain many image frames, all of different sizes.
- * This function checks if the frame dimensions are valid and corrects them if
- * necessary.
- */
-bool SkGifCodec::setFrameDimensions(const GifImageDesc& desc) {
- // Fail on non-positive dimensions
- int32_t frameLeft = desc.Left;
- int32_t frameTop = desc.Top;
- int32_t frameWidth = desc.Width;
- int32_t frameHeight = desc.Height;
- int32_t height = this->getInfo().height();
- int32_t width = this->getInfo().width();
- if (frameWidth <= 0 || frameHeight <= 0) {
+bool SkGifCodec::GetDimensions(GifFileType* gif, SkISize* size, SkIRect* frameRect) {
+ // Get the encoded dimension values
+ SavedImage* image = &gif->SavedImages[gif->ImageCount - 1];
+ const GifImageDesc& desc = image->ImageDesc;
+ int frameLeft = desc.Left;
+ int frameTop = desc.Top;
+ int frameWidth = desc.Width;
+ int frameHeight = desc.Height;
+ int width = gif->SWidth;
+ int height = gif->SHeight;
+
+ // Ensure that the decode dimensions are large enough to contain the frame
+ width = SkTMax(width, frameWidth + frameLeft);
+ height = SkTMax(height, frameHeight + frameTop);
+
+ // All of these dimensions should be positive, as they are encoded as unsigned 16-bit integers.
+ // It is unclear why giflib casts them to ints. We will go ahead and check that they are
+ // in fact positive.
+ if (frameLeft < 0 || frameTop < 0 || frameWidth < 0 || frameHeight < 0 || width <= 0 ||
+ height <= 0) {
return false;
}
- // Treat the following cases as warnings and try to fix
- if (frameWidth > width) {
- gif_warning("Image frame too wide, shrinking.\n");
- frameWidth = width;
- frameLeft = 0;
- } else if (frameLeft + frameWidth > width) {
- gif_warning("Shifting image frame to left to fit.\n");
- frameLeft = width - frameWidth;
- } else if (frameLeft < 0) {
- gif_warning("Shifting image frame to right to fit\n");
- frameLeft = 0;
- }
- if (frameHeight > height) {
- gif_warning("Image frame too tall, shrinking.\n");
- frameHeight = height;
- frameTop = 0;
- } else if (frameTop + frameHeight > height) {
- gif_warning("Shifting image frame up to fit.\n");
- frameTop = height - frameHeight;
- } else if (frameTop < 0) {
- gif_warning("Shifting image frame down to fit\n");
- frameTop = 0;
- }
- fFrameDims.setXYWH(frameLeft, frameTop, frameWidth, frameHeight);
-
- // Indicate if the frame dimensions do not match the header dimensions
- if (this->getInfo().dimensions() != fFrameDims.size()) {
- fFrameIsSubset = true;
- }
-
+ frameRect->setXYWH(frameLeft, frameTop, frameWidth, frameHeight);
+ size->set(width, height);
return true;
}
@@ -465,16 +444,6 @@
kInvalidConversion);
}
-
- // We have asserted that the image count is at least one in ReadHeader().
- SavedImage* image = &fGif->SavedImages[fGif->ImageCount - 1];
- const GifImageDesc& desc = image->ImageDesc;
-
- // Check that the frame dimensions are valid and set them
- if(!this->setFrameDimensions(desc)) {
- return gif_error("Invalid dimensions for image frame.\n", kInvalidInput);
- }
-
// Initialize color table and copy to the client if necessary
this->initializeColorTable(dstInfo, inputColorPtr, inputColorCount);
return kSuccess;
@@ -492,7 +461,7 @@
}
SkCodec::Result SkGifCodec::readRow() {
- if (GIF_ERROR == DGifGetLine(fGif, fSrcBuffer.get(), fFrameDims.width())) {
+ if (GIF_ERROR == DGifGetLine(fGif, fSrcBuffer.get(), fFrameRect.width())) {
return kIncompleteInput;
}
return kSuccess;
@@ -517,7 +486,7 @@
// Initialize the swizzler
if (fFrameIsSubset) {
- const SkImageInfo subsetDstInfo = dstInfo.makeWH(fFrameDims.width(), fFrameDims.height());
+ const SkImageInfo subsetDstInfo = dstInfo.makeWH(fFrameRect.width(), fFrameRect.height());
if (kSuccess != this->initializeSwizzler(subsetDstInfo, opts.fZeroInitialized)) {
return gif_error("Could not initialize swizzler.\n", kUnimplemented);
}
@@ -529,8 +498,8 @@
// Modify the dst pointer
const int32_t dstBytesPerPixel = SkColorTypeBytesPerPixel(dstInfo.colorType());
- dst = SkTAddOffset<void*>(dst, dstRowBytes * fFrameDims.top() +
- dstBytesPerPixel * fFrameDims.left());
+ dst = SkTAddOffset<void*>(dst, dstRowBytes * fFrameRect.top() +
+ dstBytesPerPixel * fFrameRect.left());
} else {
if (kSuccess != this->initializeSwizzler(dstInfo, opts.fZeroInitialized)) {
return gif_error("Could not initialize swizzler.\n", kUnimplemented);
@@ -538,8 +507,8 @@
}
// Check the interlace flag and iterate over rows of the input
- uint32_t width = fFrameDims.width();
- uint32_t height = fFrameDims.height();
+ uint32_t width = fFrameRect.width();
+ uint32_t height = fFrameRect.height();
if (fGif->Image.Interlace) {
// In interlace mode, the rows of input are rearranged in
// the output image. We a helper function to help us
@@ -586,7 +555,7 @@
// Initialize the swizzler
if (fFrameIsSubset) {
- const SkImageInfo subsetDstInfo = dstInfo.makeWH(fFrameDims.width(), fFrameDims.height());
+ const SkImageInfo subsetDstInfo = dstInfo.makeWH(fFrameRect.width(), fFrameRect.height());
if (kSuccess != this->initializeSwizzler(subsetDstInfo, opts.fZeroInitialized)) {
return gif_error("Could not initialize swizzler.\n", kUnimplemented);
}
@@ -607,37 +576,32 @@
colorPtr, this->options().fZeroInitialized);
// Do nothing for rows before the image frame
- // FIXME: nextScanline is not virtual, so using "INHERITED" does not change
- // behavior. Was the intent to call this->INHERITED::onNextScanline()? Same
- // for the next call down below.
- int rowsBeforeFrame = fFrameDims.top() - this->INHERITED::nextScanline();
- if (rowsBeforeFrame > 0) {
- count = SkTMin(0, count - rowsBeforeFrame);
- dst = SkTAddOffset<void>(dst, rowBytes * rowsBeforeFrame);
- }
+ int rowsBeforeFrame = SkTMax(0, fFrameRect.top() - this->INHERITED::onNextScanline());
+ count = SkTMax(0, count - rowsBeforeFrame);
+ dst = SkTAddOffset<void>(dst, rowBytes * rowsBeforeFrame);
// Do nothing for rows after the image frame
- int rowsAfterFrame = this->INHERITED::nextScanline() + count - fFrameDims.bottom();
- if (rowsAfterFrame > 0) {
- count = SkTMin(0, count - rowsAfterFrame);
- }
+ int rowsAfterFrame = SkTMax(0,
+ this->INHERITED::onNextScanline() + count - fFrameRect.bottom());
+ count = SkTMax(0, count - rowsAfterFrame);
- // Adjust dst pointer for left offset
- int bpp = SkColorTypeBytesPerPixel(this->dstInfo().colorType()) * fFrameDims.left();
- dst = SkTAddOffset<void>(dst, bpp);
+ // Adjust dst pointer for left offset
+ int offset = SkColorTypeBytesPerPixel(this->dstInfo().colorType()) * fFrameRect.left();
+ dst = SkTAddOffset<void>(dst, offset);
}
for (int i = 0; i < count; i++) {
if (kSuccess != this->readRow()) {
const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
- SkSwizzler::Fill(dst, this->dstInfo(), rowBytes, count - i, fFillIndex, colorPtr,
- this->options().fZeroInitialized);
- return gif_error("Could not decode line\n", SkCodec::kIncompleteInput);
+ SkSwizzler::Fill(dst, this->dstInfo().makeWH(fFrameRect.width(),
+ this->dstInfo().height()), rowBytes, count - i, fFillIndex, colorPtr,
+ this->options().fZeroInitialized);
+ return kIncompleteInput;
}
fSwizzler->swizzle(dst, fSrcBuffer.get());
dst = SkTAddOffset<void>(dst, rowBytes);
}
- return SkCodec::kSuccess;
+ return kSuccess;
}
SkCodec::SkScanlineOrder SkGifCodec::onGetScanlineOrder() const {
@@ -649,10 +613,13 @@
}
int SkGifCodec::onNextScanline() const {
+ int nextScanline = this->INHERITED::onNextScanline();
if (fGif->Image.Interlace) {
- return get_output_row_interlaced(this->INHERITED::onNextScanline(), this->dstInfo().height());
- } else {
- return this->INHERITED::onNextScanline();
+ if (nextScanline < fFrameRect.top() || nextScanline >= fFrameRect.bottom()) {
+ return nextScanline;
+ }
+ return get_output_row_interlaced(nextScanline - fFrameRect.top(), fFrameRect.height());
}
+ return nextScanline;
}
diff --git a/src/codec/SkCodec_libgif.h b/src/codec/SkCodec_libgif.h
index c29cbdb..f777e58 100644
--- a/src/codec/SkCodec_libgif.h
+++ b/src/codec/SkCodec_libgif.h
@@ -84,13 +84,18 @@
/*
* A gif may contain many image frames, all of different sizes.
- * This function checks if the frame dimensions are valid and corrects
- * them if necessary. It then sets fFrameDims to the corrected
- * dimensions.
+ * This function checks if the gif dimensions are valid, based on the frame
+ * dimensions, and corrects the gif dimensions if necessary.
*
- * @param desc The image frame descriptor
+ * @param gif Pointer to the library type that manages the gif decode
+ * @param size Size of the image that we will decode.
+ * Will be set by this function if the return value is true.
+ * @param frameRect Contains the dimenions and offset of the first image frame.
+ * Will be set by this function if the return value is true.
+ *
+ * @return true on success, false otherwise
*/
- bool setFrameDimensions(const GifImageDesc& desc);
+ static bool GetDimensions(GifFileType* gif, SkISize* size, SkIRect* frameRect);
/*
* Initializes the color table that we will use for decoding.
@@ -164,14 +169,15 @@
* @param transIndex The transparent index. An invalid value
* indicates that there is no transparent index.
*/
- SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream, GifFileType* gif, uint32_t transIndex);
+ SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream, GifFileType* gif, uint32_t transIndex,
+ const SkIRect& frameRect, bool frameIsSubset);
SkAutoTCallVProc<GifFileType, CloseGif> fGif; // owned
SkAutoTDeleteArray<uint8_t> fSrcBuffer;
- SkIRect fFrameDims;
+ const SkIRect fFrameRect;
const uint32_t fTransIndex;
uint32_t fFillIndex;
- bool fFrameIsSubset;
+ const bool fFrameIsSubset;
SkAutoTDelete<SkSwizzler> fSwizzler;
SkAutoTUnref<SkColorTable> fColorTable;