[camera] fix camera video codec validation #13341
Conversation
There was a problem hiding this comment.
The review previously left here is no longer valid, jump to the latest one 👉 #13341 (review)
There was a problem hiding this comment.
The review previously left here is no longer valid, jump to the latest one 👉 #13341 (review)
expo-bot
added
bot: passed checks
| return; | ||
| // it is possible that the session has been invalidated at this point | ||
| // for example, the video codec option is invalid and so this call has already rejected | ||
| if (self.movieFileOutput != nil) { |
There was a problem hiding this comment.
is the promise always guaranteed to be rejected at this point if movieFileOutput is nil? this seems potentially fragile if not, we would leave a promise dangling
There was a problem hiding this comment.
that's a very good point - thank you! I'll rework this
There was a problem hiding this comment.
The review previously left here is no longer valid, jump to the latest one 👉 #13341 (review)
| @@ -509,7 +511,8 @@ - (void)record:(NSDictionary *)options resolve:(UMPromiseResolveBlock)resolve re | |||
| } | |||
|
|
|||
| if (_movieFileOutput != nil && !_movieFileOutput.isRecording && _videoRecordedResolve == nil && _videoRecordedReject == nil) { | |||
|
|
|||
| // Reset validation flag | |||
| _isValidRecordingConfig = YES; | |||
There was a problem hiding this comment.
It might make more sense to place this inside of the setVideoOptions method (in the future, maybe we'll add a call to that method somewhere else, in which case we'd want to reset _hasValidRecordingConfig to YES then, too)
| @@ -534,6 +537,12 @@ - (void)record:(NSDictionary *)options resolve:(UMPromiseResolveBlock)resolve re | |||
| UM_WEAKIFY(self); | |||
| dispatch_async(self.sessionQueue, ^{ | |||
| UM_STRONGIFY(self); | |||
| // it is possible that the session has been invalidated at this point | |||
| // for example, the video codec option is invalid and so this call has already rejected | |||
| if (!self.isValidRecordingConfig) { | |||
There was a problem hiding this comment.
very nitpicky: I think "RecordingConfig" from isValidRecordingConfig and "VideoOptions" from setVideoOptions are referring to the same thing, so it would be good to pick one way to refer to it and stick to that
There was a problem hiding this comment.
The review previously left here is no longer valid, jump to the latest one 👉 #13341 (review)
ajsmth
force-pushed
the
andrew/camera-testsuite-fix
branch
from
2105f7a
to
86f028c
Compare
Why
The record session block was still being executed after the rejection callback had already fired - this PR prevents this from happening
How
The check is done inside the async block to ensure the state of
movieFileOutputis accurate. If it's nil then the session is invalid and we shouldn't continue with the record sessionTest Plan
Test suite for Camera passes