Bug 219007 – [GPU Process] Clean up recreating the ImageBufferBackend because of GPU crashing

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Said Abou-Hallawa]

No need to store the backend parameters in RemoteImageBufferProxy. They can be accessed from the current ImageBufferBackend via the ImageBuffer before the backend is cleared.

[Said Abou-Hallawa]

Created attachment 414293 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 414293 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:187
> +    void commitFlushDisplayList(WebCore::DisplayList::FlushIdentifier flushIdentifier) override

We need to stop using "commit" and "flush" in ways we don't all understand.

[Tim Horton]

IMO (but obv. up for discussion):

"submit" for "send display list/work to GPUP", like Wenson fixed elsewhere
"flush" is "wait till all submitted work has actually been completed"
"commit" should be just for the layer tree transactions (but is effectively the same as "submit" for a transaction)

and no combination of the above means anything so we shouldn't see them adjacent to each other :)

[Said Abou-Hallawa]

Created attachment 414861 [details]
Patch

[Said Abou-Hallawa]

Created attachment 414879 [details]
Patch

[Said Abou-Hallawa]

Created attachment 414880 [details]
Patch

[Said Abou-Hallawa]

Created attachment 414885 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71747762>

[Said Abou-Hallawa]

Created attachment 414912 [details]
Patch

[Said Abou-Hallawa]

Created attachment 414935 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 414935 [details]
Patch

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

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:297
> +    std::unique_ptr<ImageBufferBackend::Parameters> m_parameters;

Why not just store a copy of the tiny struct? Much simpler.

> Source/WebCore/platform/graphics/ImageBufferBackend.h:166
> +    const Parameters& m_parameters;

Who owns the original data? Would be safer to just store by value.

[Said Abou-Hallawa]

Created attachment 415043 [details]
Patch

[Said Abou-Hallawa]

Created attachment 415045 [details]
Patch

[Said Abou-Hallawa]

Created attachment 415046 [details]
Patch

[Said Abou-Hallawa]

Created attachment 415050 [details]
Patch

[Said Abou-Hallawa]

ImageBufferBackend::Parameters are saved by value in ImageBufferBackend and ConcreteImageBuffer. Renaming the flush/commit/flush functions was removed from this patch and will be addressed in a different patch.

[Said Abou-Hallawa]

Created attachment 415395 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 415395 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:124
> +    auto backendSize = this->backendSize();

Doesn't seem worthwhile storing this in a local.

> Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:108
> +    return { static_cast<int>(CGBitmapContextGetWidth(context().platformContext())), static_cast<int>(CGBitmapContextGetHeight(context().platformContext())) };

Let's not fetch context().platformContext() twice.

[Said Abou-Hallawa]

Created attachment 415434 [details]
Patch

[Said Abou-Hallawa]

Created attachment 415445 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 415395 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:124
>> +    auto backendSize = this->backendSize();
> 
> Doesn't seem worthwhile storing this in a local.

Actually it is worthy. sinkIntoNativeImage() destroys the backend. So we can't call backendSize() after calling sinkIntoNativeImage(). I put it back and I added a comment.

[EWS]

Committed r270458: <https://trac.webkit.org/changeset/270458>

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