Bug 155412 – Change NativeImagePtr for CG to be RetainPtr<CGImageRef>

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

This work towards CG asynchronous image decoding: https://bugs.webkit.org/show_bug.cgi?id=155322.

[Said Abou-Hallawa]

Created attachment 273904 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273907 [details]
Patch

[Said Abou-Hallawa]

Created attachment 273909 [details]
Patch

[Simon Fraser (smfr)]

Did you check for leaks after this change?

[Said Abou-Hallawa]

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<>.

[Simon Fraser (smfr)]

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.

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 274553 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274554 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274556 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274557 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274559 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274560 [details]
Patch

[Said Abou-Hallawa]

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.

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 274780 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274793 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274809 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274814 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274836 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274838 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274842 [details]
Patch

[Said Abou-Hallawa]

Created attachment 274855 [details]
Patch

[Said Abou-Hallawa]

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.

[WebKit Commit Bot]

Comment on attachment 274855 [details]
Patch

Clearing flags on attachment: 274855

Committed r198655: <http://trac.webkit.org/changeset/198655>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.