[GPU Process] Canvas image rendering can render arbitrary DOM content in the GPU process, which is against policy (for now)
Created attachment 432568 [details] WIP
<rdar://problem/76678163>
The test failure might be rdar://77527267
Created attachment 432622 [details] Patch
Comment on attachment 432622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432622&action=review > Source/WebCore/svg/graphics/SVGImage.cpp:284 > + return ImageDrawResult::DidDraw; I should probably add if (imageObserver()) imageObserver()->didDraw(*this);
Created attachment 432957 [details] Patch
Comment on attachment 432957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432957&action=review > Source/WebCore/svg/graphics/SVGImage.cpp:278 > + // Display list drawing can't handle arbitrary DOM content. > + // FIXME: Remove this when it can. > + if (!context.hasPlatformContext()) { Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how. > Source/WebCore/svg/graphics/SVGImage.cpp:281 > + if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage) Writing this as a static_cast is ugly. Let’s find a way to do without that; the static_cast makes it look like there is an unsafe conversion between enumeration types. Simplest way is to use a local variable: ImageOrientation::Orientation orientation = options.orientation(); Another way would be to add a member function to ImageOrientation to fetch the orientation value or an operator== overload to compare with a specific orientation enumeration value. Generally speaking not sure why we store the orientation enumeration in a class, ImageOrientation, with no other members. That leads to this kind of confusing code. We could instead just have a set of functions that take an ImageOrientation enumeration value.
Comment on attachment 432957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432957&action=review > Source/WebCore/svg/graphics/SVGImage.cpp:277 > + // FIXME: Remove this when it can. FIXME comments should come with bug numbers. >> Source/WebCore/svg/graphics/SVGImage.cpp:281 >> + if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage) > > Writing this as a static_cast is ugly. Let’s find a way to do without that; the static_cast makes it look like there is an unsafe conversion between enumeration types. Simplest way is to use a local variable: > > ImageOrientation::Orientation orientation = options.orientation(); > > Another way would be to add a member function to ImageOrientation to fetch the orientation value or an operator== overload to compare with a specific orientation enumeration value. Generally speaking not sure why we store the orientation enumeration in a class, ImageOrientation, with no other members. That leads to this kind of confusing code. We could instead just have a set of functions that take an ImageOrientation enumeration value. Why do we ever need to consult ImageOrientation? SVG images don't have EXIF data. > Source/WebCore/svg/graphics/SVGImage.cpp:284 > + context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions); Will this cause blurriness if a small SVG images is scaled up in the canvas?
Comment on attachment 432957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432957&action=review >> Source/WebCore/svg/graphics/SVGImage.cpp:278 >> + if (!context.hasPlatformContext()) { > > Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how. I think this can be done if we just move some of the code around: 1. Override DisplayList::Recorder::drawImage(): ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options) { InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality()); if (image.isSVGImage()) return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options); return image.draw(*this, destination, source, options); } Add SVG:: drawAsNativeImage(): ImageDrawResult SVGImage::drawAsNativeImage(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& options) { auto nativeImage = this->nativeImage(nullptr); if (nativeImage) return ImageDrawResult::DidNothing; auto localImagePaintingOptions = options; if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage) localImagePaintingOptions = ImagePaintingOptions(options, ImageOrientation::Orientation::None); context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions); if (imageObserver()) imageObserver()->didDraw(*this); return ImageDrawResult::DidDraw; } Then we can leave SVG::draw() untouched.
Comment on attachment 432957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432957&action=review >>> Source/WebCore/svg/graphics/SVGImage.cpp:278 >>> + if (!context.hasPlatformContext()) { >> >> Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how. > > I think this can be done if we just move some of the code around: > > 1. Override DisplayList::Recorder::drawImage(): > > ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options) > { > InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality()); > if (image.isSVGImage()) > return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options); > return image.draw(*this, destination, source, options); > } > > Add SVG:: drawAsNativeImage(): > > ImageDrawResult SVGImage::drawAsNativeImage(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& options) > { > auto nativeImage = this->nativeImage(nullptr); > if (nativeImage) > return ImageDrawResult::DidNothing; > > auto localImagePaintingOptions = options; > if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage) > localImagePaintingOptions = ImagePaintingOptions(options, ImageOrientation::Orientation::None); > > context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions); > if (imageObserver()) > imageObserver()->didDraw(*this); > > return ImageDrawResult::DidDraw; > } > > Then we can leave SVG::draw() untouched. Good idea! I'll do this. Pretty cool how Recorder just inherits from GraphicsContext!!! >>> Source/WebCore/svg/graphics/SVGImage.cpp:281 >>> + if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage) >> >> Writing this as a static_cast is ugly. Let’s find a way to do without that; the static_cast makes it look like there is an unsafe conversion between enumeration types. Simplest way is to use a local variable: >> >> ImageOrientation::Orientation orientation = options.orientation(); >> >> Another way would be to add a member function to ImageOrientation to fetch the orientation value or an operator== overload to compare with a specific orientation enumeration value. Generally speaking not sure why we store the orientation enumeration in a class, ImageOrientation, with no other members. That leads to this kind of confusing code. We could instead just have a set of functions that take an ImageOrientation enumeration value. > > Why do we ever need to consult ImageOrientation? SVG images don't have EXIF data. drawNativeImage() in BitmapImage.cpp says if (options.orientation().usesWidthAsHeight()) usesWidthAsHeight() says ASSERT(isValidEXIFOrientation(m_orientation)); FromImage is the only non-EXIF orientation. >> Source/WebCore/svg/graphics/SVGImage.cpp:284 >> + context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions); > > Will this cause blurriness if a small SVG images is scaled up in the canvas? Yes! Will fix.
Created attachment 433072 [details] Patch for committing
Comment on attachment 433072 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=433072&action=review > Source/WebCore/svg/graphics/SVGImage.cpp:337 > + FloatRect rectInNativeImage(FloatPoint(), destination.size()); The cool kids write this as auto rectInNativeImage = FloatRect { { }, destination.size() }; > Source/WebCore/svg/graphics/SVGImage.cpp:343 > + ImageOrientation::Orientation orientation = options.orientation(); auto?
Comment on attachment 433072 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=433072&action=review >> Source/WebCore/svg/graphics/SVGImage.cpp:343 >> + ImageOrientation::Orientation orientation = options.orientation(); > > auto? See earlier discussion with Darin.
Created attachment 433075 [details] Patch for committing
Comment on attachment 432957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432957&action=review >>>> Source/WebCore/svg/graphics/SVGImage.cpp:278 >>>> + if (!context.hasPlatformContext()) { >>> >>> Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how. >> >> I think this can be done if we just move some of the code around: >> >> 1. Override DisplayList::Recorder::drawImage(): >> >> ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options) >> { >> InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality()); >> if (image.isSVGImage()) >> return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options); >> return image.draw(*this, destination, source, options); >> } >> >> Add SVG:: drawAsNativeImage(): >> >> ImageDrawResult SVGImage::drawAsNativeImage(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& options) >> { >> auto nativeImage = this->nativeImage(nullptr); >> if (nativeImage) >> return ImageDrawResult::DidNothing; >> >> auto localImagePaintingOptions = options; >> if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage) >> localImagePaintingOptions = ImagePaintingOptions(options, ImageOrientation::Orientation::None); >> >> context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions); >> if (imageObserver()) >> imageObserver()->didDraw(*this); >> >> return ImageDrawResult::DidDraw; >> } >> >> Then we can leave SVG::draw() untouched. > > Good idea! I'll do this. > > Pretty cool how Recorder just inherits from GraphicsContext!!! Uh oh. isSVGImage() returns true on both SVGImageForContainer and SVGImage, and these are both distinct types.
Comment on attachment 432957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432957&action=review >>>>> Source/WebCore/svg/graphics/SVGImage.cpp:278 >>>>> + if (!context.hasPlatformContext()) { >>>> >>>> Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how. >>> >>> I think this can be done if we just move some of the code around: >>> >>> 1. Override DisplayList::Recorder::drawImage(): >>> >>> ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options) >>> { >>> InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality()); >>> if (image.isSVGImage()) >>> return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options); >>> return image.draw(*this, destination, source, options); >>> } >>> >>> Add SVG:: drawAsNativeImage(): >>> >>> ImageDrawResult SVGImage::drawAsNativeImage(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& options) >>> { >>> auto nativeImage = this->nativeImage(nullptr); >>> if (nativeImage) >>> return ImageDrawResult::DidNothing; >>> >>> auto localImagePaintingOptions = options; >>> if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage) >>> localImagePaintingOptions = ImagePaintingOptions(options, ImageOrientation::Orientation::None); >>> >>> context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions); >>> if (imageObserver()) >>> imageObserver()->didDraw(*this); >>> >>> return ImageDrawResult::DidDraw; >>> } >>> >>> Then we can leave SVG::draw() untouched. >> >> Good idea! I'll do this. >> >> Pretty cool how Recorder just inherits from GraphicsContext!!! > > Uh oh. isSVGImage() returns true on both SVGImageForContainer and SVGImage, and these are both distinct types. I'm starting to think this isn't worth it. This design requires: 1. Adding isSVGImageForContainer() in Image and SVGImageForContainer 2. Recorder::drawImage() becomes: ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options) { if (is<SVGImage>(image)) { // Display list drawing can't handle arbitrary DOM content. // FIXME https://bugs.webkit.org/show_bug.cgi?id=227748: Remove this when it can. InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality()); if (is<SVGImageForContainer>(image)) downcast<SVGImageForContainer>(image).drawAsNativeImage(*this, destination, source, options); else return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options); } return GraphicsContext::drawImage(image, destination, source, options); } 3. SVGImageForContainer::drawAsNativeImage() needs to be added: ImageDrawResult SVGImageForContainer::drawAsNativeImage(GraphicsContext& context, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options) { return m_image->drawAsNativeImageForContainer(context, m_containerSize, m_containerZoom, m_initialFragmentURL, dstRect, srcRect, options); } 4. SVGImage::drawAsNativeImageForContainer() needs to be added, because SVGImage::drawForContainer() calls SVGImage::draw() 5. SVGImage::drawForContainer() needs to be refactored to share code with SVGImage::drawAsNativeImageForContainer() In the end, it doesn't seem worth it to do all this just to move the FIXME from SVGImage::draw() to Recorder::drawImage().
Created attachment 433121 [details] Patch for committing
Committed r279722 (239510@main): <https://commits.webkit.org/239510@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433121 [details].
*** Bug 227824 has been marked as a duplicate of this bug. ***