Bug 192471 - [CG] Don't copy the ImageBuffer pixels data when drawing to an accelerated context
Summary: [CG] Don't copy the ImageBuffer pixels data when drawing to an accelerated co...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
Keywords: InRadar
: 183271 (view as bug list)
Depends on:
Reported: 2018-12-06 12:52 PST by Said Abou-Hallawa
Modified: 2020-03-03 10:56 PST (History)
4 users (show)

See Also:

Patch (2.74 KB, patch)
2018-12-06 13:53 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 2018-12-06 12:52:14 PST
This basically will roll out the change <http://trac.webkit.org/changeset/106095>.

ImageBuffer::copyNativeImage() has two copy behaviors:

1. DontCopyBackingStore: this passes a CGDataProvider to CGImageCreate() which is named m_data.dataProvider. The m_data.dataProvider holds the image pixels data which is named ‘m_data.data’. The same m_data.dataProvider is also backs the ImageBuffer context which is named ‘m_data.context’. Remember m_data.dataProvider is ref-counted pointer of type RetainPtr<CGDataProviderRef>.

2. CopyBackingStore: this passes the ‘m_data.context’ to CGBitmapContextCreateImage(). But this is actually does not copy the pixel data from the context. According to the documentation the returned CGImageRef follows copy-on-write semantics. That means it will copy the data only when the context changes. This does not include freeing the pixels data if the context was created by CGBitmapContextCreate().

The description of bug in https://bugs.webkit.org/show_bug.cgi?id=77185 says that “Thus, especially in the case of accelerated canvas there can be image tearing so we need to always do a deep copy if the destination context is accelerated.” So the worry was freeing the Image buffer not changing its contents. But this is confusing because DontCopyBackingStore is safer than CopyBackingStore in this regard. The image buffer may be deleted but its m_data.data will stay alive if there is a CGDataProvider referencing it.
Comment 1 Said Abou-Hallawa 2018-12-06 13:51:13 PST
Comment 2 Said Abou-Hallawa 2018-12-06 13:53:15 PST
Created attachment 356750 [details]
Comment 3 Said Abou-Hallawa 2018-12-06 13:59:46 PST
*** Bug 183271 has been marked as a duplicate of this bug. ***
Comment 4 Simon Fraser (smfr) 2018-12-06 16:30:18 PST
Comment on attachment 356750 [details]

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

> Source/WebCore/ChangeLog:3
> +        [CG] Don't copy the ImageBuffer pixels data when drawing to an accelerated context

This doesn't describe the symptoms. Bug 183271 has a much better title.

> Source/WebCore/ChangeLog:8
> +        This rolls out r106095.

How does this not revert the synchronization bug that r106095 was fixing?

> Source/WebCore/ChangeLog:11
> +        underlaying pixels data will be held in memory till the image is drawn


> Source/WebCore/ChangeLog:14
> +        The underlaying pixels data will be freed only when the CGDataProvider

underlying pixel data
Comment 5 Simon Fraser (smfr) 2020-03-03 10:56:50 PST
I don't think we need this any more.