Bug 183438 – Implement createImageBitmap(ImageData)

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

[Ms2ger (he/him; ⌚ UTC+1/+2)]

ImageBuffer::putByteArray would probably work for the default case, but not if there's any resizing involved.

[Kenneth Russell]

Taking this.

[Kenneth Russell]

Created attachment 406103 [details]
Patch

[Kenneth Russell]

The attached patch makes all of the layout tests under:

  webgl/2.0.0/conformance/textures/image_bitmap_from_image_bitmap
  webgl/2.0.0/conformance/textures/image_bitmap_from_image_data
  webgl/2.0.0/conformance2/textures/image_bitmap_from_image_bitmap
  webgl/2.0.0/conformance2/textures/image_bitmap_from_image_data

fully pass, and progresses, or makes fully pass, more tests under:

  imported/w3c/web-platform-tests/html/canvas/element/imagebitmap

[Dean Jackson]

Comment on attachment 406103 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:191
> +    if (srcColorFormat == destColorFormat) {
> +        reinterpret_cast<uint32_t*>(destPixel)[0] = reinterpret_cast<const uint32_t*>(srcPixel)[0];
> +        return;
> +    }
> +
> +    // Swap pixel channels BGRA <-> RGBA.
> +    destPixel[0] = srcPixel[2];
> +    destPixel[1] = srcPixel[1];
> +    destPixel[2] = srcPixel[0];
> +    destPixel[3] = srcPixel[3];

Could you put a FIXME in here mentioning that we could maybe use Accelerate.framework on large Buffers to do this premultiply + swizzle step faster?

[Said Abou-Hallawa]

Comment on attachment 406103 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.h:45
>      struct SerializationState {
>          bool originClean { false };
>          bool premultiplyAlpha { false };
> +        bool forciblyPremultiplyAlpha { false };
>      };

I know this struct was added in r265208. But why was it added to ImageBuffer? I can see it is only used by ImageBitmap and OffscreenCanvas. Please move it a header file where it is used. Or create a new one if needed.

> Source/WebCore/platform/graphics/ImageBuffer.h:108
> +    // destFormat=AlphaPremultiplication::Premultiplied is the code
> +    // path supported by most of the browser's rendering paths.
> +    // destFormat=AlphaPremultiplication::Unpremultiplied is supported
> +    // primarily for creating ImageBitmaps to be uploaded to WebGL
> +    // textures.

Please remove this comment. It does not add much value.

> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:192
> +static inline void copyUnpremultipliedToUnpremultiplied(ColorFormat srcColorFormat, const uint8_t* srcPixel, ColorFormat destColorFormat, uint8_t* destPixel)
> +{
> +    if (srcColorFormat == destColorFormat) {
> +        reinterpret_cast<uint32_t*>(destPixel)[0] = reinterpret_cast<const uint32_t*>(srcPixel)[0];
> +        return;
> +    }
> +
> +    // Swap pixel channels BGRA <-> RGBA.
> +    destPixel[0] = srcPixel[2];
> +    destPixel[1] = srcPixel[1];
> +    destPixel[2] = srcPixel[0];
> +    destPixel[3] = srcPixel[3];
> +}

Why do we need this function? Is not it the same as copyPremultipliedToPremultiplied()? You can rename copyPremultipliedToPremultiplied() to something like copyPixelToPixel() for example and use it for premultiplied and unpremultiplied copy.

> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:216
> +        if (srcAlphaFormat == AlphaPremultiplication::Premultiplied)
> +            copyImagePixelsUnaccelerated<copyPremultipliedToPremultiplied>(srcColorFormat, srcBytesPerRow, srcRows, destColorFormat, destBytesPerRow, destRows, size);
> +        else
> +            copyImagePixelsUnaccelerated<copyUnpremultipliedToUnpremultiplied>(srcColorFormat, srcBytesPerRow, srcRows, destColorFormat, destBytesPerRow, destRows, size);

No need for this change, I think.

> Source/WebCore/platform/graphics/ImageBufferBackend.h:101
> +    // destFormat=AlphaPremultiplication::Premultiplied is the code
> +    // path supported by most of the browser's rendering paths.
> +    // destFormat=AlphaPremultiplication::Unpremultiplied is supported
> +    // primarily for creating ImageBitmaps to be uploaded to WebGL
> +    // textures.

Ditto.

> Source/WebCore/platform/graphics/ImageBufferBackend.h:138
> +    // destFormat=AlphaPremultiplication::Premultiplied is the code
> +    // path supported by most of the browser's rendering paths.
> +    // destFormat=AlphaPremultiplication::Unpremultiplied is supported
> +    // primarily for creating ImageBitmaps to be uploaded to WebGL
> +    // textures.

Ditto.

[Said Abou-Hallawa]

Comment on attachment 406103 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:191
>> +    destPixel[3] = srcPixel[3];
> 
> Could you put a FIXME in here mentioning that we could maybe use Accelerate.framework on large Buffers to do this premultiply + swizzle step faster?

This is already done for CG, see ImageBufferCGBackend::copyImagePixels(). If we need to swizzle the colors we end up calling this code in copyImagePixelsAccelerated():

    if (srcAlphaFormat == destAlphaFormat) {
        ASSERT(srcColorFormat != destColorFormat);
        ASSERT(destAlphaFormat == AlphaPremultiplication::Premultiplied);

        // Swap pixel channels BGRA <-> RGBA.
        const uint8_t map[4] = { 2, 1, 0, 3 };
        vImagePermuteChannels_ARGB8888(&src, &dest, map, kvImageNoFlags);
        return;
    }

So I think we need to remove ASSERT(destAlphaFormat == AlphaPremultiplication::Premultiplied) from the code above.

[Kenneth Russell]

Created attachment 406111 [details]
Failed approach adding alpha mode to ImageBuffer constructor

This is an alternative approach which was attempted, in which the alpha mode (premultiplied or not) was passed to ImageBuffer's constructor, and removed from ImageBitmap. It didn't work for the because not all of the ImageBuffer backends - including, it seems, the Core Graphics backend - perform operations correctly such as premultiplying alpha when drawing a non-premultiplied image into a premultiplied one.

Handling premultiplied and non-premultiplied sources matters most to the WebGL API, so the handling was left at the ImageBitmap level and in WebGL's tex{Sub}Image2D APIs.

[Kenneth Russell]

Comment on attachment 406103 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageBuffer.h:45
>>      };
> 
> I know this struct was added in r265208. But why was it added to ImageBuffer? I can see it is only used by ImageBitmap and OffscreenCanvas. Please move it a header file where it is used. Or create a new one if needed.

The struct was added here because it's paired with ImageBuffers when they're serialized or transferred. It's not possible to put it in ImageBitmap.h because doing so would cause a layering violation in code which uses it (SerializedScriptValue.cpp specifically). The most that can be done is to move it into another header in platform/graphics, but I feel that keeping it inside ImageBuffer is the simplest way to express its inherent tie to ImageBuffer. If you feel strongly about this please file a follow-on bug. To reduce the chances of build system breakage by adding a new header I'm leaving it here for this patch.

>> Source/WebCore/platform/graphics/ImageBuffer.h:108
>> +    // textures.
> 
> Please remove this comment. It does not add much value.

OK, removed.

>>> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:191
>>> +    destPixel[3] = srcPixel[3];
>> 
>> Could you put a FIXME in here mentioning that we could maybe use Accelerate.framework on large Buffers to do this premultiply + swizzle step faster?
> 
> This is already done for CG, see ImageBufferCGBackend::copyImagePixels(). If we need to swizzle the colors we end up calling this code in copyImagePixelsAccelerated():
> 
>     if (srcAlphaFormat == destAlphaFormat) {
>         ASSERT(srcColorFormat != destColorFormat);
>         ASSERT(destAlphaFormat == AlphaPremultiplication::Premultiplied);
> 
>         // Swap pixel channels BGRA <-> RGBA.
>         const uint8_t map[4] = { 2, 1, 0, 3 };
>         vImagePermuteChannels_ARGB8888(&src, &dest, map, kvImageNoFlags);
>         return;
>     }
> 
> So I think we need to remove ASSERT(destAlphaFormat == AlphaPremultiplication::Premultiplied) from the code above.

That ASSERT has been necessarily removed in this patch; see ImageBufferCGBackend.cpp, below. The assert was triggered by several WebGL conformance tests.

>> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:192
>> +}
> 
> Why do we need this function? Is not it the same as copyPremultipliedToPremultiplied()? You can rename copyPremultipliedToPremultiplied() to something like copyPixelToPixel() for example and use it for premultiplied and unpremultiplied copy.

copyPremultipliedToPremultiplied special-cases alpha=0, which is incorrect for the unpremultiplied path. Handling the unpremultiplied case separately makes things clearer and avoids an extra if-test in the inner loop.

>> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:216
>> +            copyImagePixelsUnaccelerated<copyUnpremultipliedToUnpremultiplied>(srcColorFormat, srcBytesPerRow, srcRows, destColorFormat, destBytesPerRow, destRows, size);
> 
> No need for this change, I think.

It is necessary; please see comment above.

>> Source/WebCore/platform/graphics/ImageBufferBackend.h:101
>> +    // textures.
> 
> Ditto.

Removed.

>> Source/WebCore/platform/graphics/ImageBufferBackend.h:138
>> +    // textures.
> 
> Ditto.

Removed.

[Kenneth Russell]

Created attachment 406139 [details]
Patch

[Kenneth Russell]

Comment on attachment 406139 [details]
Patch

CQ'ing this version based on Dean's review and having addressed the code review feedback.

[EWS]

Committed r265360: <https://trac.webkit.org/changeset/265360>

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

[Radar WebKit Bug Importer]

<rdar://problem/66656004>

[Said Abou-Hallawa]

Comment on attachment 406103 [details]
Patch

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

`

>>> Source/WebCore/platform/graphics/ImageBuffer.h:45
>>>      };
>> 
>> I know this struct was added in r265208. But why was it added to ImageBuffer? I can see it is only used by ImageBitmap and OffscreenCanvas. Please move it a header file where it is used. Or create a new one if needed.
> 
> The struct was added here because it's paired with ImageBuffers when they're serialized or transferred. It's not possible to put it in ImageBitmap.h because doing so would cause a layering violation in code which uses it (SerializedScriptValue.cpp specifically). The most that can be done is to move it into another header in platform/graphics, but I feel that keeping it inside ImageBuffer is the simplest way to express its inherent tie to ImageBuffer. If you feel strongly about this please file a follow-on bug. To reduce the chances of build system breakage by adding a new header I'm leaving it here for this patch.

So put SerializationState in SerializedScriptValue.h. I really do not understand why this struct is put in this header file and inside the class ImageBuffer although it does not use it.

>>> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:192
>>> +}
>> 
>> Why do we need this function? Is not it the same as copyPremultipliedToPremultiplied()? You can rename copyPremultipliedToPremultiplied() to something like copyPixelToPixel() for example and use it for premultiplied and unpremultiplied copy.
> 
> copyPremultipliedToPremultiplied special-cases alpha=0, which is incorrect for the unpremultiplied path. Handling the unpremultiplied case separately makes things clearer and avoids an extra if-test in the inner loop.

So you could do the following:

static inline void copyPixelToPixel(ColorFormat srcColorFormat, const uint8_t* srcPixel, ColorFormat destColorFormat, uint8_t* destPixel)
{
...
}

static inline void copyPremultipliedToPremultiplied(ColorFormat srcColorFormat, const uint8_t* srcPixel, ColorFormat destColorFormat, uint8_t* destPixel)
{
    uint8_t alpha = srcPixel[3];
    if (!alpha) {
        reinterpret_cast<uint32_t*>(destPixel)[0] = 0;
        return;
    }

    copyPixelToPixel(srcColorFormat, srcPixel, destColorFormat, destPixel);
}

static inline void copyUnpremultipliedToUnpremultiplied(ColorFormat srcColorFormat, const uint8_t* srcPixel, ColorFormat destColorFormat, uint8_t* destPixel)
{
    copyPixelToPixel(srcColorFormat, srcPixel, destColorFormat, destPixel);
}

So we do not repeat the code.

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:212
> +        // The destination alpha format can be unpremultiplied in the
> +        // case of an ImageBitmap created from an ImageData with
> +        // premultiplyAlpha=="none".

No need for this comment as well. When we assert that something is true, we are saying what this function is expecting. We do not have to say why things are allowed is allowed in a certain path.

[Said Abou-Hallawa]

Comment on attachment 406103 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/ImageBuffer.h:45
>>>>      };
>>> 
>>> I know this struct was added in r265208. But why was it added to ImageBuffer? I can see it is only used by ImageBitmap and OffscreenCanvas. Please move it a header file where it is used. Or create a new one if needed.
>> 
>> The struct was added here because it's paired with ImageBuffers when they're serialized or transferred. It's not possible to put it in ImageBitmap.h because doing so would cause a layering violation in code which uses it (SerializedScriptValue.cpp specifically). The most that can be done is to move it into another header in platform/graphics, but I feel that keeping it inside ImageBuffer is the simplest way to express its inherent tie to ImageBuffer. If you feel strongly about this please file a follow-on bug. To reduce the chances of build system breakage by adding a new header I'm leaving it here for this patch.
> 
> So put SerializationState in SerializedScriptValue.h. I really do not understand why this struct is put in this header file and inside the class ImageBuffer although it does not use it.

I talked to Ken on the WebKit slack channel and he explained to me why it is difficult to move this struct from ImageBuffer.h. I think it is okay to leave it as is for now.