[webkit-reviews] review denied: [Bug 213672] Infrastructure Work for Integrating CoreImage for Accelerated CSS/SVG Filter Rendering : [Attachment 404011] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied guowei_yang at apple.com's
request for review:
Bug 213672: Infrastructure Work for Integrating CoreImage for Accelerated
CSS/SVG Filter Rendering
https://bugs.webkit.org/show_bug.cgi?id=213672

Attachment 404011: Patch

https://bugs.webkit.org/attachment.cgi?id=404011&action=review




--- Comment #19 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 404011
  --> https://bugs.webkit.org/attachment.cgi?id=404011
Patch

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

I think the main thing you need to think about is the CSSFilter stuff.

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:54
> +    ImageBufferBackend* backend() override { return m_backend.get(); }

This is breaking ImageBuffer encapsulation. I think it would be OK to allow
someone to request an IOSurface from an ImageBuffer directly, though. Just
don't expose the "backend".

> Source/WebCore/platform/graphics/ImageBuffer.cpp:134
> -
> +    

Whitespace.

> Source/WebCore/platform/graphics/coreimage/AcceleratedImage.h:33
> +class AcceleratedImage final {

This name is too generic. "accelerated" can mean lots of different things.

> Source/WebCore/platform/graphics/coreimage/AcceleratedImageContext.h:37
> +class AcceleratedImageContext final {

Likewise.

> Source/WebCore/platform/graphics/coreimage/AcceleratedImageContext.mm:48
> +    backend->context().fillRect(FloatRect(1, 1, 0, 0));

This weird thing requires a comment. Is it really necessary?

> Source/WebCore/platform/graphics/coreimage/AcceleratedImageContext.mm:55
> +    CGColorSpaceRef colorSpace =
CGColorSpaceCreateWithName(kCGColorSpaceSRGB);

This leaks colorSpace.

> Source/WebCore/platform/graphics/coreimage/CoreImageRenderingMode.h:32
> +    DISABLED,
> +    ENABLED

We don't use uppercase enum values. Does this really need its own header file?

> Source/WebCore/platform/graphics/filters/FilterEffect.h:84
>	   return m_imageBufferResult
> +#if USE(CORE_IMAGE)
> +	       || m_acceleratedImage
> +#endif
>	       || m_unmultipliedImageResult
>	       || m_premultipliedImageResult;

This is now adding to the horrible complexity that is the "FilterEffect result
buffers" problem.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:164
> +#if USE(CORE_IMAGE)
> +    std::unique_ptr<AcceleratedImage> m_acceleratedImage;
> +#endif

Move this below the methods.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:192
> +    virtual void platformApplyCoreImageAccelerated() {
platformApplySoftware(); }

Seems wrong?

> Source/WebCore/rendering/CSSFilter.cpp:425
> +std::unique_ptr<ImageBuffer>
CSSFilter::createIOSurfaceBackedImageBufferResult()
> +{
> +    auto lastEffect = m_effects.last();
> +    FloatSize clampedSize =
ImageBuffer::clampedSize(lastEffect->absolutePaintRect().size());
> +    return ImageBuffer::create(clampedSize, RenderingMode::Accelerated,
lastEffect->filter().filterScale(), lastEffect->resultColorSpace());
> +}

Who is going to call this? The output of the filter chain is already an
ImageBuffer, so I don't see why we need this.

> Source/WebCore/rendering/CSSFilter.cpp:447
> +void CSSFilter::applyAccelerated()
> +{
> +    auto& effect = m_effects.last().get();
> +    effect.applyAccelerated();
> +    effect.transformResultColorSpace(ColorSpace::SRGB);
> +}
> +#endif // USE(CORE_IMAGE) && HAVE(IOSURFACE)

Why isn't this just inside CSSFilter::apply()?

> Source/WebCore/rendering/CSSFilter.h:72
> +#if HAVE(IOSURFACE) && USE(CORE_IMAGE)
> +    void applyAccelerated();
> +    std::unique_ptr<ImageBuffer> createIOSurfaceBackedImageBufferResult();
> +    AcceleratedImage* acceleratedImageOutput() const;
> +    FloatRect coreImageDestRect();
> +#endif

Do these all need to be public?

> Source/WebCore/rendering/RenderLayerFilters.cpp:131
> +	   // If the CoreImage-Accelerated Filter Rendering feature switch is
turned on
> +	   // we will enable CoreImage rendering on Filters

Comment is not necessary.

> Source/WebCore/rendering/RenderLayerFilters.cpp:203
> +    // Applying filters will have CoreImage specific paths
> +#if USE(CORE_IMAGE)
> +    if (filter.coreImageRenderingMode() == CoreImageRenderingMode::ENABLED)
> +	   LOG_WITH_STREAM(Filters, stream << "Rendering Filters Using
CoreImage");
> +#endif

This doesn't seem necessary. Log lower down.