Skip to content

[camera_avfoundation] fix stopVideoRecording waiting indefinitely and video lag at start #7065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/camera/camera_avfoundation/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.9.17+2

* Fixes stopVideoRecording waiting indefinitely and lag at start of video.

## 0.9.17+1

* Fixes a crash due to appending sample buffers when readyForMoreMediaData is NO
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,4 +251,91 @@ - (void)testDidOutputSampleBufferMustNotAppendSampleWhenReadyForMoreMediaDataIsN
CFRelease(videoSample);
}

- (void)testStopVideoRecordingWithCompletionMustCallCompletion {
FLTCam *cam = FLTCreateCamWithCaptureSessionQueue(dispatch_queue_create("testing", NULL));

id writerMock = OCMClassMock([AVAssetWriter class]);
OCMStub([writerMock alloc]).andReturn(writerMock);
OCMStub([writerMock initWithURL:OCMOCK_ANY fileType:OCMOCK_ANY error:[OCMArg setTo:nil]])
.andReturn(writerMock);
__block AVAssetWriterStatus status = AVAssetWriterStatusUnknown;
OCMStub([writerMock startWriting]).andDo(^(NSInvocation *invocation) {
status = AVAssetWriterStatusWriting;
});
OCMStub([writerMock status]).andDo(^(NSInvocation *invocation) {
[invocation setReturnValue:&status];
});

OCMStub([writerMock finishWritingWithCompletionHandler:[OCMArg checkWithBlock:^(id param) {
XCTAssert(status == AVAssetWriterStatusWriting,
@"Cannot call finishWritingWithCompletionHandler when status is "
@"not AVAssetWriterStatusWriting.");
void (^handler)(void) = param;
handler();
return YES;
}]]);

[cam
startVideoRecordingWithCompletion:^(FlutterError *_Nullable error) {
}
messengerForStreaming:nil];

__block BOOL completionCalled = NO;
[cam stopVideoRecordingWithCompletion:^(NSString *_Nullable path, FlutterError *_Nullable error) {
completionCalled = YES;
}];
XCTAssert(completionCalled, @"Completion was not called.");
}

- (void)testStartWritingShouldNotBeCalledBetweenSampleCreationAndAppending {
FLTCam *cam = FLTCreateCamWithCaptureSessionQueue(dispatch_queue_create("testing", NULL));
CMSampleBufferRef videoSample = FLTCreateTestSampleBuffer();

id connectionMock = OCMClassMock([AVCaptureConnection class]);

id writerMock = OCMClassMock([AVAssetWriter class]);
OCMStub([writerMock alloc]).andReturn(writerMock);
OCMStub([writerMock initWithURL:OCMOCK_ANY fileType:OCMOCK_ANY error:[OCMArg setTo:nil]])
.andReturn(writerMock);
__block BOOL startWritingCalled = NO;
OCMStub([writerMock startWriting]).andDo(^(NSInvocation *invocation) {
startWritingCalled = YES;
});

__block BOOL videoAppended = NO;
id adaptorMock = OCMClassMock([AVAssetWriterInputPixelBufferAdaptor class]);
OCMStub([adaptorMock assetWriterInputPixelBufferAdaptorWithAssetWriterInput:OCMOCK_ANY
sourcePixelBufferAttributes:OCMOCK_ANY])
.andReturn(adaptorMock);
OCMStub([adaptorMock appendPixelBuffer:[OCMArg anyPointer] withPresentationTime:kCMTimeZero])
.ignoringNonObjectArgs()
.andDo(^(NSInvocation *invocation) {
videoAppended = YES;
});

id inputMock = OCMClassMock([AVAssetWriterInput class]);
OCMStub([inputMock assetWriterInputWithMediaType:OCMOCK_ANY outputSettings:OCMOCK_ANY])
.andReturn(inputMock);
OCMStub([inputMock isReadyForMoreMediaData]).andReturn(YES);

[cam
startVideoRecordingWithCompletion:^(FlutterError *_Nullable error) {
}
messengerForStreaming:nil];

BOOL startWritingCalledBefore = startWritingCalled;
[cam captureOutput:cam.captureVideoOutput
didOutputSampleBuffer:videoSample
fromConnection:connectionMock];
XCTAssert((startWritingCalledBefore && videoAppended) || (startWritingCalled && !videoAppended),
@"The startWriting was called between sample creation and appending.");

[cam captureOutput:cam.captureVideoOutput
didOutputSampleBuffer:videoSample
fromConnection:connectionMock];
XCTAssert(videoAppended, @"Video was not appended.");

CFRelease(videoSample);
}

@end
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ @interface FLTCam () <AVCaptureVideoDataOutputSampleBufferDelegate,
@property(strong, nonatomic) AVCaptureVideoDataOutput *videoOutput;
@property(strong, nonatomic) AVCaptureAudioDataOutput *audioOutput;
@property(strong, nonatomic) NSString *videoRecordingPath;
@property(assign, nonatomic) BOOL isFirstVideoSample;
@property(assign, nonatomic) BOOL isRecording;
@property(assign, nonatomic) BOOL isRecordingPaused;
@property(assign, nonatomic) BOOL videoIsDisconnected;
Expand Down Expand Up @@ -663,19 +664,19 @@ - (void)captureOutput:(AVCaptureOutput *)output

// ignore audio samples until the first video sample arrives to avoid black frames
// https://github.com/flutter/flutter/issues/57831
if (_videoWriter.status != AVAssetWriterStatusWriting && output != _captureVideoOutput) {
if (_isFirstVideoSample && output != _captureVideoOutput) {
return;
}

CMTime currentSampleTime = CMSampleBufferGetPresentationTimeStamp(sampleBuffer);

if (_videoWriter.status != AVAssetWriterStatusWriting) {
[_videoWriter startWriting];
if (_isFirstVideoSample) {
[_videoWriter startSessionAtSourceTime:currentSampleTime];
// fix sample times not being numeric when pause/resume happens before first sample buffer
// arrives https://github.com/flutter/flutter/issues/132014
_lastVideoSampleTime = currentSampleTime;
_lastAudioSampleTime = currentSampleTime;
_isFirstVideoSample = NO;
}

if (output == _captureVideoOutput) {
Expand Down Expand Up @@ -834,6 +835,14 @@ - (void)startVideoRecordingWithCompletion:(void (^)(FlutterError *_Nullable))com
details:nil]);
return;
}
// startWriting should not be called in didOutputSampleBuffer where it can cause state
// in which _isRecording is YES but _videoWriter.status is AVAssetWriterStatusUnknown
// in stopVideoRecording if it is called after startVideoRecording but before
// didOutputSampleBuffer had chance to call startWriting and lag at start of video
// https://github.com/flutter/flutter/issues/132016
// https://github.com/flutter/flutter/issues/151319
[_videoWriter startWriting];
_isFirstVideoSample = YES;
_isRecording = YES;
_isRecordingPaused = NO;
_videoTimeOffset = CMTimeMake(0, 1);
Expand All @@ -853,19 +862,21 @@ - (void)stopVideoRecordingWithCompletion:(void (^)(NSString *_Nullable,
if (_isRecording) {
_isRecording = NO;

if (_videoWriter.status != AVAssetWriterStatusUnknown) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment explaining that it's not necessary to check AVAssetWriterStatusUnknown anymore since we call startWriting immediately, even before video sample callback (please rephrase this).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but such a comment loses context here because in sources it is not visible that there was such a check, only in this diff

[_videoWriter finishWritingWithCompletionHandler:^{
if (self->_videoWriter.status == AVAssetWriterStatusCompleted) {
[self updateOrientation];
completion(self->_videoRecordingPath, nil);
self->_videoRecordingPath = nil;
} else {
completion(nil, [FlutterError errorWithCode:@"IOError"
message:@"AVAssetWriter could not finish writing!"
details:nil]);
}
}];
}
// when _isRecording is YES startWriting was already called so _videoWriter.status
// is always either AVAssetWriterStatusWriting or AVAssetWriterStatusFailed and
// finishWritingWithCompletionHandler does not throw exception so there is no need
// to check _videoWriter.status
[_videoWriter finishWritingWithCompletionHandler:^{
if (self->_videoWriter.status == AVAssetWriterStatusCompleted) {
[self updateOrientation];
completion(self->_videoRecordingPath, nil);
self->_videoRecordingPath = nil;
} else {
completion(nil, [FlutterError errorWithCode:@"IOError"
message:@"AVAssetWriter could not finish writing!"
details:nil]);
}
}];
} else {
NSError *error =
[NSError errorWithDomain:NSCocoaErrorDomain
Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera_avfoundation/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: camera_avfoundation
description: iOS implementation of the camera plugin.
repository: https://github.com/flutter/packages/tree/main/packages/camera/camera_avfoundation
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22
version: 0.9.17+1
version: 0.9.17+2

environment:
sdk: ^3.2.3
Expand Down