Attachment 288695 Details for Bug 159721 – Patch

[patch] Patch

bug-159721-20160913103009.patch (text/plain), 19.82 KB, created by Said Abou-Hallawa on 2016-09-13 10:32:44 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Said Abou-Hallawa

Created: 2016-09-13 10:32:44 PDT

Size: 19.82 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 205861)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,51 @@
>+2016-09-13  Said Abou-Hallawa  <sabouhallawa@apple.com>
>+
>+        Get rid of the m_premultiplyAlpha flag of the ImageFrame class
>+        https://bugs.webkit.org/show_bug.cgi?id=159721
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This flag was only needed when calling ImageBackingStore::create() in 
>+        ImageFrame::setSize(). Instead we can pass ImageDecoder::m_premultiplyAlpha 
>+        to ImageFrame::setSize(), which should be renamed ImageFrame::initializeBackingStore().
>+        The passed premultiplyAlpha can then be passed to ImageBackingStore::create().
>+
>+        * platform/image-decoders/ImageDecoder.cpp:
>+        (WebCore::ImageFrame::ImageFrame):
>+        (WebCore::ImageFrame::operator=):
>+        (WebCore::ImageFrame::initializeBackingStore):
>+        (WebCore::ImageFrame::copyBitmapData): Deleted.
>+        (WebCore::ImageFrame::setSize): Deleted.
>+        * platform/image-decoders/ImageDecoder.h:
>+        (WebCore::ImageFrame::disposalMethod):
>+        (WebCore::ImageFrame::setDisposalMethod):
>+        (WebCore::ImageDecoder::premultiplyAlpha):
>+        (WebCore::ImageFrame::premultiplyAlpha): Deleted.
>+        (WebCore::ImageFrame::setPremultiplyAlpha): Deleted.
>+        * platform/image-decoders/bmp/BMPImageDecoder.cpp:
>+        (WebCore::BMPImageDecoder::frameBufferAtIndex):
>+        * platform/image-decoders/bmp/BMPImageReader.cpp:
>+        (WebCore::BMPImageReader::decodeBMP):
>+        * platform/image-decoders/gif/GIFImageDecoder.cpp:
>+        (WebCore::GIFImageDecoder::decode):
>+        (WebCore::GIFImageDecoder::initFrameBuffer):
>+        * platform/image-decoders/ico/ICOImageDecoder.cpp:
>+        (WebCore::ICOImageDecoder::frameCount):
>+        * platform/image-decoders/jpeg/JPEGImageDecoder.cpp:
>+        (WebCore::JPEGImageDecoder::frameBufferAtIndex):
>+        (WebCore::JPEGImageDecoder::outputScanlines):
>+        * platform/image-decoders/png/PNGImageDecoder.cpp:
>+        (WebCore::PNGImageDecoder::frameBufferAtIndex):
>+        (WebCore::PNGImageDecoder::rowAvailable):
>+        (WebCore::PNGImageDecoder::readChunks):
>+        (WebCore::PNGImageDecoder::initFrameBuffer):
>+        (WebCore::setPixelRGB): Deleted.
>+        (WebCore::setPixelRGBA): Deleted.
>+        (WebCore::setPixelPremultipliedRGBA): Deleted.
>+        * platform/image-decoders/webp/WEBPImageDecoder.cpp:
>+        (WebCore::WEBPImageDecoder::frameBufferAtIndex):
>+        (WebCore::WEBPImageDecoder::decode):
>+
> 2016-09-12  Zalan Bujtas  <zalan@apple.com>
> 
>         Input type object and the associated render can go out of sync.
>Index: Source/WebCore/platform/image-decoders/ImageDecoder.cpp
>===================================================================
>--- Source/WebCore/platform/image-decoders/ImageDecoder.cpp	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/ImageDecoder.cpp	(working copy)
>@@ -132,7 +132,6 @@ ImageFrame::ImageFrame()
>     , m_status(FrameEmpty)
>     , m_duration(0)
>     , m_disposalMethod(DisposeNotSpecified)
>-    , m_premultiplyAlpha(true)
> {
> } 
> 
>@@ -141,7 +140,7 @@ ImageFrame& ImageFrame::operator=(const 
>     if (this == &other)
>         return *this;
> 
>-    copyBitmapData(other);
>+    initializeBackingStore(other.backingStore());
>     setHasAlpha(other.m_hasAlpha);
>     setStatus(other.status());
>     setDuration(other.duration());
>@@ -175,23 +174,24 @@ void ImageFrame::zeroFillFrameRect(const
>     setHasAlpha(true);
> }
> 
>-bool ImageFrame::copyBitmapData(const ImageFrame& other)
>+bool ImageFrame::initializeBackingStore(const ImageBackingStore* backingStore)
> {
>-    if (this == &other)
>+    if (!backingStore)
>+        return false;
>+
>+    if (backingStore == this->backingStore())
>         return true;
> 
>-    if (other.m_backingStore)
>-        m_backingStore = ImageBackingStore::create(*other.m_backingStore.get());
>-    else
>-        m_backingStore = nullptr;
>-    
>-    return true;
>+    m_backingStore = ImageBackingStore::create(backingStore);
>+    return m_backingStore != nullptr;
> }
> 
>-bool ImageFrame::setSize(const IntSize& size)
>+bool ImageFrame::initializeBackingStore(const IntSize& size, bool premultiplyAlpha)
> {
>-    ASSERT(!m_backingStore);
>-    m_backingStore = ImageBackingStore::create(size, m_premultiplyAlpha);
>+    if (size.isEmpty())
>+        return false;
>+
>+    m_backingStore = ImageBackingStore::create(size, premultiplyAlpha);
>     return m_backingStore != nullptr;
> }
> 
>Index: Source/WebCore/platform/image-decoders/ImageDecoder.h
>===================================================================
>--- Source/WebCore/platform/image-decoders/ImageDecoder.h	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/ImageDecoder.h	(working copy)
>@@ -72,12 +72,7 @@ using ColorProfile = Vector<char>;
>         void clearPixelData();
>         void zeroFillPixelData();
>         void zeroFillFrameRect(const IntRect&);
>-
>-        // Makes this frame have an independent copy of the provided image's
>-        // pixel data, so that modifications in one frame are not reflected in
>-        // the other.  Returns whether the copy succeeded.
>-        bool copyBitmapData(const ImageFrame&);
>-
>+        
>         // Copies the pixel data at [(startX, startY), (endX, startY)) to the
>         // same X-coordinates on each subsequent row up to but not including
>         // endY.
>@@ -86,10 +81,14 @@ using ColorProfile = Vector<char>;
>             m_backingStore->repeatFirstRow(IntRect(startX, startY, endX -startX , endY - startY));
>         }
> 
>-        // Allocates space for the pixel data.  Must be called before any pixels
>-        // are written.  Must only be called once.  Returns whether allocation
>-        // succeeded.
>-        bool setSize(const IntSize&);
>+        // Makes this frame have an independent copy of the provided image's
>+        // pixel data, so that modifications in one frame are not reflected in
>+        // the other. Returns whether the copy succeeded.
>+        bool initializeBackingStore(const ImageBackingStore*);
>+
>+        // Allocates space for the pixel data. Returns whether allocation succeeded.
>+        bool initializeBackingStore(const IntSize&, bool premultiplyAlpha);
>+
>         IntSize size() const { return m_backingStore ? m_backingStore->size() : IntSize(); }
> 
>         // Returns a caller-owned pointer to the underlying native image data.
>@@ -105,14 +104,12 @@ using ColorProfile = Vector<char>;
>         FrameStatus status() const { return m_status; }
>         unsigned duration() const { return m_duration; }
>         FrameDisposalMethod disposalMethod() const { return m_disposalMethod; }
>-        bool premultiplyAlpha() const { return m_premultiplyAlpha; }
> 
>         void setHasAlpha(bool alpha);
>         void setOriginalFrameRect(const IntRect&);
>         void setStatus(FrameStatus status);
>         void setDuration(unsigned duration) { m_duration = duration; }
>         void setDisposalMethod(FrameDisposalMethod method) { m_disposalMethod = method; }
>-        void setPremultiplyAlpha(bool premultiplyAlpha) { m_premultiplyAlpha = premultiplyAlpha; }
> 
>         inline RGBA32* pixelAt(int x, int y)
>         {
>@@ -146,7 +143,6 @@ using ColorProfile = Vector<char>;
>         FrameStatus m_status;
>         unsigned m_duration;
>         FrameDisposalMethod m_disposalMethod;
>-        bool m_premultiplyAlpha;
>     };
> 
>     // ImageDecoder is a base for all format-specific decoders
>@@ -174,6 +170,8 @@ using ColorProfile = Vector<char>;
>         static std::unique_ptr<ImageDecoder> create(const SharedBuffer& data, ImageSource::AlphaOption, ImageSource::GammaAndColorProfileOption);
> 
>         virtual String filenameExtension() const = 0;
>+        
>+        bool premultiplyAlpha() const { return m_premultiplyAlpha; }
> 
>         bool isAllDataReceived() const { return m_isAllDataReceived; }
> 
>Index: Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.cpp
>===================================================================
>--- Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.cpp	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.cpp	(working copy)
>@@ -70,10 +70,8 @@ ImageFrame* BMPImageDecoder::frameBuffer
>     if (index)
>         return 0;
> 
>-    if (m_frameBufferCache.isEmpty()) {
>+    if (m_frameBufferCache.isEmpty())
>         m_frameBufferCache.resize(1);
>-        m_frameBufferCache.first().setPremultiplyAlpha(m_premultiplyAlpha);
>-    }
> 
>     ImageFrame* buffer = &m_frameBufferCache.first();
>     if (buffer->status() != ImageFrame::FrameComplete)
>Index: Source/WebCore/platform/image-decoders/bmp/BMPImageReader.cpp
>===================================================================
>--- Source/WebCore/platform/image-decoders/bmp/BMPImageReader.cpp	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/bmp/BMPImageReader.cpp	(working copy)
>@@ -78,12 +78,10 @@ bool BMPImageReader::decodeBMP(bool only
>     // Initialize the framebuffer if needed.
>     ASSERT(m_buffer);  // Parent should set this before asking us to decode!
>     if (m_buffer->status() == ImageFrame::FrameEmpty) {
>-        if (!m_buffer->setSize(m_parent->size()))
>+        if (!m_buffer->initializeBackingStore(m_parent->size(), m_parent->premultiplyAlpha()))
>             return m_parent->setFailed(); // Unable to allocate.
>+
>         m_buffer->setStatus(ImageFrame::FramePartial);
>-        // setSize() calls eraseARGB(), which resets the alpha flag, so we force
>-        // it back to false here.  We'll set it true below in all cases where
>-        // these 0s could actually show through.
>         m_buffer->setHasAlpha(false);
> 
>         if (!m_isTopDown)
>Index: Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp
>===================================================================
>--- Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp	(working copy)
>@@ -321,10 +321,7 @@ void GIFImageDecoder::decode(unsigned ha
>         return;
>     }
> 
>-    const size_t oldSize = m_frameBufferCache.size();
>     m_frameBufferCache.resize(m_reader->imagesCount());
>-    for (size_t i = oldSize; i < m_reader->imagesCount(); ++i)
>-        m_frameBufferCache[i].setPremultiplyAlpha(m_premultiplyAlpha);
> 
>     if (query == GIFFrameCountQuery)
>         return;
>@@ -361,7 +358,7 @@ bool GIFImageDecoder::initFrameBuffer(un
> 
>     if (!frameIndex) {
>         // This is the first frame, so we're not relying on any previous data.
>-        if (!buffer->setSize(scaledSize()))
>+        if (!buffer->initializeBackingStore(scaledSize(), m_premultiplyAlpha))
>             return setFailed();
>     } else {
>         // The starting state for this frame depends on the previous frame's
>@@ -382,7 +379,7 @@ bool GIFImageDecoder::initFrameBuffer(un
> 
>         if ((prevMethod == ImageFrame::DisposeNotSpecified) || (prevMethod == ImageFrame::DisposeKeep)) {
>             // Preserve the last frame as the starting state for this frame.
>-            if (!buffer->copyBitmapData(*prevBuffer))
>+            if (!buffer->initializeBackingStore(prevBuffer->backStore()))
>                 return setFailed();
>         } else {
>             // We want to clear the previous frame to transparent, without
>@@ -392,11 +389,11 @@ bool GIFImageDecoder::initFrameBuffer(un
>             if (!frameIndex || prevRect.contains(IntRect(IntPoint(), scaledSize()))) {
>                 // Clearing the first frame, or a frame the size of the whole
>                 // image, results in a completely empty image.
>-                if (!buffer->setSize(bufferSize))
>+                if (!buffer->initializeBackingStore(bufferSize, m_premultiplyAlpha))
>                     return setFailed();
>             } else {
>                 // Copy the whole previous buffer, then clear just its frame.
>-                if (!buffer->copyBitmapData(*prevBuffer))
>+                if (!buffer->initializeBackingStore(prevBuffer->backingStore()))
>                     return setFailed();
>                 buffer->zeroFillFrameRect(prevRect);
>             }
>Index: Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp
>===================================================================
>--- Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp	(working copy)
>@@ -98,11 +98,8 @@ bool ICOImageDecoder::setSize(const IntS
> size_t ICOImageDecoder::frameCount()
> {
>     decode(0, true);
>-    if (m_frameBufferCache.isEmpty()) {
>+    if (m_frameBufferCache.isEmpty())
>         m_frameBufferCache.resize(m_dirEntries.size());
>-        for (size_t i = 0; i < m_dirEntries.size(); ++i)
>-            m_frameBufferCache[i].setPremultiplyAlpha(m_premultiplyAlpha);
>-    }
>     // CAUTION: We must not resize m_frameBufferCache again after this, as
>     // decodeAtIndex() may give a BMPImageReader a pointer to one of the
>     // entries.
>Index: Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
>===================================================================
>--- Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp	(working copy)
>@@ -533,10 +533,8 @@ ImageFrame* JPEGImageDecoder::frameBuffe
>     if (index)
>         return 0;
> 
>-    if (m_frameBufferCache.isEmpty()) {
>+    if (m_frameBufferCache.isEmpty())
>         m_frameBufferCache.resize(1);
>-        m_frameBufferCache[0].setPremultiplyAlpha(m_premultiplyAlpha);
>-    }
> 
>     ImageFrame& frame = m_frameBufferCache[0];
>     if (frame.status() != ImageFrame::FrameComplete)
>@@ -617,7 +615,7 @@ bool JPEGImageDecoder::outputScanlines()
>     // Initialize the framebuffer if needed.
>     ImageFrame& buffer = m_frameBufferCache[0];
>     if (buffer.status() == ImageFrame::FrameEmpty) {
>-        if (!buffer.setSize(scaledSize()))
>+        if (!buffer.initializeBackingStore(scaledSize(), m_premultiplyAlpha))
>             return setFailed();
>         buffer.setStatus(ImageFrame::FramePartial);
>         // The buffer is transparent outside the decoded area while the image is
>Index: Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp
>===================================================================
>--- Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp	(working copy)
>@@ -257,10 +257,8 @@ ImageFrame* PNGImageDecoder::frameBuffer
>         return nullptr;
> #endif
> 
>-    if (m_frameBufferCache.isEmpty()) {
>+    if (m_frameBufferCache.isEmpty())
>         m_frameBufferCache.resize(1);
>-        m_frameBufferCache[0].setPremultiplyAlpha(m_premultiplyAlpha);
>-    }
> 
>     ImageFrame& frame = m_frameBufferCache[index];
>     if (frame.status() != ImageFrame::FrameComplete)
>@@ -411,29 +409,6 @@ void PNGImageDecoder::headerAvailable()
>     }
> }
> 
>-static inline void setPixelRGB(RGBA32* dest, png_bytep pixel)
>-{
>-    *dest = 0xFF000000U | pixel[0] << 16 | pixel[1] << 8 | pixel[2];
>-}
>-
>-static inline void setPixelRGBA(RGBA32* dest, png_bytep pixel, unsigned char& nonTrivialAlphaMask)
>-{
>-    unsigned char a = pixel[3];
>-    *dest = a << 24 | pixel[0] << 16 | pixel[1] << 8 | pixel[2];
>-    nonTrivialAlphaMask |= (255 - a);
>-}
>-
>-static inline void setPixelPremultipliedRGBA(RGBA32* dest, png_bytep pixel, unsigned char& nonTrivialAlphaMask)
>-{
>-    unsigned char a = pixel[3];
>-    unsigned char r = fastDivideBy255(pixel[0] * a);
>-    unsigned char g = fastDivideBy255(pixel[1] * a);
>-    unsigned char b = fastDivideBy255(pixel[2] * a);
>-
>-    *dest = a << 24 | r << 16 | g << 8 | b;
>-    nonTrivialAlphaMask |= (255 - a);
>-}
>-
> void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer, unsigned rowIndex, int)
> {
>     if (m_frameBufferCache.isEmpty())
>@@ -447,7 +422,7 @@ void PNGImageDecoder::rowAvailable(unsig
>     ImageFrame& buffer = m_frameBufferCache[m_currentFrame];
>     if (buffer.status() == ImageFrame::FrameEmpty) {
>         png_structp png = m_reader->pngPtr();
>-        if (!buffer.setSize(scaledSize())) {
>+        if (!buffer.initializeBackingStore(scaledSize(), m_premultiplyAlpha)) {
>             longjmp(JMPBUF(png), 1);
>             return;
>         }
>@@ -545,20 +520,17 @@ void PNGImageDecoder::rowAvailable(unsig
>     {
>         png_bytep pixel = row;
>         if (hasAlpha) {
>-            if (buffer.premultiplyAlpha()) {
>-                for (int x = 0; x < width; ++x, pixel += 4)
>-                    setPixelPremultipliedRGBA(address++, pixel, nonTrivialAlphaMask);
>-            } else {
>-                for (int x = 0; x < width; ++x, pixel += 4)
>-                    setPixelRGBA(address++, pixel, nonTrivialAlphaMask);
>+            for (int x = 0; x < width; ++x, pixel += 4) {
>+                unsigned alpha = pixel[3];
>+                buffer.setPixel(address++, pixel[0], pixel[1], pixel[2], alpha);
>+                nonTrivialAlphaMask |= (255 - alpha);
>             }
>         } else {
>             for (int x = 0; x < width; ++x, pixel += 3)
>-                setPixelRGB(address++, pixel);
>+                address++ = makeRGB(pixel[0], pixel[1], pixel[2]);
>         }
>     }
> 
>-
>     if (nonTrivialAlphaMask && !buffer.hasAlpha())
>         buffer.setHasAlpha(true);
> }
>@@ -618,8 +590,6 @@ void PNGImageDecoder::readChunks(png_unk
>             return;
> 
>         m_frameBufferCache.resize(m_frameCount);
>-        for (auto& imageFrame : m_frameBufferCache)
>-            imageFrame.setPremultiplyAlpha(m_premultiplyAlpha);
>     } else if (!memcmp(chunk->name, "fcTL", 4) && chunk->size == 26) {
>         if (m_hasInfo && !m_isAnimated)
>             return;
>@@ -661,10 +631,8 @@ void PNGImageDecoder::readChunks(png_unk
>             return;
>         }
> 
>-        if (m_frameBufferCache.isEmpty()) {
>+        if (m_frameBufferCache.isEmpty())
>             m_frameBufferCache.resize(1);
>-            m_frameBufferCache[0].setPremultiplyAlpha(m_premultiplyAlpha);
>-        }
> 
>         if (m_currentFrame < m_frameBufferCache.size()) {
>             ImageFrame& buffer = m_frameBufferCache[m_currentFrame];
>@@ -813,7 +781,7 @@ void PNGImageDecoder::initFrameBuffer(si
> 
>     if (prevMethod == ImageFrame::DisposeKeep) {
>         // Preserve the last frame as the starting state for this frame.
>-        buffer.copyBitmapData(*prevBuffer);
>+        buffer.initializeBackingStore(prevBuffer->backingStore());
>     } else {
>         // We want to clear the previous frame to transparent, without
>         // affecting pixels in the image outside of the frame.
>@@ -824,7 +792,7 @@ void PNGImageDecoder::initFrameBuffer(si
>             buffer.zeroFillPixelData();
>         } else {
>             // Copy the whole previous buffer, then clear just its frame.
>-            buffer.copyBitmapData(*prevBuffer);
>+            buffer.initializeBackingStore(prevBuffer->backingStore());
>             buffer.zeroFillFrameRect(prevRect);
>         }
>     }
>Index: Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp
>===================================================================
>--- Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp	(revision 205841)
>+++ Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp	(working copy)
>@@ -78,10 +78,8 @@ ImageFrame* WEBPImageDecoder::frameBuffe
>     if (index)
>         return 0;
> 
>-    if (m_frameBufferCache.isEmpty()) {
>+    if (m_frameBufferCache.isEmpty())
>         m_frameBufferCache.resize(1);
>-        m_frameBufferCache[0].setPremultiplyAlpha(m_premultiplyAlpha);
>-    }
> 
>     ImageFrame& frame = m_frameBufferCache[0];
>     if (frame.status() != ImageFrame::FrameComplete)
>@@ -128,7 +126,7 @@ bool WEBPImageDecoder::decode(bool onlyS
>     ASSERT(buffer.status() != ImageFrame::FrameComplete);
> 
>     if (buffer.status() == ImageFrame::FrameEmpty) {
>-        if (!buffer.setSize(size()))
>+        if (!buffer.initializeBackingStore(size(), m_premultiplyAlpha))
>             return setFailed();
>         buffer.setStatus(ImageFrame::FramePartial);
>         buffer.setHasAlpha(m_hasAlpha);

Actions: View | Formatted Diff | Diff

Attachments on bug 159721: 283537 | 283557 | 283565 | 283569 | 283571 | 283577 | 283578 | 283580 | 283589 | 288644 | 288695 | 288696 | 288697 | 288698 | 288717 | 288719