Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(58)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: src/codec/SkCodec_libgif.cpp

Issue 1386973002: Fix SkGifCodec to handle gifs where frameSize != imageSize (Closed) Base URL: https://skia.googlesource.com/skia.git@master
Patch Set: Created 5 years, 2 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« src/codec/SkCodec_libgif.h ('K') | « src/codec/SkCodec_libgif.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/codec/SkCodec_libgif.cpp
diff --git a/src/codec/SkCodec_libgif.cpp b/src/codec/SkCodec_libgif.cpp
index 6134e96337a4dc3761e3cec6e42a45b168843e06..1397e69a4e06f97289a3339358d4f2493a3e9bb0 100644
--- a/src/codec/SkCodec_libgif.cpp
+++ b/src/codec/SkCodec_libgif.cpp
@@ -185,13 +185,12 @@ bool SkGifCodec::ReadHeader(SkStream* stream, SkCodec** codecOut, GifFileType**
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 (!SetDimensions(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 @@ bool SkGifCodec::ReadHeader(SkStream* stream, SkCodec** codecOut, GifFileType**
// 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 @@ SkCodec* SkGifCodec::NewFromStream(SkStream* stream) {
}
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 @@ SkGifCodec::SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream, GifFileType
// 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 @@ SkCodec::Result SkGifCodec::ReadUpToFirstImage(GifFileType* gif, uint32_t* trans
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 @@ SkCodec::Result SkGifCodec::ReadUpToFirstImage(GifFileType* gif, uint32_t* trans
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::SetDimensions(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 @@ SkCodec::Result SkGifCodec::prepareToDecode(const SkImageInfo& dstInfo, SkPMColo
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::initializeSwizzler(const SkImageInfo& dstInfo,
}
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 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
// 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 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
// 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 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
}
// 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 @@ SkCodec::Result SkGifCodec::onStartScanlineDecode(const SkImageInfo& dstInfo,
// 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 @@ SkCodec::Result SkGifCodec::onGetScanlines(void* dst, int count, size_t rowBytes
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(),
msarett 2015/10/05 20:59:32 This will be deleted soon anyway, but I'll go ahea
+ 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 @@ SkCodec::SkScanlineOrder SkGifCodec::onGetScanlineOrder() const {
}
int SkGifCodec::onNextScanline() const {
+ int nextScanline = this->INHERITED::onNextScanline();
msarett 2015/10/05 20:59:32 This is unfortunate because I think it means that
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;
}
« src/codec/SkCodec_libgif.h ('K') | « src/codec/SkCodec_libgif.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698