Confusing CGImageRef memory management in ImageBuffer::copyImage
authoraroben@webkit.org <aroben@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Oct 2013 18:50:50 +0000 (18:50 +0000)
committeraroben@webkit.org <aroben@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Oct 2013 18:50:50 +0000 (18:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=122605

Source/WebCore:

BitmapImage::create was adopting the CGImage passed into it, which
resulted in some strange contortions in ImageBuffer::copyImage that
made it look like it was leaking CGImages, when in fact it was just
relying on BitmapImage to adopt the extra references.

BitmapImage::create now retains the passed-in CGImage, and I updated
the two callers to it to expect that (one here, one in WebKit2). I
also changed ImageBuffer::copyNativeImage to return a RetainPtr to
reduce the number of adoptCF()s needed and make it harder to make
programming mistakes.

Reviewed by Simon Fraser.

No new tests because this is just a code cleanup.

* platform/graphics/ImageBuffer.h: Changed copyNativeImage to return a
RetainPtr<CGImageRef.

* platform/graphics/cg/BitmapImageCG.cpp:
(WebCore::BitmapImage::BitmapImage): Adopt the passed-in CGImage,
since we're taking ownership of it. (We release it in
FrameData::clear.)

* platform/graphics/cg/ImageBufferCG.cpp:
(WebCore::ImageBuffer::copyImage): Updated for copyNativeImage's new
return type and to take into account BitmapImage::create's new
retaining semantics. This makes this function not have to be so clever
about retain counts.

(WebCore::ImageBuffer::copyNativeImage): Changed to return a
RetainPtr<CGImageRef>.

(WebCore::ImageBuffer::draw):
(WebCore::ImageBuffer::clip):
(WebCore::ImageBuffer::putByteArray):
(WebCore::ImageBuffer::toDataURL):
Updated for changes to copyNativeImage.

Source/WebKit2:

Reviewed by Simon Fraser.

* Shared/cg/ShareableBitmapCG.cpp:
(WebKit::ShareableBitmap::createImage): BitmapImage::create now
retains the passed-in CGImage, so we don't need to dance around it
anymore. Also changed to use nullptr instead of 0 while I was in here.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/ImageBuffer.h
Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp
Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/Shared/cg/ShareableBitmapCG.cpp

index 3da60ed..7cab442 100644 (file)
@@ -1,3 +1,46 @@
+2013-10-11  Adam Roben  <aroben@webkit.org>
+
+        Confusing CGImageRef memory management in ImageBuffer::copyImage
+        https://bugs.webkit.org/show_bug.cgi?id=122605
+
+        BitmapImage::create was adopting the CGImage passed into it, which
+        resulted in some strange contortions in ImageBuffer::copyImage that
+        made it look like it was leaking CGImages, when in fact it was just
+        relying on BitmapImage to adopt the extra references.
+
+        BitmapImage::create now retains the passed-in CGImage, and I updated
+        the two callers to it to expect that (one here, one in WebKit2). I
+        also changed ImageBuffer::copyNativeImage to return a RetainPtr to
+        reduce the number of adoptCF()s needed and make it harder to make
+        programming mistakes.
+
+        Reviewed by Simon Fraser.
+
+        No new tests because this is just a code cleanup.
+
+        * platform/graphics/ImageBuffer.h: Changed copyNativeImage to return a
+        RetainPtr<CGImageRef.
+
+        * platform/graphics/cg/BitmapImageCG.cpp:
+        (WebCore::BitmapImage::BitmapImage): Adopt the passed-in CGImage,
+        since we're taking ownership of it. (We release it in
+        FrameData::clear.)
+
+        * platform/graphics/cg/ImageBufferCG.cpp:
+        (WebCore::ImageBuffer::copyImage): Updated for copyNativeImage's new
+        return type and to take into account BitmapImage::create's new
+        retaining semantics. This makes this function not have to be so clever
+        about retain counts.
+
+        (WebCore::ImageBuffer::copyNativeImage): Changed to return a
+        RetainPtr<CGImageRef>.
+
+        (WebCore::ImageBuffer::draw):
+        (WebCore::ImageBuffer::clip):
+        (WebCore::ImageBuffer::putByteArray):
+        (WebCore::ImageBuffer::toDataURL):
+        Updated for changes to copyNativeImage.
+
 2013-10-11  Bear Travis  <betravis@adobe.com>
 
         [CSS Shapes] Shape-Image-Threshold should be animatable
index 74fe2e2..f44f74c 100644 (file)
@@ -136,7 +136,7 @@ namespace WebCore {
 
     private:
 #if USE(CG)
-        PassNativeImagePtr copyNativeImage(BackingStoreCopy = CopyBackingStore) const;
+        RetainPtr<CGImageRef> copyNativeImage(BackingStoreCopy = CopyBackingStore) const;
         void flushContext() const;
         void flushContextIfNecessary() const;
 #endif
index 9cdc6fa..6c69ce0 100644 (file)
@@ -92,7 +92,7 @@ BitmapImage::BitmapImage(CGImageRef cgImage, ImageObserver* observer)
     m_sizeRespectingOrientation = IntSize(width, height);
 
     m_frames.grow(1);
-    m_frames[0].m_frame = cgImage;
+    m_frames[0].m_frame = CGImageRetain(cgImage);
     m_frames[0].m_hasAlpha = true;
     m_frames[0].m_haveMetadata = true;
 
index e480d7b..61c7dd3 100644 (file)
@@ -226,11 +226,11 @@ PassRefPtr<Image> ImageBuffer::copyImage(BackingStoreCopy copyBehavior, ScaleBeh
     if (m_resolutionScale == 1 || scaleBehavior == Unscaled)
         image = copyNativeImage(copyBehavior);
     else {
-        image = adoptCF(copyNativeImage(DontCopyBackingStore));
+        image = copyNativeImage(DontCopyBackingStore);
         RetainPtr<CGContextRef> context = adoptCF(CGBitmapContextCreate(0, logicalSize().width(), logicalSize().height(), 8, 4 * logicalSize().width(), deviceRGBColorSpaceRef(), kCGImageAlphaPremultipliedLast));
         CGContextSetBlendMode(context.get(), kCGBlendModeCopy);
         CGContextDrawImage(context.get(), CGRectMake(0, 0, logicalSize().width(), logicalSize().height()), image.get());
-        image = CGBitmapContextCreateImage(context.get());
+        image = adoptCF(CGBitmapContextCreateImage(context.get()));
     }
 
     if (!image)
@@ -247,7 +247,7 @@ BackingStoreCopy ImageBuffer::fastCopyImageMode()
     return DontCopyBackingStore;
 }
 
-PassNativeImagePtr ImageBuffer::copyNativeImage(BackingStoreCopy copyBehavior) const
+RetainPtr<CGImageRef> ImageBuffer::copyNativeImage(BackingStoreCopy copyBehavior) const
 {
     CGImageRef image = 0;
     if (!m_context->isAcceleratedContext()) {
@@ -272,7 +272,7 @@ PassNativeImagePtr ImageBuffer::copyNativeImage(BackingStoreCopy copyBehavior) c
     }
 #endif
 
-    return image;
+    return adoptCF(image);
 }
 
 void ImageBuffer::draw(GraphicsContext* destContext, ColorSpace styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, bool useLowQualityScale)
@@ -282,9 +282,9 @@ void ImageBuffer::draw(GraphicsContext* destContext, ColorSpace styleColorSpace,
 
     RetainPtr<CGImageRef> image;
     if (destContext == m_context || destContext->isAcceleratedContext())
-        image = adoptCF(copyNativeImage(CopyBackingStore)); // Drawing into our own buffer, need to deep copy.
+        image = copyNativeImage(CopyBackingStore); // Drawing into our own buffer, need to deep copy.
     else
-        image = adoptCF(copyNativeImage(DontCopyBackingStore));
+        image = copyNativeImage(DontCopyBackingStore);
 
     FloatRect adjustedSrcRect = srcRect;
     adjustedSrcRect.scale(m_resolutionScale, m_resolutionScale);
@@ -314,7 +314,7 @@ void ImageBuffer::clip(GraphicsContext* contextToClip, const FloatRect& rect) co
 {
     CGContextRef platformContextToClip = contextToClip->platformContext();
     // FIXME: This image needs to be grayscale to be used as an alpha mask here.
-    RetainPtr<CGImageRef> image = adoptCF(copyNativeImage(DontCopyBackingStore));
+    RetainPtr<CGImageRef> image = copyNativeImage(DontCopyBackingStore);
     CGContextTranslateCTM(platformContextToClip, rect.x(), rect.y() + rect.height());
     CGContextScaleCTM(platformContextToClip, 1, -1);
     CGContextClipToMask(platformContextToClip, FloatRect(FloatPoint(), rect.size()), image.get());
@@ -370,7 +370,7 @@ void ImageBuffer::putByteArray(Multiply multiplied, Uint8ClampedArray* source, c
     // Draw the image in CG coordinate space
     IntPoint destPointInCGCoords(destPoint.x() + sourceRect.x(), (coordinateSystem == LogicalCoordinateSystem ? logicalSize() : internalSize()).height() - (destPoint.y() + sourceRect.y()) - sourceRect.height());
     IntRect destRectInCGCoords(destPointInCGCoords, sourceCopySize);
-    RetainPtr<CGImageRef> sourceCopyImage = adoptCF(sourceCopy->copyNativeImage());
+    RetainPtr<CGImageRef> sourceCopyImage = sourceCopy->copyNativeImage();
     CGContextDrawImage(destContext, destRectInCGCoords, sourceCopyImage.get());
     CGContextRestoreGState(destContext);
 #endif
@@ -476,9 +476,9 @@ String ImageBuffer::toDataURL(const String& mimeType, const double* quality, Coo
                                     deviceRGBColorSpaceRef(), kCGBitmapByteOrderDefault | kCGImageAlphaNoneSkipLast,
                                     dataProvider.get(), 0, false, kCGRenderingIntentDefault));
     } else if (m_resolutionScale == 1)
-        image = adoptCF(copyNativeImage(CopyBackingStore));
+        image = copyNativeImage(CopyBackingStore);
     else {
-        image = adoptCF(copyNativeImage(DontCopyBackingStore));
+        image = copyNativeImage(DontCopyBackingStore);
         RetainPtr<CGContextRef> context = adoptCF(CGBitmapContextCreate(0, logicalSize().width(), logicalSize().height(), 8, 4 * logicalSize().width(), deviceRGBColorSpaceRef(), kCGImageAlphaPremultipliedLast));
         CGContextSetBlendMode(context.get(), kCGBlendModeCopy);
         CGContextDrawImage(context.get(), CGRectMake(0, 0, logicalSize().width(), logicalSize().height()), image.get());
index 22f0497..22bb676 100644 (file)
@@ -1,3 +1,15 @@
+2013-10-11  Adam Roben  <aroben@webkit.org>
+
+        Confusing CGImageRef memory management in ImageBuffer::copyImage
+        https://bugs.webkit.org/show_bug.cgi?id=122605
+
+        Reviewed by Simon Fraser.
+
+        * Shared/cg/ShareableBitmapCG.cpp:
+        (WebKit::ShareableBitmap::createImage): BitmapImage::create now
+        retains the passed-in CGImage, so we don't need to dance around it
+        anymore. Also changed to use nullptr instead of 0 while I was in here.
+
 2013-10-10  Byungwoo Lee  <bw80.lee@samsung.com>
 
         [EFL][WK2] Separate dispatch context from WorkQueue.
index bd1ccea..3c95896 100644 (file)
@@ -113,10 +113,9 @@ PassRefPtr<Image> ShareableBitmap::createImage()
 {
     RetainPtr<CGImageRef> platformImage = makeCGImage();
     if (!platformImage)
-        return 0;
+        return nullptr;
 
-    // BitmapImage::create adopts the CGImageRef that's passed in, which is why we need to leakRef here.
-    return BitmapImage::create(platformImage.leakRef());
+    return BitmapImage::create(platformImage.get());
 }
 
 } // namespace WebKit