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

Unified Diff: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp

Issue 1460523002: GIF decoding to Index8, unit tests and misusing unit test as benchmark (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Created 5 years 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
Index: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
index 07dd9de56970d610b5895b1f6ebbb0dc0d9b8ca7..e2a0888a8147454083127eb2805d1a1875d3196a 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -33,9 +33,11 @@
namespace blink {
-GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
+GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes, ImageFrame::ColorType colorType)
: ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
+ , phaveDecodedRow(&GIFImageDecoder::haveDecodedRowIndex8)
scroggo_chromium 2016/01/06 21:50:40 If the requested color type is N32, shouldn't this
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done. Removed the line since the initialization or
, m_repetitionCount(cAnimationLoopOnce)
+ , m_colorMode(colorType)
{
}
@@ -102,35 +104,38 @@ bool GIFImageDecoder::setFailed()
return ImageDecoder::setFailed();
}
-bool GIFImageDecoder::haveDecodedRow(size_t frameIndex, GIFRow::const_iterator rowBegin, size_t width, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
+static inline const GIFColorMap& getColorMap(const GIFFrameContext& frameContext, const GIFImageReader& reader)
{
- const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
+ return (frameContext.localColorMap().isDefined() ? frameContext.localColorMap() : reader.globalColorMap());
+}
+
+bool GIFImageDecoder::haveDecodedRowN32(const GIFFrameContext& frameContext, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
+{
+ ASSERT(m_frameBufferCache[frameContext.frameId()].bitmap().colorType() == kN32_SkColorType);
+
// 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). 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 xBegin = frameContext->xOffset();
- const int yBegin = frameContext->yOffset() + rowNumber;
- const int xEnd = std::min(static_cast<int>(frameContext->xOffset() + width), size().width());
- const int yEnd = std::min(static_cast<int>(frameContext->yOffset() + rowNumber + repeatCount), size().height());
+ const size_t width = frameContext.width();
+ const int xBegin = frameContext.xOffset();
+ const int yBegin = frameContext.yOffset() + rowNumber;
+ const int xEnd = std::min(static_cast<int>(xBegin + width), size().width());
+ const int yEnd = std::min(static_cast<int>(yBegin + repeatCount), size().height());
if (!width || (xBegin < 0) || (yBegin < 0) || (xEnd <= xBegin) || (yEnd <= yBegin))
return true;
- const GIFColorMap::Table& colorTable = frameContext->localColorMap().isDefined() ? frameContext->localColorMap().table() : m_reader->globalColorMap().table();
+ const GIFColorMap::Table& colorTable = getColorMap(frameContext, *m_reader).table();
if (colorTable.isEmpty())
return true;
GIFColorMap::Table::const_iterator colorTableIter = colorTable.begin();
- // Initialize the frame if necessary.
- ImageFrame& buffer = m_frameBufferCache[frameIndex];
- if ((buffer.status() == ImageFrame::FrameEmpty) && !initFrameBuffer(frameIndex))
- return false;
-
- const size_t transparentPixel = frameContext->transparentPixel();
+ ImageFrame& buffer = m_frameBufferCache[frameContext.frameId()];
+ const size_t transparentPixel = frameContext.transparentPixel();
GIFRow::const_iterator rowEnd = rowBegin + (xEnd - xBegin);
ImageFrame::PixelData* currentAddress = buffer.getAddr(xBegin, yBegin);
@@ -174,6 +179,85 @@ bool GIFImageDecoder::haveDecodedRow(size_t frameIndex, GIFRow::const_iterator r
return true;
}
+bool GIFImageDecoder::haveDecodedRowIndex8(const GIFFrameContext& frameContext, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
scroggo_chromium 2016/01/06 21:50:40 Is there a way to share more code here?
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done.
+{
+ ASSERT(m_frameBufferCache[frameContext.frameId()].bitmap().colorType() == kIndex_8_SkColorType);
+
+ // 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). 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 size_t width = frameContext.width();
+ const int xBegin = frameContext.xOffset();
+ const int yBegin = frameContext.yOffset() + rowNumber;
+ const int xEnd = std::min(static_cast<int>(frameContext.xOffset() + width), size().width());
+ const int yEnd = std::min(static_cast<int>(frameContext.yOffset() + rowNumber + repeatCount), size().height());
+ if (!width || (xBegin < 0) || (yBegin < 0) || (xEnd <= xBegin) || (yEnd <= yBegin))
+ return true;
+
+ const GIFColorMap::Table& colorTable = getColorMap(frameContext, *m_reader).table();
+
+ if (colorTable.isEmpty())
+ return true;
+
+
+ ImageFrame& buffer = m_frameBufferCache[frameContext.frameId()];
+ const size_t transparentPixel = frameContext.transparentPixel();
+ GIFRow::const_iterator rowEnd = rowBegin + (xEnd - xBegin);
+ ImageFrame::PixelData8* currentAddress = buffer.getAddr8(xBegin, yBegin);
+
+ bool opaque = true;
+ // writeTransparentPixels is writing without check, but calculates if there is transparency
+ // (m_currentBufferSawAlpha). If transparency was found in previous rows, no need to calculate here.
+ writeTransparentPixels = writeTransparentPixels || buffer.requiredPreviousFrameIndex() == kNotFound;
+ if (transparentPixel < colorTable.size() && !(writeTransparentPixels && m_currentBufferSawAlpha)) {
+ if (writeTransparentPixels) {
+ while (rowBegin != rowEnd) {
+ opaque = opaque && (*rowBegin ^ transparentPixel);
+ *currentAddress++ = *rowBegin++;
+ }
+ } else {
+ for (; rowBegin != rowEnd; ++rowBegin, ++currentAddress) {
+ size_t index = *rowBegin;
+ if (index == transparentPixel) {
+ opaque = false;
+ } else {
+ *currentAddress = index;
+ }
+ }
+ }
+ } else {
+ // No transparency to deal with.
+ if (xEnd - xBegin == size().width()) {
+ size_t* destination = (size_t*)currentAddress;
+ const size_t* source = (const size_t*)rowBegin;
+ const size_t count = (rowEnd - rowBegin + sizeof(size_t) - 1) / sizeof(size_t);
+ const size_t* sourceEnd = source + count;
+ while (source != sourceEnd)
+ *destination++ = *source++;
+ } else {
+ while (rowBegin != rowEnd)
+ *currentAddress++ = *rowBegin++;
+ }
+ }
+
+ if (!opaque)
+ m_currentBufferSawAlpha = true;
+
+ // Tell the frame to copy the row data if specified.
+ if (repeatCount > 1) {
+ const int rowBytes = (xEnd - xBegin) * sizeof(uint8_t);
+ const ImageFrame::PixelData8* const startAddr = buffer.getAddr8(xBegin, yBegin);
+ for (int destY = yBegin + 1; destY < yEnd; ++destY)
+ memcpy(buffer.getAddr8(xBegin, destY), startAddr, rowBytes);
+ }
+
+ buffer.setPixelsChanged(true);
+ return true;
+}
+
bool GIFImageDecoder::parseCompleted() const
{
return m_reader && m_reader->parseCompleted();
@@ -200,8 +284,8 @@ bool GIFImageDecoder::frameComplete(size_t frameIndex)
// outside its rect doesn't have alpha. To know whether this is
// true, we check the start state of the frame -- if it doesn't have
// alpha, we're safe.
- const ImageFrame* prevBuffer = &m_frameBufferCache[buffer.requiredPreviousFrameIndex()];
- ASSERT(prevBuffer->disposalMethod() != ImageFrame::DisposeOverwritePrevious);
+ const ImageFrame* previousBuffer = &m_frameBufferCache[buffer.requiredPreviousFrameIndex()];
scroggo_chromium 2016/01/06 21:50:41 Why the name change? This name change distracts f
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done.
+ ASSERT(previousBuffer->disposalMethod() != ImageFrame::DisposeOverwritePrevious);
// Now, if we're at a DisposeNotSpecified or DisposeKeep frame, then
// we can say we have no alpha if that frame had no alpha. But
@@ -211,7 +295,7 @@ bool GIFImageDecoder::frameComplete(size_t frameIndex)
// The only remaining case is a DisposeOverwriteBgcolor frame. If
// it had no alpha, and its rect is contained in the current frame's
// rect, we know the current frame has no alpha.
- if ((prevBuffer->disposalMethod() == ImageFrame::DisposeOverwriteBgcolor) && !prevBuffer->hasAlpha() && buffer.originalFrameRect().contains(prevBuffer->originalFrameRect()))
+ if ((previousBuffer->disposalMethod() == ImageFrame::DisposeOverwriteBgcolor) && !previousBuffer->hasAlpha() && buffer.originalFrameRect().contains(previousBuffer->originalFrameRect()))
buffer.setHasAlpha(false);
}
}
@@ -293,6 +377,13 @@ void GIFImageDecoder::decode(size_t index)
setFailed();
}
+void GIFImageDecoder::setupHaveDecodedRowCallbacks(bool isIndex8)
+{
+ phaveDecodedRow = isIndex8
+ ? (&GIFImageDecoder::haveDecodedRowIndex8)
+ : (&GIFImageDecoder::haveDecodedRowN32);
+}
+
void GIFImageDecoder::parse(GIFParseQuery query)
{
if (failed())
@@ -307,33 +398,162 @@ void GIFImageDecoder::parse(GIFParseQuery query)
setFailed();
}
-bool GIFImageDecoder::initFrameBuffer(size_t frameIndex)
+bool GIFImageDecoder::initFrameBufferFromPreviousN32(ImageFrame* const buffer, const ImageFrame& previousBuffer)
scroggo_chromium 2016/01/06 21:50:41 I was a little surprised to see a const pointer. A
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done. Thanks. Method renamed to initFrameBufferFro
{
+ // Preserve the last frame as the starting state for this frame.
+ if (!buffer->copyBitmapData(previousBuffer, ImageFrame::N32))
+ return false;
+
+ if (previousBuffer.disposalMethod() == ImageFrame::DisposeOverwriteBgcolor) {
+ // We want to clear the previous frame to transparent, without
+ // affecting pixels in the image outside of the frame.
+ const IntRect& prevRect = previousBuffer.originalFrameRect();
+ ASSERT(!prevRect.contains(IntRect(IntPoint(), size())));
+ buffer->zeroFillFrameRect(prevRect);
+ }
+ return true;
+}
+
+bool GIFImageDecoder::initFrameBufferN32(size_t frameIndex)
+{
+ setupHaveDecodedRowCallbacks(false);
+
// Initialize the frame rect in our buffer.
ImageFrame* const buffer = &m_frameBufferCache[frameIndex];
size_t requiredPreviousFrameIndex = buffer->requiredPreviousFrameIndex();
if (requiredPreviousFrameIndex == kNotFound) {
// This frame doesn't rely on any previous data.
- if (!buffer->setSize(size().width(), size().height()))
+ if (!buffer->setSize(size().width(), size().height(), getBackgroundColor(frameIndex)))
return setFailed();
} else {
- const ImageFrame* prevBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
- ASSERT(prevBuffer->status() == ImageFrame::FrameComplete);
-
- // Preserve the last frame as the starting state for this frame.
- if (!buffer->copyBitmapData(*prevBuffer))
+ const ImageFrame* previousBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
scroggo_chromium 2016/01/06 21:50:41 Again, I think the name change is distracting.
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done.
+ ASSERT(previousBuffer->status() == ImageFrame::FrameComplete);
+ if (!initFrameBufferFromPreviousN32(buffer, *previousBuffer))
return setFailed();
+ }
- if (prevBuffer->disposalMethod() == ImageFrame::DisposeOverwriteBgcolor) {
- // We want to clear the previous frame to transparent, without
- // affecting pixels in the image outside of the frame.
- const IntRect& prevRect = prevBuffer->originalFrameRect();
- ASSERT(!prevRect.contains(IntRect(IntPoint(), size())));
- buffer->zeroFillFrameRect(prevRect);
+ // Update our status to be partially complete.
+ buffer->setStatus(ImageFrame::FramePartial);
+
+ // Reset the alpha pixel tracker for this frame.
+ m_currentBufferSawAlpha = false;
+ return true;
+}
+
+bool GIFImageDecoder::isIndex8Applicable(const GIFFrameContext& frame, size_t requiredPreviousFrameIndex) const
+{
+ const GIFColorMap& colorMap = getColorMap(frame, *m_reader);
+ const bool useGlobalColorMap = frame.localColorMap().isDefined();
+
+ if (requiredPreviousFrameIndex == kNotFound) {
+ if (colorMap.getTableSize() == 256 && frame.transparentPixel() >= colorMap.getTableSize()
+ && (useGlobalColorMap && m_reader->backgroundIndex() >= colorMap.getTableSize())
scroggo_chromium 2016/01/06 21:50:40 Do you need the parentheses here?
aleksandar.stojiljkovic 2016/01/18 13:58:49 Done.
+ && (!isAllDataReceived() || !frame.frameRect().contains(IntRect(IntPoint(), size())))) {
+ // Background is filled with transparency or with background color (opaque gifs).
+ // Return false when there is a need for transparency and no space in colormap for it;
+ // when colormap is opaque and full (256 entries) and in the same time:
scroggo_chromium 2016/01/06 21:50:40 nit: at* the same time
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done.
+ // - image is not fully received, so missing part is presented as transparent.
+ // - all frames are not fullscreen, i.e. they are decoded to sub rectangle of image rectangle.
scroggo_chromium 2016/01/06 21:50:40 This says "all frames", but this only checks a sin
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done. Yes, the check is per frame, not for all - f
+ return false;
}
+ return true;
}
+ // If frame is sharing colormap with required previous frame, it is possible to keep using Index8.
scroggo_chromium 2016/01/06 21:50:40 nit: If frame shares* ...
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done.
+ const GIFFrameContext& previousFrame(*(m_reader->frameContext(requiredPreviousFrameIndex)));
+ const GIFColorMap& previousColorMap = getColorMap(previousFrame, *m_reader);
+ return ((colorMap.getPosition() == previousColorMap.getPosition()) && (frame.transparentPixel() == previousFrame.transparentPixel()));
+}
+
+// Helper method for recomputing if frame decoding has no dependency to previous frames.
scroggo_chromium 2016/01/06 21:50:40 I find this sentence a little awkward. How about:
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done, "to see " removed.
+void GIFImageDecoder::updateRequiredPreviousFrame(ImageFrame* buffer, const GIFFrameContext& frameContext)
+{
+ ASSERT(m_colorMode == ImageFrame::Index8);
+ if (buffer->requiredPreviousFrameIndex() == kNotFound)
+ return;
+
+ bool isFullScreen = frameContext.frameRect().contains(IntRect(IntPoint(), size()));
+
+ // If transparent pixel is outside color table, there is no transparent pixel.
+ // e.g. Color table in some of the GIFs is having 40 elements and transparentPixel set to 0xFF.
scroggo_chromium 2016/01/06 21:50:41 nit: "is having" -> "has"
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done.
+ bool opaque = frameContext.transparentPixel() >= getColorMap(frameContext, *m_reader).getTableSize();
scroggo_chromium 2016/01/06 21:50:40 nit: If isFullScreen is false, we do not need to c
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done.
+
+ // If fullscreen and opaque, the frame is independent of previous frame.
+ if (isFullScreen && opaque)
+ buffer->setRequiredPreviousFrameIndex(kNotFound);
+}
+
+// Returns transparent index or background index.
+unsigned char GIFImageDecoder::getBackgroundIndex(const GIFFrameContext& frameContext) const
+{
+ const size_t background = frameContext.transparentPixel();
+ const GIFColorMap& colorMap = getColorMap(frameContext, *m_reader);
+ if (background < colorMap.getTableSize())
+ return background;
+
+ if (!frameContext.localColorMap().isDefined() && m_reader->backgroundIndex() < colorMap.getTableSize())
+ return m_reader->backgroundIndex();
+ return 0xFF;
scroggo_chromium 2016/01/06 21:50:40 This still concerns me. Used as an index, 0xFF wil
aleksandar.stojiljkovic 2016/01/18 13:58:50 Done. There is check in isIndex8Applicable is not
+}
+
+SkColor GIFImageDecoder::getBackgroundColor(size_t frameIndex) const
+{
+ const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
+ unsigned char backgroundIndex = getBackgroundIndex(*frameContext);
+ if (frameContext->transparentPixel() == backgroundIndex)
+ return 0;
scroggo_chromium 2016/01/06 21:50:40 SK_ColorTRANSPARENT ? Or do we use 0 elsewhere?
aleksandar.stojiljkovic 2016/01/18 13:58:50 Changed here to SK_ColorTRANSPARENT. 0 is/was in
+ const GIFColorMap::Table& table = getColorMap(*frameContext, *m_reader).table();
+ return (backgroundIndex < table.size() ? table[backgroundIndex]: 0);
+}
+
+bool GIFImageDecoder::initFrameBuffer(size_t frameIndex)
+{
+ ImageFrame* buffer = &m_frameBufferCache[frameIndex];
+ if (buffer->status() != ImageFrame::FrameEmpty) {
+ // If it is partial, decode to the same bitmap.
+ setupHaveDecodedRowCallbacks(buffer->getSkBitmap().colorType() == kIndex_8_SkColorType);
scroggo_chromium 2016/01/06 21:50:40 If it is partial, shouldn't we already be using th
aleksandar.stojiljkovic 2016/01/18 13:58:49 e.g. if partial frame is N32, and previous require
+ return true;
+ }
+
+ if (m_colorMode == ImageFrame::N32) {
+ return initFrameBufferN32(frameIndex);
+ }
+ const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
+ updateRequiredPreviousFrame(buffer, *frameContext);
+ const size_t requiredPreviousFrameIndex = buffer->requiredPreviousFrameIndex();
+
+ bool useIndex8 = true;
+ if (requiredPreviousFrameIndex == kNotFound) {
+ if (isIndex8Applicable(*frameContext, requiredPreviousFrameIndex)) {
+ const unsigned char backgroundIndex = getBackgroundIndex(*frameContext);
+ if (!buffer->setSizeIndex8(size().width(), size().height(), getColorMap(*frameContext, *m_reader).table(),
+ backgroundIndex, backgroundIndex == frameContext->transparentPixel()))
+ return setFailed();
scroggo_chromium 2016/01/06 21:50:40 I don't see this addressed in the style guide, but
aleksandar.stojiljkovic 2016/01/18 13:58:49 Not sure if I did the right thing, let's see if it
+ } else {
+ return initFrameBufferN32(frameIndex);
+ }
+ } else {
+ const ImageFrame* previousBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
+ ASSERT(previousBuffer->status() == ImageFrame::FrameComplete);
+
+ // Copy the required completed frame as the starting state for this frame.
+ useIndex8 = previousBuffer->getSkBitmap().colorType() == kIndex_8_SkColorType;
+ if (useIndex8) {
+ if (isIndex8Applicable(*frameContext, requiredPreviousFrameIndex)) {
+ if (!buffer->copyBitmapData(*previousBuffer))
+ return setFailed();
+ ASSERT(buffer->getSkBitmap().colorType() == kIndex_8_SkColorType);
+ } else {
+ useIndex8 = false;
+ }
+ }
+ // If needed to decode to N32, frame starts as copy of required frame (Index8/N32) to N32.
+ if (!useIndex8 && !initFrameBufferFromPreviousN32(buffer, *previousBuffer))
+ return setFailed();
+ }
+ setupHaveDecodedRowCallbacks(useIndex8);
+
// Update our status to be partially complete.
buffer->setStatus(ImageFrame::FramePartial);
@@ -342,4 +562,30 @@ bool GIFImageDecoder::initFrameBuffer(size_t frameIndex)
return true;
}
+bool GIFImageDecoder::canDecodeTo(size_t index, ImageFrame::ColorType outputType)
+{
+ if ((index >= frameCount()) || (m_colorMode == ImageFrame::N32) || failed())
scroggo_chromium 2016/01/06 21:50:40 if outputType == N32, shouldn't we always return t
aleksandar.stojiljkovic 2016/01/18 13:58:50 No, the recent changes made this deterministic ear
+ return (outputType == m_colorMode);
+
+ // Go from one to previous until calculating if Index8 is supported.
+ size_t frameToDecode = index;
+ ImageFrame::ColorType calculatedOutput = ImageFrame::Index8;
+ while (frameToDecode != kNotFound) {
+ ImageFrame* buffer = &m_frameBufferCache[frameToDecode];
+ if (buffer->status() == ImageFrame::FrameComplete) {
+ // In this case, color type from complete frame is kept.
+ calculatedOutput = static_cast<ImageFrame::ColorType>(buffer->getSkBitmap().colorType());
+ break;
+ }
+ const GIFFrameContext* frameContext = m_reader->frameContext(frameToDecode);
+ updateRequiredPreviousFrame(buffer, *frameContext);
+ if (!isIndex8Applicable(*frameContext, buffer->requiredPreviousFrameIndex())) {
+ calculatedOutput = ImageFrame::N32;
+ break;
+ }
+ frameToDecode = buffer->requiredPreviousFrameIndex();
+ }
+ return (calculatedOutput == outputType);
+}
+
} // namespace blink

Powered by Google App Engine
This is Rietveld 408576698