Bug 226384 - Make ImageBuffer be able to put pixels directly into its backend from another ImageBuffer
Summary: Make ImageBuffer be able to put pixels directly into its backend from another...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-28 11:36 PDT by Said Abou-Hallawa
Modified: 2021-06-04 11:37 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.86 KB, patch)
2021-05-28 11:38 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2021-05-28 13:32 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2021-05-28 15:53 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2021-05-28 16:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (56.73 KB, patch)
2021-05-29 14:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (61.81 KB, patch)
2021-05-29 14:24 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (35.43 KB, patch)
2021-05-31 04:43 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (35.55 KB, patch)
2021-05-31 05:33 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (35.58 KB, patch)
2021-05-31 05:46 PDT, Said Abou-Hallawa
sabouhallawa: review?
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 2021-05-28 11:36:02 PDT
This can replace the call to ImageBuffer::draw() where alpha blending or scaling is required, for example the call in SourceGraphic::platformApplySoftware().
Comment 1 Said Abou-Hallawa 2021-05-28 11:37:26 PDT
I meant: ...where alpha blending or scaling is NOT required.
Comment 2 Said Abou-Hallawa 2021-05-28 11:38:24 PDT
Created attachment 430032 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-05-28 11:39:40 PDT
Comment on attachment 430032 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.cpp:195
> +    if (auto pixelBuffer = sourceImage.getPixelBuffer(format, srcRect))

For buffers with GPU backing (IOSurfaces) does this trigger readback?
Comment 4 Simon Fraser (smfr) 2021-05-28 11:41:40 PDT
Comment on attachment 430032 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.cpp:187
> +    if (resolutionScale() != sourceImage.resolutionScale()) {

Doesn't this also need to check color profiles and maybe more?
Comment 5 Said Abou-Hallawa 2021-05-28 11:44:23 PDT
Comment on attachment 430032 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:195
>> +    if (auto pixelBuffer = sourceImage.getPixelBuffer(format, srcRect))
> 
> For buffers with GPU backing (IOSurfaces) does this trigger readback?

Oh yes. I was thinking about filters in WebProcess. For GPUP case, I think we need to create a DL item which can only be used if both source and destination are in GPUP.
Comment 6 Said Abou-Hallawa 2021-05-28 11:49:50 PDT
This code can be optimized more if we copy from the source backend to the destination backend directly without going through the PixelBuffer.
Comment 7 Said Abou-Hallawa 2021-05-28 12:45:53 PDT
Comment on attachment 430032 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:187
>> +    if (resolutionScale() != sourceImage.resolutionScale()) {
> 
> Doesn't this also need to check color profiles and maybe more?

I think getPixelBuffer() will take care of these. The source PixelBufferFormat below is set to get the pixels in the destination colorSpace() and pixelFormat().
Comment 8 Said Abou-Hallawa 2021-05-28 13:32:59 PDT
Created attachment 430046 [details]
Patch
Comment 9 Said Abou-Hallawa 2021-05-28 15:53:26 PDT
Created attachment 430065 [details]
Patch
Comment 10 Said Abou-Hallawa 2021-05-28 16:04:39 PDT
Created attachment 430066 [details]
Patch
Comment 11 Sam Weinig 2021-05-29 09:09:35 PDT
Comment on attachment 430066 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2088
> +    PutImageBuffer(RenderingResourceIdentifier imageBufferIdentifier, const IntPoint& destPoint)

Does anything need to be done to ensure the sourceImage is not mutated before this can happen?
Comment 12 Said Abou-Hallawa 2021-05-29 14:06:51 PDT
Created attachment 430110 [details]
Patch
Comment 13 Said Abou-Hallawa 2021-05-29 14:11:13 PDT
Comment on attachment 430066 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2088
>> +    PutImageBuffer(RenderingResourceIdentifier imageBufferIdentifier, const IntPoint& destPoint)
> 
> Does anything need to be done to ensure the sourceImage is not mutated before this can happen?

Yes you are right. PutImageBuffer should be a synchronous message after making sure all the pending items in both source and destination ImageBuffers are flushed to their backends.
Comment 14 Said Abou-Hallawa 2021-05-29 14:24:00 PDT
Created attachment 430111 [details]
Patch
Comment 15 Said Abou-Hallawa 2021-05-31 04:43:41 PDT
Created attachment 430196 [details]
Patch
Comment 16 Said Abou-Hallawa 2021-05-31 05:33:58 PDT
Created attachment 430198 [details]
Patch
Comment 17 Said Abou-Hallawa 2021-05-31 05:46:42 PDT
Created attachment 430199 [details]
Patch
Comment 18 Radar WebKit Bug Importer 2021-06-04 11:37:53 PDT
<rdar://problem/78878491>