-
Notifications
You must be signed in to change notification settings - Fork 514
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.
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
[CoreImage] Update Xcode 13.0 bindings betas 1 and 3 #12601
[CoreImage] Update Xcode 13.0 bindings betas 1 and 3 #12601
Conversation
src/coreimage.cs
Outdated
// [iOS (15,0), Mac (12,0), MacCatalyst (15,0), TV (15,0)] | ||
// [Wrap ("GetHeif10Representation (This, image, colorSpace, options.GetDictionary ()!, out error)")] | ||
// [return: NullAllowed] | ||
// NSData GetHeif10Representation (CIImage image, CGColorSpace colorSpace, NSDictionary<NSString, NSObject> options, [NullAllowed] out NSError errorPtr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I need to comment in these wrappers? haven't hit any issues yet, but similar existing methods (see below) have them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are they commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them in as comments, because I wanted to ask about them / thought I might need to add them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they should be uncommented
the wrapper let developer uses the CIImageRepresentationOptions
strongly-typed class instead of the weakly-typed NSDictionary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Minor changes
I'll be trying it locally to reproduce the intro error!
src/coreimage.cs
Outdated
// [iOS (15,0), Mac (12,0), MacCatalyst (15,0), TV (15,0)] | ||
// [Wrap ("GetHeif10Representation (This, image, colorSpace, options.GetDictionary ()!, out error)")] | ||
// [return: NullAllowed] | ||
// NSData GetHeif10Representation (CIImage image, CGColorSpace colorSpace, NSDictionary<NSString, NSObject> options, [NullAllowed] out NSError errorPtr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are they commented out?
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results8 tests failed, 102 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results10 tests failed, 100 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
src/coreimage.cs
Outdated
[Static] | ||
[Export ("filterWithImageURL:")] | ||
[return: NullAllowed] | ||
CIRawFilter Filter (NSUrl url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method will not filter the url
, it will create a filter from the url
rename if to Create
src/coreimage.cs
Outdated
[Static] | ||
[Export ("filterWithImageData:identifierHint:")] | ||
[return: NullAllowed] | ||
CIRawFilter Filter (NSData data, [NullAllowed] string identifierHint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
src/coreimage.cs
Outdated
[Static] | ||
[Export ("filterWithCVPixelBuffer:properties:")] | ||
[return: NullAllowed] | ||
CIRawFilter Filter (CVPixelBuffer buffer, NSDictionary properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results8 tests failed, 102 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure the comments for CIRawFilter
are part of the committed message (when you merge), otherwise they'll be hard to find
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 109 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
src/coreimage.cs
Outdated
@@ -430,7 +430,7 @@ interface CIContext_ImageRepresentation { | |||
[Wrap ("GetJpegRepresentation (This, image, colorSpace, options.GetDictionary ()!)")] | |||
[return: NullAllowed] | |||
NSData GetJpegRepresentation (CIImage image, CGColorSpace colorSpace, CIImageRepresentationOptions options); | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is an extra space with this newline or if it is just a newline!
src/coreimage.cs
Outdated
[TV (10,0)] | ||
[Static] | ||
[Internal] | ||
interface CIRawFilterKeys { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left two small whitespace suggestions!
This looks great! |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results4 tests failed, 100 tests passed.Failed tests
Pipeline on Agent XAMBOT-1096.BigSur' |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results4 tests failed, 100 tests passed.Failed tests
Pipeline on Agent XAMBOT-1102.BigSur' |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results2 tests failed, 102 tests passed.Failed tests
Pipeline on Agent XAMBOT-1096.BigSur' |
from Sebastien: "most filters are key-based natively (not-NSObject subclasses)) and we expose them as C# _user-type_ CIFilter-subclasses in recent years _most_ filters have also been exposed natively as protocols (not classes), we expose them as `*Protocol` interface types `CIRAWFilter` is a special case, it's a native `CIFilter` subclass so we're not using [CoreImageFilter] and [CoreImageFilterProperty] attributes to define it which also means we cannot use the "extra" tests to validate the filter properties. So we skip it here. Do not fear it's still tested, like any _normal_, NSObject subclass we have bound :-)"
19dcf3f
to
66a2826
Compare
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results3 tests failed, 101 tests passed.Failed tests
Pipeline on Agent XAMBOT-1106.BigSur' |
xtro and intro tests are all passing beautifully |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results3 tests failed, 101 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget the CIRAWFilter comments in your commit message!
No updates for betas 2, 4, 5
Unclear of Catalyst availability for APIs left in todo/backlog - xtro indicates availability, but I haven't found anything in header files or web docs