Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement drawing CGPaths through CGContext #1307

Merged
merged 5 commits into from
Nov 5, 2016
Merged

Implement drawing CGPaths through CGContext #1307

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d43b075
Implement drawing CGPaths through CGContext
MSFTFox Oct 26, 2016
5f79b50
Fix calls to return an HRESULT.
MSFTFox Nov 3, 2016
5eab616
Address CR Feedback.
MSFTFox Nov 4, 2016
24f5707
Fix poor comptr usage and leaks. Return to static initializer model.
MSFTFox Nov 4, 2016
34bd6d5
More macros.
MSFTFox Nov 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Frameworks/CoreGraphics/CGBitmapImage.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

#import "LoggingNative.h"

#import "D2DWrapper.h"

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-register"

Expand Down Expand Up @@ -452,7 +454,7 @@
BYTE* imageData = static_cast<BYTE*>(LockImageData());
ComPtr<IWICBitmap> wicBitmap = Make<CGIWICBitmap>(this, SurfaceFormat());
ComPtr<ID2D1Factory> d2dFactory;
THROW_IF_FAILED(D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, __uuidof(ID2D1Factory), &d2dFactory));
FAIL_FAST_IF_FAILED(_CGGetD2DFactory(&d2dFactory));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please no, THROW_IF_FAILED() is marginally better. FAIL_FAST is essentially abort, and as framework we should seldom be using it. And in other places too, per our offline discussion.

ComPtr<ID2D1RenderTarget> renderTarget;
THROW_IF_FAILED(d2dFactory->CreateWicBitmapRenderTarget(wicBitmap.Get(), D2D1::RenderTargetProperties(), &renderTarget));
Copy link
Member

@bbowman bbowman Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-( super sad at the inconsistencies here. I thought the decision was to be error returning out to the public layer at which point a decision would be made on the correct course of action. This still seems internal as it returns a D2D object.

Additionally, returning a Com object with a raw * usually speaks to a problematic design pattern. This is because it requires the "unjacketing" of the ComObject to return it to the caller without a clear communication on transfer/sharing of ownership which is by definition occuring by handing it out. #Resolved

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The render target generators in CGImage and friends are going away (and are only being updated as necessary as part of this PR); ideally, yes, we'd fix everything all at once. There's little point when it's gonna be turfed. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code has gone away, i'll be sending out the PR soon.
Actually this whole file is gone.

In reply to: 86612430 [](ancestors = 86612430)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should sync up.

In reply to: 86614591 [](ancestors = 86614591,86612430)

_renderTarget = renderTarget.Detach();
Expand Down
9 changes: 7 additions & 2 deletions Frameworks/CoreGraphics/CGContext.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#import <CoreGraphics/CGGradient.h>
#import "CGColorSpaceInternal.h"
#import "CGContextInternal.h"
#import "CGPathInternal.h"

#import <CFCppBase.h>

Expand Down Expand Up @@ -1646,8 +1647,12 @@ void CGContextFillRects(CGContextRef context, const CGRect* rects, size_t count)
*/
void CGContextDrawPath(CGContextRef context, CGPathDrawingMode mode) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();
context->ClearPath();
if (context->HasPath()) {
ID2D1Geometry* pGeometry;
Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this a ComPtr<ID2D1Geometry>. Right now you're getting a +1 reference to a Geometry and then dropping it on the floor.

You should almost never use bare COM interface pointers. :) #Resolved

Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you're at a module boundary, or a boundary that makes sense (like CGContext <-> CGPath), and even then only in function parameters. #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, yes, yes.

using raw * means you really have to think about it and its easy to forget something / mess up. Com stuff gets a ComPtr almost always.

In reply to: 86611265 [](ancestors = 86611265)

FAIL_FAST_IF_FAILED(_CGPathGetGeometry(context->Path(), &pGeometry));
__CGContextDrawGeometry(context, pGeometry, mode);
context->ClearPath();
Copy link
Contributor

@aballway aballway Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should do something with the HRESULT here #Resolved

}
}

/**
Expand Down
4 changes: 3 additions & 1 deletion Frameworks/CoreGraphics/CGGraphicBufferImage.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#import "LoggingNative.h"
#import <CGGraphicBufferImage.h>

#import "D2DWrapper.h"

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-register"

Expand Down Expand Up @@ -156,7 +158,7 @@
BYTE* imageData = static_cast<BYTE*>(LockImageData());
ComPtr<IWICBitmap> wicBitmap = Make<CGIWICBitmap>(this, SurfaceFormat());
ComPtr<ID2D1Factory> d2dFactory;
THROW_IF_FAILED(D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, __uuidof(ID2D1Factory), &d2dFactory));
FAIL_FAST_IF_FAILED(_CGGetD2DFactory(&d2dFactory));
ComPtr<ID2D1RenderTarget> renderTarget;
THROW_IF_FAILED(d2dFactory->CreateWicBitmapRenderTarget(wicBitmap.Get(), D2D1::RenderTargetProperties(), &renderTarget));
_renderTarget = renderTarget.Detach();
Expand Down
38 changes: 26 additions & 12 deletions Frameworks/CoreGraphics/CGPath.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@
#import <CoreFoundation/CoreFoundation.h>
#import <CFRuntime.h>
#import "D2DWrapper.h"
#import "CGPathInternal.h"

#include <COMIncludes.h>
#import <wrl/client.h>
#import <D2d1.h>
#include <COMIncludes_End.h>

#import <CFCPPBase.h>

static const wchar_t* TAG = L"CGPath";
Expand Down Expand Up @@ -91,7 +92,8 @@ void SetStartingPoint(CGPoint newPoint) {
HRESULT PreparePathForEditing() {
if (!_impl.geometrySink) {
// Re-open this geometry.
ComPtr<ID2D1Factory> factory = _GetD2DFactoryInstance();
ComPtr<ID2D1Factory> factory;
RETURN_IF_FAILED(_CGGetD2DFactory(&factory));

// Create temp vars for new path/sink
ComPtr<ID2D1PathGeometry> newPath;
Expand All @@ -113,12 +115,13 @@ HRESULT PreparePathForEditing() {
return S_OK;
}

void ClosePath() {
HRESULT ClosePath() {
if (_impl.geometrySink) {
EndFigure(D2D1_FIGURE_END_OPEN);
Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this fail? #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

virtual void EndFigure(
D2D1_FIGURE_END figureEnd
) = 0;

It cannot. Calling it multiple times can result in a bad state though which makes this API slightly weird since it doesn't report that itself.

In reply to: 86611357 [](ancestors = 86611357)

_impl.geometrySink->Close();
RETURN_IF_FAILED(_impl.geometrySink->Close());
_impl.geometrySink = nullptr;
}
return S_OK;
}

void BeginFigure() {
Expand All @@ -136,7 +139,8 @@ void EndFigure(D2D1_FIGURE_END figureStatus) {
}

HRESULT InitializeGeometries() {
ComPtr<ID2D1Factory> factory = _GetD2DFactoryInstance();
ComPtr<ID2D1Factory> factory;
RETURN_IF_FAILED(_CGGetD2DFactory(&factory));

RETURN_IF_FAILED(factory->CreatePathGeometry(&_impl.pathGeometry));
RETURN_IF_FAILED(_impl.pathGeometry->Open(&_impl.geometrySink));
Expand All @@ -145,6 +149,14 @@ HRESULT InitializeGeometries() {
}
};

HRESULT _CGPathGetGeometry(CGPathRef path, ID2D1Geometry** pGeometry) {
Copy link
Contributor

@msft-Jeyaram msft-Jeyaram Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pGeometry [](start = 59, length = 9)

if pGeometry is invalid, we end up returning S_OK.
adding a check for pGeoMetry and path should help.

RETURN_HR_IF_NULL(E_POINTER,pGeometry);
RETURN_HR_IF_NULL(E_POINTER,path); #Resolved

if (path && pGeometry) {
RETURN_IF_FAILED(path->ClosePath());
*pGeometry = path->GetPathGeometry().Get();
Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to .CopyTo this. COM out pointers are expected to be given a new +1. #Resolved

Copy link
Member

@bbowman bbowman Nov 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not might. Definitely need. CopyTo is ever so slightly worse than a .Detach here as CopyTo does an extra AddRef Release that isn't needed for the anonymous rvalue vs detach but yes. The more efficient and more sane, probably, pattern is a raw ** here for the GetPathGeometry method.

In reply to: 86611487 [](ancestors = 86611487)

}
return S_OK;
}

CFTypeID CGPathGetTypeID() {
return __CGPath::GetTypeID();
}
Expand All @@ -159,8 +171,8 @@ static Boolean __CGPathEqual(CFTypeRef cf1, CFTypeRef cf2) {
__CGPath* path1 = (__CGPath*)cf1;
__CGPath* path2 = (__CGPath*)cf2;

path1->ClosePath();
path2->ClosePath();
RETURN_FALSE_IF_FAILED(path1->ClosePath());
RETURN_FALSE_IF_FAILED(path2->ClosePath());

// ID2D1 Geometries have no isEquals method. However, for two geometries to be equal they are both reported to contain the other.
// Thus we must do two comparisons.
Expand Down Expand Up @@ -212,7 +224,7 @@ CGMutablePathRef CGPathCreateMutableCopy(CGPathRef path) {
// In order to call stream and copy the contents of the original path into the
// new copy we must close this path.
// Otherwise the D2D calls will return that a bad state has been entered.
path->ClosePath();
FAIL_FAST_IF_FAILED(path->ClosePath());

FAIL_FAST_IF_FAILED(path->GetPathGeometry()->Stream(mutableRet->GetGeometrySink().Get()));

Expand Down Expand Up @@ -451,7 +463,7 @@ void CGPathAddPath(CGMutablePathRef path, const CGAffineTransform* transform, CG
RETURN_IF(!path || !toAdd);

// Close the path we're adding and open the path we need to add to.
toAdd->ClosePath();
FAIL_FAST_IF_FAILED(toAdd->ClosePath());
FAIL_FAST_IF_FAILED(path->PreparePathForEditing());
FAIL_FAST_IF_FAILED(toAdd->GetPathGeometry()->Stream(path->GetGeometrySink().Get()));
}
Expand Down Expand Up @@ -510,7 +522,9 @@ CGRect CGPathGetBoundingBox(CGPathRef path) {

D2D1_RECT_F bounds;

path->ClosePath();
if (FAILED(path->ClosePath())) {
return CGRectNull;
}

if (FAILED(path->GetPathGeometry()->GetBounds(D2D1::IdentityMatrix(), &bounds))) {
return CGRectNull;
Expand All @@ -530,7 +544,7 @@ bool CGPathIsEmpty(CGPathRef path) {

UINT32 count;

path->ClosePath();
RETURN_FALSE_IF_FAILED(path->ClosePath());

RETURN_FALSE_IF_FAILED(path->GetPathGeometry()->GetFigureCount(&count));
return count == 0;
Expand Down Expand Up @@ -673,7 +687,7 @@ bool CGPathContainsPoint(CGPathRef path, const CGAffineTransform* transform, CGP

BOOL containsPoint = FALSE;

path->ClosePath();
RETURN_FALSE_IF_FAILED(path->ClosePath());
RETURN_FALSE_IF_FAILED(path->GetPathGeometry()->FillContainsPoint(_CGPointToD2D_F(point), D2D1::IdentityMatrix(), &containsPoint));

return (containsPoint ? true : false);
Expand Down
4 changes: 2 additions & 2 deletions Frameworks/CoreGraphics/D2DWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#import <CoreGraphics/CGGeometry.h>
#import <CoreGraphics/CGBase.h>

Microsoft::WRL::ComPtr<ID2D1Factory> _GetD2DFactoryInstance();
HRESULT _CGGetD2DFactory(ID2D1Factory** factory);

Microsoft::WRL::ComPtr<IWICImagingFactory> _GetWICFactory();

Expand All @@ -40,4 +40,4 @@ inline CGRect _D2DRectToCGRect(D2D1_RECT_F rect) {
CGFloat height = rect.bottom - y;

return CGRectMake(x, y, width, height);
}
}
20 changes: 13 additions & 7 deletions Frameworks/CoreGraphics/D2DWrapper.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,21 @@
using namespace Microsoft::WRL;

// Private helper for creating a D2DFactory
static ComPtr<ID2D1Factory> __createD2DFactory() {
ComPtr<ID2D1Factory> d2dFactory;
FAIL_FAST_IF_FAILED(D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, __uuidof(ID2D1Factory), &d2dFactory));
return d2dFactory;
static HRESULT __createD2DFactory(ID2D1Factory** factory) {
return D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, factory);
Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not using a magic static, there is no real point in having this be a separate function #Resolved

}

ComPtr<ID2D1Factory> _GetD2DFactoryInstance() {
static ComPtr<ID2D1Factory> factory = __createD2DFactory();
return factory;
HRESULT _CGGetD2DFactory(ID2D1Factory** factory) {
static ComPtr<ID2D1Factory> sFactory;
static HRESULT sHr;
static dispatch_once_t dispatchToken;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one static referring to another as part of its initialization is safe, then this should be totally fine.

dispatch_once(&dispatchToken, ^{
Copy link
Member

@bbowman bbowman Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did we go back to dispatch once? as mentioned before, dispatch_once doesn't buy you much except make code more dependent on a third party library and harder to read. I preferred the second helper function / you could do something with a c++ lambda if you are concerned about the "multi-return" / "return transforming" you need to do. That is a language feature that we can be more confident in than our pseudo hacked up libdispatch just to get a static initialized. #Resolved

sHr = __createD2DFactory(&sFactory);
});
sFactory.Get()->AddRef();
Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only CopyTo
Now you are doing +2. #Resolved

Copy link
Member

@bbowman bbowman Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sFactory.Get()->AddRef(); [](start = 4, length = 25)

!!!!!!!!!ACHTUNG!!!!!!!!!

manual AddRef / Release of Com Objects almost always speaks to a design patten issue. Especially when used in conjuction with ComPtr methods in other places.

What is the goal here? You are AddRefing once and then again in the CopyTo. Hooray extra amazing leakage. #Resolved

Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1307 (comment) 😉 #Closed

Copy link
Contributor

@aballway aballway Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

achtung? atchung sounds like an old heavy cash register #Closed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spell real goodly.

In reply to: 86616809 [](ancestors = 86616809)

*factory = sFactory.Get();
Copy link

@DHowett-MSFT DHowett-MSFT Nov 4, 2016 •
edited by MSFTFox
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer sFactory.CopyTo(pFactory) #Resolved

return sHr;
}

static ComPtr<IWICImagingFactory> __createWICFactory() {
Expand Down
6 changes: 6 additions & 0 deletions Frameworks/include/CGPathInternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
#include "CoreGraphics/CGContext.h"
#include "CoreGraphics/CGPath.h"

#include <COMIncludes.h>
#include <d2d1.h>
#include <COMIncludes_End.h>

const int kCGPathMaxPointCount = 3;

struct CGPathElementInternal : CGPathElement {
Expand Down Expand Up @@ -51,4 +55,6 @@ struct CGPathElementInternal : CGPathElement {
};
typedef struct CGPathElementInternal CGPathElementInternal;

HRESULT _CGPathGetGeometry(CGPathRef path, ID2D1Geometry** pGeometry);

#endif
4 changes: 2 additions & 2 deletions tests/unittests/CoreGraphics/CGPathTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ CGPathRef newPathForRoundRect(CGRect rect, CGFloat radius) {
return path;
}

TEST(CGPath, CGPathContainsPointOutsideRect) {
DISABLED_TEST(CGPath, CGPathContainsPointOutsideRect) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DISABLED_TEST [](start = 0, length = 13)

These tests should have been disabled initially since they involve work with arcs/curves. These will be re-enabled then.

CGFloat originX = 10.0f;
CGFloat originY = 20.0f;
CGFloat pathWidth = 100.0f;
Expand All @@ -438,7 +438,7 @@ CGPathRef newPathForRoundRect(CGRect rect, CGFloat radius) {
EXPECT_FALSE(test);
}

TEST(CGPath, CGPathContainsPointInsideRectOutsidePath) {
DISABLED_TEST(CGPath, CGPathContainsPointInsideRectOutsidePath) {
CGFloat originX = 10.0f;
CGFloat originY = 20.0f;
CGFloat pathWidth = 100.0f;
Expand Down