Merge #1705 to CGD2D #1750

ms-jihua · 2017-01-20T01:26:10Z

Add PushBeginDraw/PopEndDraw pairs to UISegment, UIImage
(not on develop since this likely would've actually hurt performance there)
Add Escape/Unescape pairs to CGContext areas where the target is changed

This change is

ms-jihua · 2017-01-20T01:27:16Z

Frameworks/CoreGraphics/CGContext.mm

@@ -2138,8 +2174,9 @@ void CGContextClearRect(CGContextRef context, CGRect rect) {
    ComPtr<ID2D1CommandList> commandList;
    RETURN_IF_FAILED(deviceContext->CreateCommandList(&commandList));

-    deviceContext->BeginDraw();
+    EscapeBeginEndDrawStack();
    deviceContext->SetTarget(commandList.Get());


@DHowett-MSFT I think this is more correct placed here, right? #Resolved

I think you're correct. I recall Begin->Set->End being specified in one of the MSDN documents. #Resolved

I think I lied and it doesn't matter https://msdn.microsoft.com/en-us/library/windows/desktop/hh404533(v=vs.85).aspx leaving as-is for now, will look more deeply at this in the followup PR

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

ms-jihua · 2017-01-20T01:31:45Z

Frameworks/CoreGraphics/CGContext.mm

@@ -2138,8 +2174,9 @@ void CGContextClearRect(CGContextRef context, CGRect rect) {
    ComPtr<ID2D1CommandList> commandList;
    RETURN_IF_FAILED(deviceContext->CreateCommandList(&commandList));

-    deviceContext->BeginDraw();
+    EscapeBeginEndDrawStack();


Because of the need to escape before changing targets, and because most functions in this file currently filter down to this function and thus change targets, #1705 has close-to-zero or probably negative impact on CGD2D as it currently is. Since we don't need a new command list each time this is called, I am going to add another iteration to this PR, tying the creation of new command lists to the stack. #WontFix

Going to do this in a separate PR instead

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

Please open a tracking issue to do this work. #Resolved

#1810

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

aballway

Approval pending command list iteration

aballway · 2017-01-20T17:05:29Z

Frameworks/UIKit/UISegment.mm

            }
        }
    } else {
        if (!_noDefaultImages) {
-            CGContextRef ctx = UIGraphicsGetCurrentContext();
+            CGContextRef ctx = ctx;


nit: not needed #Resolved

microsoft#1705) * CoreText Performance: Call ID2D1RenderTarget::BeginDraw()/EndDraw() fewer times Fixes microsoft#1620 * - Added Begin/EndDraw pairs to a few more places - Mitigated an issue with cairo where ID2D1RenderTarget would sometimes cache a before state during begin draw, and wipe out any changes cairo made, breaking APIs like CGContextFillRect. - Changed implementation/naming of the 'escape the current begin/end draw stack' functions

msft-Jeyaram · 2017-01-23T23:10:07Z

Frameworks/CoreGraphics/CGContext.mm

+
+    inline void EscapeBeginEndDrawStack() {
+        if ((_beginEndDrawDepth > 0) && ((_escapeBeginEndDrawDepth)++ == 0)) {
+            THROW_IF_FAILED(deviceContext->EndDraw());


THROW_IF_FAILED [](start = 12, length = 15)

who is catching the throws?
We standardized on FAIL_FAST #Resolved

The standardization we agreed on was to capture exceptions at the API boundary (which is usually the API entry point). Failing fast in the leaf nodes will lead to code that just arbitrarily quits with no possible recovery. Particularly when drawing to hardware device contexts, EndDraw() can fail, so we need to handle this with care. See #1194. #Pending

I'm not sure what you're actually advocating I do here, considering handling #1194 appears too large for the scope of this PR. As such, I'm going to take @msft-Jeyaram 's recommendation to follow the standard for now.

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

The recommendation is wrong, we agreed not to FAIL_FAST at arbitrary points in the CG code and instead throw. Any exceptions must be caught at the "C" entry point, where we decide either to return an error code/null or fail-fast if it is void.

In reply to: 97695158 [](ancestors = 97695158,97468410)

Although this is only one step away from the entry point.

In reply to: 97696710 [](ancestors = 97696710,97695158,97468410)

I am going to approve this PR given that this is only one step away from the entry point.

In reply to: 97696881 [](ancestors = 97696881,97696710,97695158,97468410)

i actually already made the change anyway

In reply to: 97697849 [](ancestors = 97697849,97696881,97696710,97695158,97468410)

msft-Jeyaram · 2017-01-23T23:10:21Z

Frameworks/CoreGraphics/CGContext.mm

+
+    inline void PopEndDraw() {
+        if (--(_beginEndDrawDepth) == 0) {
+            THROW_IF_FAILED(deviceContext->EndDraw());


same here #Resolved

msft-Jeyaram · 2017-01-23T23:14:27Z

rajsesh · 2017-01-24T03:10:02Z

Frameworks/include/CGContextInternal.h

+// For scenarios where a Begin/EndDraw pair needs to be temporarily escaped, to be returned to at a later time
+// Ie:
+//      - Switching render targets - Illegal to do so if currently in a Begin/EndDraw pair
+//      - Cairo - ID2D1RenderTarget is considered to 'own' the bitmap during Begin/EndDraw,


Can safely remove comment about cairo now. #Resolved

rajsesh · 2017-01-24T03:19:47Z

Frameworks/UIKit/UIView.mm


    CGRect bounds;
    // TODO(DH)
    bounds = self.bounds;
-    //bounds = CGContextGetClipBoundingBox(context);
+    // bounds = CGContextGetClipBoundingBox(context);


nit: remove commented out line. Also, the whole thing could be CGRect bounds = self.bounds, or even just [self drawRect:self.bounds] since it is not being reused. #WontFix

@DHowett-MSFT is taking care of this

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

@DHowett-MSFT, do we have an issue tracking this TODO? #Resolved

#1811 #Resolved

rajsesh · 2017-01-24T03:26:45Z

build/CoreGraphics/dll/CoreGraphics.def

@@ -242,6 +242,10 @@ LIBRARY CoreGraphics
        _CGContextCreateWithD2DRenderTarget
        _CGGetD2DFactory
        _CGGetPixelFormatProperties
+        _CGContextPushBeginDraw
+        _CGContextPopEndDraw
+        _CGContextEscapeBeginEndDrawStack


These names are not ideal - they are vague (what does escaping the stack mean), and reveal the implementation detail (there is a stack). The later is not a big deal, but essentially what you doing here is ability to deal with a layer, so it might make sense to just call this _CGContextForceEndDraw() and _CGContextForceBeginDraw(), or _CGContextBeginLayerDraw/EndLayerDraw(). #WontFix

but they don't force an enddraw or a begindraw, nor are they exclusively tied to layers. they are 'enddraw if needed' and 'begindraw if needed', where 'needed' is kind of vague (if currently in the stack) and difficult to describe without mentioning the stack.

i agree that these names are bad, but the suggestions are less factually accurate, so i am won'tfixing this for now.

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

I think BeginDrawIfNeeded(), EndDrawIfNeeded() are more descriptive than EscapeStack. Won't fix is okay. #WontFix

rajsesh · 2017-01-24T03:27:38Z

Frameworks/UIKit/UITextView.mm

@@ -759,7 +763,7 @@ - (CGSize)sizeThatFits:(CGSize)fitSize {
    centerRect.origin.y = 0;
    centerRect.size = fontExtent;
    // TODO(DH)
-    //EbrCenterTextInRectVertically(&centerRect, &fontExtent, _font);
+    // EbrCenterTextInRectVertically(&centerRect, &fontExtent, _font);


remove commented code? also looks like there is a missing TODO issue, lets create it now. #WontFix

@DHowett-MSFT is taking care of this

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

This might not be necessary, but I'll add an issue to track "something about vertical text centering in UITextField"

This is more of a CT concern, but EbrCenterTextInRectVertically was part of CGContext for some unknown reason. #Resolved

#1812 #Resolved

rajsesh · 2017-01-24T03:30:18Z

Frameworks/CoreGraphics/CGContext.mm

+
+    inline void EscapeBeginEndDrawStack() {
+        if ((_beginEndDrawDepth > 0) && ((_escapeBeginEndDrawDepth)++ == 0)) {
+            THROW_IF_FAILED(deviceContext->EndDraw());


The standardization we agreed on was to capture exceptions at the API boundary (which is usually the API entry point). Failing fast in the leaf nodes will lead to code that just arbitrarily quits with no possible recovery. Particularly when drawing to hardware device contexts, EndDraw() can fail, so we need to handle this with care. See #1194. #Pending

DHowett-MSFT

Contingent upon @rajsesh-msft's requested changes, I think this is safe to go in.

We can improve the drawing stack later, but this will unblock our merge.

DHowett-MSFT · 2017-01-25T00:47:30Z

Frameworks/CoreGraphics/CGContext.mm

@@ -2138,8 +2174,9 @@ void CGContextClearRect(CGContextRef context, CGRect rect) {
    ComPtr<ID2D1CommandList> commandList;
    RETURN_IF_FAILED(deviceContext->CreateCommandList(&commandList));

-    deviceContext->BeginDraw();
+    EscapeBeginEndDrawStack();
    deviceContext->SetTarget(commandList.Get());


I think you're correct. I recall Begin->Set->End being specified in one of the MSDN documents. #Resolved

ms-jihua · 2017-01-25T01:28:41Z

Then okay, let me just respond to the feedback on the table for now and get this actually performant later. #Resolved

DHowett-MSFT · 2017-01-25T02:10:10Z

Frameworks/CoreGraphics/CGContext.mm

+
+    inline void PopEndDraw() {
+        if (--(_beginEndDrawDepth) == 0) {
+            FAIL_FAST_IF_FAILED(deviceContext->EndDraw());


The true recommendation was to have inner functions return HRESULT and FAIL_FAST or ignore the error gracefully at the C API boundary. #Pending

- Add PushBeginDraw/PopEndDraw pairs to UISegment, UIImage (not on develop since this likely would've actually hurt performance there) - Add Escape/Unescape pairs to CGContext areas where the target is changed Fixes microsoft#1635 cr feedback cr feedback

msftclas added the cla-not-required label Jan 20, 2017

ms-jihua commented Jan 20, 2017

View reviewed changes

aballway approved these changes Jan 20, 2017

View reviewed changes

msft-Jeyaram reviewed Jan 23, 2017

View reviewed changes

rajsesh suggested changes Jan 24, 2017

View reviewed changes

DHowett-MSFT approved these changes Jan 25, 2017

View reviewed changes

ms-jihua mentioned this pull request Jan 25, 2017

Change targets fewer times in post-CGD2D CGContext #1810

Closed

ms-jihua force-pushed the enddraw_cgd2d_2 branch from 2ba1689 to 19bebee Compare January 25, 2017 01:55

DHowett-MSFT reviewed Jan 25, 2017

View reviewed changes

rajsesh approved these changes Jan 25, 2017

View reviewed changes

Merge microsoft#1705 to CGD2D

0ac5491

- Add PushBeginDraw/PopEndDraw pairs to UISegment, UIImage (not on develop since this likely would've actually hurt performance there) - Add Escape/Unescape pairs to CGContext areas where the target is changed Fixes microsoft#1635 cr feedback cr feedback

ms-jihua force-pushed the enddraw_cgd2d_2 branch from a7c7161 to 0ac5491 Compare January 25, 2017 04:27

ms-jihua merged commit 795c98c into microsoft:CGD2D Jan 25, 2017

DHowett-MSFT mentioned this pull request Jan 25, 2017

[CGD2D] Merge develop from 20170125. #1824

Merged

ms-jihua mentioned this pull request Jan 28, 2017

[CGD2D] CGContextClearRect crashes if used under CGContextPushBegin/PopEndDraw #1869

Closed

Merge #1705 to CGD2D #1750

Merge #1705 to CGD2D #1750

Conversation

ms-jihua commented Jan 20, 2017 • edited by rajsesh Loading

ms-jihua Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

ms-jihua Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

ms-jihua Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsesh Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway left a comment

Choose a reason for hiding this comment

aballway Jan 20, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

msft-Jeyaram Jan 23, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

rajsesh Jan 24, 2017 • edited by ms-jihua 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

msft-Jeyaram Jan 23, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

msft-Jeyaram commented Jan 23, 2017

rajsesh Jan 24, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

rajsesh Jan 24, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsesh Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

DHowett-MSFT Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

rajsesh Jan 24, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsesh Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

rajsesh Jan 24, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

DHowett-MSFT Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

rajsesh Jan 24, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

ms-jihua commented Jan 25, 2017 • edited Loading

DHowett-MSFT Jan 25, 2017 • edited by ms-jihua Loading

Choose a reason for hiding this comment

ms-jihua commented Jan 20, 2017 •

edited by rajsesh

Loading

ms-jihua Jan 20, 2017 •

edited

Loading

DHowett-MSFT Jan 25, 2017 •

edited by ms-jihua

Loading

ms-jihua Jan 25, 2017 •

edited

Loading

ms-jihua Jan 20, 2017 •

edited

Loading

rajsesh Jan 25, 2017 •

edited by ms-jihua

Loading

aballway Jan 20, 2017 •

edited by ms-jihua

Loading

msft-Jeyaram Jan 23, 2017 •

edited by ms-jihua

Loading

rajsesh Jan 24, 2017 •

edited by ms-jihua

Loading

msft-Jeyaram Jan 23, 2017 •

edited by ms-jihua

Loading

rajsesh Jan 24, 2017 •

edited by ms-jihua

Loading

rajsesh Jan 24, 2017 •

edited by ms-jihua

Loading

rajsesh Jan 25, 2017 •

edited by ms-jihua

Loading

DHowett-MSFT Jan 25, 2017 •

edited by ms-jihua

Loading

rajsesh Jan 24, 2017 •

edited by ms-jihua

Loading

rajsesh Jan 25, 2017 •

edited by ms-jihua

Loading

rajsesh Jan 24, 2017 •

edited by ms-jihua

Loading

DHowett-MSFT Jan 25, 2017 •

edited by ms-jihua

Loading

DHowett-MSFT Jan 25, 2017 •

edited by ms-jihua

Loading

rajsesh Jan 24, 2017 •

edited by ms-jihua

Loading

DHowett-MSFT Jan 25, 2017 •

edited by ms-jihua

Loading

ms-jihua commented Jan 25, 2017 •

edited

Loading

DHowett-MSFT Jan 25, 2017 •

edited by ms-jihua

Loading