fix(ios): stop camera setup from discarding the resizeMode prop - #819
Open
ZayanKhan-12 wants to merge 1 commit into
Open
ZayanKhan-12 wants to merge 1 commit into
ZayanKhan-12 wants to merge 1 commit into
Conversation
`RealCamera.setup()` hard-coded the preview layer's videoGravity:
self.cameraPreview.session = self.session
self.cameraPreview.previewLayer.videoGravity = .resizeAspect
`.resizeAspect` is `resizeMode: 'contain'`, so whenever setup ran after
the prop had been delivered it silently reverted `resizeMode: 'cover'`
back to `contain`.
That ordering is the normal one on a cold start. `didSetProps` sets
`hasPropBeenSetup`, but `setupCamera()` also waits on
`hasPermissionBeenGranted`. When authorization is `.notDetermined` the
system prompt resolves asynchronously, so the sequence is:
1. didSetProps -> update(resizeMode: .cover) -> .resizeAspectFill
2. user accepts the permission prompt
3. hasPermissionBeenGranted -> setup() -> .resizeAspect <- overwrites
The preview then letterboxes inside its view instead of filling it,
which reads as the camera ignoring the size it was given.
`update(resizeMode:)` also never stored its argument, leaving the
`resizeMode` property declared on line 41 assigned by nothing and read
by nothing. Storing it and applying it from both call sites fixes the
revert and gives that property its intended job.
Also documents the actual default (`contain`) in the README, which
previously said only "Default behavior depends on the specific use case".
Refs teslamotors#335
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #335
The bug
RealCamera.setup()hard-codes the preview layer'svideoGravity:.resizeAspectisresizeMode: 'contain'. So whenever setup runs after the prop has been delivered, it silently revertsresizeMode: 'cover'back tocontain, and the preview letterboxes inside its view instead of filling it — which reads as the camera ignoring the size it was given.That ordering is the normal one on a cold start.
didSetPropssetshasPropBeenSetup, butsetupCamera()also waits onhasPermissionBeenGranted. When authorization is.notDetermined, the system prompt resolves asynchronously, so:didSetProps→update(resizeMode: .cover)→.resizeAspectFill✅hasPermissionBeenGranted→setupCamera()→setup()→.resizeAspect❌ overwritesOn second launch, permission is already
.authorized, sosetup()is enqueued beforeupdate(resizeMode:)within the samedidSetPropsandcoversurvives. The result is aresizeModethat works on some launches and not others.There is a second, corroborating symptom in the same code:
update(resizeMode:)never stored its argument, so theproperty declared on line 41 was assigned by nothing and read by nothing — dead state that shows the mode was always meant to be remembered and re-applied.
The fix
Store the mode, and apply it from both call sites:
setup()now callsapplyResizeMode()instead of hard-coding.resizeAspect, so it re-asserts whatever the prop asked for rather than clobbering it. Behaviour is unchanged for anyone who never setsresizeMode(the default is.contain, exactly what was hard-coded).Also documents the actual default in the README — the
resizeModerow previously ended with "Default behavior depends on the specific use case", which tells the reader nothing.Scope
This addresses the iOS half of #335. Two notes for maintainers on the rest of that (2020, pre-rewrite) report:
measure(EXACTLY, EXACTLY)thenlayout()on the native view for every layout change, on both architectures, andCameraViewpins its subviews with constraints (addFullSizeSubview). What was left was the preview gravity inside the correctly-sized view, which is what this PR fixes.resizeModeis iOS-only. Android has noscaleTypehandling at all; CameraX'sPreviewViewdefaults toFILL_CENTER(equivalent tocover). The README already lists the prop under "iOS only", so this PR does not change Android, but it does mean the two platforms disagree by default. Happy to follow up with an AndroidscaleTypemapping if you'd like that tracked separately.Verification
swiftlinton the changed file: 17 violations before, 17 after — identical tomaster, none in the changed region, and CI's non-strict invocation exits 0.yarn build(tsc) andyarn lintpass.ReactNativeCameraKitpod target builds clean foriphoneos/arm64 against the example app'sCocoaPods setup, with Fabric codegen run —
** BUILD SUCCEEDED **. The fullCameraKitExamplescheme could not be built locally: the vendoredfmtpod fails on Xcode 26with
call to consteval function ... is not a constant expression, which is a React Nativedependency issue unrelated to this change.