[webkit-reviews] review denied: [Bug 213672] Infrastructure Work for Integrating CoreImage for Accelerated CSS/SVG Filter Rendering : [Attachment 404618] 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 404618: Patch

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




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

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

Let's do one more round. Would it be hard to get a single filter working here,
so we can land this with some tests?

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:31
> +#if USE(CORE_IMAGE)
> +
> +#import "config.h"
> +#import "FilterEffectRendererCoreImage.h"
> +
> +#import "Filter.h"

The order here should be:

#import "config.h"
#import "FilterEffectRendererCoreImage.h"

#if USE(CORE_IMAGE)

#import "Filter.h
...

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:59
> +    Vector<CIImage*> inputImages;

What's the ownership model for the CIImage*? Who hold the ref to them?

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:119
> +    buffer->flushContext();

Why do you need to flushContext right after creating it?

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:126
> +    m_outputImageBuffer = createAcceleratedImageBuffer(clampedSize,
lastEffect.filter().filterScale(), lastEffect.resultColorSpace());

createAcceleratedImageBuffer() is basically one line, so maybe just move its
contents to here.

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:134
> +    auto surface =
static_cast<AcceleratedImageBuffer&>(*m_outputImageBuffer).surface();

You shouldn't use static_cast here. You need to make it possible to ask "is
this an accelerated image buffer", and use is<> and downcast<>.

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:161
> +    CGColorSpaceRef colorSpace =
cachedCGColorSpace(lastEffect.resultColorSpace());
> +    m_context = [CIContext contextWithOptions: @{
kCIContextWorkingColorSpace: (__bridge id)colorSpace}];

It doesn't seem right to assume that the working colorspace is the result
colorspace of the last effect.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:128
> +    virtual Type filterEffectClassType() const = 0;

Rather than all the virtual functions, I'd do this by storing Type as a const
member var (see FilterOperation for an example).