Bug 218427

Created attachment 412868 [details]
Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Created attachment 412877 [details]
Patch

Created attachment 412879 [details]
Patch

Created attachment 412880 [details]
Patch

Created attachment 412881 [details]
Patch

Created attachment 412885 [details]
Patch

Created attachment 412887 [details]
Patch

Created attachment 412890 [details]
Patch

Created attachment 412891 [details]
Patch

Created attachment 412893 [details]
Patch

Created attachment 412894 [details]
Patch

Comment on attachment 412894 [details]
Patch

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

> Source/WebCore/platform/graphics/NativeImage.h:74
> +    RenderingResourceIdentifier m_renderingResourceIdentifier { RenderingResourceIdentifier::generate() };

It's a bit surprising the Image owns this itself, rather than a higher level

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:460
> +        subImage = tileImage->platformImage();

I wish there was a way to get rid of this extra typing. Requiring it doesn't seem to actually add much clarity to the code. Can we do an operator PlatformImagePtr()?

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:87
> +        image = NativeImage::create(CGBitmapContextCreateImage(context.get()));

I wish we could make this implicit.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:864
> +DrawNativeImage::DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options)

Most of this patch is an introduction and migration to a new type, but this looks like an intentional behavior change. It seems like a good idea to split this out, just in case.

Comment on attachment 412894 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:369
> +        m_decodedImage = NativeImage::create(CGBitmapContextCreateImage(bitmapContext.get()));

Does this leak the image?

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:73
> +        return NativeImage::create(CGImageCreateWithImageInRect(image->platformImage().get(), CGRectMake(0, 0, backendSize.width(), backendSize.height())));

(Ditto about the leak)

>> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:87
>> +        image = NativeImage::create(CGBitmapContextCreateImage(context.get()));
> 
> I wish we could make this implicit.

(Ditto about the leak)

Comment on attachment 412894 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:159
> +        image = NativeImage::create(CGImageCreate(pixelArrayDimensions.width(), pixelArrayDimensions.height(), 8, 32, 4 * pixelArrayDimensions.width(), sRGBColorSpaceRef(), kCGBitmapByteOrderDefault | kCGImageAlphaNoneSkipLast, dataProvider.get(), 0, false, kCGRenderingIntentDefault));

IIUC, these are going to leak as well since CGImageCreate returns a +1 object.

We could use adoptCF here to fix this.

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:169
> +        image = NativeImage::create(CGBitmapContextCreateImage(context.get()));

(Ditto)

> Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:114
> +        return NativeImage::create(CGBitmapContextCreateImage(context().platformContext()));

(Ditto)

> Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:117
> +        return NativeImage::create(CGImageCreate(

(Ditto)

Created attachment 413507 [details]
Patch

Created attachment 413508 [details]
Patch

Created attachment 413511 [details]
Patch

Created attachment 413517 [details]
Patch

Created attachment 413518 [details]
Patch

Created attachment 413521 [details]
Patch

Comment on attachment 413521 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:369
> +        m_decodedImage = NativeImage::create(CGBitmapContextCreateImage(bitmapContext.get()));

Nit - still have a leak here.

<rdar://problem/71167282>

Created attachment 413566 [details]
Patch

Comment on attachment 412894 [details]
Patch

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

>> Source/WebCore/platform/graphics/NativeImage.h:74
>> +    RenderingResourceIdentifier m_renderingResourceIdentifier { RenderingResourceIdentifier::generate() };
> 
> It's a bit surprising the Image owns this itself, rather than a higher level

The identifier is removed for now. 

We agreed on having a RenderingResourceIdentifier for ImageBuffer regardless it is a remote ImageBuffer or not. This solved the problem of recording and replaying resource items such as DrawImageBuffer. A HashMap of Ref<ImageBuffer> is filled while recording and is passed when replaying back. DrawImageBuffer contains only the RenderingResourceIdentifier of the ImageBuffer. So I may use the technique with NativeImage.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:460
>> +        subImage = tileImage->platformImage();
> 
> I wish there was a way to get rid of this extra typing. Requiring it doesn't seem to actually add much clarity to the code. Can we do an operator PlatformImagePtr()?

I added it at some point but I realized that I need it in cases like CGImageGetWidth(tileImage->platformImage().get()). It was a little bit confusing when you need and when you do not need it. The RefPtr has its operator to NativeImage and this makes difficult to differentiate between them.

>> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:369
>> +        m_decodedImage = NativeImage::create(CGBitmapContextCreateImage(bitmapContext.get()));
> 
> Does this leak the image?

Yes. Thanks for catching this. I think I could avoid writing adoptCF. I did not realized is going to increment the ref count instead of adopting the existing one.

>> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:73
>> +        return NativeImage::create(CGImageCreateWithImageInRect(image->platformImage().get(), CGRectMake(0, 0, backendSize.width(), backendSize.height())));
> 
> (Ditto about the leak)

Yes searching for "NativeImage::create(CG" should get this as well.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:864
>> +DrawNativeImage::DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options)
> 
> Most of this patch is an introduction and migration to a new type, but this looks like an intentional behavior change. It seems like a good idea to split this out, just in case.

Done.

Comment on attachment 413566 [details]
Patch

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

> Source/WebCore/platform/graphics/NativeImage.h:37
> +class NativeImage : public RefCounted<NativeImage> {

WTF_MAKE_FAST_ALLOCATED

> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:341
> +    RetainPtr<CGImageRef> image = m_cgImage->platformImage();

The member variable isn't a CGImage any more, so maybe rename it.

> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:376
> +    image = m_cgImage->platformImage();

Hard to see where 'image' comes from.

> Source/WebKitLegacy/mac/DOM/DOM.mm:539
> +            RetainPtr<CGImageRef> contentImage = image->nativeImage()->platformImage();

auto?

Created attachment 413639 [details]
Patch

Created attachment 413645 [details]
Patch

Committed r269614: <https://trac.webkit.org/changeset/269614>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413645 [details].