Create drawing tests for Core Text by aballway · Pull Request #1503 · microsoft/WinObjC

aballway · 2016-12-02T20:29:40Z

Cherry picks @DHowett-MSFT's framework for image comparison tests and adds several tests for supported Core Text draw methods and options. Fixes small issues found when comparing against reference platform results and when using custom fonts, but now compares against images generated by our platform for pixel comparison. All differences between our platform and the reference platform are documented in #1502.

This change is

DHowett-MSFT · 2016-12-02T20:43:24Z

+#include <memory>
+
+#include <CoreFoundation/CoreFoundation.h>
+#include <UIKit/UIKit.h>


#ifdef WINOBJC // NOTE or whatever #import <UIKit/UIKit.h> static void anchorUIKit() { [UIView class]; } #endif

This change set introduces DRAW_TEST and DRAW_TEST_F, as well as a handful of image drawing test fixtures. Commands rendered to a test's context as part of its test body will be rasterized and saved in a file named TestImage.$TESTCASE.$TESTNAME.png The test driver's ideal interface is multi-modal; it needs to be able to generate a corpus of reference images on disk, and it needs to be able to diff them. For this, it needs a custom EntryPoint (included), and the ability to parse command-line arguments. The ideal default mode will be one that loads reference images from disk and fails tests if a number of pixels differ. That work is not yet complete. Hopefully, the draw tests here will be able to be plugged into another module that can perhaps render them to screen, or display them as part of a UI-driven flow for comparison and demoing purposes. Refs microsoft#1271.

…oft#1434)

…t failure. (microsoft#1438) CoreGraphics.Drawing.UnitTests can now be invoked in one of two modes, mediated by command line flags: * `--compare[=/path/to/reference/folder]` (_default mode_) compare all rendered images against their baselines, failing the test if any images differ by even a single pixel. * `--generate [--out=/path/to/output/directory]` generate output images only (do not run comparisons.) A failing test will emit a brightly-colored diff image to `Greenlines.TestImage.CASE.TEST.png` in the _output directory_, and attach attributes to the gtest output XML data specifying where to find the images. A compatible test runner can load these images and display them side-by-side. **Open Questions/Concerns** * I do not like the design of `rgba/bgraPixels`. * I do not take stride into account; if the image is width-padded, the tests will ikely fail. * The reference platform loads images in RGBA, and we load them in BGRA. This is not an incompatibility, but it does make manual buffer manipulation dicey. * The fix here is to render the image into a NEW context, but that presents a few issues of its own: * We must trust our DrawImage function * We must further validate bitmap context, and trust our bitmap context as an integral part of test execution. * All tests will be disabled as of the merge of microsoft#1434. We can re-enable them and reintroduce reference images after build validation. Closes microsoft#1271.

…th CGD2D

aballway · 2016-12-05T18:54:32Z

Updated tests with custom font, squashed into two commits, and rebased on top of develop

DHowett-MSFT · 2016-12-06T06:39:20Z

When you merge this, please make sure to use the [Merge] Option. This will make conflict resolution in the CGD2D branch easier, as the commits have been cherry-picked.

DHowett-MSFT · 2016-12-06T06:39:48Z

The Rebase option may work as well.

aballway · 2016-12-06T17:46:02Z

Updated to disable tests on build machines, which seem to have minor differences in text drawing. Not sure if this is worth investigating. Tests can be run by enabling disabled tests

ms-jihua · 2016-12-06T19:45:33Z

+    </ProjectReference>
+    <ProjectReference Include="..\..\..\WinObjCRT\dll\WinObjCRT.vcxproj">
+      <Project>{585b4870-0d6b-43a6-8e7e-ad08f7f507b6}</Project>
+    </ProjectReference>


haven't gotten to the rest of the review yet but right now i'm skeptical that you need this? #Closed

The vcxproj or this reference? #ByDesign

This reference

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

Apparently required for SmartTypes #ByDesign

In reply to: 91159703 [](ancestors = 91159703,91159582)

ms-jihua · 2016-12-06T19:48:07Z

+        uint32_t unusedIndex;
+        RETURN_IF_FAILED(_userCreatedFontCollection->FindFamilyName(fontFamilyName, &unusedIndex, &userCreatedFont));
+        if (userCreatedFont) {
+            return dwriteFactory->CreateTextFormat(fontFamilyName,


ah geez i need to think about this in the context of my CR, but, is this actually needed? shouldn't usercreatedfontcollection have been registered to the factory?

It's registered but this only uses the given font collection, or system font collection if nullptr

Grrr

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

ms-jihua · 2016-12-06T19:50:44Z

        CGContextScaleCTM(ctx, 1.0f, -1.0f);

-        for (size_t i = 0; i < frame->_lineOrigins.size(); ++i) {
+        for (size_t i = 0; i < frame->_lineOrigins.size() && frame->_lineOrigins[i].y < frame->_frameRect.size.height; ++i) {


nit: parens

ms-jihua · 2016-12-06T19:52:10Z

+            // Forces run breaks without interfering with any layout features
+            // Necessary for attributes which DWrite does not support during layout (e.g. Color)
+            RETURN_NULL_IF_FAILED(typography->AddFontFeature({ DWRITE_FONT_FEATURE_TAG_KERNING, ++incompatibleAttributeFlag }));
        }


note to self to incorporate this for my CR if this gets in first

ms-jihua · 2016-12-06T20:03:01Z

 #endif

+#ifndef MAX_PATH
+#define MAX_PATH PATH_MAX


er, any reason why we can't just standardize on the underscore version?

I believe @DHowett-MSFT said underscoreless is standard?

Yeah: MAX_PATH lives in minwindef standalone and _MAX_PATH lives in stdlib with proximity to the standard library path functions.

ms-jihua · 2016-12-06T20:17:20Z

+    CGBlendMode blendMode;
+};
+
+CGNamedBlendMode porterDuffBlendModes[] =


static const, c_
same for your other constants below #WontFix

Will leave the CG* tests for @DHowett-MSFT to clean up :D

ms-jihua · 2016-12-06T21:19:13Z

@@ -0,0 +1,105 @@
+//******************************************************************************


This file has a lot of duplicated code. Can you refactor it into reusable functions or parameterized tests?

I'll take this on as I wrote this code. I'm hoping to reduce merge conflicts from develop in the meantime.

ms-jihua · 2016-12-06T21:20:02Z

@@ -0,0 +1,52 @@
+//******************************************************************************


same with this file, to a lesser extent

ms-jihua · 2016-12-06T21:30:54Z

+    CFStringRef keys[2] = { kCTFontAttributeName, kCTParagraphStyleAttributeName };
+    CFTypeRef values[2] = { myCFFont.get(), paragraphStyle.get() };
+
+    woc::unique_cf<CFDictionaryRef> dict{ CFDictionaryCreate(nullptr,


can probably refactor 389-403 here to a separate function, possibly 403-416 as well (would exclude CTRun, BasicDrawingTest, WhiteBackgroundTest though)

ms-jihua · 2016-12-06T21:35:06Z

+}
+
+CFStringRef _CFStringCreateAbsolutePath(CFStringRef relativePath) {
+    std::unique_ptr<char[]> owningFilenamePtr;


85-101 here is repeated from above

ms-jihua · 2016-12-06T21:36:01Z

nit: CoreGraphics.Drawing > CoreGraphics.drawing, probably

ms-jihua · 2016-12-06T21:36:21Z

🕐

DHowett-MSFT · 2016-12-06T22:18:54Z

ms-jihua

Discussed offline - in the interest of ease of merging with CGD2D, code quality-related comments will be taken care of separately at a later time by @DHowett-MSFT

aballway · 2016-12-06T22:51:16Z

Updated with changes to CTDrawingTests

DHowett-MSFT · 2016-12-06T23:56:05Z

msftclas added the cla-not-required label Dec 2, 2016

aballway assigned DHowett-MSFT and ms-jihua Dec 2, 2016

DHowett-MSFT reviewed Dec 2, 2016

View reviewed changes

aballway force-pushed the tests branch from ca829c8 to ee2e0c7 Compare December 2, 2016 21:03

DHowett and others added 4 commits December 5, 2016 09:59

Disable every drawing test manually instead of using exit(0). (micros…

e069009

…oft#1434)

Seperate commit for compatability with old CG, remove when merging wi…

e4912ff

…th CGD2D

aballway force-pushed the tests branch from ee2e0c7 to 96a9a53 Compare December 5, 2016 18:53

DHowett-MSFT mentioned this pull request Dec 5, 2016

Get D2D greenline tests running on develop #1420

Closed

aballway force-pushed the tests branch from 96a9a53 to a53be24 Compare December 6, 2016 17:44

ms-jihua reviewed Dec 6, 2016

View reviewed changes

ms-jihua approved these changes Dec 6, 2016

View reviewed changes

Add drawing tests for CoreText

a5c798d

aballway force-pushed the tests branch from a53be24 to a5c798d Compare December 6, 2016 22:49

msft-Jeyaram approved these changes Dec 6, 2016

View reviewed changes

aballway merged commit 502794e into microsoft:develop Dec 7, 2016

rajsesh mentioned this pull request Jan 6, 2017

Enable coretext automated unit tests so they run on build machine #1646

Closed

aballway deleted the tests branch January 12, 2017 18:56

		@@ -0,0 +1,105 @@
		//******************************************************************************

		@@ -0,0 +1,52 @@
		//******************************************************************************

Conversation

aballway commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway commented Dec 5, 2016

Uh oh!

DHowett-MSFT commented Dec 6, 2016

Uh oh!

DHowett-MSFT commented Dec 6, 2016

Uh oh!

aballway commented Dec 6, 2016

Uh oh!

ms-jihua Dec 6, 2016 • edited by aballway Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-jihua Dec 6, 2016 • edited by aballway Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-jihua commented Dec 6, 2016

Uh oh!

ms-jihua commented Dec 6, 2016

Uh oh!

DHowett-MSFT commented Dec 6, 2016

Uh oh!

ms-jihua left a comment

Choose a reason for hiding this comment

Uh oh!

aballway commented Dec 6, 2016

Uh oh!

DHowett-MSFT commented Dec 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

aballway commented Dec 2, 2016 •

edited

Loading

ms-jihua Dec 6, 2016 •

edited by aballway

Loading

aballway Dec 6, 2016 •

edited

Loading

ms-jihua Dec 6, 2016 •

edited by aballway

Loading