Bug 218427 - [GPU Process] Control the life cycle of the platform image by a new class named NativeImage
Summary: [GPU Process] Control the life cycle of the platform image by a new class nam...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 217596
  Show dependency treegraph
 
Reported: 2020-11-01 11:30 PST by Said Abou-Hallawa
Modified: 2021-02-08 06:52 PST (History)
34 users (show)

See Also:


Attachments
Patch (222.97 KB, patch)
2020-11-01 12:50 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (226.35 KB, patch)
2020-11-01 20:26 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (223.58 KB, patch)
2020-11-01 20:52 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (228.51 KB, patch)
2020-11-01 21:16 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (229.78 KB, patch)
2020-11-01 21:38 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (238.14 KB, patch)
2020-11-01 22:20 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (238.65 KB, patch)
2020-11-01 23:19 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (239.70 KB, patch)
2020-11-02 00:12 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (239.70 KB, patch)
2020-11-02 00:16 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (239.42 KB, patch)
2020-11-02 00:27 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (240.20 KB, patch)
2020-11-02 01:44 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (246.10 KB, patch)
2020-11-06 20:03 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (263.80 KB, patch)
2020-11-06 20:12 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (247.53 KB, patch)
2020-11-06 20:45 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (247.48 KB, patch)
2020-11-06 21:45 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (247.51 KB, patch)
2020-11-06 22:20 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (247.99 KB, patch)
2020-11-06 23:40 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (235.29 KB, patch)
2020-11-09 01:15 PST, Said Abou-Hallawa
simon.fraser: review+
Details | Formatted Diff | Diff
Patch (235.32 KB, patch)
2020-11-09 16:20 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (235.76 KB, patch)
2020-11-09 16:48 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-11-01 11:30:42 PST
A new class is needed to allow:

1. Caching the platform image in the GPU side when it is first painted. This can be done by assigning a unique RenderingBackendIdentifier to each NativeImage.
2. Remove the platform image from the GPU side when the NativeImage is being destroyed.
Comment 1 Said Abou-Hallawa 2020-11-01 12:50:58 PST
Created attachment 412868 [details]
Patch
Comment 2 EWS Watchlist 2020-11-01 12:51:51 PST
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
Comment 3 Said Abou-Hallawa 2020-11-01 20:26:55 PST
Created attachment 412877 [details]
Patch
Comment 4 Said Abou-Hallawa 2020-11-01 20:52:50 PST
Created attachment 412879 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-11-01 21:16:25 PST
Created attachment 412880 [details]
Patch
Comment 6 Said Abou-Hallawa 2020-11-01 21:38:21 PST
Created attachment 412881 [details]
Patch
Comment 7 Said Abou-Hallawa 2020-11-01 22:20:28 PST
Created attachment 412885 [details]
Patch
Comment 8 Said Abou-Hallawa 2020-11-01 23:19:52 PST
Created attachment 412887 [details]
Patch
Comment 9 Said Abou-Hallawa 2020-11-02 00:12:04 PST
Created attachment 412890 [details]
Patch
Comment 10 Said Abou-Hallawa 2020-11-02 00:16:33 PST
Created attachment 412891 [details]
Patch
Comment 11 Said Abou-Hallawa 2020-11-02 00:27:40 PST
Created attachment 412893 [details]
Patch
Comment 12 Said Abou-Hallawa 2020-11-02 01:44:11 PST
Created attachment 412894 [details]
Patch
Comment 13 Myles C. Maxfield 2020-11-02 23:40:54 PST
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 14 Wenson Hsieh 2020-11-04 12:52:21 PST
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 15 Wenson Hsieh 2020-11-04 13:22:04 PST
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)
Comment 16 Said Abou-Hallawa 2020-11-06 20:03:35 PST
Created attachment 413507 [details]
Patch
Comment 17 Said Abou-Hallawa 2020-11-06 20:12:22 PST
Created attachment 413508 [details]
Patch
Comment 18 Said Abou-Hallawa 2020-11-06 20:45:40 PST
Created attachment 413511 [details]
Patch
Comment 19 Said Abou-Hallawa 2020-11-06 21:45:11 PST
Created attachment 413517 [details]
Patch
Comment 20 Said Abou-Hallawa 2020-11-06 22:20:22 PST
Created attachment 413518 [details]
Patch
Comment 21 Said Abou-Hallawa 2020-11-06 23:40:20 PST
Created attachment 413521 [details]
Patch
Comment 22 Wenson Hsieh 2020-11-07 08:46:34 PST
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.
Comment 23 Radar WebKit Bug Importer 2020-11-08 11:31:58 PST
<rdar://problem/71167282>
Comment 24 Said Abou-Hallawa 2020-11-09 01:15:25 PST
Created attachment 413566 [details]
Patch
Comment 25 Said Abou-Hallawa 2020-11-09 10:02:36 PST
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 26 Simon Fraser (smfr) 2020-11-09 13:41:30 PST
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?
Comment 27 Said Abou-Hallawa 2020-11-09 16:20:24 PST
Created attachment 413639 [details]
Patch
Comment 28 Said Abou-Hallawa 2020-11-09 16:48:50 PST
Created attachment 413645 [details]
Patch
Comment 29 EWS 2020-11-09 19:14:43 PST
Committed r269614: <https://trac.webkit.org/changeset/269614>

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