Unlock partially decoded images after passing them to the ImageDecodingStore
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Feb 2013 22:49:32 +0000 (22:49 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Feb 2013 22:49:32 +0000 (22:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=110778

Patch by Min Qin <qinmin@chromium.org> on 2013-02-27
Reviewed by Stephen White.

Source/WebCore:

For partially decoded images, we need to unlock them so that the memory can be freed.
This change unlocks all the image frames after they are passed to ImageDecodingStore.
Unit tests are added in ImageFrameGeneratorTest.

* platform/graphics/chromium/ImageFrameGenerator.cpp:
(WebCore::ImageFrameGenerator::tryToResumeDecodeAndScale):
(WebCore::ImageFrameGenerator::tryToDecodeAndScale):
(WebCore::ImageFrameGenerator::decode):
* platform/image-decoders/ImageDecoder.h:
(ImageDecoder):
(WebCore::ImageDecoder::lockFrameBuffers):
(WebCore::ImageDecoder::unlockFrameBuffers):

Source/WebKit/chromium:

Test for testing that image frames are unlocked after passing to ImageDecodingStore.

* tests/ImageFrameGeneratorTest.cpp:
(WebCore::ImageFrameGeneratorTest::SetUp):
(WebCore::ImageFrameGeneratorTest::frameBuffersUnlocked):
(ImageFrameGeneratorTest):
(WebCore::ImageFrameGeneratorTest::frameBuffersLocked):
(WebCore::TEST_F):
* tests/MockImageDecoder.h:
(WebCore::MockImageDecoderClient::frameBuffersLocked):
(WebCore::MockImageDecoderClient::frameBuffersUnlocked):
(WebCore::MockImageDecoder::unlockFrameBuffers):
(WebCore::MockImageDecoder::lockFrameBuffers):
(MockImageDecoder):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@144242 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp
Source/WebCore/platform/image-decoders/ImageDecoder.h
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/tests/ImageFrameGeneratorTest.cpp
Source/WebKit/chromium/tests/MockImageDecoder.h

index 013d346..e9416fc 100644 (file)
@@ -1,3 +1,23 @@
+2013-02-27  Min Qin  <qinmin@chromium.org>
+
+        Unlock partially decoded images after passing them to the ImageDecodingStore
+        https://bugs.webkit.org/show_bug.cgi?id=110778
+
+        Reviewed by Stephen White.
+
+        For partially decoded images, we need to unlock them so that the memory can be freed.
+        This change unlocks all the image frames after they are passed to ImageDecodingStore.
+        Unit tests are added in ImageFrameGeneratorTest.
+
+        * platform/graphics/chromium/ImageFrameGenerator.cpp:
+        (WebCore::ImageFrameGenerator::tryToResumeDecodeAndScale):
+        (WebCore::ImageFrameGenerator::tryToDecodeAndScale):
+        (WebCore::ImageFrameGenerator::decode):
+        * platform/image-decoders/ImageDecoder.h:
+        (ImageDecoder):
+        (WebCore::ImageDecoder::lockFrameBuffers):
+        (WebCore::ImageDecoder::unlockFrameBuffers):
+
 2013-02-27  Kenneth Russell  <kbr@google.com>
 
         Insufficient validation when uploading depth textures to WebGL
index d34f546..4dd5215 100644 (file)
@@ -142,9 +142,21 @@ const ScaledImageFragment* ImageFrameGenerator::tryToResumeDecodeAndScale(const
     ASSERT(cachedDecoder);
 
     if (m_data.hasNewData()) {
+        // For single frame images, the above ImageDecodingStore::lockCache()
+        // call should lock the pixelRef. As a result, this lockFrameBuffers()
+        // call should always succeed.
+        // TODO: this does not work for the multiframe images, which are not
+        // yet supported by this class.
+        bool frameBuffersLocked = cachedDecoder->lockFrameBuffers();
+        ASSERT_UNUSED(frameBuffersLocked, frameBuffersLocked);
         // Only do decoding if there is new data.
         OwnPtr<ScaledImageFragment> fullSizeImage = decode(&cachedDecoder);
         cachedImage = ImageDecodingStore::instance()->overwriteAndLockCache(this, cachedImage, fullSizeImage.release());
+        // If the image is partially decoded, unlock the frames so that it
+        // can be evicted from the memory. For fully decoded images,
+        // ImageDecodingStore should have deleted the decoder here.
+        if (!cachedImage->isComplete())
+            cachedDecoder->unlockFrameBuffers();
     }
 
     if (m_fullSize == scaledSize)
@@ -171,6 +183,10 @@ const ScaledImageFragment* ImageFrameGenerator::tryToDecodeAndScale(const SkISiz
 
     const ScaledImageFragment* cachedFullSizeImage = ImageDecodingStore::instance()->insertAndLockCache(
         this, fullSizeImage.release(), decoderContainer.release());
+    // The newly created SkBitmap in the decoder is locked. Unlock it here
+    // if the image is partially decoded.
+    if (!cachedFullSizeImage->isComplete())
+        decoder->unlockFrameBuffers();
 
     if (m_fullSize == scaledSize)
         return cachedFullSizeImage;
@@ -201,9 +217,8 @@ PassOwnPtr<ScaledImageFragment> ImageFrameGenerator::decode(ImageDecoder** decod
     (*decoder)->setData(data, allDataReceived);
     // If this call returns a newly allocated DiscardablePixelRef, then
     // ImageFrame::m_bitmap and the contained DiscardablePixelRef are locked.
-    // They will be unlocked when ImageDecoder is destroyed since ImageDecoder
-    // owns the ImageFrame. Partially decoded SkBitmap is thus inserted into the
-    // ImageDecodingStore while locked.
+    // They will be unlocked after the image fragment is inserted into
+    // ImageDecodingStore.
     ImageFrame* frame = (*decoder)->frameBufferAtIndex(0);
     (*decoder)->setData(0, false); // Unref SharedBuffer from ImageDecoder.
 
index 2e0b57f..10424a5 100644 (file)
@@ -422,6 +422,23 @@ namespace WebCore {
                 m_frameBufferCache.resize(1);
             m_frameBufferCache[0].setMemoryAllocator(allocator);
         }
+
+        virtual bool lockFrameBuffers()
+        {
+            bool ret = true;
+            for (unsigned i = 0; i < m_frameBufferCache.size(); ++i) {
+                const SkBitmap& bitmap = m_frameBufferCache[i].getSkBitmap();
+                bitmap.lockPixels();
+                ret = ret && bitmap.getPixels();
+            }
+            return ret;
+        }
+
+        virtual void unlockFrameBuffers()
+        {
+            for (unsigned i = 0; i < m_frameBufferCache.size(); ++i)
+                m_frameBufferCache[i].getSkBitmap().unlockPixels();
+        }
 #endif
     protected:
         void prepareScaleDataIfNecessary();
index 3a23679..c9e39ee 100644 (file)
@@ -1,3 +1,25 @@
+2013-02-27  Min Qin  <qinmin@chromium.org>
+
+        Unlock partially decoded images after passing them to the ImageDecodingStore
+        https://bugs.webkit.org/show_bug.cgi?id=110778
+
+        Reviewed by Stephen White.
+
+        Test for testing that image frames are unlocked after passing to ImageDecodingStore.
+
+        * tests/ImageFrameGeneratorTest.cpp:
+        (WebCore::ImageFrameGeneratorTest::SetUp):
+        (WebCore::ImageFrameGeneratorTest::frameBuffersUnlocked):
+        (ImageFrameGeneratorTest):
+        (WebCore::ImageFrameGeneratorTest::frameBuffersLocked):
+        (WebCore::TEST_F):
+        * tests/MockImageDecoder.h:
+        (WebCore::MockImageDecoderClient::frameBuffersLocked):
+        (WebCore::MockImageDecoderClient::frameBuffersUnlocked):
+        (WebCore::MockImageDecoder::unlockFrameBuffers):
+        (WebCore::MockImageDecoder::lockFrameBuffers):
+        (MockImageDecoder):
+
 2013-02-27  John Bauman  <jbauman@chromium.org>
 
         Plugin in iframe may not display
index 113d788..5983a43 100644 (file)
@@ -73,6 +73,8 @@ public:
         m_generator->setImageDecoderFactoryForTesting(MockImageDecoderFactory::create(this));
         m_decodersDestroyed = 0;
         m_frameBufferRequestCount = 0;
+        m_frameBufferLockCount = 0;
+        m_frameBufferUnlockCount = 0;
         m_frameStatus = ImageFrame::FrameEmpty;
     }
 
@@ -91,6 +93,16 @@ public:
         ++m_frameBufferRequestCount;
     }
 
+    virtual void frameBuffersUnlocked()
+    {
+        ++m_frameBufferUnlockCount;
+    }
+
+    virtual void frameBuffersLocked()
+    {
+        ++m_frameBufferLockCount;
+    }
+
     virtual ImageFrame::FrameStatus frameStatus()
     {
         return m_frameStatus;
@@ -117,6 +129,8 @@ protected:
     RefPtr<ImageFrameGenerator> m_generator;
     int m_decodersDestroyed;
     int m_frameBufferRequestCount;
+    int m_frameBufferLockCount;
+    int m_frameBufferUnlockCount;
     ImageFrame::FrameStatus m_frameStatus;
 };
 
@@ -141,6 +155,7 @@ TEST_F(ImageFrameGeneratorTest, cacheHit)
     EXPECT_TRUE(m_generator->hasAlpha());
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(0, m_frameBufferRequestCount);
+    EXPECT_EQ(0, m_frameBufferLockCount);
 }
 
 TEST_F(ImageFrameGeneratorTest, cacheMissWithScale)
@@ -164,6 +179,8 @@ TEST_F(ImageFrameGeneratorTest, cacheMissWithScale)
     EXPECT_TRUE(m_generator->hasAlpha());
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(0, m_frameBufferRequestCount);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(0, m_frameBufferUnlockCount);
 }
 
 TEST_F(ImageFrameGeneratorTest, cacheMissWithDecodeAndScale)
@@ -173,6 +190,8 @@ TEST_F(ImageFrameGeneratorTest, cacheMissWithDecodeAndScale)
     // Cache miss.
     const ScaledImageFragment* scaledImage = m_generator->decodeAndScale(scaledSize());
     EXPECT_EQ(1, m_frameBufferRequestCount);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(0, m_frameBufferUnlockCount);
     EXPECT_EQ(scaledSize(), scaledImage->scaledSize());
     EXPECT_FALSE(m_generator->hasAlpha());
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), scaledImage);
@@ -192,6 +211,8 @@ TEST_F(ImageFrameGeneratorTest, cacheMissWithDecodeAndScale)
     EXPECT_FALSE(m_generator->hasAlpha());
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(1, m_frameBufferRequestCount);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(0, m_frameBufferUnlockCount);
 }
 
 TEST_F(ImageFrameGeneratorTest, cacheMissWithIncompleteDecode)
@@ -201,6 +222,8 @@ TEST_F(ImageFrameGeneratorTest, cacheMissWithIncompleteDecode)
     const ScaledImageFragment* tempImage= m_generator->decodeAndScale(fullSize());
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(1, m_frameBufferRequestCount);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(1u, ImageDecodingStore::instance()->cacheEntries());
 
@@ -208,6 +231,8 @@ TEST_F(ImageFrameGeneratorTest, cacheMissWithIncompleteDecode)
     tempImage = m_generator->decodeAndScale(fullSize());
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(2, m_frameBufferRequestCount);
+    EXPECT_EQ(1, m_frameBufferLockCount);
+    EXPECT_EQ(2, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(1u, ImageDecodingStore::instance()->cacheEntries());
     EXPECT_EQ(0, m_decodersDestroyed);
@@ -220,12 +245,16 @@ TEST_F(ImageFrameGeneratorTest, cacheMissWithIncompleteDecodeNoNewData)
     const ScaledImageFragment* tempImage= m_generator->decodeAndScale(fullSize());
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(1, m_frameBufferRequestCount);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(1u, ImageDecodingStore::instance()->cacheEntries());
 
     tempImage = m_generator->decodeAndScale(fullSize());
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(1, m_frameBufferRequestCount);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(1u, ImageDecodingStore::instance()->cacheEntries());
     EXPECT_EQ(0, m_decodersDestroyed);
@@ -238,6 +267,8 @@ TEST_F(ImageFrameGeneratorTest, cacheMissWithIncompleteDecodeAndScale)
     const ScaledImageFragment* tempImage= m_generator->decodeAndScale(scaledSize());
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(1, m_frameBufferRequestCount);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(2u, ImageDecodingStore::instance()->cacheEntries());
 
@@ -245,6 +276,8 @@ TEST_F(ImageFrameGeneratorTest, cacheMissWithIncompleteDecodeAndScale)
     tempImage = m_generator->decodeAndScale(scaledSize());
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(2, m_frameBufferRequestCount);
+    EXPECT_EQ(1, m_frameBufferLockCount);
+    EXPECT_EQ(2, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(2u, ImageDecodingStore::instance()->cacheEntries());
     EXPECT_EQ(0, m_decodersDestroyed);
@@ -258,6 +291,8 @@ TEST_F(ImageFrameGeneratorTest, incompleteDecodeBecomesComplete)
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(1, m_frameBufferRequestCount);
     EXPECT_EQ(0, m_decodersDestroyed);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(1u, ImageDecodingStore::instance()->cacheEntries());
 
@@ -268,6 +303,8 @@ TEST_F(ImageFrameGeneratorTest, incompleteDecodeBecomesComplete)
     EXPECT_TRUE(tempImage->isComplete());
     EXPECT_EQ(2, m_frameBufferRequestCount);
     EXPECT_EQ(1, m_decodersDestroyed);
+    EXPECT_EQ(1, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(1u, ImageDecodingStore::instance()->cacheEntries());
 
@@ -285,6 +322,8 @@ TEST_F(ImageFrameGeneratorTest, incompleteDecodeAndScaleBecomesComplete)
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(1, m_frameBufferRequestCount);
     EXPECT_EQ(0, m_decodersDestroyed);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(2u, ImageDecodingStore::instance()->cacheEntries());
 
@@ -295,6 +334,8 @@ TEST_F(ImageFrameGeneratorTest, incompleteDecodeAndScaleBecomesComplete)
     EXPECT_TRUE(tempImage->isComplete());
     EXPECT_EQ(2, m_frameBufferRequestCount);
     EXPECT_EQ(1, m_decodersDestroyed);
+    EXPECT_EQ(1, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(2u, ImageDecodingStore::instance()->cacheEntries());
 
@@ -325,6 +366,8 @@ TEST_F(ImageFrameGeneratorTest, incompleteDecodeBecomesCompleteMultiThreaded)
     EXPECT_FALSE(tempImage->isComplete());
     EXPECT_EQ(1, m_frameBufferRequestCount);
     EXPECT_EQ(0, m_decodersDestroyed);
+    EXPECT_EQ(0, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
     EXPECT_EQ(1u, ImageDecodingStore::instance()->cacheEntries());
 
@@ -336,11 +379,15 @@ TEST_F(ImageFrameGeneratorTest, incompleteDecodeBecomesCompleteMultiThreaded)
 
     EXPECT_EQ(2, m_frameBufferRequestCount);
     EXPECT_EQ(1, m_decodersDestroyed);
+    EXPECT_EQ(1, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     EXPECT_EQ(1u, ImageDecodingStore::instance()->cacheEntries());
 
     tempImage = m_generator->decodeAndScale(fullSize());
     EXPECT_TRUE(tempImage->isComplete());
     EXPECT_EQ(2, m_frameBufferRequestCount);
+    EXPECT_EQ(1, m_frameBufferLockCount);
+    EXPECT_EQ(1, m_frameBufferUnlockCount);
     ImageDecodingStore::instance()->unlockCache(m_generator.get(), tempImage);
 }
 
index ba34217..67b2489 100644 (file)
@@ -33,6 +33,8 @@ class MockImageDecoderClient {
 public:
     virtual void decoderBeingDestroyed() = 0;
     virtual void frameBufferRequested() = 0;
+    virtual void frameBuffersLocked() { }
+    virtual void frameBuffersUnlocked() { }
     virtual ImageFrame::FrameStatus frameStatus() = 0;
 };
 
@@ -75,6 +77,13 @@ public:
         return &m_frameBufferCache[0];
     }
 
+    virtual bool lockFrameBuffers()
+    {
+        m_client->frameBuffersLocked();
+        return true;
+    }
+    virtual void unlockFrameBuffers() { m_client->frameBuffersUnlocked(); }
+
     int frameBufferRequestCount() const { return m_frameBufferRequestCount; }
 
 private: