Bug 219368 – GPU Process: IOSurfaces should not be mapped into the Web Content Process

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

GPU Process: IOSurfaces should not be mapped into the Web Content Process

[Tim Horton]

Created attachment 415054 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 415054 [details]
Patch

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

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:110
> +        if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM)) {
> +            if (m_acceleratesDrawing)

Can we move this into a helper factory function instead of putting it inside encode function?

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableUnmappedIOSurfaceBackend.cpp:43
> +        ASSERT_NOT_REACHED();

Can we release assert this?

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableUnmappedIOSurfaceBackend.cpp:63
> +    ASSERT_NOT_REACHED();

Can we release assert this? And ditto for the rest.

[Tim Horton]

(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 415054 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415054&action=review
> 
> > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:110
> > +        if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM)) {
> > +            if (m_acceleratesDrawing)
> 
> Can we move this into a helper factory function instead of putting it inside
> encode function?

I'm going to try to move createImageBufferBackendHandle up in the hierarchy so that this just dissolves.

> > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableUnmappedIOSurfaceBackend.cpp:43
> > +        ASSERT_NOT_REACHED();
> 
> Can we release assert this?
> 
> > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableUnmappedIOSurfaceBackend.cpp:63
> > +    ASSERT_NOT_REACHED();
> 
> Can we release assert this? And ditto for the rest.

Sure, why not!

[Tim Horton]

Created attachment 415063 [details]
Patch

[Tim Horton]

Created attachment 415064 [details]
Patch

[EWS]

Committed r270275: <https://trac.webkit.org/changeset/270275>

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

[Radar WebKit Bug Importer]

<rdar://problem/71828486>

[Tim Horton]

Looking into the API test failures. Silly me, thinking that the only GPUP bots were post-landing bots. Forgot about the API tests.

[Tim Horton]

Reverted r270275 for reason:

broke canvas painting

Committed r270280: <https://trac.webkit.org/changeset/270280>

[Tim Horton]

Whoops:

        3   0x4f97b8a6f WebKit::ImageBufferShareableUnmappedIOSurfaceBackend::draw(WebCore::GraphicsContext&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&)
        4   0x4f97d0e49 WebCore::ConcreteImageBuffer<WebKit::ImageBufferShareableUnmappedIOSurfaceBackend>::draw(WebCore::GraphicsContext&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&)
        5   0x50a8af6d5 WebCore::GraphicsContext::drawImageBuffer(WebCore::ImageBuffer&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&)
        6   0x50a8af774 WebCore::GraphicsContext::drawImageBuffer(WebCore::ImageBuffer&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&)
        7   0x509bd2ddb WebCore::HTMLCanvasElement::paint(WebCore::GraphicsContext&, WebCore::LayoutRect const&)
        8   0x50ae37f34 WebCore::RenderHTMLCanvas::paintReplaced(WebCore::PaintInfo&, WebCore::LayoutPoint const&)

[Tim Horton]

Created attachment 415182 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 415182 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h:73
> +    static constexpr bool canMapBackingStore = true;

I think you can remove this. ImageBufferIOSurfaceBackend has canMapBackingStore = true from ImageBufferBackend.

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:40
> +std::unique_ptr<ImageBufferShareableIOSurfaceBackend> ImageBufferShareableIOSurfaceBackend::create(const FloatSize& logicalSize, const IntSize& internalSize, float resolutionScale, ColorSpace colorSpace, PixelFormat pixelFormat, ImageBufferBackendHandle handle)

Please replace internalSize with backendSize.

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:58
> +    RELEASE_ASSERT_NOT_REACHED();
> +    return *(GraphicsContext*)nullptr;

Should all the release_assert implementations be moved to ImageBufferBackend pure virtual methods instead? I think the release_assert implementations in ImageBufferBackend will serve the two purposes well: 'Don't create a backend unless you implement these functions "or" never call any of them'.

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.h:45
> +    ImageBufferShareableIOSurfaceBackend(const WebCore::FloatSize& logicalSize, const WebCore::IntSize& physicalSize, float resolutionScale, WebCore::ColorSpace colorSpace, WebCore::PixelFormat pixelFormat, ImageBufferBackendHandle&& handle)

Please replace physicalSize with backendSize.

[Tim Horton]

Created attachment 415188 [details]
Patch

[EWS]

Committed r270342: <https://trac.webkit.org/changeset/270342>

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