[webkit-reviews] review denied: [Bug 80323] [CSS Filters] Drop Shadow is not repainting correctly when repaint area is smaller than the filtered element : [Attachment 132088] Patch V4

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 80323: [CSS Filters] Drop Shadow is not repainting correctly when repaint
area is smaller than the filtered element
https://bugs.webkit.org/show_bug.cgi?id=80323

Attachment 132088: Patch V4
https://bugs.webkit.org/attachment.cgi?id=132088&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=132088&action=review


I think it would be possible to split this into multiple patches. For example,
the changes to computeRectForRepaint and adding calculateLayerBounds seem
independent of the changes in the filter repainting code. Similarly
beginFilterEffect and applyFilterEffect -> prepareFilterEffect is standalone.
Anyway, it's done now - just worth considering for the future.

I think one more patch with the change Stephen suggests and we're good to go.

> LayoutTests/css3/filters/filter-repaint-composited-fallback-crash.html:4
> +    NOTE: It is using the fact that Safari can draw drop-shadows in GPU only
if the filter is the last one in the filter chain.

I don't like the fact that this test requires Safari's drop-shadow behaviour.
We'll want to fix that asap and it would be a pain to break unrelated tests.

> LayoutTests/css3/filters/filter-repaint-composited-fallback.html:4
> +    NOTE: It is using the fact that Safari can draw drop-shadows in GPU only
if the filter is the last one in the filter chain.

ditto

>> Source/WebCore/rendering/FilterEffectRenderer.cpp:32
>> +#define DEBUG_FILTER_RENDERING 0
> 
> This debugging code probably shouldn't be checked in.

It is useful though. I wonder if we could extract it into DRT the same way the
layer/compositing tree is?

> Source/WebCore/rendering/FilterEffectRenderer.cpp:354
> +#ifdef DEBUG_FILTER_RENDERING
> +// In order to use this option, make sure this process has access to file
API (ie. single process).
> +static void logFiltersMessage(const String& message)
> +{

Repeating comment about moving this into a layoutTestController
property/function. Could we file a bug on that?

Or maybe it is useful to content as well? I could imagine that people would
find it handy if the Web Inspector showed each step of a filter chain
(especially for SVG filters).

>>> Source/WebCore/rendering/FilterEffectRenderer.cpp:394
>>> +	 bool needsFullImageSource = filter->needsFullImageSource();
>> 
>> Rather than requiring the full image, is there a way to determine the
minimum required source rect, at least for the blur and drop shadow?  (I can
see how it would be problematic for custom filters, since you'd need to know
what the shaders are going to do).  If this dirty rect is also used for
intermediate filters, and that intermediate filter is not pixel-moving, it
seems like you'd be doing a lot of extra processing that you won't use (e.g., a
sepia followed by a blur).
> 
> Yes, I thought about that, but I would prefer to do it in a different step.
I've added https://bugs.webkit.org/show_bug.cgi?id=81263 .

Agree with Stephen. This might be a bigger project to implement a pull model
for filter operations.

> Source/WebCore/rendering/RenderLayer.cpp:953
> +    for (const RenderLayer* curr = parent(); curr; curr = curr->parent()) {
> +	   if (curr->requiresFullLayerImageForFilters())
> +	       return const_cast<RenderLayer*>(curr);
> +    }

Link here to the bug about optimizing regions.

> Source/WebCore/rendering/RenderLayer.cpp:986
> +    m_filterRepaintRect.unite(rectForRepaint);

What happens if I reduce the outsets on a filter? Do we ever shrink the repaint
rect (obviously we need a full repaint at least once)?

> Source/WebCore/rendering/RenderLayer.cpp:4058
> +	       LayoutUnit rw = frameView->contentsWidth();
> +	       LayoutUnit rh = frameView->contentsHeight();
> +	   
> +	       boundingBoxRect.setWidth(max(boundingBoxRect.width(), rw -
boundingBoxRect.x()));
> +	       boundingBoxRect.setHeight(max(boundingBoxRect.height(), rh -
boundingBoxRect.y()));

Nit: name these vars contentsWidth or something. (I know it was copied from the
original)