Bug 216565 - Move SerializationState from ImageBuffer to ImageBitmap
Summary: Move SerializationState from ImageBuffer to ImageBitmap
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: 211484
Blocks:
  Show dependency treegraph
 
Reported: 2020-09-15 09:44 PDT by Said Abou-Hallawa
Modified: 2020-09-25 18:42 PDT (History)
16 users (show)

See Also:


Attachments
Patch (29.06 KB, patch)
2020-09-15 10:13 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (30.07 KB, patch)
2020-09-15 10:52 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (36.65 KB, patch)
2020-09-16 16:38 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (36.73 KB, patch)
2020-09-16 17:39 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (37.22 KB, patch)
2020-09-16 18:23 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (45.93 KB, patch)
2020-09-20 12:49 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (45.91 KB, patch)
2020-09-20 13:12 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (46.07 KB, patch)
2020-09-20 21:14 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (46.36 KB, patch)
2020-09-25 15:06 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.39 KB, patch)
2020-09-25 15:53 PDT, 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-09-15 09:44:48 PDT
This structure belongs to ImageBitmap. It describes how the ImageBitmap can deal with its ImageBuffer.
Comment 1 Said Abou-Hallawa 2020-09-15 10:13:38 PDT
Created attachment 408828 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-09-15 10:52:16 PDT
Created attachment 408833 [details]
Patch
Comment 3 Said Abou-Hallawa 2020-09-16 16:38:52 PDT
Created attachment 408961 [details]
Patch
Comment 4 Said Abou-Hallawa 2020-09-16 17:39:54 PDT
Created attachment 408969 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-09-16 18:23:06 PDT
Created attachment 408975 [details]
Patch
Comment 6 Kenneth Russell 2020-09-17 18:59:04 PDT
Comment on attachment 408975 [details]
Patch

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

There are several nice aspects to this patch - the use of the OptionSet is a good cleanup. A few thoughts - happy to r+ this if you prefer things in the form presented here.

> Source/WebCore/html/ImageBitmap.cpp:839
>      m_bitmapData = nullptr;

It seems undesirable to me to reach up into the superclass's implementation for this operation. It will probably make it harder to add any GPU acceleration for ImageBitmap later, when the ImageBitmap's backing store is a texture rather than CPU-side bytes.

> Source/WebCore/html/SerializableImageBuffer.h:34
> +class SerializableImageBuffer {

This name is confusing to me because this class isn't an ImageBuffer. Perhaps call it something like "ImageBufferSerializationHelper"? (Also not a great name.)

Pulling so much of the ImageBitmap's functionality into a superclass, and using it for two purposes - constructing ImageBitmaps themselves, and as the data which is serialized/deserialized - makes the code harder to think about in my opinion. I recommend composition over inheritance.

> Source/WebCore/html/SerializableImageBuffer.h:61
> +    std::unique_ptr<ImageBuffer> m_bitmapData;

The fact that this data member is protected rather than private makes it more complicated to reason about correct states of this class and its subclass, ImageBitmap. I think it would be better to make it private and access it via accessors.
Comment 7 Sam Weinig 2020-09-19 17:39:53 PDT
Comment on attachment 408975 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        std::pair<std::unique_ptr<ImageBuffer>, OptionSet<SerializationState>>
> +        can be replaced by a new class named SerializableImageBuffer. This class
> +        will also be a superclass of ImageBitmap. All the operations on ImageBuffer

What benefit is there in making it the superclass? Could it just be a member of ImageBitmap?
Comment 8 Said Abou-Hallawa 2020-09-20 12:49:45 PDT
Created attachment 409241 [details]
Patch
Comment 9 Said Abou-Hallawa 2020-09-20 13:12:59 PDT
Created attachment 409245 [details]
Patch
Comment 10 Said Abou-Hallawa 2020-09-20 21:14:47 PDT
Created attachment 409258 [details]
Patch
Comment 11 Said Abou-Hallawa 2020-09-21 10:20:54 PDT
Comment on attachment 408975 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        will also be a superclass of ImageBitmap. All the operations on ImageBuffer
> 
> What benefit is there in making it the superclass? Could it just be a member of ImageBitmap?

I added a new class named ImageBitmapSource and I made it a member of ImageBitmap.

>> Source/WebCore/html/ImageBitmap.cpp:839
>>      m_bitmapData = nullptr;
> 
> It seems undesirable to me to reach up into the superclass's implementation for this operation. It will probably make it harder to add any GPU acceleration for ImageBitmap later, when the ImageBitmap's backing store is a texture rather than CPU-side bytes.

I added a new class named ImageBitmapSource and I made it a member of ImageBitmap. In the future it can be made an abstract class and inherited by ImageBitmapBufferSource and ImageBitmapTextureSource.

>> Source/WebCore/html/SerializableImageBuffer.h:34
>> +class SerializableImageBuffer {
> 
> This name is confusing to me because this class isn't an ImageBuffer. Perhaps call it something like "ImageBufferSerializationHelper"? (Also not a great name.)
> 
> Pulling so much of the ImageBitmap's functionality into a superclass, and using it for two purposes - constructing ImageBitmaps themselves, and as the data which is serialized/deserialized - makes the code harder to think about in my opinion. I recommend composition over inheritance.

I named it ImageBitmapSource to make it more generic name so it can be extended for non CPU buffer back end.

>> Source/WebCore/html/SerializableImageBuffer.h:61
>> +    std::unique_ptr<ImageBuffer> m_bitmapData;
> 
> The fact that this data member is protected rather than private makes it more complicated to reason about correct states of this class and its subclass, ImageBitmap. I think it would be better to make it private and access it via accessors.

Done since this class is now a member of ImageBitmap.
Comment 12 Radar WebKit Bug Importer 2020-09-22 09:45:23 PDT
<rdar://problem/69375838>
Comment 13 Kenneth Russell 2020-09-23 11:53:33 PDT
Comment on attachment 409258 [details]
Patch

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

Looks good to me. A few minor comments. Please fix the style issues before landing. r+

> Source/WebCore/Headers.cmake:644
> +    html/ImageBitmapSource.h

Per below: since this is the backing store per the ChangeLog comment, I think ImageBitmapBacking would be a better name.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1046
> +        write(static_cast<uint8_t>(imageBitmap.serializationState().toRaw()));

Should this file #include ImageBitmapSource.h? It looks like it's getting it implicitly through the unified build.

> Source/WebCore/bindings/js/SerializedScriptValue.h:54
>  class ImageBitmap;

Is the ImageBitmap forward declaration needed?

> Source/WebCore/bindings/js/SerializedScriptValue.h:56
> +class ImageBitmapSource;

This header #includes ImageBuffer.h. Should that be removed now?

> Source/WebCore/html/ImageBitmap.h:29
> +#include "ImageBitmapSource.h"

Per below: I think "ImageBitmapBacking" would be a more descriptive name.

> Source/WebCore/html/ImageBitmap.h:91
> +    std::unique_ptr<ImageBuffer> takeImageBuffer();

Maybe worth a comment that this has the implicit side-effect of detaching the backing store, and that it returns nullptr if the ImageBitmap's already detached.

> Source/WebCore/html/ImageBitmap.h:101
> +    Optional<ImageBitmapSource> takeImageBitmapSource();

Suggest "takeImageBitmapBacking" as the name.

> Source/WebCore/html/ImageBitmap.h:130
> +    Optional<ImageBitmapSource> m_imageSource;

I think m_backing or m_backingStore would be a better choice of name.

> Source/WebCore/html/ImageBitmapSource.cpp:31
> +ImageBitmapSource::ImageBitmapSource(std::unique_ptr<ImageBuffer>&& bitmapData, OptionSet<SerializationState> serializationState)

See below regarding naming.

> Source/WebCore/html/ImageBitmapSource.h:39
> +class ImageBitmapSource {

Per the ChangeLog comment, since this is now acting as the ImageBitmap's backing store, I think "ImageBitmapBacking" would be a better name.

> Source/WebCore/html/ImageBitmapSource.h:41
> +    ImageBitmapSource() = default;

This constructor leaves m_serializationState thinking it's not origin-clean. Is that what was intended?
Comment 14 Said Abou-Hallawa 2020-09-25 15:06:51 PDT
Created attachment 409745 [details]
Patch
Comment 15 Said Abou-Hallawa 2020-09-25 15:53:40 PDT
Created attachment 409752 [details]
Patch
Comment 16 EWS 2020-09-25 18:42:13 PDT
Committed r267615: <https://trac.webkit.org/changeset/267615>

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