Add support for "enhanced error handling" to CGContext and CALayer #2119

DHowett-MSFT · 2017-03-01T02:31:39Z

Direct2D can emit a failure or remove the backing hardware device from your render target at the end of any drawing operation. However, CoreGraphics does not natively have support for communicating failure states out to interested consumers.

Enhanced Error Handling, when enabled, allows a sufficiently motivated developer to become part of CGContext's rendering lifecycle.

The error of first and foremost importance is D2DERR_RECREATE_TARGET: It is not an error, but a plea to retry the last rendering operation after regenerating the render target. While CoreGraphics could batch up a list of pending drawing operations and replay them on a new target, it opts instead to report this to an Enhanced Error Handling consumer through kCGContextErrorDeviceReset.

All other failing drawing operations result in kCGContextErrorInvalidParameter.

In this pull request, CALayer has been augmented (in a bid to better prepare us for a hardware-accelerated future) to retry its rendering up to three (3) times if the device goes away. Since a layer can draw itself from start to finish, -display is the best point at which to handle the newly-minted CGContext errors.

Fixes #1194.

DHowett-MSFT · 2017-03-01T02:31:52Z

Frameworks/CoreGraphics/CGContext.mm

@@ -47,6 +47,7 @@
 #import <cmath>
 #import <list>
 #import <vector>
+#import <array>


DHowett-MSFT · 2017-03-01T02:32:12Z

Frameworks/CoreGraphics/CGContext.mm

@@ -471,21 +475,32 @@ inline void ClearPath() {
    }

    inline bool ShouldDraw() {
-        return CurrentGState().ShouldDraw();
+        return FAILED(_firstErrorHr) || CurrentGState().ShouldDraw();


Hilarious. prefer SUCCEEDED or !FAILED #Closed

And probably && #Closed

aballway · 2017-03-01T02:51:10Z

Frameworks/CoreGraphics/CGContext.mm

@@ -274,6 +275,9 @@ inline HRESULT GetTarget(ID2D1Image** pTarget) {
    // Since nothing needs to actually be put on a stack, just increment a counter insteads
    std::atomic_uint32_t _beginEndDrawDepth = { 0 };

+    bool _useEnhancedErrorHandling{ false };
+    HRESULT _firstErrorHr{ S_OK };


nit: just _firstError, no need to put type in name

aballway · 2017-03-01T02:55:40Z

Frameworks/CoreGraphics/CGContext.mm

            deviceContext->BeginDraw();
        }
    }

    inline HRESULT PopEndDraw() {
+        HRESULT hr = S_OK;
        if (--(_beginEndDrawDepth) == 0) {


in PushBeginDraw we'll not increment _beginEndDrawDepth if FAILED, but here we always decrement, so we may wrap around into the middle of nowhere. Should we even attempt to draw here if the context failed at some previous point? #Resolved

aballway · 2017-03-01T02:56:50Z

Frameworks/CoreGraphics/CGContext.mm

+#pragma endregion
+
+#pragma region Enhanced Error Handling
+const CFStringRef kCGErrorDomainIslandwood = CFSTR("kCGErrorDomainIslandwood");


nit: WinObjC?

@aballway hm. we go back and forth on this a lot.

I like standardizing on WinOBJC :)

I promise I removed the comment above before submitting my review. :/

aballway · 2017-03-01T02:58:19Z

Frameworks/CoreGraphics/CGContext.mm

+        return hr;
+    }
+
+    inline void EnableEnhancedErrorHandling() {


Would this make more sense as a global setting vs. per context? #WontFix

@aballway no; external bitmap context consumers are not enlightened as to error handling. #ByDesign

aballway · 2017-03-01T02:59:16Z

Frameworks/CoreGraphics/CGContext.mm

+    auto error = woc::MakeStrongCF<CFErrorRef>(CFErrorCreate(nullptr, kCGErrorDomainIslandwood, errorCode, nullptr));
+
+    if (outError) {
+        *outError = error.detach();


nit: just make the error here, rather than making StrongCF and detaching #Resolved

cleverer. #Closed

aballway · 2017-03-01T03:05:03Z

Frameworks/QuartzCore/CALayer.mm

-            CGContextSetFillColorWithColor(drawContext, [static_cast<UIColor*>(priv->_backgroundColor) CGColor]);
+            // UIKit and CALayer consumers expect the origin to be in the top left.
+            // CoreGraphics defaults to the bottom left, so we must flip and translate the canvas.
+            CGContextTranslateCTM(drawContext, 0, heightInPoints);


nit: squish this into one transformation like in the other places this is done

bah, i suppose ;)

aballway · 2017-03-01T03:06:50Z

Frameworks/QuartzCore/CALayer.mm

+            priv->savedContext = CGContextRetain(drawContext);
+            priv->contents = CGImageRetain(target);
+            break;
+        } while (tries < 3);


nit: this is a for loop with a fake nose and a handlebar mustache

also, no magic number. Maybe stuff it in the context (if we keep the enabled flag in there) or a global that can be set by user #Resolved

I don't know that there's worth in exposing this. #Resolved

I don't know about exposing it, but I agree that giving it a name/placing it next to the declaration of tries might be slightly more readable. #Resolved

aballway · 2017-03-01T03:09:25Z

Frameworks/QuartzCore/CALayer.mm

-        } else {
-            priv->contents = target;
+        if (!priv->contents) {
+            NSTraceError(TAG, @"Failed to render layer %@", self);
        }
    } else if (priv->contents) {


nit: I know you didn't modify this but please get rid of the if here

aballway · 2017-03-01T03:12:34Z

Frameworks/QuartzCore/CALayer.mm

+                        NSTraceInfo(TAG, @"Hardware device disappeared when rendering %@; retrying.", self);
+                        ++tries;
+                        continue;
+                    default: {


nit: use the constant here for this case, and have a default "something went terribly wrong" case. #ByDesign

@aballway default IS the "something went terribly wrong" case. #ByDesign

aye but it could also be "something went terribly horribly wrong" where we don't even have a name for it 😜 #ByDesign

msft-Jeyaram · 2017-03-01T06:18:57Z

Frameworks/CoreGraphics/CGContext.mm

@@ -2367,6 +2382,10 @@ void CGContextClearRect(CGContextRef context, CGRect rect) {
 HRESULT __CGContext::Draw(_CGCoordinateMode coordinateMode, CGAffineTransform* additionalTransform, Lambda&& drawLambda) {
    auto& state = CurrentGState();

+    if (FAILED(_firstErrorHr)) {


nit: I assume S_FALSE is valid to have here?
or should we take care of that?

S_FALSE is a valid "success" return :)

msft-Jeyaram · 2017-03-01T06:24:20Z

Frameworks/CoreGraphics/CGContext.mm

        if (--(_beginEndDrawDepth) == 0) {
-            RETURN_IF_FAILED(deviceContext->EndDraw());
+            hr = deviceContext->EndDraw();


i'm thinking about S_FALSE, looking at the documentation, i don't see much about it for EndDraw(), but given it's technically a success, I assume we don't have to worry about it? #ByDesign

msft-Jeyaram · 2017-03-01T06:32:25Z

Frameworks/CoreGraphics/CGContext.mm

@@ -2402,12 +2421,6 @@ void CGContextClearRect(CGContextRef context, CGRect rect) {
    }

    auto cleanup = wil::ScopeExit([this, layer]() {


auto cleanup = wil::ScopeExit(this, layer { [](start = 3, length = 48)

no longer needed?

Good catch.

msft-Jeyaram · 2017-03-01T06:58:33Z

Frameworks/QuartzCore/CALayer.mm

-                                  priv->backgroundColor.a);
-        } else {
-            CGContextClearToColor(drawContext, 0, 0, 0, 0);
+                CGRect wholeRect = CGRectMake(0, 0, width, height);


ect wholeRect = CGRectMake(0, 0, width, height); [](start = 19, length = 48)

nit: you can directly pass it

msft-Jeyaram · 2017-03-01T07:03:09Z

include/CoreGraphics/CGContext.h

+// A developer using enhanced error handling MUST check the status of a completed set of drawing
+// operations using CGContextIwGetError() and determine whether to redraw the frame.
+COREGRAPHICS_EXPORT void CGContextIwEnableEnhancedErrorHandling(CGContextRef context);


maybe we should have some tag to indicate we have an extension?

Also it would make it easier to look at all the extensions we support/have

msft-Jeyaram · 2017-03-01T07:12:22Z

include/CoreGraphics/CGContext.h

+
+typedef CF_ENUM(CFIndex, CGContextIwErrorCode) {
+    kCGContextErrorDeviceReset,


nit: set the values explicitly to reduce future mishaps (if it were to ever happen)

msft-Jeyaram · 2017-03-01T07:14:32Z

include/CoreGraphics/CGContext.h

+};
+
+COREGRAPHICS_EXPORT bool CGContextIwGetError(CGContextRef context, CFErrorRef* /* returns-retained */ error);


/* returns-retained */ [](start = 79, length = 22)

since this is a public api and we are adding it, we should add c/c++ style document to it, to explicitly say what it returns and what's going on.

ms-jihua · 2017-03-01T22:05:13Z

Frameworks/CoreGraphics/CGContext.mm

-        if (!FAILED(_firstErrorHr) && (_beginEndDrawDepth)++ == 0) {
-            deviceContext->BeginDraw();
+        if ((_beginEndDrawDepth)++ == 0) {
+            if (__builtin_expect(SUCCEEDED(_firstErrorHr), true)) {


fancy #Resolved

DHowett-MSFT · 2017-03-01T22:07:04Z

The doc comment format here matches that used in CoreFoundation (!)

aballway · 2017-03-01T22:08:57Z

Frameworks/CoreGraphics/CGContext.mm

+        return false;
+    }
+
+    CGContextIwErrorCode errorCode = kCGContextErrorInvalidParameter;


nit: nice case for a ternary

msft-Jeyaram · 2017-03-01T22:28:45Z

Frameworks/QuartzCore/CALayer.mm

@@ -85,6 +86,10 @@
 NSString* const kCAFilterNearest = @"kCAFilterNearest";
 NSString* const kCAFilterTrilinear = @"kCAFilterTrilinear";

+// The number of rendering attempts a CALayer will make when
+// its backing device disappears.
+static const unsigned int _kCALayerRenderAttempts = 3;


super nit: _skCA.. ?

or s_kCA... or sc_CA...

How about static_const_CFStyleConstantWithAK_CALayer...

or just inline the value since it's only used in one place 😉

msft-Jeyaram

💯

DHowett-MSFT · 2017-03-01T23:23:52Z

That can't be good.

2017-03-01T23:15:21.8217323Z ##[error]frameworks\coregraphics\cgcontext.mm(482,0):
  Error C1001: An internal error has occurred in the compiler.

rajsesh · 2017-03-01T05:49:11Z

Frameworks/CoreGraphics/CGContext.mm

+    }
+
+    CGContextIwErrorCode errorCode = kCGContextErrorInvalidParameter;
+    switch (hr) {


switch missing default. Also could use an if here instead. Other errors are likely catastrophic with no way to recover.

rajsesh · 2017-03-02T00:10:00Z

Frameworks/CoreGraphics/CGContext.mm

    }

    inline void PushBeginDraw() {
        if ((_beginEndDrawDepth)++ == 0) {
-            deviceContext->BeginDraw();
+            if (__builtin_expect(SUCCEEDED(_firstErrorHr), true)) {


other than the ICE, there is really no need to use __builtin_expect to optimize here - this is not a hot code path :).

rajsesh · 2017-03-02T00:12:18Z

Frameworks/CoreGraphics/CGContext.mm

+#pragma endregion
+
+#pragma region Enhanced Error Handling
+const CFStringRef kCGErrorDomainIslandwood = CFSTR("kCGErrorDomainIslandwood");


I like standardizing on WinOBJC :)

rajsesh · 2017-03-02T00:22:15Z

Frameworks/QuartzCore/CALayer.mm

+                        continue;
+                    default: {
+                        NSTraceError(TAG, @"Error %@ rendering %@.", renderError.get(), self);
+                        break;


Fail_fast here instead. It would give us enough to up the number of retries. Another thing to note is that we should probably have another counter that falls back to warp if the particular device fails many times. That is, if each frame takes 2 attempts to draw, we are probably much better off using warp. This of course isn't necessary until we move to h/w acceleration.

DHowett-MSFT · 2017-03-02T00:36:44Z

Frameworks/QuartzCore/CALayer.mm

@@ -577,7 +577,10 @@ - (void)display {
                        ++tries;
                        continue;
                    default: {
-                        NSTraceError(TAG, @"Error %@ rendering %@.", renderError.get(), self);
+                        FAIL_FAST_MSG("Failed to render <%hs %p>: %hs",


The error description can be augmented to include the failing HRESULT, printing it plus the layer should allow us to correlate events and layer classes.

rajsesh · 2017-03-02T00:37:30Z

Nit: Could you update the description with Fixes# before merge?

rajsesh · 2017-03-02T00:38:28Z

Frameworks/CoreGraphics/CGContext.mm

+#pragma endregion
+
+#pragma region Enhanced Error Handling
+const CFStringRef kCGErrorDomainIslandwood = CFSTR("kCGErrorDomainIslandwood");


I promise I removed the comment above before submitting my review. :/

DHowett-MSFT · 2017-03-02T00:45:34Z

@msft-Jeyaram: Whichever of this pull request or #2133 merges second will have to add error handling to __CGContext::ClearRect

DHowett-MSFT · 2017-03-02T00:46:18Z

@MSFTFox: Whichever of this pull request or #2019 merges second will have to endure a merge conflict.

DHowett-MSFT · 2017-03-02T01:19:46Z

Ugh, the build agent is stuck downloading LFS file again.

DHowett-MSFT · 2017-03-02T21:23:04Z

The latest change here adds this cool thing:

E/WIL: E:\src\WinObjC\Frameworks\QuartzCore\CALayer.mm(583)\(null)!01E2C3CE:
    (caller: 01E2B337) FailFast(1) tid(5438) 8000FFFF Catastrophic failure
    Msg:[Failed to render <CALayer 13A3A984>:
    Error Domain=kCGErrorDomainIslandwood Code=2 "(null)"
    UserInfo={hresult=-2003238902}] [-[CALayer display]]

The important bit being hresult=-2003238902 😄

DHowett-MSFT · 2017-03-02T21:30:24Z

Frameworks/QuartzCore/CALayer.mm

-            } else {
-                CGContextClearToColor(drawContext, 0, 0, 0, 0);
-
+            if (priv->_backgroundColor != nil && (int)[static_cast<UIColor*>(priv->_backgroundColor) _type] != solidBrush) {


@MSFTFox this is straight from your pull request. however, i'm concerned about what it's checking.

It looks like it will NOT fill the background if there is a solid color/solid brush background color?

…ror-handling

aballway · 2017-03-02T23:17:16Z

Frameworks/QuartzCore/CALayer.mm

@@ -524,51 +531,69 @@ - (void)display {
            return;
        }

-        // Create the contents
-        CGContextRef drawContext = CreateLayerContentsBitmapContext32(width, height, priv->contentsScale);
+        unsigned int tries = 0;


nit: for (unsigned int tries = 0; tries < _kCA...; ++tries)

aballway · 2017-03-02T23:17:57Z

Frameworks/QuartzCore/CALayer.mm

+                                      object_getClassName(self),
+                                      self,
+                                      [[static_cast<NSError*>(renderError.get()) debugDescription] UTF8String]);
+                        break;


nit: do we need a break if we're FAIL_FAST?

Enhanced error handling.

86de6e0

DHowett-MSFT requested review from aballway, jaredhms and rajsesh March 1, 2017 02:31

msftclas added the cla-not-required label Mar 1, 2017

DHowett-MSFT commented Mar 1, 2017

View reviewed changes

aballway suggested changes Mar 1, 2017

View reviewed changes

msft-Jeyaram reviewed Mar 1, 2017

View reviewed changes

Address code review feedback.

9a921c5

ms-jihua reviewed Mar 1, 2017

View reviewed changes

aballway approved these changes Mar 1, 2017

View reviewed changes

Argh.

af8b6c4

msft-Jeyaram reviewed Mar 1, 2017

View reviewed changes

msft-Jeyaram approved these changes Mar 1, 2017

View reviewed changes

MSFTFox approved these changes Mar 1, 2017

View reviewed changes

rajsesh suggested changes Mar 2, 2017

View reviewed changes

Fix __builtin_expect & address CR feedback.

27e3fce

DHowett-MSFT commented Mar 2, 2017

View reviewed changes

rajsesh approved these changes Mar 2, 2017

View reviewed changes

DHowett-MSFT mentioned this pull request Mar 2, 2017

Improve CAShapeLayer #2134

Merged

Store the HRESULT in the error UserInfo for later retrieval.

7367cf6

Merge remote-tracking branch 'github/develop'.

24cb899

DHowett-MSFT commented Mar 2, 2017

View reviewed changes

DHowett added 2 commits March 2, 2017 14:59

Merge remote-tracking branch 'github/develop' into 201702-enhanced-er…

a645ead

…ror-handling

Guard ClearRect with firstfail checks as well.

fd0f2ec

msft-Jeyaram approved these changes Mar 2, 2017

View reviewed changes

aballway approved these changes Mar 2, 2017

View reviewed changes

DHowett-MSFT merged commit abfaa3f into microsoft:develop Mar 3, 2017

DHowett-MSFT deleted the 201702-enhanced-error-handling branch March 3, 2017 00:39

		@@ -2402,12 +2421,6 @@ void CGContextClearRect(CGContextRef context, CGRect rect) {
		}

		auto cleanup = wil::ScopeExit([this, layer]() {


		typedef CF_ENUM(CFIndex, CGContextIwErrorCode) {
		kCGContextErrorDeviceReset,

		};

		COREGRAPHICS_EXPORT bool CGContextIwGetError(CGContextRef context, CFErrorRef* /* returns-retained */ error);

Add support for "enhanced error handling" to CGContext and CALayer #2119

Add support for "enhanced error handling" to CGContext and CALayer #2119

Conversation

DHowett-MSFT commented Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

ms-jihua Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua Mar 1, 2017 • edited by DHowett-MSFT Loading

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsesh commented Mar 2, 2017

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 2, 2017

DHowett-MSFT commented Mar 2, 2017

DHowett-MSFT commented Mar 2, 2017

DHowett-MSFT commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 1, 2017 •

edited

Loading

DHowett-MSFT Mar 1, 2017 •

edited

Loading

DHowett-MSFT Mar 1, 2017 •

edited

Loading

aballway Mar 1, 2017 •

edited by DHowett-MSFT

Loading

aballway Mar 1, 2017 •

edited by DHowett-MSFT

Loading

DHowett-MSFT Mar 1, 2017 •

edited

Loading

aballway Mar 1, 2017 •

edited by DHowett-MSFT

Loading

DHowett-MSFT Mar 1, 2017 •

edited

Loading

aballway Mar 1, 2017 •

edited by DHowett-MSFT

Loading

DHowett-MSFT Mar 1, 2017 •

edited

Loading

ms-jihua Mar 1, 2017 •

edited by DHowett-MSFT

Loading

aballway Mar 1, 2017 •

edited by DHowett-MSFT

Loading

DHowett-MSFT Mar 1, 2017 •

edited

Loading

aballway Mar 1, 2017 •

edited by DHowett-MSFT

Loading

msft-Jeyaram Mar 1, 2017 •

edited by DHowett-MSFT

Loading

ms-jihua Mar 1, 2017 •

edited by DHowett-MSFT

Loading

DHowett-MSFT commented Mar 1, 2017 •

edited

Loading