Bug 207048 - Allow different back-ends for ImageBuffer
Summary: Allow different back-ends for ImageBuffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on: 206621
Blocks: 204955 207109
  Show dependency treegraph
 
Reported: 2020-01-31 08:59 PST by Said Abou-Hallawa
Modified: 2020-10-14 11:13 PDT (History)
26 users (show)

See Also:


Attachments
Patch (345.99 KB, patch)
2020-01-31 09:10 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (347.00 KB, patch)
2020-01-31 09:17 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (351.11 KB, patch)
2020-01-31 09:46 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (352.85 KB, patch)
2020-01-31 14:22 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (293.21 KB, patch)
2020-01-31 19:00 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (298.48 KB, patch)
2020-02-18 09:58 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (298.49 KB, patch)
2020-02-18 10:17 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (302.65 KB, patch)
2020-02-18 15:34 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (299.30 KB, patch)
2020-02-18 16:08 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 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>