Bug 207048

Summary: Allow different back-ends for ImageBuffer
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, annulen, beidson, cdumez, commit-queue, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, graouts, gyuyoung.kim, hta, jer.noble, jsbell, kondapallykalyan, noam, pdr, philipj, ryuan.choi, schenney, sergio, thorton, tommyw, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=217384
https://bugs.webkit.org/show_bug.cgi?id=217603
Bug Depends on: 206621    
Bug Blocks: 204955, 207109    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for review
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2020-01-31 08:59:48 PST
This will decouple the platform specifics and the back-end details from the interface "ImageBuffer". In following patches new types of ImageBuffers backed by new ImageBufferBackends will be added. These new ImageBufferBackends will implement DisplayList drawing and synchronize remote rendering.
Comment 1 Said Abou-Hallawa 2020-01-31 09:10:49 PST
Created attachment 389356 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-01-31 09:17:26 PST
Created attachment 389357 [details]
Patch
Comment 3 Said Abou-Hallawa 2020-01-31 09:46:18 PST
Created attachment 389366 [details]
Patch
Comment 4 Said Abou-Hallawa 2020-01-31 14:22:08 PST
Created attachment 389414 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-01-31 19:00:23 PST
Created attachment 389449 [details]
Patch  for review
Comment 6 Tim Horton 2020-02-18 00:45:13 PST
Comment on attachment 389449 [details]
Patch  for review

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

I think this is fine, except the name.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:834
> -		2DDE1D081F574D0E00D1A365 /* JSVRStageParameters.h in Headers */ = {isa = PBXBuildFile; fileRef = 2DDE1CF61F574BFF00D1A365 /* JSVRStageParameters.h */; };
> +		2DDE1CE31F574AE500D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };

This all doesn't look good

> Source/WebCore/platform/graphics/ContextualImageBuffer.h:33
> +class ContextualImageBuffer : public ImageBuffer {

I'm not sure I follow the "Contextual" name (but maybe you can explain)

(also, I'm not sure why it exists? Why isn't this just all IN ImageBuffer?)

(reading more... is it so you can hide BackendType from ImageBuffer clients? why not just `using` it away?)

> Source/WebCore/platform/graphics/ContextualImageBuffer.h:145
> +        const_cast<ContextualImageBuffer&>(*this).flushContext();

Maybe we should just bite the bullet and de-const getImageData if it's a lie?
Comment 7 Said Abou-Hallawa 2020-02-18 09:58:06 PST
Created attachment 391061 [details]
Patch
Comment 8 Said Abou-Hallawa 2020-02-18 10:17:14 PST
Created attachment 391063 [details]
Patch
Comment 9 Said Abou-Hallawa 2020-02-18 13:34:23 PST
Comment on attachment 389449 [details]
Patch  for review

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:834
>> +		2DDE1CE31F574AE500D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> 
> This all doesn't look good

This was fixed in the latest patch.

>> Source/WebCore/platform/graphics/ContextualImageBuffer.h:33
>> +class ContextualImageBuffer : public ImageBuffer {
> 
> I'm not sure I follow the "Contextual" name (but maybe you can explain)
> 
> (also, I'm not sure why it exists? Why isn't this just all IN ImageBuffer?)
> 
> (reading more... is it so you can hide BackendType from ImageBuffer clients? why not just `using` it away?)

Contextual here means an ImageBuffer which can provide a GraphicsContext. ContextualImageBuffer is a template class which is derived from ImageBuffer. All the callers can still keep a. pointer to an ImageBuffer which represents the interface of all the ContextualImageBuffers. It is also a bridge design pattern. As you said it hides the backend type from the callers. The creator can choose the backend type but caller which receives an std::unique_ptr<ImageBuffer> does not have to know any details about  this backend.

Other names: BackendedImageBuffer, ImageBufferWithBackend, PhysicalImageBuffer, ConcreteImageBuffer, ImageBufferImpl, ImageBufferBridge..

>> Source/WebCore/platform/graphics/ContextualImageBuffer.h:145
>> +        const_cast<ContextualImageBuffer&>(*this).flushContext();
> 
> Maybe we should just bite the bullet and de-const getImageData if it's a lie?

I think it is okay to keep the function const. flushContext() is an internal function which does nothing expect in the case of IOSurface backed ended ImageBuffer and the remote ImageBuffer.
Comment 10 Said Abou-Hallawa 2020-02-18 15:34:51 PST
Created attachment 391110 [details]
Patch
Comment 11 Said Abou-Hallawa 2020-02-18 16:08:36 PST
Created attachment 391114 [details]
Patch
Comment 12 WebKit Commit Bot 2020-02-18 17:12:54 PST
Comment on attachment 391114 [details]
Patch

Clearing flags on attachment: 391114

Committed r256892: <https://trac.webkit.org/changeset/256892>
Comment 13 WebKit Commit Bot 2020-02-18 17:12:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2020-02-18 17:13:29 PST
<rdar://problem/59573099>