ImageBuffer::putByteArray would probably work for the default case, but not if there's any resizing involved.
Taking this.
Created attachment 406103 [details] Patch
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
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?
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.
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.
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.
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.
Created attachment 406139 [details] Patch
Comment on attachment 406139 [details] Patch CQ'ing this version based on Dean's review and having addressed the code review feedback.
Committed r265360: <https://trac.webkit.org/changeset/265360> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406139 [details].
<rdar://problem/66656004>
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.
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.