[AVFoundation] Add missing categories from Xcode 9 and add new API from Xcode 9.1 by mandel-macaque · Pull Request #2919 · dotnet/macios

mandel-macaque · 2017-10-24T16:17:18Z

No description provided.

…om Xcode 9.1

monojenkins · 2017-10-24T17:35:43Z

Build failure

spouliot · 2017-10-24T17:41:17Z

 		[Export ("preferredPeakBitRate")]
 		double PreferredPeakBitRate { get; set; }

+		[iOS (11, 0)]


since only iOS headers were updated you need to add [NoMac] (XM intro tests are failing) [NoTV] and [NoWatch] attributes.

Correct, Sorry for thatI forgot theNoMac and NoTV, watch is already ignored since is in the top of the class.

spouliot · 2017-10-24T17:41:53Z


+		[TV (11, 0), NoWatch, Mac (10, 13), iOS (11, 0)]
+		[Field ("AVSampleBufferAudioRendererWasFlushedAutomaticallyNotification")]
+		NSString AudioRendererWasFlushedAutomaticallyNotification { get; }


the naming makes it sound like a [Notification] attribute is missing (and intro complains about this)

spouliot · 2017-10-24T17:42:37Z

 		[Field ("AVCaptureDeviceTypeBuiltInDualCamera")]
 		BuiltInDualCamera,
+
+		[iOS (11, 1)]


missing NoMac, NoTV and NoWatch

Those decorators are already present in line 9422 on top of the enum, we have NoTV, NoMac, NoWatch.

spouliot · 2017-10-24T17:42:57Z

 		VideoDeviceInUseByAnotherClient = 3,
-		VideoDeviceNotAvailableWithMultipleForegroundApps = 4
+		VideoDeviceNotAvailableWithMultipleForegroundApps = 4,
+		[iOS (11, 1)]


NoMac, NoTV and NoWatch missing

It already has a [NoTV, NoWatch] in the top of the enum and it is inside a #if !MONOMAC which means that is not necessary, right?

Yeah technically it is not needed but I think it was the plan that at some point we would get rid of those conditionals if possible so having that metadata there won't hurt I guess, unless @spouliot says otherwise :)

We'd get rid of #if !MONOMAC, not of availability attributes on the parent (so lack of [NoTV, NoWatch] still valid).

We could just add a [NoMac] to public enum AVCaptureSessionInterruptionReason but that would be just noise in this case, better do it for things we add.

k, we'll soon have xtro results so it will be easier to spot them :)

Ok, leaving as it is to reduce noise in the diff. We can do this later for new types or in a pr just to get rid of the #if !MONOMAC

spouliot · 2017-10-24T17:43:53Z

 		[Export ("automaticallyConfiguresCaptureDeviceForWideColor")]
 		bool AutomaticallyConfiguresCaptureDeviceForWideColor { get; set; }
+
+		[iOS (11, 1)]


the #if are not clear in the diff but this cannot be added in Mac, TV or Watch since the headers were not updated

This is within a #if !MONOMAC in line 8156 to line 8184 that includes the only iOS methods. The class has a NoWatch NoTV already, therefore AFAIK, it is not added in Mac, TV or Watch.

I agree that because the class has [NoWatch] and [NoTV] we shouldn't add those but I would still add the [NoMac] even though there's a #if !MONOMAC. Remember we're trying to move away from them, it adds clarity.

even a [NoMac] should be added on the type, so we would not see it in the diff :|
hopefully xtro will make this easier to review (really soon now)

Adding NoMac and moving out side the #if !MONOMAC.

spouliot · 2017-10-24T17:46:49Z

+
+		[Mac (10, 13), iOS (11, 0), TV (11, 0)]
+		[Export ("sourceTrackIDForFrameTiming")]
+		int SourceTrackIDForFrameTiming { get; set; }


Id - not ID since it's not an acronym

spouliot · 2017-10-24T17:48:00Z

+
+		[iOS (11, 0)]
+		[Export ("metadata", ArgumentSemantic.Copy)]
+		NSDictionary<NSString, NSObject> Metadata { get; set; }


use an NSDictionary so we can use a strong dictionary later

spouliot · 2017-10-24T17:49:36Z

+
+		[iOS (11,0)]
+		[Export ("supportedPhotoPixelFormatTypesForFileType:")]
+		NSNumber[] SupportedPhotoPixelFormatTypesForFileType (string fileType);


-> Get SupportedPhotoPixelFormatTypes

spouliot · 2017-10-24T17:49:58Z

+
+		[iOS (11,0)]
+		[Export ("supportedPhotoCodecTypesForFileType:")]
+		string[] SupportedPhotoCodecTypesForFileType (string fileType);


-> GetSupportedPhotoCodecTypes

spouliot · 2017-10-24T17:50:14Z

+
+		[iOS (11,0)]
+		[Export ("supportedRawPhotoPixelFormatTypesForFileType:")]
+		NSNumber[] SupportedRawPhotoPixelFormatTypesForFileType (string fileType);


-> Get SupportedRawPhotoPixelFormatTypes

dalexsoto · 2017-10-25T00:17:29Z

+		[NullAllowed, Export ("error")]
+		NSError Error { get; }
+
+		[NullAllowed, Export ("audioOutputDeviceUniqueID")]


I am not familiar with the API so to be honest I am not sure of what the unique id is for, maybe we should keep the full property name AudioOutputDeviceUniqueId

maybe we should keep the full property name AudioOutputDeviceUniqueId

Yes we should, the class is AVSampleBufferAudioRenderer not AudioOutputDevice so it makes sense to keep the full name, see swift version too: https://developer.apple.com/documentation/avfoundation/avsamplebufferaudiorenderer/2866182-audiooutputdeviceuniqueid

dalexsoto · 2017-10-25T00:35:36Z

+		[iOS (11,0)]
+		[Static]
+		[Export ("photoSettingsWithRawPixelFormatType:rawFileType:processedFormat:processedFileType:")]
+		AVCapturePhotoSettings FromRawPixelFormatType (uint rawPixelFormatType, [NullAllowed] string rawFileType, [NullAllowed] NSDictionary<NSString, NSObject> processedFormat, [NullAllowed] string processedFileType);


Do you think we can make a strong dictionary in the future of NSDictionary<NSString, NSObject> processedFormat if so please just do NSDictionary

Over here I followed the pattern from the other static creators. If you look you will find:

[Static]
[Export ("photoSettingsWithRawPixelFormatType:processedFormat:")]
AVCapturePhotoSettings FromRawPixelFormatType (uint rawPixelFormatType,
[NullAllowed] NSDictionary<NSString, NSObject> processedFormat);

And this new method is an overload with an extra string parameter as a second parameter:

AVCapturePhotoSettings FromRawPixelFormatType (uint rawPixelFormatType, [NullAllowed] string rawFileType, [NullAllowed] NSDictionary<NSString, NSObject> processedFormat, [NullAllowed] string processedFileType);

I don't know if we want to have one (that is already a public API) with the template dir and a new one with a simple NSDictionary, makes the API look funny.

@spouliot comments?

let's be consistent then

I think we need a something like NSDictionary GetWeakDictionary (); added to NSDictionary<T,V> (and maybe a GetStrongDictionary<T,V> () to NSDictionary too ?

That would end up with multiple managed objects per native handle.

We should really look into fixing our runtime to make that a supported scenario.

dalexsoto · 2017-10-25T00:39:52Z

+
+		[iOS (11, 0)]
+		[Export ("availableEmbeddedThumbnailPhotoCodecTypes")]
+		string[] AvailableEmbeddedThumbnailPhotoCodecTypes { get; }


This sounds like it returns a bunch of NSString constants (likely a smart enum) if that is the case please use NSString[] instead of string or better yet, if you have the info return a smart enum ;)

dalexsoto · 2017-10-25T00:46:48Z

+
+		[iOS (11, 0)]
+		[Export ("availablePhotoFileTypes")]
+		string[] AvailablePhotoFileTypes { get; }


This sounds like it returns a bunch of NSString constants (likely a smart enum) if that is the case please use NSString[] instead of string or better yet, if you have the info return a smart enum ;)

We can do that since AFAIK it should be a AVVideoCodecType.

dalexsoto · 2017-10-25T00:46:57Z

+
+		[iOS (11, 0)]
+		[Export ("availableRawPhotoFileTypes")]
+		string[] AvailableRawPhotoFileTypes { get; }


This sounds like it returns a bunch of NSString constants (likely a smart enum) if that is the case please use NSString[] instead of string or better yet, if you have the info return a smart enum ;)

Yes, will do that with a simple wrap.

dalexsoto · 2017-10-25T04:58:34Z

+		NSString AudioRendererWasFlushedAutomaticallyNotification { get; }
+
+		[TV (11, 0), NoWatch, Mac (10, 13), iOS (11, 0)]
+		[Field ("AVSampleBufferAudioRendererFlushTimeKey")]


Is this part of the Flushed Automatically Notification payload?

Yes, it is part of the AVSampleBufferAudioRendererWasFlushedAutomaticallyNotification, as per the headers:

The renderer may flush enqueued media data when the user routes playback to a new destination. The renderer may also flush enqueued media data when the playback rat
e of the attached AVSampleBufferRenderSynchronizer is changed (e.g. 1.0 -> 2.0 or 1.0 -> 0.0 -> 2.0), however no flush will occur for normal pauses (non-zero -> 0.0) and resumes (0.
0 -> same non-zero rate as before).

When an automatic flush occurs, the attached render synchronizer's timebase will remain running at its current rate. It is typically best to respond to this notification by enqueueing media data with timestamps starting at the timebase's current time. To the listener, this will sound similar to muting the audio for a short period of time. If it is more desirable to ensure that all audio is played than to keep the timeline moving, you may also stop the synchronizer, set the synchronizer's current time to the value of AVSampleBufferAudioRendererFlushTimeKey, start reenqueueing sample buffers with timestamps starting at that time, and restart the synchronizer. To the listener, this will sound similar to pausing the audio for a short period of time. This notification is delivered on an arbitrary thread. If sample buffers are being enqueued with the renderer concurrently with the receipt of this notification, it is possible that one or more sample buffers will remain enqueued in the renderer. This is generally undesirable, because the sample buffers that remain will likely have timestamps far ahead of the timebase's current time and so won't be rendered for some time. The best practice is to invoke the -flush method, in a manner that is serialized with enqueueing sample buffers, after receiving this notification and before resuming the enqueueing of sample buffers.

Then you need to add it to the Notification attribute as described here https://developer.xamarin.com/guides/cross-platform/macios/binding/binding-types-reference/#NotificationAttribute

look for this

If your notification contains parameters, you should specify a helper EventArgs interface, like this:

…that required them and used smart enums where possible.

monojenkins · 2017-10-25T11:30:12Z

Build failure

monojenkins · 2017-10-25T12:51:32Z

Build success

VincentDondain · 2017-10-25T16:20:10Z

+		[NullAllowed, Export ("error")]
+		NSError Error { get; }
+
+		[NullAllowed, Export ("audioOutputDeviceUniqueID")]


maybe we should keep the full property name AudioOutputDeviceUniqueId

Yes we should, the class is AVSampleBufferAudioRenderer not AudioOutputDevice so it makes sense to keep the full name, see swift version too: https://developer.apple.com/documentation/avfoundation/avsamplebufferaudiorenderer/2866182-audiooutputdeviceuniqueid

VincentDondain · 2017-10-25T16:22:40Z

 		[Export ("automaticallyConfiguresCaptureDeviceForWideColor")]
 		bool AutomaticallyConfiguresCaptureDeviceForWideColor { get; set; }
+
+		[iOS (11, 1)]


I agree that because the class has [NoWatch] and [NoTV] we shouldn't add those but I would still add the [NoMac] even though there's a #if !MONOMAC. Remember we're trying to move away from them, it adds clarity.

VincentDondain · 2017-10-25T16:28:24Z

+
+		// From @interface AVCapturePhotoBracketedCapture (AVCapturePhoto)
+
+#if !MONOMAC


Could we go [NoMac] here? Are AVCaptureBracketedStillImageSettings and AVCaptureLensStabilizationStatus just not available?

SequenceCount could definitely avoid the #if !MONOMAC

AVCaptureBracketedStillImageSettings and AVCaptureLensStabilizationStatus is not present in the mac AFAIK. Moving the other ones out.

Comment is misleading AVCaptureLensStabilizationStatus seems to be present since you did not put it in the #if !MONOMAC and it built just fine (:

@spouliot @mandel-macaque should we take this approach instead? https://github.com/xamarin/xamarin-macios/blob/master/src/scenekit.cs#L76-L97

It's also a comment for the future, I'd like to know the approach we prefer, moving forward.

Sorry, I meant just one of them. tested the others :)

spouliot · 2017-10-26T20:33:49Z

+		NSObject AddPeriodicTimeObserver (CMTime interval, [NullAllowed] DispatchQueue queue, Action<CMTime> handler);
+
+		[Export ("addBoundaryTimeObserverForTimes:queue:usingBlock:")]
+		NSObject AddBoundaryTimeObserver (NSValue[] times, [NullAllowed] DispatchQueue queue, Action handler);


let's add a comment here and the one above (and remove its [Async])

spouliot · 2017-10-26T20:37:41Z

+		[iOS (11,0)]
+		[Static]
+		[Export ("photoSettingsWithRawPixelFormatType:rawFileType:processedFormat:processedFileType:")]
+		AVCapturePhotoSettings FromRawPixelFormatType (uint rawPixelFormatType, [NullAllowed] string rawFileType, [NullAllowed] NSDictionary<NSString, NSObject> processedFormat, [NullAllowed] string processedFileType);


let's be consistent then

I think we need a something like NSDictionary GetWeakDictionary (); added to NSDictionary<T,V> (and maybe a GetStrongDictionary<T,V> () to NSDictionary too ?

…added event args for the flush notification. A helper property was added to go from NSValue to CMTime.

monojenkins · 2017-10-27T18:32:39Z

Build success

VincentDondain

https://github.com/xamarin/xamarin-macios/pull/2919/files#r147492859 is still unanswered, I think we could avoid #if !MONOMAC completely but that's not really a blocker for the merge (:

mandel-macaque · 2017-10-30T15:12:34Z

Merging after talking with Vincent as his comment is not a blocked and we do not want to be waiting for this regarding xcode 9.1

[AVFoundation] Add missing categories from Xcode 9 and add new API fr…

db7794e

…om Xcode 9.1

dnfclas added the cla-already-signed label Oct 24, 2017

mandel-macaque mentioned this pull request Oct 24, 2017

[AVFoundation] Add missing types and categories. #2694

Closed

spouliot suggested changes Oct 24, 2017

View reviewed changes

dalexsoto requested changes Oct 25, 2017

View reviewed changes

mandel-macaque added 2 commits October 25, 2017 12:43

Addresses review comments. Added missinf attr, Get* to those methods …

d8bef4e

…that required them and used smart enums where possible.

Remove not correct use of async attr. Add missing smart enum property.

ea156eb

VincentDondain suggested changes Oct 25, 2017

View reviewed changes

rolfbjarne approved these changes Oct 25, 2017

View reviewed changes

spouliot suggested changes Oct 26, 2017

View reviewed changes

Updated bidings as per reviews. Mainly added NoMac when possible and …

ab9f2c5

…added event args for the flush notification. A helper property was added to go from NSValue to CMTime.

dalexsoto approved these changes Oct 27, 2017

View reviewed changes

spouliot approved these changes Oct 27, 2017

View reviewed changes

VincentDondain approved these changes Oct 30, 2017

View reviewed changes

mandel-macaque merged commit a26d527 into dotnet:xcode9.1 Oct 30, 2017

mandel-macaque deleted the avfoundation-xcode9.1 branch October 30, 2017 15:12


		// From @interface AVCapturePhotoBracketedCapture (AVCapturePhoto)

		#if !MONOMAC

Uh oh!

Conversation

mandel-macaque commented Oct 24, 2017

Uh oh!

monojenkins commented Oct 24, 2017

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!

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!

dalexsoto Oct 25, 2017 • edited by VincentDondain Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VincentDondain Oct 25, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalexsoto Oct 25, 2017 •

edited by VincentDondain

Loading

VincentDondain Oct 25, 2017 •

edited

Loading

VincentDondain Oct 25, 2017 •

edited

Loading