Bug 205200 - Implement encoding/decoding for DisplayList::DrawNativeImage
Summary: Implement encoding/decoding for DisplayList::DrawNativeImage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-12 22:05 PST by Wenson Hsieh
Modified: 2019-12-13 13:27 PST (History)
7 users (show)

See Also:

Attachments
Patch (11.04 KB, patch)
2019-12-12 22:23 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Fix GTK/WPE (11.14 KB, patch)
2019-12-12 22:36 PST, Wenson Hsieh 		simon.fraser: review+
Details | Formatted Diff | Diff
Patch (14.03 KB, patch)
2019-12-13 12:24 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2019-12-12 22:05:21 PST 
This is exercised by multiple failing layout tests in fast/canvas, when rendering canvas in the GPU process.
Comment 1 Wenson Hsieh 2019-12-12 22:23:02 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2019-12-12 22:36:17 PST 
Created attachment 385580 [details]
Fix GTK/WPE
Comment 3 Simon Fraser (smfr) 2019-12-13 10:15:36 PST 
Comment on attachment 385580 [details]
Fix GTK/WPE

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

> Source/WebKit/ChangeLog:10
> +        Add helper functions to encode and decode NativeImagePtr (RetainPtr<CGImageRef> on Cocoa platforms). This
> +        mirrors the implementation of encoding and decoding for WebCore::Image, which create and draws into
> +        ShareableBitmap, and send a handle to the data over IPC.

I think we should try to remove drawNativeImage. Here you're forced to call into CG in the web process, and we're trying to avoid that in general.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1384
> +    Optional<FloatRect> destination;

destinationRect

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1050
> +    if (graphicsContext)
> +        graphicsContext->drawNativeImage(image, { }, FloatRect({ }, imageSize), FloatRect({ }, imageSize));

If we fail to make a graphicsContext shouldn't the encoding fail?
Comment 4 Wenson Hsieh 2019-12-13 12:01:47 PST 
Comment on attachment 385580 [details]
Fix GTK/WPE

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

Thanks for the review!

>> Source/WebKit/ChangeLog:10
>> +        ShareableBitmap, and send a handle to the data over IPC.
> 
> I think we should try to remove drawNativeImage. Here you're forced to call into CG in the web process, and we're trying to avoid that in general.

Added a sentence to the ChangeLog describing this, and also filed webkit.org/b/205213.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1384
>> +    Optional<FloatRect> destination;
> 
> destinationRect

👍🏻. Also, renamed `m_destination` to `m_destinationRect`.

>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1050
>> +        graphicsContext->drawNativeImage(image, { }, FloatRect({ }, imageSize), FloatRect({ }, imageSize));
> 
> If we fail to make a graphicsContext shouldn't the encoding fail?

Good call — I changed both this function and the existing encodeImage() function to encode `!!graphicsContext`, and then made decoding fail if we failed to make a graphicsContext.
Comment 5 Wenson Hsieh 2019-12-13 12:24:49 PST 
Created attachment 385630 [details]
Patch
Comment 6 WebKit Commit Bot 2019-12-13 13:22:31 PST 
Comment on attachment 385630 [details]
Patch

Clearing flags on attachment: 385630

Committed r253495: <https://trac.webkit.org/changeset/253495>
Comment 7 Radar WebKit Bug Importer 2019-12-13 13:27:18 PST 
<rdar://problem/57923065>