-
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
[AVFoundation] Add support for xcode 13 beta 5. #12550
[AVFoundation] Add support for xcode 13 beta 5. #12550
Conversation
src/avfoundation.cs
Outdated
[Async] | ||
[Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0), MacCatalyst (15,0)] | ||
[Export ("loadChapterMetadataGroupsBestMatchingPreferredLanguages:completionHandler:")] | ||
void LoadChapterMetadataGroups (string[] preferredLanguages, Action<NSArray<AVTimedMetadataGroup>, NSError> completionHandler); |
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.
The BestMatching got dropped here, thoughts?
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.
For that matter, I'd add the language to in the name of the method.
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.
In doubt double check with the swift name -> loadChapterMetadataGroups
but the first argument is bestMatchingPreferredLanguages
https://developer.apple.com/documentation/avfoundation/avasset/3746525-loadchaptermetadatagroups
|
||
[NoWatch, NoTV, NoiOS, Mac (12, 0), MacCatalyst (15,0)] | ||
[Field ("AVFileTypeAppleiTT")] | ||
AppleiTT = 21, |
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.
iTT looks weird but I think it's right?!
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 english, so I went for the name from apple
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.
The UTI for the Apple iTT caption file format => The value of this UTI is com.apple.itunes-timed-text. Files of this type have an .itt extension.
[DisableDefaultCtor] | ||
interface AVPlaybackCoordinator | ||
{ | ||
|
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 line
src/avfoundation.cs
Outdated
|
||
[NoMacCatalyst] | ||
[Export ("captionsNotPresentInPreviousGroupsInCaptionGroup:")] | ||
AVCaption[] GetCaptionsNotPresent (AVCaptionGroup captionGroup); |
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.
Name dropped "inPrevious", maybe arg should be named prevousCaptionGroup?
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 think I should add the full name.
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.
yep, GetCaptionsNotPresentInPreviousGroups
I've been out of the game for awhile, so get another set of 👀 before landing. |
🔥 Tests failed catastrophically on Build (no summary found). 🔥Result file $(TEST_SUMMARY_PATH) not found. |
src/AVFoundation/AVTypes.cs
Outdated
static extern AVCaptionDimension AVCaptionDimensionMake (nfloat dimension, AVCaptionUnitsType units); | ||
|
||
public static AVCaptionDimension Create (nfloat dimension, AVCaptionUnitsType units) | ||
=> AVCaptionDimensionMake (dimension, units); |
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.
you can merge both by using EntryPoint = "AVCaptionDimensionMake"
on the DllImport
(it reduce metadata)
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.
Because it's a [Native]
enum in the signature, you still need the wrapper function.
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.
@rolfbjarne so I did right, correct? (might have been pure luck, but nobody knows)
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.
good point for this p/invoke
however the other ones can still be simplified
static extern AVCaptionPoint AVCaptionPointMake (AVCaptionDimension x, AVCaptionDimension y); | ||
|
||
public static AVCaptionPoint Create (AVCaptionDimension x, AVCaptionDimension y) | ||
=> AVCaptionPointMake (x,y); |
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
static extern AVCaptionSize AVCaptionSizeMake (AVCaptionDimension width, AVCaptionDimension height); | ||
|
||
public static AVCaptionSize Create (AVCaptionDimension width, AVCaptionDimension height) | ||
=> AVCaptionSizeMake (width, height); |
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/AVFoundation/Enums.cs
Outdated
#if !NET | ||
[iOS (15,0), TV (15,0), Watch (8,0), Mac (10,10), MacCatalyst (15,0)] | ||
#else | ||
[SupportedOSPlatform ("ios15.0"), SupportedOSPlatform ("tvos15.0"), SupportedOSPlatform ("macos10.10"), SupportedOSPlatform ("maccatalyst15.0")] |
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.
[SupportedOSPlatform ("macos10.10")]
should not be needed since the assembly sets the minimum to 10.14
We should add some (NET specific) tests for that.
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.
still present
not a big deal since, iirc, the generator will just ignore it since this is a processed file
AVFOUNDATION_API_SOURCES = \
AVFoundation/Enums.cs \
which also means it did not require any availability changes ;-)
src/avfoundation.cs
Outdated
{ | ||
[Static] | ||
[Export ("appleITTTopRegion")] | ||
AVCaptionRegion AppleITTTopRegion { get; } |
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.
Itt
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.
If it's the same iTT
as above, it should be AppleiTTTopRegion
...
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.
Is the same iTT, it stands I believe for iTunes Timed Text
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.
it's a 3 letters acronym so it would be Itt
src/avfoundation.cs
Outdated
|
||
[Static] | ||
[Export ("appleITTBottomRegion")] | ||
AVCaptionRegion AppleITTBottomRegion { get; } |
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/avfoundation.cs
Outdated
|
||
[Static] | ||
[Export ("appleITTLeftRegion")] | ||
AVCaptionRegion AppleITTLeftRegion { get; } |
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/avfoundation.cs
Outdated
|
||
[Static] | ||
[Export ("appleITTRightRegion")] | ||
AVCaptionRegion AppleITTRightRegion { get; } |
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 results24 tests failed, 84 tests passed.Failed tests
Pipeline on Agent XAMBOT-1097.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.
Also missing tests for the manual bindings (P/Invokes).
src/AVFoundation/AVTypes.cs
Outdated
static extern AVCaptionDimension AVCaptionDimensionMake (nfloat dimension, AVCaptionUnitsType units); | ||
|
||
public static AVCaptionDimension Create (nfloat dimension, AVCaptionUnitsType units) | ||
=> AVCaptionDimensionMake (dimension, units); |
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.
Because it's a [Native]
enum in the signature, you still need the wrapper function.
|
||
[NoWatch, NoTV, NoiOS, Mac (12, 0), MacCatalyst (15,0)] | ||
[Field ("AVFileTypeAppleiTT")] | ||
AppleiTT = 21, |
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.
The UTI for the Apple iTT caption file format => The value of this UTI is com.apple.itunes-timed-text. Files of this type have an .itt extension.
src/avfoundation.cs
Outdated
{ | ||
[Static] | ||
[Export ("appleITTTopRegion")] | ||
AVCaptionRegion AppleITTTopRegion { get; } |
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.
If it's the same iTT
as above, it should be AppleiTTTopRegion
...
❌ [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, 106 tests passed.Failed tests
Pipeline on Agent XAMBOT-1096.BigSur' |
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
…e/xamarin-macios into avfoundation-xcode13-beta5
❌ [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, 106 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
src/avfoundation.cs
Outdated
{ | ||
[Static] | ||
[Export ("appleITTTopRegion")] | ||
AVCaptionRegion AppleiTTTopRegion { get; } |
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.
-> AppleIttTopRegion(even the selector name uses an uppercase
I` here)
src/avfoundation.cs
Outdated
|
||
[Static] | ||
[Export ("appleITTBottomRegion")] | ||
AVCaptionRegion AppleiTTBottomRegion { get; } |
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/avfoundation.cs
Outdated
|
||
[Static] | ||
[Export ("appleITTLeftRegion")] | ||
AVCaptionRegion AppleiTTLeftRegion { get; } |
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/avfoundation.cs
Outdated
|
||
[Static] | ||
[Export ("appleITTRightRegion")] | ||
AVCaptionRegion AppleiTTRightRegion { get; } |
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 results3 tests failed, 105 tests passed.Failed tests
Pipeline on Agent XAMBOT-1094.BigSur' |
src/avfoundation.cs
Outdated
AVCoordinatedPlaybackSuspension BeginSuspension (string suspensionReason); | ||
|
||
[Export ("expectedItemTimeAtHostTime:")] | ||
CMTime ExpectedItemTimeAtHostTime (CMTime hostClockTime); |
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.
CMTime ExpectedItemTimeAtHostTime (CMTime hostClockTime); | |
CMTime GetExpectedItemTime (CMTime hostClockTime); |
Perhaps verb-ify and remove suffix
AVCaptionRegion AppleiTTLeftRegion { get; } | ||
|
||
[Static] | ||
[Export ("appleITTRightRegion")] |
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.
Maybe not anything but, for me, Github shows the color of the code only being black from this point on. Is there perhaps a syntax thing going on somewhere?
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.
No, is that the file is too long and probably the js is not dealing with it.
✅ [PR Build] Tests passed on Build. ✅Tests passed 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): 🎉 All 108 tests passed 🎉Pipeline on Agent XAMBOT-1099.BigSur' |
src/avfoundation.cs
Outdated
@@ -11381,14 +11381,17 @@ interface AVPlayerRateDidChangeEventArgs { | |||
NSString RateDidChangeOriginatingParticipant { get; } | |||
} | |||
|
|||
[iOS (10, 0), TV (10,0), Mac (10,12), MacCatalyst (15,0)] | |||
[iOS (10, 0), TV (10,0), Mac (10,12), MacCatalyst (14,0)] |
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 14,0
? the oldest member is 14,5
src/avfoundation.cs
Outdated
{ | ||
[Static] | ||
[Export ("appleITTTopRegion")] | ||
AVCaptionRegion AppleITTTopRegion { get; } |
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.
it's a 3 letters acronym so it would be Itt
we need to get intro on monterey + catalyst to land this. |
src/avfoundation.cs
Outdated
@@ -3205,7 +3320,7 @@ interface AVFragmentedAssetMinder { | |||
interface AVFragmentedAssetTrack { | |||
} | |||
|
|||
#if MONOMAC | |||
#if MONOMAC || __MACCATALYST__ |
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.
Yes, just move the IAVCaptureFileOutputDelegate
out of the #if MONOMAC
block, that makes the code more obvious.
src/avfoundation.cs
Outdated
[Export ("currentSampleAudioDependencyInfo")] | ||
AVSampleCursorAudioDependencyInfo CurrentSampleAudioDependencyInfo { get; } | ||
|
||
[NullAllowed] | ||
[NoWatch, NoTV, NoiOS, Mac (12, 0), MacCatalyst (15,0)] |
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.
The type is already NoiOS, NoTV, NoWatch, so 3/5ths of this is not necessary.
The type is also MacCatalyst (15, 0), so there's another 1/5th which is unnecessary.
Same goes for the member just above.
That also means that the #if MONOMAC || __MACCATALYST__
condition can be completely removed.
#if MONOMAC | ||
[Unavailable (PlatformName.MacCatalyst)] | ||
#if MONOMAC || __MACCATALYST__ | ||
[NoMacCatalyst] |
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.
You're adding || __MACCATALYST__
and then also adding [NoMacCatalyst]
.
In this case it's better to move the #endif
further up (to not include the RecordingPaused property), and then add [NoiOS]
on that property.
As long as reasonably possible, we should strive towards removing/reducing #if <platform>
code, not make it more complex.
src/avfoundation.cs
Outdated
|
||
[Export ("renditionSpecificAttributesForMediaOption:")] | ||
[return: NullAllowed] | ||
AVAssetVariantAudioRenditionSpecificAttributes GetRenditionSpecificAttributesForMediaOption (AVMediaSelectionOption mediaSelectionOption); |
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.
AVAssetVariantAudioRenditionSpecificAttributes GetRenditionSpecificAttributesForMediaOption (AVMediaSelectionOption mediaSelectionOption); | |
AVAssetVariantAudioRenditionSpecificAttributes GetRenditionSpecificAttributes (AVMediaSelectionOption mediaSelectionOption); |
[NullAllowed, Export ("identifier")] | ||
string Identifier { get; } | ||
|
||
#if MONOMAC // needed because the structs are inside a #if too |
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.
You could do this at the top of the file:
#if !MONOMAC
using AVCaptionPoint=Foundation.NSObject;
...
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.
is a bunch of them...
❌ [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, 100 tests passed.Failed tests
Pipeline on Agent XAMBOT-1094.BigSur' |
src/AVFoundation/Enums.cs
Outdated
#if !NET | ||
[iOS (15,0), TV (15,0), Watch (8,0), Mac (10,10), MacCatalyst (15,0)] | ||
#else | ||
[SupportedOSPlatform ("ios15.0"), SupportedOSPlatform ("tvos15.0"), SupportedOSPlatform ("macos10.10"), SupportedOSPlatform ("maccatalyst15.0")] |
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.
still present
not a big deal since, iirc, the generator will just ignore it since this is a processed file
AVFOUNDATION_API_SOURCES = \
AVFoundation/Enums.cs \
which also means it did not require any availability changes ;-)
❌ [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, 100 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur' |
Known issues: #12640 |
/sudo backport xcode13-ios |
Backport Job to branch xcode13-ios Created! The magic is happening here |
Oh no! Backport failed! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5207975 for more details. |
Only new types/struct have the new attrs to simplify the review else we had too many additions (was done and reverted).