Bug 202394 - ImageBitmap should be serializable
Summary: ImageBitmap should be serializable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on:
Blocks: 183720
  Show dependency treegraph
 
Reported: 2019-10-01 02:16 PDT by Chris Lord
Modified: 2019-10-04 07:54 PDT (History)
14 users (show)

See Also:


Attachments
Patch (9.19 KB, patch)
2019-10-01 03:45 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (11.12 KB, patch)
2019-10-04 03:00 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (11.28 KB, patch)
2019-10-04 04:44 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (11.57 KB, patch)
2019-10-04 07:04 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2019-10-01 02:16:18 PDT
Currently an ImageBitmap can be sent in sendMessage if it's transferred, but it can't be copied. As it is serializable in the spec[1], this should work.

[1] https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#imagebitmap
Comment 1 Chris Lord 2019-10-01 03:45:44 PDT
Created attachment 379887 [details]
Patch
Comment 2 Chris Lord 2019-10-04 03:00:11 PDT
Created attachment 380208 [details]
Patch
Comment 3 Zan Dobersek 2019-10-04 04:08:17 PDT
Comment on attachment 380208 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:915
> +        auto buffer = imageBitmap.buffer();

It's usually preferable to use `auto*` to explicitly denote that a pointer of some sorts is being returned.

Given the pointer, what should happen if it is null? It can be, at least theoretically. Should a validation error be reported in this case. or should serialization still succeed?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:927
> +        write(static_cast<int32_t>(imageBitmap.originClean()));

origin-clean being a boolean, I think using uint8_t type here and in the deserialization makes more sense, if possible.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:932
> +        write(arrayBuffer->byteLength());

Nit: perhaps this should be explicitly casted to uint32_t to make the intended type clear, even when `unsigned` as returned by byteLength() is an equivalent.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2769
> +        read(originClean);
> +        read(logicalWidth);
> +        read(logicalHeight);
> +        read(resolutionScale);

These returns should also probable be checked, perhaps in the same if-statement to keep things compact.
Comment 4 Chris Lord 2019-10-04 04:44:45 PDT
Created attachment 380211 [details]
Patch
Comment 5 Zan Dobersek 2019-10-04 06:24:49 PDT
Comment on attachment 380211 [details]
Patch

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

Patch looks good, just some additional checks required to cover the odd cases.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:923
> +        auto imageData = buffer->getPremultipliedImageData(IntRect(0, 0, logicalSize.width(), logicalSize.height()));

Looked it up only now -- getPremultipliedImageData() can possibly return null, so we have to null-check around that as well.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2776
> +        auto imageData = Uint8ClampedArray::tryCreate(WTFMove(arrayBuffer), 0, arrayBuffer->byteLength());

tryCreate() possibly returns null, so we have to null-check it.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2777
> +        auto buffer = ImageBuffer::create(FloatSize(logicalWidth, logicalHeight), Unaccelerated, resolutionScale);

Similar here -- ImageBuffer::create() can return null if internally the initialization doesn't succeed (e.g. memory allocation or some other step fails).
Comment 6 Chris Lord 2019-10-04 07:04:56 PDT
Created attachment 380218 [details]
Patch
Comment 7 WebKit Commit Bot 2019-10-04 07:53:24 PDT
Comment on attachment 380218 [details]
Patch

Clearing flags on attachment: 380218

Committed r250721: <https://trac.webkit.org/changeset/250721>
Comment 8 WebKit Commit Bot 2019-10-04 07:53:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-10-04 07:54:14 PDT
<rdar://problem/55982648>