Make sure to always reset the cached filter in ReferenceFilterOperation

third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 130 matching lines @@
   // Create a parent filter for shorthand filters. These have already been
   // scaled by the CSS code for page zoom, so scale is 1.0 here.
   Filter* parentFilter = Filter::create(1.0f);
   FilterEffect* previousEffect = parentFilter->getSourceGraphic();
   for (FilterOperation* filterOperation : operations.operations()) {
     FilterEffect* effect = nullptr;
     switch (filterOperation->type()) {
       case FilterOperation::REFERENCE: {
         ReferenceFilterOperation& referenceOperation =
             toReferenceFilterOperation(*filterOperation);
-        if (Filter* referenceFilter =
-                buildReferenceFilter(referenceOperation, previousEffect)) {
+        Filter* referenceFilter =
+            buildReferenceFilter(referenceOperation, previousEffect);
+        if (referenceFilter) {
+          effect = referenceFilter->lastEffect();

[Stephen White, 2016/10/27 17:10:51]

I'm not sure I understand why we now grab the last effect here, from the
first-generated reference filter, when the one we're going to use in the
operation is the one we generate below. (Then again, I don't totally understand
why we generate it twice, either.)

[fs, 2016/10/28 08:46:31]

That's what we did previously as well (I only reordered the to lines - modulo
the comment), only the first call to buildReferenceFilter takes the previous
effect as an argument, while the second takes null. The second call is there to
please PaintLayer::mapRectForFilter.

[Stephen White, 2016/10/28 14:58:06]

Oh right; I misread.
I understand what, I'm just not clear on why. Different SourceGraphic 
so that mapRectForFilter doesn't traverse into the non-reference-filter 
inputs? If that's the case, another option here might be to defer 
connecting reference filters to the rest of the filter chain until 
we convert it to SkImageFilters. IIRC, that's what the accelerated 
path does, using SkComposeImageFilter to hook them up. Then we'd only 
have to build the reference filter once. The downside is that there 
are some optimizations that Skia can't apply across an 
SkComposeImageFilter, such as collapsing matrices. But I don't 
imagine that's a very common case (reference filter plus shorthand 
filters). Anyway all of that is unrelated to this patch.

[fs, 2016/10/31 09:19:12]

I'd expect something along those lines to be the reason at least.
That might be nice. I'll try to keep that in mind to have a look at a solution
like that.

           // TODO(fs): This is essentially only needed for the
           // side-effects (mapRect). The filter differs from the one
           // computed just above in what the SourceGraphic is, and how
           // it's connected to the filter-chain.
-          referenceOperation.setFilter(
-              buildReferenceFilter(referenceOperation, nullptr));
-          effect = referenceFilter->lastEffect();
+          referenceFilter = buildReferenceFilter(referenceOperation, nullptr);
         }
+        referenceOperation.setFilter(referenceFilter);
         break;
       }
       case FilterOperation::GRAYSCALE: {
         Vector<float> inputParameters = grayscaleMatrix(
             toBasicColorMatrixFilterOperation(filterOperation)->amount());
         effect = FEColorMatrix::create(parentFilter, FECOLORMATRIX_TYPE_MATRIX,
                                        inputParameters);
         break;
       }
       case FilterOperation::SEPIA: {
@@ skipping 130 matching lines @@
 
   CompositorFilterOperations filters;
   for (FilterOperation* op : operations.operations()) {
     switch (op->type()) {
       case FilterOperation::REFERENCE: {
         ReferenceFilterOperation& referenceOperation =
             toReferenceFilterOperation(*op);
         Filter* referenceFilter =
             buildReferenceFilter(referenceOperation, nullptr);
         if (referenceFilter && referenceFilter->lastEffect()) {
-          referenceOperation.setFilter(referenceFilter);
           SkiaImageFilterBuilder::populateSourceGraphicImageFilters(
               referenceFilter->getSourceGraphic(), nullptr, currentColorSpace);
 
           FilterEffect* filterEffect = referenceFilter->lastEffect();
           currentColorSpace = filterEffect->operatingColorSpace();
           filters.appendReferenceFilter(
               SkiaImageFilterBuilder::build(filterEffect, currentColorSpace));
         }
+        referenceOperation.setFilter(referenceFilter);
         break;
       }
       case FilterOperation::GRAYSCALE:
       case FilterOperation::SEPIA:
       case FilterOperation::SATURATE:
       case FilterOperation::HUE_ROTATE: {
         float amount = toBasicColorMatrixFilterOperation(*op).amount();
         switch (op->type()) {
           case FilterOperation::GRAYSCALE:
             filters.appendGrayscaleFilter(amount);
@@ skipping 104 matching lines @@
       Filter::create(m_referenceBox, filterRegion, m_zoom, unitScaling);
   if (!previousEffect)
     previousEffect = result->getSourceGraphic();
   SVGFilterBuilder builder(previousEffect, nodeMap, m_fillPaint, m_strokePaint);
   builder.buildGraph(result, filterElement, m_referenceBox);
   result->setLastEffect(builder.lastEffect());
   return result;
 }
 
 }  // namespace blink