[webkit-reviews] review denied: [Bug 213673] CoreImage Implementation of SourceGraphic and saturate, hue-rotate, grayscale and sepia : [Attachment 407508] 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 213673: CoreImage Implementation of SourceGraphic and saturate, hue-rotate,
grayscale and sepia
https://bugs.webkit.org/show_bug.cgi?id=213673

Attachment 407508: Patch

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




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

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

> Source/WebCore/ChangeLog:11
> +	   Tests: css3/filters/effect-grayscale-square.html
> +		  css3/filters/effect-hue-rotate-square.html
> +		  css3/filters/effect-saturate-square.html
> +		  css3/filters/effect-sepia-square.html

Since you're adding caching of CIFilters here, you need
1. a test that renders twice, changing the value of an input in between
2. a test that renders twice, changing the filter chain
3. Maybe more.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
> +    CIImage *feSourceGraphicImage(SourceGraphic&);

imageForSourceGraphic

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:62
> +    CIImage *feColorMatrixImage(FEColorMatrix&, const Vector<CIImage *>&,
int);

imageForFEMatrixFilter. Name the non-obvious parameters.

I think it would nicer to explicitly ref-count these and return
RetainPtr<CIImage>

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:65
> +    HashMap<int, Vector<RetainPtr<CIFilter>>> m_ciFilterStorageMap;

What is the key? Why did you have to change this?

I think we can also improve the name of m_ciFilterStorageMap. It's clear from
the type that it's a map, and of course it's storage. The name should describe
how it maps. fooToFooMap.

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:57
> +CIContext *FilterEffectRendererCoreImage::getCIContext()
> +{
> +    static NeverDestroyed<RetainPtr<CIContext>> ciContext;
> +    if (!ciContext.get())
> +	   ciContext.get() = adoptNS([CIContext contextWithOptions: @{
kCIContextWorkingColorSpace: (__bridge
id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}]);
> +    return [ciContext.get() retain];
> +}

I don't think we can use a single shared CIContext. We risk state leaking
between documents, which might lead to things like timing attacks. Maybe cache
one per Document (e.g. via RenderLayerCompositor).

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:67
> +	   switch (static_cast<FEColorMatrix&>(effect).type()) {

You should add SPECIALIZE_TYPE_TRAITS for FilterEffect subclasses to avoid this
static_cast. This should be a downcast<>.

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:109
> +CIImage *FilterEffectRendererCoreImage::connectCIFilters(FilterEffect&
effect, int traversalOrder)

Order or index?

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:172
> +CIImage *FilterEffectRendererCoreImage::feColorMatrixImage(FEColorMatrix&
effect, const Vector<CIImage *>& inputImages, int traversalOrder)

This is both creating the filter, and setting the inputImage. Might be clearer
to split those two responsibilities.

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:204
> +	   Vector<RetainPtr<CIFilter>> filtersToBeCached;
> +	   filtersToBeCached.insert(0, colorMatrixFilter);
> +	   m_ciFilterStorageMap.add(traversalOrder, filtersToBeCached);

There's a way to do this with map.ensure().

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:271
> +    CIRenderDestination* destination = [[[CIRenderDestination alloc]
initWithIOSurface:(::IOSurface*)surface.surface()] autorelease];

Use RetainPtr<> and avoid autorelease.

>
Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:297
> +    , m_ciFilterStorageMap()

Not necessary.