Bug 225841

Created attachment 428728 [details] Patch

Going to leave out updating ImageData serialized script value updating from this one, as that is a whole todo test wise.

Created attachment 428735 [details] Patch

Windows failure in EWS looks real, related to P3 support.

(In reply to Darin Adler from comment #7) > Windows failure in EWS looks real, related to P3 support. Yep, I forgot to add a setting to enable the new canvas color space setting for windows (it is the one that is still not generated) to fix the objectstore-autoincrement-types.html test and also need to skip the P3 test on windows as I am for the glib platforms.

Do you want me to review before you upload the patch that deals with the Windows part?

Created attachment 428751 [details] Patch

(In reply to Darin Adler from comment #9) > Do you want me to review before you upload the patch that deals with the > Windows part? Should be dealt with now (though we will see). It should be good to review now.

Comment on attachment 428751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428751&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3148 > + auto buffer = ImageBitmap::createImageBuffer(*scriptExecutionContextFromExecState(m_lexicalGlobalObject), logicalSize, RenderingMode::Unaccelerated, resolutionScale); This function is still named scriptExecutionContextFromExecState! No longer seems quite such a good name given we don’t have something named ExecState any more. > Source/WebCore/html/ImageData.cpp:50 > + if (settings && settings->colorSpace) > + return *settings->colorSpace; > + return defaultColorSpace; We don’t have valueSquaredOr, more’s the pity. > Source/WebCore/html/ImageData.cpp:57 > + auto colorSpace = toPredefinedColorSpace(pixelBuffer.format().colorSpace); > + RELEASE_ASSERT(colorSpace); > + return adoptRef(*new ImageData(pixelBuffer.size(), pixelBuffer.takeData(), *colorSpace)); The Optional::operator*() function already includes a RELEASE_ASSERT, so we don’t need to add one. (I wonder if that will be an impediment to our moving from WTF::Optional to std::optional.) > Source/WebCore/html/ImageData.cpp:96 > + // FIXME: Does this need to be a "real" out of memory error with setOutOfMemoryError called on it? How much longer are we going to move this comment around from file to file without asking around and fixing it? > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2158 > + // FIXME: Add support for initializing the ImageData when settings.alpha is false, requiring > + // every 4th byte to be 0xFF. This seems a peculiar FIXME. It’s about a problem that will exist once we add a feature, and the comment is only needed if we don’t test the feature! > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2171 > + if (newImageData.hasException()) > + return newImageData.releaseException(); > + > + initializeEmptyImageData(newImageData.returnValue()); > + > + return newImageData; Can we write this instead? if (!newImageData.hasException()) initializeEmptyImageData(newImageData.returnValue()); return newImageData; > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2185 > + auto imageData = ImageData::createUninitialized(std::abs(sw), std::abs(sh), m_settings.colorSpace, settings); > + if (imageData.hasException()) > + return imageData.releaseException(); > + > + initializeEmptyImageData(imageData.returnValue()); > + > + return imageData; Ditto. > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2218 > + auto imageData = ImageData::createUninitialized(imageDataRect.width(), imageDataRect.height(), m_settings.colorSpace, settings); > + if (imageData.hasException()) > + return imageData.releaseException(); > + > + initializeEmptyImageData(imageData.returnValue()); > + > + return imageData; Ditto. > Source/WebCore/html/canvas/PredefinedColorSpace.h:29 > +#include <wtf/Optional.h> I think Forward.h is sufficient. Maybe trouble if some automatically generated code tries to use the return value of toPredefinedColorSpace, but otherwise should not be a problem. > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:120 > + // FIXME: Add support for non 8-bit pixel formats. > + ASSERT(destinationFormat.pixelFormat == PixelFormat::RGBA8 || destinationFormat.pixelFormat == PixelFormat::BGRA8); What makes it safe to assert here without returning nullopt? > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:157 > + // FIXME: Add support for non 8-bit pixel formats. I’d write non-8-bit. But also given how this is written it’s supporting only two specific formats. The comment implies these are the only two 8-bit ones, which might be accurate now, but will it be forever? > Source/WebCore/platform/graphics/PixelBuffer.cpp:72 > + // NOTE: Only 8-bit formats are currently supported. > + ASSERT(format.pixelFormat == PixelFormat::RGBA8 || format.pixelFormat == PixelFormat::BGRA8); I wish there was a way that all these assertions shared a single comment, and a single "list of 8-bit formats". Also, I am not clear exactly what level protects us and makes this safe to assert rather than runtime check and runtime fail (as I asked above). > Source/WebCore/testing/Internals.cpp:6206 > + auto imageData = ImageData::create(static_cast<unsigned>(image->width()), static_cast<unsigned>(image->height()), { { PredefinedColorSpace::SRGB } }); Better to use static_cast than function-call-style cast, but are these type casts both needed and safe?

Thanks for the review! (In reply to Darin Adler from comment #12) > Comment on attachment 428751 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428751&action=review > > > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3148 > > + auto buffer = ImageBitmap::createImageBuffer(*scriptExecutionContextFromExecState(m_lexicalGlobalObject), logicalSize, RenderingMode::Unaccelerated, resolutionScale); > > This function is still named scriptExecutionContextFromExecState! No longer > seems quite such a good name given we don’t have something named ExecState > any more. So much ExecState baggage still exists! Most just need to be renamed GlobalObject, but its not 100% don't believe so I have never done the "replace all". > > > Source/WebCore/html/ImageData.cpp:50 > > + if (settings && settings->colorSpace) > > + return *settings->colorSpace; > > + return defaultColorSpace; > > We don’t have valueSquaredOr, more’s the pity. One day perhaps we will work in a language with optional-chaining and the world will be our oyster. > > > Source/WebCore/html/ImageData.cpp:57 > > + auto colorSpace = toPredefinedColorSpace(pixelBuffer.format().colorSpace); > > + RELEASE_ASSERT(colorSpace); > > + return adoptRef(*new ImageData(pixelBuffer.size(), pixelBuffer.takeData(), *colorSpace)); > > The Optional::operator*() function already includes a RELEASE_ASSERT, so we > don’t need to add one. (I wonder if that will be an impediment to our moving > from WTF::Optional to std::optional.) I kind of feel like this is too sneaky now and I want to just make this create return a RefPtr<> instead. > > > Source/WebCore/html/ImageData.cpp:96 > > + // FIXME: Does this need to be a "real" out of memory error with setOutOfMemoryError called on it? > > How much longer are we going to move this comment around from file to file > without asking around and fixing it? Hm, like two-to-three more refactors. > > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2158 > > + // FIXME: Add support for initializing the ImageData when settings.alpha is false, requiring > > + // every 4th byte to be 0xFF. > > This seems a peculiar FIXME. It’s about a problem that will exist once we > add a feature, and the comment is only needed if we don’t test the feature! This is what happens when I write a much bigger patch and then break it up. Going to just remove the FIXME since it is really only for myself. > > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2171 > > + if (newImageData.hasException()) > > + return newImageData.releaseException(); > > + > > + initializeEmptyImageData(newImageData.returnValue()); > > + > > + return newImageData; > > Can we write this instead? > > if (!newImageData.hasException()) > initializeEmptyImageData(newImageData.returnValue()); > return newImageData; Yep. Will fix. > > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2185 > > + auto imageData = ImageData::createUninitialized(std::abs(sw), std::abs(sh), m_settings.colorSpace, settings); > > + if (imageData.hasException()) > > + return imageData.releaseException(); > > + > > + initializeEmptyImageData(imageData.returnValue()); > > + > > + return imageData; > > Ditto. Yep. Will fix. > > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2218 > > + auto imageData = ImageData::createUninitialized(imageDataRect.width(), imageDataRect.height(), m_settings.colorSpace, settings); > > + if (imageData.hasException()) > > + return imageData.releaseException(); > > + > > + initializeEmptyImageData(imageData.returnValue()); > > + > > + return imageData; > > Ditto. Yep. Will fix. > > > Source/WebCore/html/canvas/PredefinedColorSpace.h:29 > > +#include <wtf/Optional.h> > > I think Forward.h is sufficient. Maybe trouble if some automatically > generated code tries to use the return value of toPredefinedColorSpace, but > otherwise should not be a problem. Will fix. > > > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:120 > > + // FIXME: Add support for non 8-bit pixel formats. > > + ASSERT(destinationFormat.pixelFormat == PixelFormat::RGBA8 || destinationFormat.pixelFormat == PixelFormat::BGRA8); > > What makes it safe to assert here without returning nullopt? Nothing. In practice, we currently only ever request PixelFormat::RGBA8 at the moment, but that is not something to hang my hat on. The other two pixel formats, PixelFormat::RGB10 and PixelFormat::RGB10A8, are only used to create ImageBuffers currently, and if we ever add support for something like "deep buffer" canvas (e.g. non-8-bits per component), it is likely we will follow CG convention and use half-float per-component read back rather than doing anything with the packed 10-bit per-component buffers themselves. I'm going to try to do better here by investigating differentiating between ImageBuffer backing memory and pixel buffer representation, though I imagine it will take some iteration to get right. > > > Source/WebCore/platform/graphics/ImageBufferBackend.cpp:157 > > + // FIXME: Add support for non 8-bit pixel formats. > > I’d write non-8-bit. But also given how this is written it’s supporting only > two specific formats. The comment implies these are the only two 8-bit ones, > which might be accurate now, but will it be forever? Good point. > > > Source/WebCore/platform/graphics/PixelBuffer.cpp:72 > > + // NOTE: Only 8-bit formats are currently supported. > > + ASSERT(format.pixelFormat == PixelFormat::RGBA8 || format.pixelFormat == PixelFormat::BGRA8); > > I wish there was a way that all these assertions shared a single comment, > and a single "list of 8-bit formats". > > Also, I am not clear exactly what level protects us and makes this safe to > assert rather than runtime check and runtime fail (as I asked above). I'll add a single choke point for these asserts and continue to refine this. > > > Source/WebCore/testing/Internals.cpp:6206 > > + auto imageData = ImageData::create(static_cast<unsigned>(image->width()), static_cast<unsigned>(image->height()), { { PredefinedColorSpace::SRGB } }); > > Better to use static_cast than function-call-style cast, but are these type > casts both needed and safe? I'll try to remove them and see. They are safe, though for large enough floats (which is what image->width() and image->height() return) will return an exception, which the code handles.

Comment on attachment 428751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428751&action=review >>> Source/WebCore/testing/Internals.cpp:6206 >>> + auto imageData = ImageData::create(static_cast<unsigned>(image->width()), static_cast<unsigned>(image->height()), { { PredefinedColorSpace::SRGB } }); >> >> Better to use static_cast than function-call-style cast, but are these type casts both needed and safe? > > I'll try to remove them and see. They are safe, though for large enough floats (which is what image->width() and image->height() return) will return an exception, which the code handles. I think you are saying that out of range floating point values clamp when converting to an integer, which I wasn’t sure of. If so, then clamping negative values to 0 and positive ones to huge sizes probably works fine. Seems like implicit conversion and a cast probably do the same thing.

Created attachment 428766 [details] Patch

Created attachment 428772 [details] Patch

Created attachment 428787 [details] Patch

Committed r277569 (237797@main): <https://commits.webkit.org/237797@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428787 [details].

<rdar://problem/78078911>

r277569 introduced new assertion failures for GTK, WPE and WinCairo ports. Filed: Bug 225907 – ASSERTION FAILED: m_imageBufferResult->colorSpace() == m_resultColorSpace in FilterEffect::copyPremultipliedResult