"Ensure frame rect is on screen or empty" - take 2
Revert commit e13b82fac077383d9f93631a177573509be44f38
(I215b312b421a30bf985f681424a4c4198f069862), and fix a different way.
The prior fix resulted in incorrectly decoding a frame whose bounds
extended beyond the canvas. Instead of changing the internal frame rect,
just change the one passed to the client.
Change-Id: I82568cb672f1027a9dc17ec0a15d355d561c356d
Reviewed-on: https://skia-review.googlesource.com/c/libgifcodec/+/353445
Reviewed-by: Derek Sollenberger <djsollen@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>
diff --git a/SkGifImageReader.cpp b/SkGifImageReader.cpp
index 6d37d59..f0184cb 100644
--- a/SkGifImageReader.cpp
+++ b/SkGifImageReader.cpp
@@ -767,12 +767,6 @@
if (currentFrameIsFirstFrame()) {
fScreenHeight = std::max(fScreenHeight, yOffset + height);
fScreenWidth = std::max(fScreenWidth, xOffset + width);
- } else {
- // If a non-first frame is offscreen, it will have no effect on
- // the output image. Modify its offsets to be consistent with
- // the Wuffs implementation.
- if (xOffset > fScreenWidth) xOffset = fScreenWidth;
- if (yOffset > fScreenHeight) yOffset = fScreenHeight;
}
// NOTE: Chromium placed this block after setHeaderDefined, down
@@ -820,8 +814,7 @@
return SkCodec::kSuccess;
}
- width = std::min(width, fScreenWidth - xOffset);
- height = std::min(height, fScreenHeight - yOffset);
+
currentFrame->setXYWH(xOffset, yOffset, width, height);
currentFrame->setInterlaced(SkToBool(currentComponent[8] & 0x40));
diff --git a/SkLibGifCodec.cpp b/SkLibGifCodec.cpp
index b76c18f..95f5bf1 100644
--- a/SkLibGifCodec.cpp
+++ b/SkLibGifCodec.cpp
@@ -130,6 +130,18 @@
if (frameInfo) {
frameContext->fillIn(frameInfo, frameContext->isComplete());
+
+ auto* rect = &frameInfo->fFrameRect;
+ auto bounds = this->bounds();
+ if (!rect->intersect(bounds)) {
+ // If a frame is offscreen, it will have no effect on the output
+ // image. Modify its bounds to be consistent with the Wuffs
+ // implementation.
+ rect->setLTRB(std::min(rect->left(), bounds.right()),
+ std::min(rect->top(), bounds.bottom()),
+ std::min(rect->right(), bounds.right()),
+ std::min(rect->bottom(), bounds.bottom()));
+ }
}
return true;
}
@@ -225,9 +237,12 @@
// This is only called by prepareToDecode, which ensures frameIndex is in range.
SkASSERT(frame);
- // Swizzler::Make only reads left and right of this rect.
- SkIRect swizzleRect = frame->frameRect();
- SkASSERT(swizzleRect.right() <= fReader->screenWidth());
+ const int xBegin = frame->xOffset();
+ const int xEnd = std::min(frame->frameRect().right(), fReader->screenWidth());
+
+ // CreateSwizzler only reads left and right of the frame. We cannot use the frame's raw
+ // frameRect, since it might extend beyond the edge of the frame.
+ SkIRect swizzleRect = SkIRect::MakeLTRB(xBegin, 0, xEnd, 0);
SkImageInfo swizzlerInfo = dstInfo;
if (this->colorXform()) {
@@ -404,15 +419,15 @@
const SkGIFFrameContext* frameContext = fReader->frameContext(frameIndex);
// The pixel data and coordinates supplied to us are relative to the frame's
// origin within the entire image size, i.e.
- // (frameContext->xOffset, frameContext->yOffset).
+ // (frameContext->xOffset, frameContext->yOffset). There is no guarantee
+ // that width == (size().width() - frameContext->xOffset), so
+ // we must ensure we don't run off the end of either the source data or the
+ // row's X-coordinates.
const int width = frameContext->width();
const int xBegin = frameContext->xOffset();
const int yBegin = frameContext->yOffset() + rowNumber;
- const int xEnd = xBegin + width;
+ const int xEnd = std::min(xBegin + width, this->dimensions().width());
const int yEnd = std::min(yBegin + repeatCount, this->dimensions().height());
-
- SkASSERT(xEnd <= this->dimensions().width());
-
// FIXME: No need to make the checks on width/xBegin/xEnd for every row. We could instead do
// this once in prepareToDecode.
if (!width || (xBegin < 0) || (yBegin < 0) || (xEnd <= xBegin) || (yEnd <= yBegin))
