drawVertices and drawPath apply blend between paint and primitive color.

Bug: skia:12662
Change-Id: Ic2924257fce3ea9a2df5e49d0ab26ad085693d30
Cq-Include-Trybots: luci.skia.skia.primary:Canary-Flutter
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/473676
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

gm/imagefiltersbase.cpp

@@ -179,7 +179,7 @@ static void draw_patch(SkCanvas* canvas, SkImage*, const SkRect& r, sk_sp<SkImag
     SkAutoCanvasRestore acr(canvas, /*doSave=*/true);
     canvas->translate(-r.fLeft, -r.fTop);
     canvas->scale(r.width() / 400.0, r.height() / 400.0);
-    canvas->drawPatch(gCubics, colors, /*texCoords=*/nullptr, SkBlendMode::kSrc, paint);
+    canvas->drawPatch(gCubics, colors, /*texCoords=*/nullptr, SkBlendMode::kDst, paint);
 }
 
 static void draw_atlas(SkCanvas* canvas, SkImage* atlas, const SkRect& r,

gm/patch.cpp

@@ -96,11 +96,12 @@ const SkPoint gTexCoords[SkPatchUtils::kNumCorners] = {
 static void dopatch(SkCanvas* canvas, const SkColor colors[], sk_sp<SkImage> img,
                     const SkMatrix* localMatrix) {
     SkPaint paint;
+    paint.setColor(SK_ColorGREEN);
 
     const SkBlendMode modes[] = {
         SkBlendMode::kSrc,
         SkBlendMode::kDst,
-        SkBlendMode::kModulate,
+        SkBlendMode::kColorDodge,
     };
 
     SkPoint texStorage[4];
@@ -189,7 +190,7 @@ DEF_SIMPLE_GM(patch_alpha_test, canvas, 550, 250) {
         0x80FF0000, 0x80FF0000, 0x80FF0000, 0x80FF0000,
     };
     SkPaint paint;
-    canvas->drawPatch(gCubics, colors, nullptr, SkBlendMode::kModulate, paint);
+    canvas->drawPatch(gCubics, colors, nullptr, SkBlendMode::kDst, paint);
 
     canvas->translate(300, 0);
 

gm/runtimecolorfilter.cpp

@@ -124,7 +124,7 @@ DEF_SIMPLE_GM(runtimecolorfilter_vertices_atlas_and_patch, canvas, 404, 404) {
                                   kPremul_SkAlphaType,
                                   canvas->imageInfo().refColorSpace());
     auto surf = SkSurface::MakeRaster(info);
-    surf->getCanvas()->drawVertices(verts, SkBlendMode::kModulate, SkPaint());
+    surf->getCanvas()->drawVertices(verts, SkBlendMode::kDst, SkPaint());
     auto atlas = surf->makeImageSnapshot();
     auto xform = SkRSXform::Make(1, 0, 0, 0);
 
@@ -158,7 +158,7 @@ DEF_SIMPLE_GM(runtimecolorfilter_vertices_atlas_and_patch, canvas, 404, 404) {
         SkAutoCanvasRestore acr(canvas, /*doSave=*/true);
         canvas->translate(x, 0);
         // Use just the shader or just the vertex colors.
-        auto mode = useShader ? SkBlendMode::kDst : SkBlendMode::kSrc;
+        auto mode = useShader ? SkBlendMode::kSrc : SkBlendMode::kDst;
         canvas->drawVertices(verts, mode, makePaint(useCF, useShader));
     };
 

gm/vertices.cpp

@@ -252,6 +252,7 @@ static void draw_batching(SkCanvas* canvas) {
                 canvas->concat(m);
                 SkPaint paint;
                 paint.setShader(useShader ? shader : nullptr);
+                paint.setColor(SK_ColorWHITE);
 
                 const SkPoint* t = useTex ? texs : nullptr;
                 auto v = SkVertices::MakeCopy(SkVertices::kTriangles_VertexMode, kMeshVertexCnt,

gn/flutter_defines.gni

@@ -14,6 +14,7 @@ flutter_defines = [
 
   # Staging
   "SK_LEGACY_INNER_JOINS",
+  "SK_LEGACY_IGNORE_DRAW_VERTICES_BLEND_WITH_NO_SHADER",
 
   # Fast low-precision software rendering isn't a priority for Flutter.
   "SK_DISABLE_LEGACY_SHADERCONTEXT",

src/core/SkBitmapDevice.cpp

@@ -529,6 +529,11 @@ void SkBitmapDevice::onDrawGlyphRunList(const SkGlyphRunList& glyphRunList, cons
 void SkBitmapDevice::drawVertices(const SkVertices* vertices,
                                   sk_sp<SkBlender> blender,
                                   const SkPaint& paint) {
+#ifdef SK_LEGACY_IGNORE_DRAW_VERTICES_BLEND_WITH_NO_SHADER
+    if (!paint.getShader()) {
+        blender = SkBlender::Mode(SkBlendMode::kDst);
+    }
+#endif
     BDDraw(this).drawVertices(vertices, std::move(blender), paint);
 }
 

src/core/SkCanvas.cpp

@@ -1812,7 +1812,6 @@ void SkCanvas::drawVertices(const SkVertices* vertices, SkBlendMode mode, const
         return;
     }
 #endif
-
     this->onDrawVerticesObject(vertices, mode, paint);
 }
 

src/core/SkDraw_vertices.cpp

@@ -20,6 +20,7 @@
 #include "src/core/SkVMBlitter.h"
 #include "src/core/SkVertState.h"
 #include "src/core/SkVerticesPriv.h"
+#include "src/shaders/SkColorShader.h"
 #include "src/shaders/SkComposeShader.h"
 #include "src/shaders/SkShaderBase.h"
 
@@ -331,15 +332,16 @@ void SkDraw::drawFixedVertices(const SkVertices* vertices,
         texCoords = nullptr;
     }
 
-    // We can simplify things for certain blend modes. This is for speed, and SkComposeShader
+    bool blenderIsDst = false;
+    // We can simplify things for certain blend modes. This is for speed, and SkShader_Blend
     // itself insists we don't pass kSrc or kDst to it.
-    if (skstd::optional<SkBlendMode> bm = as_BB(blender)->asBlendMode();
+    if (skstd::optional<SkBlendMode> bm = as_BB(blender)->asBlendMode(); bm.has_value() && colors) {
-        bm.has_value() && colors && texCoords) {
         switch (*bm) {
             case SkBlendMode::kSrc:
                 colors = nullptr;
                 break;
             case SkBlendMode::kDst:
+                blenderIsDst = true;
                 texCoords = nullptr;
                 paintShader = nullptr;
                 break;
@@ -353,26 +355,31 @@ void SkDraw::drawFixedVertices(const SkVertices* vertices,
     SkMatrix ctm = fMatrixProvider->localToDevice();
     const bool usePerspective = ctm.hasPerspective();
 
+    SkShader* shader = paintShader;
+    SkTriColorShader* triShader = nullptr;
+    SkPMColor4f* dstColors = nullptr;
+
+    if (colors) {
+        dstColors = convert_colors(colors, vertexCount, fDst.colorSpace(), outerAlloc);
+        triShader = outerAlloc->make<SkTriColorShader>(compute_is_opaque(colors, vertexCount),
+                                                       usePerspective);
+        if (blenderIsDst) {
+            shader = triShader;
+        } else {
+            if (!shader) {
+                // When there is no shader then the blender applies to the vertex colors and
+                // opaque paint color.
+                shader = outerAlloc->make<SkColor4Shader>(paint.getColor4f().makeOpaque(), nullptr);
+            }
+            shader = outerAlloc->make<SkShader_Blend>(
+                    blender, sk_ref_sp(triShader), sk_ref_sp(shader));
+        }
+    }
+
     auto rpblit = [&]() {
         VertState state(vertexCount, indices, indexCount);
         VertState::Proc vertProc = state.chooseProc(info.mode());
 
-        SkShader* shader = paintShader;
-        SkTriColorShader* triShader = nullptr;
-        SkPMColor4f* dstColors = nullptr;
-
-        if (colors) {
-            dstColors = convert_colors(colors, vertexCount, fDst.colorSpace(), outerAlloc);
-            triShader = outerAlloc->make<SkTriColorShader>(compute_is_opaque(colors, vertexCount),
-                                                           usePerspective);
-            if (shader) {
-                shader = outerAlloc->make<SkShader_Blend>(
-                        blender, sk_ref_sp(triShader), sk_ref_sp(shader));
-            } else {
-                shader = triShader;
-            }
-        }
-
         SkPaint shaderPaint(paint);
         shaderPaint.setShader(sk_ref_sp(shader));
 
@@ -471,27 +478,12 @@ void SkDraw::drawFixedVertices(const SkVertices* vertices,
 
         // No colors are changing and no texture coordinates are changing, so no updates between
         // triangles are needed. Use SkVM to blit the triangles.
-        SkShader* shader = paintShader;
         SkUpdatableShader* texCoordShader = nullptr;
         if (texCoords && texCoords != positions) {
             texCoordShader = as_SB(shader)->updatableShader(outerAlloc);
             shader = texCoordShader;
         }
 
-        SkTriColorShader* colorShader = nullptr;
-        SkPMColor4f* dstColors = nullptr;
-        if (colors) {
-            colorShader = outerAlloc->make<SkTriColorShader>(compute_is_opaque(colors, vertexCount),
-                                                             usePerspective);
-            dstColors = convert_colors(colors, vertexCount, fDst.colorSpace(), outerAlloc);
-            if (shader) {
-                shader = outerAlloc->make<SkShader_Blend>(
-                        std::move(blender), sk_ref_sp(colorShader), sk_ref_sp(shader));
-            } else {
-                shader = colorShader;
-            }
-        }
-
         SkPaint shaderPaint{paint};
         shaderPaint.setShader(sk_ref_sp(shader));
         auto blitter = SkVMBlitter::Make(
@@ -506,8 +498,8 @@ void SkDraw::drawFixedVertices(const SkVertices* vertices,
                 continue;
             }
 
-            if (colorShader && !colorShader->update(ctmInverse, positions, dstColors,state.f0,
+            if (triShader && !triShader->update(ctmInverse, positions, dstColors,state.f0,
-                                                    state.f1, state.f2)) {
+                                                state.f1, state.f2)) {
                 continue;
             }
 

src/gpu/SkGr.cpp

@@ -413,13 +413,14 @@ static std::unique_ptr<GrFragmentProcessor> make_dither_effect(
 }
 #endif
 
-static inline bool skpaint_to_grpaint_impl(GrRecordingContext* context,
+static inline bool skpaint_to_grpaint_impl(
-                                           const GrColorInfo& dstColorInfo,
+        GrRecordingContext* context,
-                                           const SkPaint& skPaint,
+        const GrColorInfo& dstColorInfo,
-                                           const SkMatrixProvider& matrixProvider,
+        const SkPaint& skPaint,
-                                           std::unique_ptr<GrFragmentProcessor>* shaderProcessor,
+        const SkMatrixProvider& matrixProvider,
-                                           SkBlender* primColorBlender,
+        skstd::optional<std::unique_ptr<GrFragmentProcessor>> shaderFP,
-                                           GrPaint* grPaint) {
+        SkBlender* primColorBlender,
+        GrPaint* grPaint) {
     // Convert SkPaint color to 4f format in the destination color space
     SkColor4f origColor = SkColor4fPrepForDst(skPaint.getColor4f(), dstColorInfo);
 
@@ -428,9 +429,10 @@ static inline bool skpaint_to_grpaint_impl(GrRecordingContext* context,
     // Setup the initial color considering the shader, the SkPaint color, and the presence or not
     // of per-vertex colors.
     std::unique_ptr<GrFragmentProcessor> paintFP;
+    const bool gpProvidesShader = shaderFP.has_value() && !*shaderFP;
     if (!primColorBlender || blender_requires_shader(primColorBlender)) {
-        if (shaderProcessor) {
+        if (shaderFP.has_value()) {
-            paintFP = std::move(*shaderProcessor);
+            paintFP = std::move(*shaderFP);
         } else {
             if (const SkShaderBase* shader = as_SB(skPaint.getShader())) {
                 paintFP = shader->asFragmentProcessor(fpArgs);
@@ -490,25 +492,23 @@ static inline bool skpaint_to_grpaint_impl(GrRecordingContext* context,
         }
     } else {
         if (primColorBlender) {
-            // Examining all of the SkPaintToGrPaintFoo methods and their uses, it turns out that
+            // The primitive itself has color (e.g. interpolated vertex color) and this is what
-            // we can only encounter this code path when we're *just* using the primitive color.
+            // the GP will output. Thus, we must get the paint color in separately below as a color
-            // Literally no code path cares about blending the primitive color with the paint
+            // FP. This could be made more efficient if the relevant GPs used GrPaint color and
-            // color. This makes sense, if you think about the SkCanvas draw calls that use
+            // took the SkBlender to apply with primitive color. As it stands changing the SkPaint
-            // primitive color - none of them are specified to do anything with paint color.
+            // color will break batches.
-            SkASSERT(as_BB(primColorBlender)->asBlendMode().has_value() &&
+            grPaint->setColor4f(SK_PMColor4fWHITE);  // won't be used.
-                     *as_BB(primColorBlender)->asBlendMode() == SkBlendMode::kDst);
+            if (blender_requires_shader(primColorBlender)) {
-
+                paintFP = GrFragmentProcessor::MakeColor(origColor.makeOpaque().premul());
-            // There is no "blend" - the output of that blend is just the primitive color.
+                paintFP = as_BB(primColorBlender)->asFragmentProcessor(std::move(paintFP),
-            // We still put the opaque paint color on the GrPaint.
+                                                                       /*dstFP=*/nullptr,
-            // TODO: Is this even necessary? It seems entirely superfluous. Any op that uses this
+                                                                       fpArgs);
-            // code path should be ignoring the paint color, right?
+            }
-            grPaint->setColor4f(origColor.makeOpaque().premul());
-
-            // The paint's *alpha* is applied to the primitive color:
 
+            // The paint's *alpha* is applied after the paint/primitive color blend:
             // We can ignore origColor here - alpha is unchanged by gamma
             float paintAlpha = skPaint.getColor4f().fA;
-            if (1.0f != paintAlpha) {
+            if (paintAlpha != 1.0f) {
                 // No gamut conversion - paintAlpha is a (linear) alpha value, splatted to all
                 // color channels. It's value should be treated as the same in ANY color space.
                 paintFP = GrFragmentProcessor::ModulateRGBA(
@@ -517,7 +517,8 @@ static inline bool skpaint_to_grpaint_impl(GrRecordingContext* context,
         } else {
             // No shader, no primitive color.
             grPaint->setColor4f(origColor.premul());
-            applyColorFilterToPaintColor = true;
+            // We can do this if there isn't a GP that is acting as the shader.
+            applyColorFilterToPaintColor = !gpProvidesShader;
         }
     }
 
@@ -600,7 +601,7 @@ bool SkPaintToGrPaint(GrRecordingContext* context,
                                    dstColorInfo,
                                    skPaint,
                                    matrixProvider,
-                                   /*shaderProcessor=*/nullptr,
+                                   /*shaderFP=*/skstd::nullopt,
                                    /*primColorBlender=*/nullptr,
                                    grPaint);
 }
@@ -612,14 +613,11 @@ bool SkPaintToGrPaintReplaceShader(GrRecordingContext* context,
                                    const SkMatrixProvider& matrixProvider,
                                    std::unique_ptr<GrFragmentProcessor> shaderFP,
                                    GrPaint* grPaint) {
-    if (!shaderFP) {
-        return false;
-    }
     return skpaint_to_grpaint_impl(context,
                                    dstColorInfo,
                                    skPaint,
                                    matrixProvider,
-                                   &shaderFP,
+                                   std::move(shaderFP),
                                    /*primColorBlender=*/nullptr,
                                    grPaint);
 }
@@ -636,21 +634,7 @@ bool SkPaintToGrPaintWithBlend(GrRecordingContext* context,
                                    dstColorInfo,
                                    skPaint,
                                    matrixProvider,
-                                   /*shaderProcessor=*/nullptr,
+                                   /*shaderFP=*/skstd::nullopt,
                                    primColorBlender,
                                    grPaint);
 }
-
-bool SkPaintToGrPaintWithBlend(GrRecordingContext* context,
-                               const GrColorInfo& dstColorInfo,
-                               const SkPaint& skPaint,
-                               const SkMatrixProvider& matrixProvider,
-                               SkBlendMode primColorMode,
-                               GrPaint* grPaint) {
-    return SkPaintToGrPaintWithBlend(context,
-                                     dstColorInfo,
-                                     skPaint,
-                                     matrixProvider,
-                                     SkBlender::Mode(primColorMode).get(),
-                                     grPaint);
-}

src/gpu/SkGr.h

@@ -90,9 +90,9 @@ bool SkPaintToGrPaint(GrRecordingContext*,
                       const SkMatrixProvider& matrixProvider,
                       GrPaint* grPaint);
 
-/** Replaces the SkShader (if any) on skPaint with the passed in GrFragmentProcessor. The processor
+/** Replaces the SkShader (if any) on skPaint with the passed in GrFragmentProcessor, if not null.
-    should expect an unpremul input color and produce a premultiplied output color. There is
+    If null then it is assumed that the geometry processor is implementing a shader replacement.
-    no primitive color. */
+    The processor should expect an unpremul input color and produce a premultiplied output color. */
 bool SkPaintToGrPaintReplaceShader(GrRecordingContext*,
                                    const GrColorInfo& dstColorInfo,
                                    const SkPaint& skPaint,
@@ -109,23 +109,6 @@ bool SkPaintToGrPaintWithBlend(GrRecordingContext* context,
                                SkBlender* primColorBlender,
                                GrPaint* grPaint);
 
-/** This is used when there is a primitive color, but the shader should be ignored. Currently,
-    the expectation is that the primitive color will be premultiplied, though it really should be
-    unpremultiplied so that interpolation is done in unpremul space. The paint's alpha will be
-    applied to the primitive color after interpolation. */
-inline bool SkPaintToGrPaintWithPrimitiveColor(GrRecordingContext* context,
-                                               const GrColorInfo& dstColorInfo,
-                                               const SkPaint& skPaint,
-                                               const SkMatrixProvider& matrixProvider,
-                                               GrPaint* grPaint) {
-    return SkPaintToGrPaintWithBlend(context,
-                                     dstColorInfo,
-                                     skPaint,
-                                     matrixProvider,
-                                     SkBlender::Mode(SkBlendMode::kDst).get(),
-                                     grPaint);
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // Misc Sk to Gr type conversions
 

src/gpu/text/GrTextBlob.cpp

@@ -93,12 +93,12 @@ SkPMColor4f calculate_colors(skgpu::SurfaceContext* sc,
     GrRecordingContext* rContext = sc->recordingContext();
     const GrColorInfo& colorInfo = sc->colorInfo();
     if (grMaskFormat == kARGB_GrMaskFormat) {
-        SkPaintToGrPaintWithPrimitiveColor(rContext, colorInfo, paint, matrix, grPaint);
+        SkPaintToGrPaintReplaceShader(rContext, colorInfo, paint, matrix, nullptr, grPaint);
-        return SK_PMColor4fWHITE;
+        float a = grPaint->getColor4f().fA;
-    } else {
+        return {a, a, a, a};
-        SkPaintToGrPaint(rContext, colorInfo, paint, matrix, grPaint);
-        return grPaint->getColor4f();
     }
+    SkPaintToGrPaint(rContext, colorInfo, paint, matrix, grPaint);
+    return grPaint->getColor4f();
 }
 
 template<typename Quad, typename VertexData>

src/gpu/v1/Device.cpp

@@ -92,11 +92,6 @@ bool init_vertices_paint(GrRecordingContext* rContext,
                          bool hasColors,
                          GrPaint* grPaint) {
     if (hasColors) {
-        // When there are colors and a shader, the shader and colors are combined using bmode.
-        // With no shader, we just use the colors (kDst).
-        if (!skPaint.getShader()) {
-            blender = SkBlender::Mode(SkBlendMode::kDst);
-        }
         return SkPaintToGrPaintWithBlend(rContext,
                                          colorInfo,
                                          skPaint,
@@ -776,9 +771,13 @@ void Device::drawViewLattice(GrSurfaceProxyView view,
         paint.writable()->setColor(SkColorSetARGB(origPaint.getAlpha(), 0xFF, 0xFF, 0xFF));
     }
     GrPaint grPaint;
-    if (!SkPaintToGrPaintWithPrimitiveColor(this->recordingContext(),
+    // Passing null as shaderFP indicates that the GP will provide the shader.
-                                            fSurfaceDrawContext->colorInfo(), *paint,
+    if (!SkPaintToGrPaintReplaceShader(this->recordingContext(),
-                                            this->asMatrixProvider(), &grPaint)) {
+                                       fSurfaceDrawContext->colorInfo(),
+                                       *paint,
+                                       this->asMatrixProvider(),
+                                       /*shaderFP=*/nullptr,
+                                       &grPaint)) {
         return;
     }
 
@@ -819,6 +818,12 @@ void Device::drawVertices(const SkVertices* vertices,
     GR_CREATE_TRACE_MARKER_CONTEXT("skgpu::v1::Device", "drawVertices", fContext.get());
     SkASSERT(vertices);
 
+#ifdef SK_LEGACY_IGNORE_DRAW_VERTICES_BLEND_WITH_NO_SHADER
+    if (!paint.getShader()) {
+        blender = SkBlender::Mode(SkBlendMode::kDst);
+    }
+#endif
+
     SkVerticesPriv info(vertices->priv());
 
     GrPaint grPaint;