Bug 155412

Summary: Change NativeImagePtr for CG to be RetainPtr<CGImageRef>
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, commit-queue, dino, japhet, jonlee, kondapallykalyan, roger_fong, sam, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 155322, 155422    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2016-03-13 15:46:36 PDT
This work towards CG asynchronous image decoding: https://bugs.webkit.org/show_bug.cgi?id=155322.
Comment 1 Said Abou-Hallawa 2016-03-13 16:17:00 PDT
Created attachment 273904 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-03-13 17:14:59 PDT
Created attachment 273907 [details]
Patch
Comment 3 Said Abou-Hallawa 2016-03-13 17:22:31 PDT
Created attachment 273909 [details]
Patch
Comment 4 Simon Fraser (smfr) 2016-03-14 12:08:25 PDT
Did you check for leaks after this change?
Comment 5 Said Abou-Hallawa 2016-03-15 14:35:53 PDT
I did run-webkit-tests --debug --leak LayoutTests/fast/ and it did not show any leak related to the conversion form raw pointer to RetainPtr<>.
Comment 6 Simon Fraser (smfr) 2016-03-16 15:17:09 PDT
Comment on attachment 273909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273909&action=review

> Source/WebCore/platform/graphics/NativeImagePtr.h:45
> +typedef RetainPtr<CGImageRef> NativeImagePtr;

Passing NativeImagePtr by value  (which happens in various places) will cause reference churn. I don't really like hiding the fact that it's a RetainPtr<> inside the typedef, because it makes leaky and churny code too easy to write.

Elsewhere we've done typedef CALayer PlatformLayer, allowing us to write RetainPtr<PlatformLayer> which I prefer.
Comment 7 Darin Adler 2016-03-16 16:33:43 PDT
Comment on attachment 273909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273909&action=review

If NativeImagePtr is now going to always be a smart pointer, we should be using move semantics with it to decrease churn.

> Source/WebCore/platform/graphics/BitmapImage.h:58
> -    WTF_MAKE_NONCOPYABLE(FrameData);
>  public:

No need to keep the "public" if you are removing the WTF_MAKE_NONCOPYABLE.

> Source/WebCore/platform/graphics/BitmapImage.h:-77
> -        , m_hasAlpha(true) 

Before, m_hasAlpha defaulted to true, now it seems to default to false. Why is that change OK? No mention of this in the change log.

> Source/WebCore/platform/graphics/BitmapImage.h:81
> +        uint8_t m_flags[sizeof(bool)] { 0 };

What does this sizeof(bool) accomplish?

> Source/WebCore/platform/graphics/Icon.h:72
> -    Icon(CGImageRef);
> -    RetainPtr<CGImageRef> m_cgImage;
> +    Icon(NativeImagePtr);
> +    NativeImagePtr m_cgImage;

This doesn’t seem like an improvement to me.

>> Source/WebCore/platform/graphics/NativeImagePtr.h:45
>> +typedef RetainPtr<CGImageRef> NativeImagePtr;
> 
> Passing NativeImagePtr by value  (which happens in various places) will cause reference churn. I don't really like hiding the fact that it's a RetainPtr<> inside the typedef, because it makes leaky and churny code too easy to write.
> 
> Elsewhere we've done typedef CALayer PlatformLayer, allowing us to write RetainPtr<PlatformLayer> which I prefer.

The real issue is that sometimes it’s a RetainPtr and sometimes it’s a RefPtr. The techniques for the two different types can be the same, though, in today’s world. Use rvalue references and WTFMove as needed.

Maybe it would be cleaner to use an actual class for this instead of just a typedef?

> Source/WebCore/platform/graphics/NativeImagePtr.h:48
>  typedef PassRefPtr<cairo_surface_t> PassNativeImagePtr;

Need to get rid of PassNativeImagePtr and use rvalue references and other modern techniques that work with both RefPtr and RetainPtr.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1040
> -        CGImageRef newImage = image->nativeImageForCurrentFrame();
> +        CGImageRef newImage = image->nativeImageForCurrentFrame().get();

I don’t understand why this is safe. It’s usually not safe to take a RetainPtr, get the raw pointer out of it, and let the RetainPtr fall out of scope. And if that is safe, then the function should have been returning a raw pointer instead. THe safer change here is to make newImage be a RetainPtr (could just be auto).

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:175
> -    return frameAtIndex(0);
> +    return frameImageAtIndex(0).get();

Why is it safe to return a raw pointer here? Same question as above.

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:182
> +        PassNativeImagePtr image = frameImageAtIndex(i);

Using the type PassNativeImagePtr here is a bad idea. Just use auto.

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:199
> +        if (NativeImagePtr currFrame = frameImageAtIndex(i))

Again, I think the type we want is auto.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:314
> +    RetainPtr<CGImageRef> tileImage = image.nativeImageForCurrentFrame();

I suggest the type auto here.
Comment 8 Said Abou-Hallawa 2016-03-20 17:58:10 PDT
Created attachment 274553 [details]
Patch
Comment 9 Said Abou-Hallawa 2016-03-20 18:25:37 PDT
Created attachment 274554 [details]
Patch
Comment 10 Said Abou-Hallawa 2016-03-20 18:51:20 PDT
Created attachment 274556 [details]
Patch
Comment 11 Said Abou-Hallawa 2016-03-20 19:17:51 PDT
Created attachment 274557 [details]
Patch
Comment 12 Said Abou-Hallawa 2016-03-20 19:44:09 PDT
Created attachment 274559 [details]
Patch
Comment 13 Said Abou-Hallawa 2016-03-20 20:24:27 PDT
Created attachment 274560 [details]
Patch
Comment 14 Said Abou-Hallawa 2016-03-20 21:58:08 PDT
Comment on attachment 273909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273909&action=review

>> Source/WebCore/platform/graphics/BitmapImage.h:58
>>  public:
> 
> No need to keep the "public" if you are removing the WTF_MAKE_NONCOPYABLE.

I reverted all the changes in this struct and I will make all of them in https://bugs.webkit.org/show_bug.cgi?id=155422.

>> Source/WebCore/platform/graphics/BitmapImage.h:-77
>> -        , m_hasAlpha(true) 
> 
> Before, m_hasAlpha defaulted to true, now it seems to default to false. Why is that change OK? No mention of this in the change log.

You are right. I reverted all the changes in this struct and I will make all of them in https://bugs.webkit.org/show_bug.cgi?id=155422.

>> Source/WebCore/platform/graphics/BitmapImage.h:81
>> +        uint8_t m_flags[sizeof(bool)] { 0 };
> 
> What does this sizeof(bool) accomplish?

I reverted all the changes in this struct and I will make all of them in https://bugs.webkit.org/show_bug.cgi?id=155422.

>> Source/WebCore/platform/graphics/NativeImagePtr.h:48
>>  typedef PassRefPtr<cairo_surface_t> PassNativeImagePtr;
> 
> Need to get rid of PassNativeImagePtr and use rvalue references and other modern techniques that work with both RefPtr and RetainPtr.

Done.

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1040
>> +        CGImageRef newImage = image->nativeImageForCurrentFrame().get();
> 
> I don’t understand why this is safe. It’s usually not safe to take a RetainPtr, get the raw pointer out of it, and let the RetainPtr fall out of scope. And if that is safe, then the function should have been returning a raw pointer instead. THe safer change here is to make newImage be a RetainPtr (could just be auto).

Done.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:175
>> +    return frameImageAtIndex(0).get();
> 
> Why is it safe to return a raw pointer here? Same question as above.

BitmapImage::getCGImageRef() now returns RetainPtr<CGImageRef> which requires many changes in WebKit and WebKit2.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:182
>> +        PassNativeImagePtr image = frameImageAtIndex(i);
> 
> Using the type PassNativeImagePtr here is a bad idea. Just use auto.

Done.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:199
>> +        if (NativeImagePtr currFrame = frameImageAtIndex(i))
> 
> Again, I think the type we want is auto.

Done.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:314
>> +    RetainPtr<CGImageRef> tileImage = image.nativeImageForCurrentFrame();
> 
> I suggest the type auto here.

Done.
Comment 15 Darin Adler 2016-03-22 22:49:25 PDT
Comment on attachment 274560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274560&action=review

> Source/WebCore/bindings/objc/DOM.mm:609
> +            *cgImage = (CGImageRef)CFAutorelease(image->getCGImageRef().leakRef());

Should have been able to use RetainPtr’s autorelease() function instead of CFAutorelease(leakRef()).

> Source/WebCore/platform/graphics/BitmapImage.h:-57
> -namespace WTF {
> -    template<> struct VectorTraits<WebCore::FrameData> : public SimpleClassVectorTraits {
> -        static const bool canInitializeWithMemset = false; // Not all FrameData members initialize to 0.
> -    };
> -}

This change is incorrect, since m_hasAlpha initializes to true. I think this needs to be left out of this patch as with some of the other changes to FrameData.

> Source/WebCore/platform/graphics/BitmapImage.h:60
> +        : m_image(nullptr)

No need for this since m_image is a smart pointer now. It will initialize to nullptr even if we don’t specify it here.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2352
>      return m_lastImage.get();

I don’t think the .get() is needed here.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1019
> +        m_uncorrectedContentsImage = m_pendingContentsImage = WTFMove(newImage);

We do not normally use this style in WebKit, assigning more than one variable on the same line.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:262
> +void GraphicsContext::drawNativeImage(const NativeImagePtr& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)

Argument should just be named image, not imagePtr.

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:71
> +BitmapImage::BitmapImage(const RetainPtr<CGImageRef>& image, ImageObserver* observer)

I think this argument should be NativeImagePtr&&; we could use WTFMove below when setting m_image on m_frames[0].

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:147
> -    CGImageRef image = nullptr;
> +    RetainPtr<CGImageRef> image;

Don’t think this type change was a good idea.

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:149
> +        image = m_frames[0].m_image;

Should use .get(); no need to use RetainPtr for the local variable.

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:173
> +RetainPtr<CGImageRef> BitmapImage::getCGImageRef()

I am not sure I understand why we have made this change.

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:178
> +RetainPtr<CGImageRef> BitmapImage::getFirstCGImageRefOfSize(const IntSize& size)

I am not sure I understand why we have made this change.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:165
> +void GraphicsContext::drawNativeImage(const RetainPtr<CGImageRef>& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)

This should be named “image” not “imagePtr”.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:494
> +DrawNativeImage::DrawNativeImage(const NativeImagePtr& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)

Should just be named image, not imagePtr.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:642
> +    static Ref<DrawNativeImage> create(const NativeImagePtr& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)

Should just be named image, not imagePtr.

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:122
> +void Recorder::drawNativeImage(const NativeImagePtr& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)

Should just be named image, not imagePtr.

> Source/WebCore/platform/graphics/mac/ImageMac.mm:96
> +    Vector<RetainPtr<CGImageRef>> images;

I don’t understand why this change is necessary.
Comment 16 Said Abou-Hallawa 2016-03-23 15:01:09 PDT
Created attachment 274780 [details]
Patch
Comment 17 Said Abou-Hallawa 2016-03-23 16:51:32 PDT
Created attachment 274793 [details]
Patch
Comment 18 Said Abou-Hallawa 2016-03-23 18:48:26 PDT
Created attachment 274809 [details]
Patch
Comment 19 Said Abou-Hallawa 2016-03-23 20:12:55 PDT
Created attachment 274814 [details]
Patch
Comment 20 Said Abou-Hallawa 2016-03-24 09:22:09 PDT
Created attachment 274836 [details]
Patch
Comment 21 Said Abou-Hallawa 2016-03-24 09:55:27 PDT
Created attachment 274838 [details]
Patch
Comment 22 Said Abou-Hallawa 2016-03-24 10:58:16 PDT
Created attachment 274842 [details]
Patch
Comment 23 Said Abou-Hallawa 2016-03-24 14:24:41 PDT
Created attachment 274855 [details]
Patch
Comment 24 Said Abou-Hallawa 2016-03-24 17:20:00 PDT
Comment on attachment 274560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274560&action=review

>> Source/WebCore/bindings/objc/DOM.mm:609
>> +            *cgImage = (CGImageRef)CFAutorelease(image->getCGImageRef().leakRef());
> 
> Should have been able to use RetainPtr’s autorelease() function instead of CFAutorelease(leakRef()).

This change was removed because I reverted back the change related to the return type of Image::getCGImageRef(). Now, it returns a raw pointer as before.

>> Source/WebCore/platform/graphics/BitmapImage.h:-57
>> -}
> 
> This change is incorrect, since m_hasAlpha initializes to true. I think this needs to be left out of this patch as with some of the other changes to FrameData.

Done.

>> Source/WebCore/platform/graphics/BitmapImage.h:60
>> +        : m_image(nullptr)
> 
> No need for this since m_image is a smart pointer now. It will initialize to nullptr even if we don’t specify it here.

Done. The initialization was removed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2352
>>      return m_lastImage.get();
> 
> I don’t think the .get() is needed here.

You are right. .get() was removed.

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1019
>> +        m_uncorrectedContentsImage = m_pendingContentsImage = WTFMove(newImage);
> 
> We do not normally use this style in WebKit, assigning more than one variable on the same line.

Done.

>> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:262
>> +void GraphicsContext::drawNativeImage(const NativeImagePtr& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)
> 
> Argument should just be named image, not imagePtr.

Done. The argument's name was changed from 'imagePtr' to 'image'. And the local variable 'image' was also removed.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:71
>> +BitmapImage::BitmapImage(const RetainPtr<CGImageRef>& image, ImageObserver* observer)
> 
> I think this argument should be NativeImagePtr&&; we could use WTFMove below when setting m_image on m_frames[0].

Done.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:147
>> +    RetainPtr<CGImageRef> image;
> 
> Don’t think this type change was a good idea.

Done. The type of the local variable is set back to CGImageRef.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:149
>> +        image = m_frames[0].m_image;
> 
> Should use .get(); no need to use RetainPtr for the local variable.

Done.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:173
>> +RetainPtr<CGImageRef> BitmapImage::getCGImageRef()
> 
> I am not sure I understand why we have made this change.

Done. The change was reverted back.

My goal was to ensure the CGImageRef would not be deleted as long as the caller is holding a reference to it. But since the returned value is actually referencing a stored CGImageRef in the FrameData cache and this cache does not get deleted unless the BitmapImage object is deleted, I think it is safe to return the code back as it was,

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:178
>> +RetainPtr<CGImageRef> BitmapImage::getFirstCGImageRefOfSize(const IntSize& size)
> 
> I am not sure I understand why we have made this change.

Done. The change was reverted back.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:165
>> +void GraphicsContext::drawNativeImage(const RetainPtr<CGImageRef>& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)
> 
> This should be named “image” not “imagePtr”.

Done. The name of the argument was changed.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:494
>> +DrawNativeImage::DrawNativeImage(const NativeImagePtr& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)
> 
> Should just be named image, not imagePtr.

Done. The name of the argument was changed.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:642
>> +    static Ref<DrawNativeImage> create(const NativeImagePtr& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)
> 
> Should just be named image, not imagePtr.

Done. The name of the argument was changed.

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:122
>> +void Recorder::drawNativeImage(const NativeImagePtr& imagePtr, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator op, BlendMode blendMode, ImageOrientation orientation)
> 
> Should just be named image, not imagePtr.

Done. The name of the argument was changed.

>> Source/WebCore/platform/graphics/mac/ImageMac.mm:96
>> +    Vector<RetainPtr<CGImageRef>> images;
> 
> I don’t understand why this change is necessary.

Done. The local variable type change is reverted back.
Comment 25 WebKit Commit Bot 2016-03-24 18:13:12 PDT
Comment on attachment 274855 [details]
Patch

Clearing flags on attachment: 274855

Committed r198655: <http://trac.webkit.org/changeset/198655>
Comment 26 WebKit Commit Bot 2016-03-24 18:13:18 PDT
All reviewed patches have been landed.  Closing bug.