Bug 1902253 - When the compositing mode is not OVER, we must first render the color glyph to a temporary surface, then composite it with the appropriate operator. r=gfx-reviewers,lsalzman

Differential Revision: https://phabricator.services.mozilla.com/D215408

gfx/thebes/COLRFonts.cpp

@@ -2392,8 +2392,7 @@ bool COLRFonts::ValidateColorGlyphs(hb_blob_t* aCOLR, hb_blob_t* aCPAL) {
   }
 
   if (uint16_t(colr->version) == 1) {
-    return StaticPrefs::gfx_font_rendering_colr_v1_enabled() &&
+    return colrLength >= sizeof(COLRv1Header) &&
-           colrLength >= sizeof(COLRv1Header) &&
            reinterpret_cast<const COLRv1Header*>(colr)->Validate(colrLength);
   }
 

gfx/thebes/gfxFont.cpp

@@ -2664,82 +2664,126 @@ bool gfxFont::RenderColorGlyph(DrawTarget* aDrawTarget, gfxContext* aContext,
   }
 
   auto* colr = GetFontEntry()->GetCOLR();
-  if (const auto* paintGraph = COLRFonts::GetGlyphPaintGraph(colr, aGlyphId)) {
+  const auto* paintGraph = COLRFonts::GetGlyphPaintGraph(colr, aGlyphId);
-    const auto* hbShaper = GetHarfBuzzShaper();
+  const gfxHarfBuzzShaper* hbShaper = nullptr;
-    if (hbShaper && hbShaper->IsInitialized()) {
+  if (paintGraph) {
+    // We need the hbShaper to get color glyph bounds, so check that it's
+    // usable.
+    hbShaper = GetHarfBuzzShaper();
+    if (!hbShaper && !hbShaper->IsInitialized()) {
+      return false;
+    }
     if (aTextDrawer) {
       aTextDrawer->FoundUnsupportedFeature();
       return true;
     }
+  }
+  const auto* layers =
+      paintGraph ? nullptr : COLRFonts::GetGlyphLayers(colr, aGlyphId);
+
+  if (!paintGraph && !layers) {
+    return false;
+  }
 
   // For reasonable font sizes, use a cache of rasterized glyphs.
-      if (GetAdjustedSize() <= 256.0) {
+  bool useCache = GetAdjustedSize() <= 256.0;
+
+  // If the composition op is not OVER, rasterize to a temporary surface
+  // and then composite to the destination, even if we're not caching.
+  // But we can't do this if the target is a TextDrawTarget, as it doesn't
+  // support DrawSurface.
+  RefPtr<SourceSurface> snapshot;
+  if ((useCache ||
+       aFontParams.drawOptions.mCompositionOp != CompositionOp::OP_OVER) &&
+      aDrawTarget->GetBackendType() != BackendType::WEBRENDER_TEXT) {
     AutoWriteLock lock(mLock);
-        if (!mColorGlyphCache) {
+    if (!mColorGlyphCache && useCache) {
       mColorGlyphCache = MakeUnique<ColorGlyphCache>();
     }
 
-        // Tell the cache what colors we're using; if they have changed, it will
+    Rect bounds;
-        // discard any currently-cached entries.
+    if (paintGraph) {
-        mColorGlyphCache->SetColors(aFontParams.currentColor,
+      bounds = COLRFonts::GetColorGlyphBounds(
-                                    aFontParams.palette);
-
-        bool ok = false;
-        auto cached = mColorGlyphCache->mCache.lookupForAdd(aGlyphId);
-        Rect bounds = COLRFonts::GetColorGlyphBounds(
           colr, hbShaper->GetHBFont(), aGlyphId, aDrawTarget,
           aFontParams.scaledFont, mFUnitsConvFactor);
+    } else {
+      bounds = GetFontEntry()->GetFontExtents(mFUnitsConvFactor);
+    }
     bounds.RoundOut();
 
+    // Tell the cache what colors we're using; if they have changed, it
+    // will discard any currently-cached entries.
+    HashMap<uint32_t, RefPtr<SourceSurface>>::AddPtr cached;
+    if (useCache) {
+      mColorGlyphCache->SetColors(aFontParams.currentColor,
+                                  aFontParams.palette);
+      cached = mColorGlyphCache->mCache.lookupForAdd(aGlyphId);
       if (cached) {
-          ok = true;
+        snapshot = cached->value();
-        } else {
+      }
-          // Create a temporary DrawTarget, render the glyph, and save a
+    }
-          // snapshot of the rendering in the cache.
+
+    if (!snapshot) {
+      // Create a temporary DrawTarget and render the glyph to it.
       IntSize size(int(bounds.width), int(bounds.height));
       SurfaceFormat format = SurfaceFormat::B8G8R8A8;
       RefPtr target =
           Factory::CreateDrawTarget(BackendType::SKIA, size, format);
       if (target) {
+        // Use OP_OVER to create the glyph snapshot.
+        DrawOptions drawOptions(aFontParams.drawOptions);
+        drawOptions.mCompositionOp = CompositionOp::OP_OVER;
+        bool ok = false;
+        if (paintGraph) {
           ok = COLRFonts::PaintGlyphGraph(
-                GetFontEntry()->GetCOLR(), hbShaper->GetHBFont(), paintGraph,
+              colr, hbShaper->GetHBFont(), paintGraph, target, nullptr,
-                target, nullptr, aFontParams.scaledFont,
+              aFontParams.scaledFont, drawOptions, -bounds.TopLeft(),
-                aFontParams.drawOptions, -bounds.TopLeft(),
+              aFontParams.currentColor, aFontParams.palette->Colors(), aGlyphId,
-                aFontParams.currentColor, aFontParams.palette->Colors(),
+              mFUnitsConvFactor);
-                aGlyphId, mFUnitsConvFactor);
+        } else {
-            if (ok) {
+          auto face(GetFontEntry()->GetHBFace());
-              RefPtr snapshot = target->Snapshot();
+          ok = COLRFonts::PaintGlyphLayers(
-              ok = mColorGlyphCache->mCache.add(cached, aGlyphId, snapshot);
+              colr, face, layers, target, nullptr, aFontParams.scaledFont,
-            }
+              drawOptions, -bounds.TopLeft(), aFontParams.currentColor,
-          }
+              aFontParams.palette->Colors());
         }
         if (ok) {
-          // Paint the snapshot from cached->value(), and return.
+          snapshot = target->Snapshot();
-          aDrawTarget->DrawSurface(
+          if (useCache) {
-              cached->value(), Rect(aPoint + bounds.TopLeft(), bounds.Size()),
+            // Save a snapshot of the rendering in the cache.
-              Rect(Point(), bounds.Size()));
+            // (We ignore potential failure here, and just paint the snapshot
+            // without caching it.)
+            Unused << mColorGlyphCache->mCache.add(cached, aGlyphId, snapshot);
+          }
+        }
+      }
+    }
+    if (snapshot) {
+      // Paint the snapshot using the appropriate composition op.
+      aDrawTarget->DrawSurface(snapshot,
+                               Rect(aPoint + bounds.TopLeft(), bounds.Size()),
+                               Rect(Point(), bounds.Size()),
+                               DrawSurfaceOptions(), aFontParams.drawOptions);
       return true;
     }
   }
 
-      // If we failed to cache the glyph, or it was too large to even try,
+  // If we didn't paint from a cached or temporary snapshot, just render
-      // just paint directly to the target.
+  // directly to the destination drawTarget.
+  if (paintGraph) {
     return COLRFonts::PaintGlyphGraph(
         colr, hbShaper->GetHBFont(), paintGraph, aDrawTarget, aTextDrawer,
         aFontParams.scaledFont, aFontParams.drawOptions, aPoint,
         aFontParams.currentColor, aFontParams.palette->Colors(), aGlyphId,
         mFUnitsConvFactor);
   }
-  }
 
-  if (const auto* layers =
+  if (layers) {
-          COLRFonts::GetGlyphLayers(GetFontEntry()->GetCOLR(), aGlyphId)) {
     auto face(GetFontEntry()->GetHBFace());
-    bool ok = COLRFonts::PaintGlyphLayers(
+    return COLRFonts::PaintGlyphLayers(
         colr, face, layers, aDrawTarget, aTextDrawer, aFontParams.scaledFont,
         aFontParams.drawOptions, aPoint, aFontParams.currentColor,
         aFontParams.palette->Colors());
-    return ok;
   }
 
   return false;

layout/reftests/text/reftest.list

@@ -328,7 +328,7 @@ pref(layout.css.moz-control-character-visibility.enabled,true) pref(layout.css.c
 == segment-break-transformation-1.html segment-break-transformation-1-ref.html
 
 # color glyphs, opacity, and RTL runs - bug 1318539
-== color-opacity-rtl-1.html color-opacity-rtl-1-ref.html
+fuzzy-if(winWidget,0-1,0-4) == color-opacity-rtl-1.html color-opacity-rtl-1-ref.html
 fuzzy-if(cocoaWidget,0-1,0-1421) == color-opacity-rtl-2.html color-opacity-rtl-2-ref.html
 
 == letter-spacing-nolig-1.html letter-spacing-nolig-1-ref.html