Avoid D2D1Group fill mode conversion for CGPath.

Frameworks/CoreGraphics/CGContext.mm

@@ -156,8 +156,9 @@ inline HRESULT IntersectClippingGeometry(ID2D1Geometry* incomingGeometry, CGPath
         D2D1_FILL_MODE d2dFillMode = (pathMode & kCGPathEOFill) == kCGPathEOFill ? D2D1_FILL_MODE_ALTERNATE : D2D1_FILL_MODE_WINDING;
 
         if (!clippingGeometry) {
-            // If we don't have a clipping geometry, we are free to take this one wholesale (after EO/Winding conversion.)
-            return _CGConvertD2DGeometryToFillMode(incomingGeometry, d2dFillMode, &clippingGeometry);
+            // If we don't have a clipping geometry, we are free to take this one wholesale.
+            clippingGeometry = incomingGeometry;
+            return S_OK;
         }
 
         ComPtr<ID2D1Factory> factory;
@@ -1053,7 +1054,7 @@ bool CGContextPathContainsPoint(CGContextRef context, CGPoint point, CGPathDrawi
     }
 
     ComPtr<ID2D1Geometry> additionalClippingGeometry;
-    RETURN_IF_FAILED(_CGPathGetGeometry(Path(), &additionalClippingGeometry));
+    RETURN_IF_FAILED(_CGPathGetGeometryWithFillMode(Path(), pathMode, &additionalClippingGeometry));
     ClearPath();
 
     auto& state = CurrentGState();
@@ -2383,12 +2384,7 @@ void CGContextClearRect(CGContextRef context, CGRect rect) {
     auto& state = context->CurrentGState();
     if (drawMode & kCGPathFill) {
         state.fillBrush->SetOpacity(state.alpha);
-
-        ComPtr<ID2D1Geometry> geometryToFill;
-        D2D1_FILL_MODE d2dFillMode = (drawMode & kCGPathEOFill) == kCGPathEOFill ? D2D1_FILL_MODE_ALTERNATE : D2D1_FILL_MODE_WINDING;
-        RETURN_IF_FAILED(_CGConvertD2DGeometryToFillMode(geometry, d2dFillMode, &geometryToFill));
-
-        deviceContext->FillGeometry(geometryToFill.Get(), state.fillBrush.Get());
+        deviceContext->FillGeometry(geometry, state.fillBrush.Get());
     }
 
     if (drawMode & kCGPathStroke && std::fpclassify(state.lineWidth) != FP_ZERO) {
@@ -2551,7 +2547,7 @@ void CGContextDrawPath(CGContextRef context, CGPathDrawingMode mode) {
 
     if (context->HasPath()) {
         ComPtr<ID2D1Geometry> pGeometry;
-        FAIL_FAST_IF_FAILED(_CGPathGetGeometry(context->Path(), &pGeometry));
+        FAIL_FAST_IF_FAILED(_CGPathGetGeometryWithFillMode(context->Path(), mode, &pGeometry));
         FAIL_FAST_IF_FAILED(context->DrawGeometry(_kCGCoordinateModeDeviceSpace, pGeometry.Get(), mode));
         context->ClearPath();
     }

Frameworks/CoreGraphics/CGPath.mm

@@ -46,7 +46,7 @@
         return m_geometrySink.Get();
     }
 
-    _CGPathCustomSink(_In_ ID2D1GeometrySink* sink) : m_geometrySink(sink), m_lastPoint{0, 0}, m_isFigureOpen(false) {
+    _CGPathCustomSink(_In_ ID2D1GeometrySink* sink) : m_geometrySink(sink), m_lastPoint{ 0, 0 }, m_isFigureOpen(false) {
     }
 
     STDMETHOD_(void, SetFillMode)(D2D1_FILL_MODE fillMode) {
@@ -271,11 +271,26 @@ HRESULT AddGeometryToPathWithTransformation(const ID2D1Geometry* geometry, const
     }
 };
 
-HRESULT _CGPathGetGeometry(CGPathRef path, ID2D1Geometry** pGeometry) {
-    RETURN_HR_IF_NULL(E_POINTER, pGeometry);
+HRESULT _CGPathGetGeometryWithFillMode(CGPathRef path, CGPathDrawingMode fillMode, ID2D1Geometry** pNewGeometry) {
+    RETURN_HR_IF_NULL(E_POINTER, pNewGeometry);
     RETURN_HR_IF_NULL(E_POINTER, path);
+
     RETURN_IF_FAILED(path->ClosePath());
-    path->pathGeometry.CopyTo(pGeometry);
+    if (fillMode == kCGPathEOFill || fillMode == kCGPathEOFillStroke) {
+        ID2D1Geometry* geometry = path->GetPathGeometry();
+        ComPtr<ID2D1Factory> factory;
+        geometry->GetFactory(&factory);
+
+        ComPtr<ID2D1GeometryGroup> geometryGroup;
+        RETURN_IF_FAILED(factory->CreateGeometryGroup(D2D1_FILL_MODE_ALTERNATE, &geometry, 1, &geometryGroup));
+
+        ComPtr<ID2D1Geometry> outGeometry;
+        RETURN_IF_FAILED(geometryGroup.As(&outGeometry));
+
+        *pNewGeometry = outGeometry.Detach();
+    } else {
+        path->pathGeometry.CopyTo(pNewGeometry);
+    }
     return S_OK;
 }
 

Frameworks/CoreGraphics/D2DWrapper.mm

@@ -31,19 +31,4 @@ HRESULT _CGGetWICFactory(IWICImagingFactory** factory) {
     static HRESULT sHr = CoCreateInstance(CLSID_WICImagingFactory, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&sWicFactory));
     sWicFactory.CopyTo(factory);
     RETURN_HR(sHr);
-}
-
-// TODO GH#1375: Remove this when CGPath's fill mode has been worked out.
-HRESULT _CGConvertD2DGeometryToFillMode(ID2D1Geometry* geometry, D2D1_FILL_MODE fillMode, ID2D1Geometry** pNewGeometry) {
-    ComPtr<ID2D1Factory> factory;
-    geometry->GetFactory(&factory);
-
-    ComPtr<ID2D1GeometryGroup> geometryGroup;
-    RETURN_IF_FAILED(factory->CreateGeometryGroup(fillMode, &geometry, 1, &geometryGroup));
-
-    ComPtr<ID2D1Geometry> outGeometry;
-    RETURN_IF_FAILED(geometryGroup.As(&outGeometry));
-
-    *pNewGeometry = outGeometry.Detach();
-    return S_OK;
 }

Frameworks/include/CGPathInternal.h

@@ -68,8 +68,8 @@ struct CGPathElementInternal : CGPathElement {
 };
 typedef struct CGPathElementInternal CGPathElementInternal;
 
-HRESULT _CGPathGetGeometry(CGPathRef path, ID2D1Geometry** pGeometry);
 HRESULT _CGPathApplyInternal(ID2D1PathGeometry* pathGeometry, void* info, CGPathApplierFunction function);
+HRESULT _CGPathGetGeometryWithFillMode(CGPathRef path, CGPathDrawingMode fillMode, ID2D1Geometry** pNewGeometry);
 
 #if defined __clang__
 #pragma clang diagnostic pop

Frameworks/include/CoreGraphics/D2DWrapper.h

@@ -30,8 +30,6 @@ HRESULT _CGGetD2DFactory(ID2D1Factory** factory);
 
 HRESULT _CGGetWICFactory(IWICImagingFactory** factory);
 
-HRESULT _CGConvertD2DGeometryToFillMode(ID2D1Geometry* geometry, D2D1_FILL_MODE fillMode, ID2D1Geometry** pNewGeometry);
-
 inline D2D_POINT_2F _CGPointToD2D_F(CGPoint point) {
     return { point.x, point.y };
 }

build/Tests/UnitTests/CoreGraphics.Drawing/CoreGraphics.Drawing.UnitTests.vcxproj

@@ -231,7 +231,6 @@
     </ClangCompile>
   </ItemDefinitionGroup>
   <ItemGroup>
-
     <ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\TAEFEntryPoint.cpp" />
   </ItemGroup>
   <ItemGroup>
@@ -246,6 +245,7 @@
     <ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGContextDrawing_GradientTests.cpp" />
     <ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGContextDrawing_ImageDrawingTests.cpp" />
     <ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGContextDrawing_PatternTests.cpp" />
+    <ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.drawing\CGContextDrawing_FillModeTests.cpp" />
     <ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\CGPathDrawingTests.cpp" />
     <ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.Drawing\DrawingTest.cpp" />
     <ClangCompile Include="$(StarboardBasePath)\tests\unittests\CoreGraphics.drawing\ImageComparison.cpp" />
@@ -266,4 +266,4 @@
   <ItemGroup>
     <None Include="project.json" />
   </ItemGroup>
-</Project>
+</Project>

tests/unittests/CoreGraphics.Drawing/CGPathDrawingTests.cpp

@@ -497,41 +497,5 @@ DRAW_TEST_F(CGPath, FillStraightLines, UIKitMimicTest<>) {
     CGContextFillPath(context);
     CGContextStrokePath(context);
 
-    CGPathRelease(thepath);
-}
-
-DRAW_TEST_F(CGPath, FillModeCircles, UIKitMimicTest<>) {
-    CGContextRef context = GetDrawingContext();
-    CGRect bounds = GetDrawingBounds();
-    CGFloat width = bounds.size.width;
-    CGFloat height = bounds.size.height;
-    CGFloat xstart = bounds.origin.x;
-    CGFloat ystart = bounds.origin.y;
-
-    CGMutablePathRef thepath = CGPathCreateMutable();
-
-    CGPathMoveToPoint(thepath, NULL, xstart + .5 * width + .4 * height, ystart + .5 * height);
-    CGPathAddArc(thepath, NULL, xstart + .5 * width, ystart + .5 * height, .4 * height, 0, M_PI, true);
-    CGPathAddArc(thepath, NULL, xstart + .5 * width, ystart + .5 * height, .4 * height, M_PI, 0, true);
-
-    CGPathMoveToPoint(thepath, NULL, xstart + .5 * width + .3 * height, ystart + .5 * height);
-    CGPathAddArc(thepath, NULL, xstart + .5 * width, ystart + .5 * height, .3 * height, 0, M_PI, true);
-    CGPathAddArc(thepath, NULL, xstart + .5 * width, ystart + .5 * height, .3 * height, M_PI, 0, true);
-
-    CGPathMoveToPoint(thepath, NULL, xstart + .5 * width + .2 * height, ystart + .5 * height);
-    CGPathAddArc(thepath, NULL, xstart + .5 * width, ystart + .5 * height, .2 * height, 0, M_PI, true);
-    CGPathAddArc(thepath, NULL, xstart + .5 * width, ystart + .5 * height, .2 * height, M_PI, 0, true);
-
-    CGPathMoveToPoint(thepath, NULL, xstart + .5 * width + .1 * height, ystart + .5 * height);
-    CGPathAddArc(thepath, NULL, xstart + .5 * width, ystart + .5 * height, .1 * height, 0, M_PI, true);
-    CGPathAddArc(thepath, NULL, xstart + .5 * width, ystart + .5 * height, .1 * height, M_PI, 0, true);
-
-    CGPathCloseSubpath(thepath);
-
-    CGContextAddPath(context, thepath);
-    CGContextSetRGBFillColor(context, 0, 0, 1, 1);
-    CGContextEOFillPath(context);
-    CGContextStrokePath(context);
-
     CGPathRelease(thepath);
 }

tests/unittests/CoreGraphics.drawing/CGContextDrawing_FillModeTests.cpp

@@ -0,0 +1,108 @@
+//******************************************************************************
+//
+// Copyright (c) Microsoft. All rights reserved.
+//
+// This code is licensed under the MIT License (MIT).
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+//
+//******************************************************************************
+
+#include "DrawingTest.h"
+
+CGPathDrawingMode fillModes[] = { kCGPathFill, kCGPathFillStroke, kCGPathEOFill, kCGPathEOFillStroke };
+
+class CGContextFillMode : public WhiteBackgroundTest<>, public ::testing::WithParamInterface<CGPathDrawingMode> {
+    CFStringRef CreateOutputFilename() {
+        CGPathDrawingMode fillMode = GetParam();
+
+        char* fillModeName;
+        switch (fillMode) {
+            case kCGPathFill:
+                fillModeName = "Fill";
+                break;
+            case kCGPathFillStroke:
+                fillModeName = "FillStroke";
+                break;
+            case kCGPathEOFill:
+                fillModeName = "EOFill";
+                break;
+            case kCGPathEOFillStroke:
+                fillModeName = "EOFillStroke";
+                break;
+            default:
+                break;
+        }
+
+        return CFStringCreateWithFormat(nullptr, nullptr, CFSTR("TestImage.CGContext.FillMode.%s.png"), fillModeName);
+    }
+};
+
+DRAW_TEST_P(CGContextFillMode, OverlappedEllipses) {
+    CGContextRef context = GetDrawingContext();
+    CGRect bounds = GetDrawingBounds();
+    bounds = CGRectInset(bounds, 16.f, 16.f);
+    CGFloat width = bounds.size.width;
+    CGFloat height = bounds.size.height;
+    CGFloat xstart = bounds.origin.x;
+    CGFloat ystart = bounds.origin.y;
+
+    CGPathDrawingMode fillMode = GetParam();
+
+    CGMutablePathRef leftCircles = CGPathCreateMutable();
+
+    CGPathMoveToPoint(leftCircles, NULL, xstart + .25 * width + .4 * height, ystart + .5 * height);
+    CGPathAddArc(leftCircles, NULL, xstart + .25 * width, ystart + .5 * height, .4 * height, 0, M_PI, true);
+    CGPathAddArc(leftCircles, NULL, xstart + .25 * width, ystart + .5 * height, .4 * height, M_PI, 0, true);
+
+    CGPathMoveToPoint(leftCircles, NULL, xstart + .25 * width + .3 * height, ystart + .5 * height);
+    CGPathAddArc(leftCircles, NULL, xstart + .25 * width, ystart + .5 * height, .3 * height, 0, M_PI, true);
+    CGPathAddArc(leftCircles, NULL, xstart + .25 * width, ystart + .5 * height, .3 * height, M_PI, 0, true);
+
+    CGPathMoveToPoint(leftCircles, NULL, xstart + .25 * width + .2 * height, ystart + .5 * height);
+    CGPathAddArc(leftCircles, NULL, xstart + .25 * width, ystart + .5 * height, .2 * height, 0, M_PI, true);
+    CGPathAddArc(leftCircles, NULL, xstart + .25 * width, ystart + .5 * height, .2 * height, M_PI, 0, true);
+
+    CGPathMoveToPoint(leftCircles, NULL, xstart + .25 * width + .1 * height, ystart + .5 * height);
+    CGPathAddArc(leftCircles, NULL, xstart + .25 * width, ystart + .5 * height, .1 * height, 0, M_PI, true);
+    CGPathAddArc(leftCircles, NULL, xstart + .25 * width, ystart + .5 * height, .1 * height, M_PI, 0, true);
+
+    CGPathCloseSubpath(leftCircles);
+
+    CGMutablePathRef rightCircles = CGPathCreateMutable();
+
+    CGPathMoveToPoint(rightCircles, NULL, xstart + .75 * width + .4 * height, ystart + .5 * height);
+    CGPathAddArc(rightCircles, NULL, xstart + .75 * width, ystart + .5 * height, .4 * height, 0, M_PI, false);
+    CGPathAddArc(rightCircles, NULL, xstart + .75 * width, ystart + .5 * height, .4 * height, M_PI, 0, false);
+
+    CGPathMoveToPoint(rightCircles, NULL, xstart + .75 * width + .3 * height, ystart + .5 * height);
+    CGPathAddArc(rightCircles, NULL, xstart + .75 * width, ystart + .5 * height, .3 * height, 0, M_PI, true);
+    CGPathAddArc(rightCircles, NULL, xstart + .75 * width, ystart + .5 * height, .3 * height, M_PI, 0, true);
+
+    CGPathMoveToPoint(rightCircles, NULL, xstart + .75 * width + .2 * height, ystart + .5 * height);
+    CGPathAddArc(rightCircles, NULL, xstart + .75 * width, ystart + .5 * height, .2 * height, 0, M_PI, false);
+    CGPathAddArc(rightCircles, NULL, xstart + .75 * width, ystart + .5 * height, .2 * height, M_PI, 0, false);
+
+    CGPathMoveToPoint(rightCircles, NULL, xstart + .75 * width + .1 * height, ystart + .5 * height);
+    CGPathAddArc(rightCircles, NULL, xstart + .75 * width, ystart + .5 * height, .1 * height, 0, M_PI, true);
+    CGPathAddArc(rightCircles, NULL, xstart + .75 * width, ystart + .5 * height, .1 * height, M_PI, 0, true);
+
+    CGPathCloseSubpath(rightCircles);
+
+    CGContextAddPath(context, leftCircles);
+    CGContextAddPath(context, rightCircles);
+    CGContextSetRGBFillColor(context, 0, 0, 1, 1);
+    CGContextSetRGBStrokeColor(context, 1, 0, 0, 1);
+    CGContextDrawPath(context, fillMode);
+
+    CGPathRelease(leftCircles);
+    CGPathRelease(rightCircles);
+}
+
+INSTANTIATE_TEST_CASE_P(FillModes, CGContextFillMode, ::testing::ValuesIn(fillModes));