[coreimage] Update for Xcode 11[.2] #7216

spouliot · 2019-10-10T19:32:39Z

Apple decided to expose most (but not all) CIFilter using protocols
(instead of weakly named dictionaries). Most of this maps well with
our strong bindings but there are cases where we:

missing some properties (easy, there were added); or
used a different types [1] and that requires new members / obsoletion

[1] Often ours are better (using float for a bool value is not
optimal) but we do not have [BindAs] for protocols :( to fix them

Note: this replace draft PR #7120 but it's has quite a bit of changes in filter generation (inlining protocols) and that affected bindings too.

Apple decided to expose most (but not all) `CIFilter` using protocols (instead of weakly named dictionaries). Most of this maps well with our strong bindings but there are cases where we: * missing some properties (easy, there were added); or * used a different types [1] and that requires new members / obsoletion [1] Often ours are better (using `float` for a `bool` value is not optimal) but we do not have `[BindAs]` for protocols :( to _fix_ them Note: this replace draft PR xamarin#7120 but it's has quite a bit of changes in filter generation (inlining protocols) and that affected bindings too.

VincentDondain

Re-read everything and I could not spot anything so LGTM 👍

This PR is a beast :P

VincentDondain · 2019-10-10T19:40:00Z

src/coreimage.cs

-	interface CIKeystoneCorrectionHorizontal {
+	interface CIKeystoneCorrectionHorizontal : ICIKeystoneCorrectionHorizontalProtocol {
+
+#if false // no documentation about the type


I thought we tried not to push dead code 😉. Couldn't it just be in the xtro ignore file with a comment?

there's (almost) no headers for CIFilters, so no xtro reminders

almost because some stuff is now defined in protocols - and those are in headers, but the rest is not (it's all key/value based)

VincentDondain · 2019-10-10T19:45:51Z

src/coreimage.cs

+	// `CILinearGradientProtocol` is a bit of a lie - but it would not compile (registrar) otherwise
+	interface CISmoothLinearGradientProtocol : CILinearGradientProtocol {
+
+		/* we get those from ICILinearGradientProtocol


So if we're keeping this code in case it's ever added again, wouldn't a #if false be better?
Similar to my comment above I don't know if we want dead code (:

Good catch, left over from the original attempt. I think that's fixed (properly) now. I'll uncomment it

uncommented (that scenario works fine now)

dalexsoto · 2019-10-10T21:32:20Z

src/coreimage.cs

@@ -2645,29 +2788,23 @@ interface CIImageProcessorKernel {
 	[iOS (8,0)]
 	[Mac (10,10)]
 	[BaseType (typeof (CIFilter))]
-	interface CIAccordionFoldTransition {
+	interface CIAccordionFoldTransition : ICIAccordionFoldTransitionProtocol {


@spouliot I see you are using the I version of CIAccordionFoldTransitionProtocol is that intentional? IIRC you have to use the I less version when doing this else the generator won't do the right thing.

Also I see this being mixed in the whole PR, for example the following does what I expect :)

interface CIAccordionFoldTransitionProtocol : CITransitionFilterProtocol {

good catch, I'll review those

CIFilter subclasses are generated differently that normal NSObjectso the end-result might be (nearly) identical. IOW I think my generator changes are not correct, with incorrect data giving the correct output.

mandel-macaque · 2019-10-10T21:24:47Z

src/CoreImage/CIFilter.cs

+				if (v is NSNumber)
+					return (v as NSNumber).NIntValue;


Can it be done with just using the as? Like:

var n = v as NSNumber; return n?.NIntValue ?? 0;

if (v is NSNumber n) return n.NIntValue;

it was a copy/paste of the existing GetInt method, but I can fix all of them
who's counting the lines anyway :)

fixed in newer commits

mandel-macaque · 2019-10-10T21:27:04Z

src/CoreImage/CIFilter.cs

 		{
-			return ValueForKey (CIFilterInputKey.Image) as CIImage;
+			var v = ValueForKey (key) as CIVector;


as can return null, right? So unless we are 100% sure.. I'd check that.

mandel-macaque · 2019-10-10T21:27:30Z

src/CoreImage/CIFilter.cs

-			using (var nsstr = new NSString (key))
-				return ValueForKey (nsstr) as CIImage;
+			var v = ValueForKey (key) as CIVector;
+			return new CGRect (v.X, v.Y, v.Z, v.W);


mandel-macaque · 2019-10-10T21:32:03Z

src/coreimage.cs

+
+		[iOS (13,0)][TV (13,0)][Watch (6,0)][Mac (10,15)]
+		[Field ("kCIImageAuxiliarySemanticSegmentationTeethMatte")]
+		NSString AuxiliarySemanticSegmentationTeethMatteKey { get; }


This look like they could be a smart enum.

it's actually a inside a type used tp create a strong dictionary [StrongDictionary ("CIImageInitializationOptionsKeys")] but I did not add the keys to it :| I'll fix this

Makes me wish for #6785 even more

mandel-macaque · 2019-10-10T21:33:40Z

src/coreimage.cs

+
+		[iOS (13,0)][TV (13,0)][Watch (6,0)][Mac (10,15)]
+		[Static]
+		[Wrap ("FromCGImageSource (source, index, options == null ? null : options.Dictionary)")]


No need to change, but I think options?.Dictionary would be the same.

mandel-macaque · 2019-10-10T21:34:05Z

src/coreimage.cs

+		IntPtr Constructor (CGImageSource source, nuint index, [NullAllowed] NSDictionary options);
+
+		[iOS (13,0)][TV (13,0)][Watch (6,0)][Mac (10,15)]
+		[Wrap ("this (source, index, options == null ? null : options.Dictionary)")]


Same options?.Dictionary should be the same.

mandel-macaque · 2019-10-10T21:34:52Z

src/coreimage.cs

+		[Static]
+		[Export ("imageWithSemanticSegmentationMatte:options:")]
+		[return: NullAllowed]
+		CIImage FromSemanticSegmentationMatte (AVSemanticSegmentationMatte matte, [NullAllowed] NSDictionary options);


Do we know about the options? Could it be a strong dictionary?

mandel-macaque · 2019-10-10T21:35:10Z

src/coreimage.cs

+
+		[TV (13,0), iOS (13,0), Mac (10,15)]
+		[Export ("initWithSemanticSegmentationMatte:options:")]
+		IntPtr Constructor (AVSemanticSegmentationMatte matte, [NullAllowed] NSDictionary options);


Do we know about the options? Could it be a string dictionary?

string ? no, strong ? maybe :)

It was not documented before but now that
typedef NSString *CIImageOption;
was added we can be pretty sure it's the same as the one I mentioned above

However there's a lot of older members that also use NSDictionary so, to keep this PR from calling unicorns, I'll fix that in a separate PR.

mandel-macaque · 2019-10-10T21:40:42Z

src/coreimage.cs

+
+		[iOS (13,0)][TV (13,0)][Watch (6,0)][Mac (10,15)]
+		[Field ("kCIImageRepresentationSemanticSegmentationTeethMatteImage")]
+		NSString SemanticSegmentationTeethMatteImageKey { get; }


These look like they could be a smart enum.

no, but it's part of the type used for a strong dictionary: CIImageRepresentationOptions

I'll look if the types are documented, we are already missing a few of those

yes, headers (not web doc) do mention the types of those (and the one we were missing from last year)

mandel-macaque · 2019-10-10T21:42:08Z

src/coreimage.cs

+
+		[Static]
+		[NullAllowed, Export ("customAttributes")]
+		NSDictionary<NSString, NSObject> CustomAttributes { get; }


Do we know anything about the attributes? Strong dict will be nicer if possible.

no :(
https://developer.apple.com/documentation/coreimage/cifilter/3228047-customattributes?language=objc

monojenkins · 2019-10-11T00:37:19Z

Build failure
✅ Build succeeded
⚠️ Mono built from source
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

22 tests failed, 73 tests passed.

Failed tests

apitest/Mac Modern/Debug: Failed (Test run crashed (exit code: 134).)
introspection/Mac Modern/Debug: Failed (Test run failed.)
xammac tests/Mac Modern/Debug: Failed (Test run crashed (exit code: 134).)
xammac tests/Mac Modern/Release: Failed (Test run crashed (exit code: 134).)
xammac tests/Mac Modern/Release: Failed (Test run crashed (exit code: 134).)
apitest/Mac Full/Debug: Failed (Test run crashed (exit code: 134).)
monotouch-test/iOS Unified 32-bits - simulator/Debug: Failed
introspection/iOS Unified 32-bits - simulator/Debug: Failed
monotouch-test/iOS Unified 32-bits - simulator/Debug (LinkSdk): Failed
monotouch-test/iOS Unified 32-bits - simulator/Debug (static registrar): Failed
monotouch-test/iOS Unified 32-bits - simulator/Release (all optimizations): Failed
introspection/iOS Unified 32-bits - simulator/Debug (iOS 10.3): Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug: Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (LinkSdk): Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (static registrar): Failed
monotouch-test/iOS Unified 64-bits - simulator/Release (all optimizations): Failed
monotouch-test/tvOS - simulator/Debug: Failed
monotouch-test/tvOS - simulator/Debug (LinkSdk): Failed
monotouch-test/tvOS - simulator/Debug (static registrar): Failed
monotouch-test/tvOS - simulator/Release (all optimizations): Failed
introspection/iOS Unified 64-bits - simulator/Debug (iOS 10.3): Failed
introspection/tvOS - simulator/Debug (tvOS 10.2): Failed

rolfbjarne · 2019-10-11T05:33:05Z

src/generator-filters.cs

+			// we can skip the name when it's identical to a protocol selector
+			if (name == null) {
+				if (export == null)
+					throw new BindingException (9999, true, $"Missing [CoreImageFilterProperty] attribute on {type.Name} property {ptype}");


Actual error code is needed.

Also don't use string interpolation (we'll have to make everything use string.Format-syntax when we start translating text anyway).

hmm... how would translating {0} be different from {ptype} ?

The problem isn't translating it, but using the translated string in C#. String interpolation requires the string to be a literal string, and translated strings aren't literal (https://stackoverflow.com/a/32360534/183422)

… ones) to simplify (all of) them

…nderlying type)

…nd remove [Watch] attributes since that framework is not (yet?) part of watchOS

monojenkins · 2019-10-11T17:55:43Z

Build failure
❌ Build failed or was aborted

🔥 Provisioning failed 🔥

monojenkins · 2019-10-11T18:02:36Z

Build failure
❌ Build failed or was aborted

🔥 Provisioning failed 🔥

spouliot · 2019-10-11T18:19:27Z

build

…s are available

monojenkins · 2019-10-11T22:56:41Z

Build failure
✅ Build succeeded
⚠️ Mono built from source
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

9 tests failed, 86 tests passed.

Failed tests

introspection/Mac Modern/Debug: Failed (Test run failed.)
Xtro/Mac: BuildFailure
introspection/iOS Unified 32-bits - simulator/Debug: Failed
introspection/iOS Unified 32-bits - simulator/Debug (iOS 10.3): Failed
introspection/iOS Unified 64-bits - simulator/Debug: Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (static registrar): Crashed
introspection/tvOS - simulator/Debug: Failed
introspection/iOS Unified 64-bits - simulator/Debug (iOS 10.3): Failed
introspection/tvOS - simulator/Debug (tvOS 10.2): Failed

… (no hack)

monojenkins · 2019-10-12T05:58:57Z

Build failure
✅ Build succeeded
⚠️ Mono built from source
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

2 tests failed, 93 tests passed.

Failed tests

introspection/iOS Unified 64-bits - simulator/Debug: Failed
introspection/tvOS - simulator/Debug: Failed

monojenkins · 2019-10-13T18:15:19Z

Build success
✅ Build succeeded
⚠️ Mono built from source
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
✅ Test run succeeded

…are used for other NSObject types

spouliot · 2019-10-15T15:40:46Z

@mandel-macaque @dalexsoto your feedback should all be addressed now

monojenkins · 2019-10-15T20:28:43Z

Build success
✅ Build succeeded
⚠️ Mono built from source
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
✅ Test run succeeded

dalexsoto

LGTM now 👍

spouliot requested review from chamons, dalexsoto, rolfbjarne and VincentDondain as code owners October 10, 2019 19:32

spouliot mentioned this pull request Oct 10, 2019

[coreimage] Update for Xcode 11[.1] #7120

Closed

VincentDondain approved these changes Oct 10, 2019

View reviewed changes

dalexsoto requested changes Oct 10, 2019

View reviewed changes

mandel-macaque requested changes Oct 10, 2019

View reviewed changes

rolfbjarne requested changes Oct 11, 2019

View reviewed changes

spouliot added 5 commits October 11, 2019 11:48

Fix error code and document it

594672f

Fix Get* methods for possible NRE and apply some feedback (on the new…

5b5c3ad

… ones) to simplify (all of) them

Remove outdated comments and hide 2nd OutputRotationFilter (unknown u…

a3f93f9

…nderlying type)

Add kCIImageAuxiliarySemanticSegmentation* to the strong dictionary a…

109f3be

…nd remove [Watch] attributes since that framework is not (yet?) part of watchOS

Use newer, nicer ?. syntax in [Wrap]

5bb6620

rolfbjarne approved these changes Oct 11, 2019

View reviewed changes

Complete StrongDictionary (new and old entries) and check that setter…

470ee3c

…s are available

spouliot added 2 commits October 11, 2019 22:52

Update xtro since CISmoothLinearGradient is now implemented correctly…

84bef7f

… (no hack)

Fix intro in macOS and older iOS versions

9e64226

Inconsistent across versions

7bce413

Make CIFilter generator more consistent with how (protocol) bindings …

d228c7d

…are used for other NSObject types

mandel-macaque approved these changes Oct 15, 2019

View reviewed changes

dalexsoto approved these changes Oct 16, 2019

View reviewed changes

spouliot merged commit a50c753 into xamarin:xcode11.2 Oct 16, 2019

spouliot deleted the xcode112-coreimage branch October 16, 2019 13:16

[coreimage] Update for Xcode 11[.2] #7216

[coreimage] Update for Xcode 11[.2] #7216

Conversation

spouliot commented Oct 10, 2019

VincentDondain left a 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

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

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

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

monojenkins commented Oct 11, 2019

Test results

Failed tests

rolfbjarne Oct 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Oct 11, 2019

monojenkins commented Oct 11, 2019

spouliot commented Oct 11, 2019

monojenkins commented Oct 11, 2019

Test results

Failed tests

monojenkins commented Oct 12, 2019

Test results

Failed tests

monojenkins commented Oct 13, 2019

spouliot commented Oct 15, 2019

monojenkins commented Oct 15, 2019

dalexsoto left a comment

Choose a reason for hiding this comment

rolfbjarne Oct 11, 2019 •

edited