feat(android): add high-speed video recording mode#3803
feat(android): add high-speed video recording mode#3803Aniketiitk21 wants to merge 1 commit intomrousavy:mainfrom
Conversation
|
@Aniketiitk21 is attempting to deploy a commit to the Margelo Team on Vercel. A member of the Team first needs to authorize it. |
mrousavy
left a comment
mrousavy
left a comment
There was a problem hiding this comment.
Thank you for your PR, but I am unfortunately not a big fan of this prop at all.
Two reasons:
- I think the concept of High Speed Recording sessions should not be exposed to the user - it is an implementation detail imo - just like how iOS does it. We can abstract this away in VisionCamera too
- High Speed Recording Video Sessions currently have way too many limitations for them to be a seriously first-class supported feature imo. They dont support Camera Extensions, no Mirror Mode, and afaik no ImageAnalysis (Frame Processor) or ImageCapture (photos). That's basically crippling the Camera
I think overall this is something I don't yet want in VisionCamera - maybe soon if High Speed Capture Sessions are becoming a bit more powerful in CameraX, but for now this is too limiting.
| if (config.videoRecordingMode == null) { | ||
| // High-speed sessions cannot set mirror mode on VideoCapture. | ||
| setMirrorMode(mirrorMode.toMirrorMode()) | ||
| } |
There was a problem hiding this comment.
Limitations like these are super weird, and I personally would love to completely avoid that unsafe code.
It's possible by API design to write code that crashes here, and I don't understand why a High Speed Video Session doesn't support mirror mode tbh.
There was a problem hiding this comment.
Agreed. The guard is only there because CameraX crashes when mirror mode is set on a high-speed VideoCapture, which is itself a sign that this is too leaky for a public first-class API.
| }, | ||
| { true }, // PixelFormat: let downgrade loop handle it | ||
| { true }, // Binning: not configurable in CameraX | ||
| { Recorder.getHighSpeedVideoCapabilities(cameraInfo) != null }, |
There was a problem hiding this comment.
Not very explicit - I would probably add an extension (or a var above) like isHighSpeedVideoRecordingSupported instead of comparing for null
There was a problem hiding this comment.
Agreed. If this direction were to continue, I would rename that for clarity. But after your broader feedback, I think the bigger fix is to avoid surfacing this mode directly in the public API at all.
| let r = binned.resolve(for: format) | ||
| return ConstraintEvaluation(penalty: r.penalty, resolved: .formatOnly) | ||
| case .ninth( /* videoRecordingMode */ _): | ||
| // High-speed session orchestration is currently Android-only. |
There was a problem hiding this comment.
Not currently; it is an Android only concept. iOS doesnt care about high speed or "normal" - everything is normal. You can just record higher FPS. Just like a Camera library should work, not two completely separate concepts just because FPS go higher.
There was a problem hiding this comment.
Yep, agreed. I only threaded it through iOS to keep the bridge shape compiling consistently, not because I think iOS should expose a separate “high-speed vs normal” concept.
| * | ||
| * @platform Android | ||
| */ | ||
| export interface VideoRecordingModeConstraint { |
Thanks, that makes sense. I agree with your higher-level concern here: I also agree that the current limitations are too heavy for something that would look like a first-class feature: no mirror mode, no camera extensions, and restricted output combinations make it much harder to present this cleanly in the library surface. So I’m aligned with not pushing this prop-based version further. The better long-term direction seems to be keeping the public API centered on Happy to leave this PR as-is for reference or close it and revisit later with that abstraction instead. |
|
Yea that makes sense. Thanks for your insights. I think it's best if we leave this PR open for future reference (not only code, but also the discussion around it), and in the future when CameraX has better support for high speed capture sessions we can abstract this more nicely and blend high speed with normal capture sessions. |
2 participants
Summary
{ videoRecordingMode: 'high-speed' | 'slow-motion' }constraint for Android video sessionsselectedVideoRecordingModeonCameraSessionConfigHighSpeedVideoSessionConfigsessions when the requested outputs are compatible{ fps: ... }constraint is providedWhy
Issue #3688 asks for a way to use Android high-speed recording from VisionCamera.
Today, developers can request higher FPS values, but there is no direct way to tell the session resolver that the recording itself should be negotiated as a high-speed or slow-motion session. That makes the behavior harder to reason about and leaves some devices unable to enter their dedicated high-speed capture path even when CameraX supports it.
This change adds an explicit recording-mode constraint so apps can opt into that capability in a predictable way.
Implementation
VideoRecordingModetype with'high-speed' | 'slow-motion'VideoRecordingModeConstraintto the constraint systemCameraSessionConfigasselectedVideoRecordingModeConstraintResolverto:HighSpeedVideoSessionConfigVideoCapturemirror-mode configuration in high-speed sessions because CameraX does not allow it therevideoRecordingMode, how it interacts withfps, and what fallback behavior to expectBehavior
{ videoRecordingMode: 'high-speed' }requests a high-speed recording session{ videoRecordingMode: 'slow-motion' }requests a high-speed session that is flagged for slow-motion capturefpsis omitted, the resolver selects the lowest supported high-speed FPS for the chosen format/deviceselectedVideoRecordingModeTesting
npm run typechecktsc --noEmit false && nitrogen.\gradlew.bat :react-native-vision-camera:compileDebugKotlin --no-daemon --console=plainNotes
Closes #3688