Android video capture #433
Conversation
Add the missing binding to the native Java video capture on Android. The binding is done with some Java class AndroidCameraInterop based on the WebRTC project's UnityUtility.java. It enables interop between the Java camera object and the C++ video track source object. This both avoids having to build and merge UnityUtility.java, as well as allow extending it with new functionality like video capture format selection. Bug: microsoft#246
Request user permission to access the camera or microphone on Android when WebcamSource or MicrophonSource are starting, before the native implementation tries to actually access the camera or microphone.
This is not implemented by IL2CPP. TODO: Add [Conditional] attribute to strip caller.
djee-ms
added
enhancement
| Debug.Log("User granted authorization to access microphone, starting MicrophoneSource now..."); | ||
| OnEnable(); | ||
| } | ||
| else if (Time.time <= _androidRecordAudioRequestRetryUntilTime) |
There was a problem hiding this comment.
It seems that if we take this branch then we'll get stuck (won't enable the source nor log the error message) until the application loses focus and gets it again, is that correct? Rather than relying on focus, should we check every e.g. 1s with a coroutine?
There was a problem hiding this comment.
For how long do you suggest to check? There is no way to tell if the user already denied access and there's no dialog, or if it is the first time and the dialog is still open and the user didn't select an option yet; Java has the distinction but Unity doesn't expose it and return the same result in both cases. The latter can take an indeterminate amount of time, and having a timeout on a user dialog is pretty bad. But if it's the former, then you will continue to check forever every second for no reason, which is almost worse.
With the current approach:
- If the user already denied, we never check again. Granted we lose the error message; we could maybe have a warning when we try the first time in
OnEnable()just for debugging. But at least we likely will never check again, which is what should happen since there's no chance we'll get permission. - If the dialog is open, then everything works as intended since the focus will change and we'll get the timeout.
There was a problem hiding this comment.
For how long do you suggest to check?
300s? 😄
Let me re-frame the problem. I agree everything works fine if the user accepts the dialog and permission is granted (first branch here). If the user waits 5 min and then denies permission, it also works fine (third branch here).
If the user denies permission before 5 min (second branch), capture silently fails. An error message is printed only when (if) a focus switch happens after 5 min. Also the component being disabled or not depends on when the focus switch happens. So the behavior can be improved here.
I'd say we should:
- keep checking for permission granted (and only for that) in
OnApplicationFocus - check on a 5min timeout in a coroutine started from
OnEnablerather than inOnApplicationFocus, so whether a denial is detected doesn't depend on focus switches - avoid disabling the component on permission denied? Since we can't reliably detect denial I'd avoid influencing the app behavior.
There was a problem hiding this comment.
MicrophoneSource and WebcamSource always disable themselves if failing to create a track, as we tie the track lifetime to the enable/disable state. Failing to do so only in one specific case and only on Android will cause confusion.
There was a problem hiding this comment.
(well at least after #436 which I was about to open)
There was a problem hiding this comment.
only in one specific case and only on Android
Of course we'd need to adopt the same logic on UWP too. And I'd avoid disabling the component on failure to create a track too, for the reason below.
we tie the track lifetime to the enable/disable state
Not really - starting the track is asynchronous so there is inevitably a lapse of time where the component is enabled but there is no track. Users should use IsStreaming/XStreamStarted to detect whether there is an active track. My proposal is that the component would stay in that state forever on failure, which is less surprising IMHO than staying there for a few seconds or for 5 min and then automatically reverting to disabled. But I guess this might be subjective.
There was a problem hiding this comment.
Alright. I guess the argument is also that we need to stick to OnEnable() for creating the track to make sure that if the track exists then the Update() method is called, so a separate public StartCaptureAsync() is too dangerous for that reason.
One thing bothering me though : what happen if you enable then disable then enable quickly, so that the first track has not yet finished being created. I suspect that because of await you end up trying to create a second track while the first is not done yet unfortunately, so we cannot just rely on OnDisable() always occurring after the track is created, nor OnEnable() after the track was destroyed, and need more synchronizing.
There was a problem hiding this comment.
Logged as #439 to look at that issue specifically.
Add video capture support on Android via a dedicated Java class
AndroidCameraHelperwhich does the interop with the utilities oflibwebrtc.The implementation is straightforward, with the notable difference that the
Java camera object is owned by the video track source and needs to be kept
alive by it.
This change slightly refactors the video capture device and format enumerations
by moving them into the
DeviceVideoTrackSourceclass for consistency, and usesome structs for parameter passing in the interop for better extensibility.
Bug: #246