Bug 214515

Summary: [CG] Flushing ImageBuffer IOSurface cached image can happen when creating an image for encoding only
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2020-07-17 23:55:01 PDT
This is related to bug 65767 and bug 157966ز

Flushing the IOSurface of the ImageBuffer after calling putImageData() is expensive. Drawing the ImageBuffer to a GraphicsContext causes the cached image of the IOSurface to be flushed. But we should flush the cached image when getting the CFData of the ImageBuffer.

There is no direct command to flush the cached image of an IOSurface. We do this by drawing an empty rectangle to the context of the IOSurface.
Comment 1 Said Abou-Hallawa 2020-07-17 23:56:58 PDT
Created attachment 404637 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-07-19 14:45:10 PDT
Created attachment 404687 [details]
Patch
Comment 3 Said Abou-Hallawa 2020-07-19 14:49:11 PDT
This patch gives a 15% progression to the Images test on MacBookPro15,2. I would expect all macOS and iOS devices will see similar progression.
Comment 4 Said Abou-Hallawa 2020-07-19 14:49:30 PDT
<rdar://problem/65735991>
Comment 5 Said Abou-Hallawa 2020-07-19 14:52:04 PDT
Created attachment 404688 [details]
Patch
Comment 6 Simon Fraser (smfr) 2020-07-20 10:16:05 PDT
Comment on attachment 404688 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h:71
> +    mutable bool m_pendingFlushCachedImage { false };

I think this needs a better name. Maybe m_requiresDrawAfterPutImageData or something. It's not really a "pending" (that's used for "something is going to happen in future").

Can we set m_pendingFlushCachedImage to false whenever any other CG drawing takes place as well?
Comment 7 Said Abou-Hallawa 2020-07-20 11:33:19 PDT
Created attachment 404732 [details]
Patch
Comment 8 Said Abou-Hallawa 2020-07-20 11:39:20 PDT
Comment on attachment 404688 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h:71
>> +    mutable bool m_pendingFlushCachedImage { false };
> 
> I think this needs a better name. Maybe m_requiresDrawAfterPutImageData or something. It's not really a "pending" (that's used for "something is going to happen in future").
> 
> Can we set m_pendingFlushCachedImage to false whenever any other CG drawing takes place as well?

Setting m_pendingFlushCachedImage after any other CG drawing takes place will be a little bit involving because we have to hack all the CanvasRenderingContext2D drawing functions. I think we can look at this later.
Comment 9 EWS 2020-07-20 13:18:34 PDT
Committed r264615: <https://trac.webkit.org/changeset/264615>

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