From a22d21e447e41d97e32212aab0413455a818411d Mon Sep 17 00:00:00 2001
From: Tyler Denniston <tdenniston@google.com>
Date: Fri, 15 Jan 2021 12:12:35 -0500
Subject: [PATCH] [svg] Fix incorrect optimization for opacity layer

When both filter and opacity attributes are set on a leaf node, the
opacity must be applied as a separate layer so that the results of the
filter are modified by the opacity. Previously in this circumstance we
were incorrectly applying the opacity to the paints only (without a
layer).

To illustrate:

<svg viewBox="0 0 1000 500" version="1.1"
     xmlns="http://www.w3.org/2000/svg"
     xmlns:xlink="http://www.w3.org/1999/xlink">
  <defs>
    <filter id="Green" x="0%" y="0%" width="100%" height="100%">
      <feFlood flood-color="lime" flood-opacity="1" />
    </filter>
  </defs>
  <rect x="30" y="20" width="400" height="100" fill="red" opacity="0.1"
        filter="url(#Green)"/>
  <g filter="url(#Green)">
    <rect x="30" y="200" width="400" height="100" fill="red" opacity="0.1"/>
  </g>
</svg>

The two rects should render differently. In the <g> case, the filter
output (opaque green) overrides the translucent red pixels of the rect.
In the <rect> case, the filter output overrides the translucent red
pixels with opaque green, but then is modified by the opacity on the
<rect>.

Relevant W3C test is filters-blend-01-b (and possibly others).

Change-Id: I165eed36c546f1f99457865cee58ee2b3bffe6f1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354879
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Tyler Denniston <tdenniston@google.com>
---
 modules/svg/include/SkSVGRenderContext.h |  2 +-
 modules/svg/src/SkSVGRenderContext.cpp   | 19 +++++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/modules/svg/include/SkSVGRenderContext.h b/modules/svg/include/SkSVGRenderContext.h
index 9f01958b8e..6139fa0961 100644
--- a/modules/svg/include/SkSVGRenderContext.h
+++ b/modules/svg/include/SkSVGRenderContext.h
@@ -141,7 +141,7 @@ private:
     void* operator new(size_t, void*)                        = delete;
     SkSVGRenderContext& operator=(const SkSVGRenderContext&) = delete;
 
-    void applyOpacity(SkScalar opacity, uint32_t flags);
+    void applyOpacity(SkScalar opacity, uint32_t flags, bool hasFilter);
     void applyFilter(const SkSVGFuncIRI&);
     void applyClip(const SkSVGFuncIRI&);
     void applyMask(const SkSVGFuncIRI&);
diff --git a/modules/svg/src/SkSVGRenderContext.cpp b/modules/svg/src/SkSVGRenderContext.cpp
index 1939f26a1b..27193045c2 100644
--- a/modules/svg/src/SkSVGRenderContext.cpp
+++ b/modules/svg/src/SkSVGRenderContext.cpp
@@ -352,8 +352,9 @@ void SkSVGRenderContext::applyPresentationAttributes(const SkSVGPresentationAttr
 
     // Uninherited attributes.  Only apply to the current context.
 
+    const bool hasFilter = attrs.fFilter.isValue();
     if (attrs.fOpacity.isValue()) {
-        this->applyOpacity(*attrs.fOpacity, flags);
+        this->applyOpacity(*attrs.fOpacity, flags, hasFilter);
     }
 
     if (attrs.fClipPath.isValue()) {
@@ -365,7 +366,7 @@ void SkSVGRenderContext::applyPresentationAttributes(const SkSVGPresentationAttr
     }
 
     // TODO: when both a filter and opacity are present, we can apply both with a single layer
-    if (attrs.fFilter.isValue()) {
+    if (hasFilter) {
         this->applyFilter(*attrs.fFilter);
     }
 
@@ -378,7 +379,7 @@ void SkSVGRenderContext::applyPresentationAttributes(const SkSVGPresentationAttr
     // - flood-opacity
 }
 
-void SkSVGRenderContext::applyOpacity(SkScalar opacity, uint32_t flags) {
+void SkSVGRenderContext::applyOpacity(SkScalar opacity, uint32_t flags, bool hasFilter) {
     if (opacity >= 1) {
         return;
     }
@@ -386,11 +387,13 @@ void SkSVGRenderContext::applyOpacity(SkScalar opacity, uint32_t flags) {
     const bool hasFill   = SkToBool(this->fillPaint());
     const bool hasStroke = SkToBool(this->strokePaint());
 
-    // We can apply the opacity as paint alpha iif it only affects one atomic draw.
-    // For now, this means a) the target node doesn't have any descendants, and
-    // b) it only has a stroke or a fill (but not both).  Going forward, we may need
-    // to refine this heuristic (e.g. to accommodate markers).
-    if ((flags & kLeaf) && (hasFill ^ hasStroke)) {
+    // We can apply the opacity as paint alpha if it only affects one atomic draw.
+    // For now, this means all of the following must be true:
+    //   - the target node doesn't have any descendants;
+    //   - it only has a stroke or a fill (but not both);
+    //   - it does not have a filter.
+    // Going forward, we may needto refine this heuristic (e.g. to accommodate markers).
+    if ((flags & kLeaf) && (hasFill ^ hasStroke) && !hasFilter) {
         auto* pctx = fPresentationContext.writable();
         if (hasFill) {
             pctx->fFillPaint.setAlpha(