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 5 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+1

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

## 0.9.17

* Adds Swift Package Manager compatibility.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,4 +200,86 @@ - (void)testDidOutputSampleBufferSampleTimesMustBeNumericAfterPauseResume {
CFRelease(audioSample);
}

- (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;
});

[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 isFirstSample;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 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 (_isFirstSample && output != _captureVideoOutput) {
return;
}

CMTime currentSampleTime = CMSampleBufferGetPresentationTimeStamp(sampleBuffer);

if (_videoWriter.status != AVAssetWriterStatusWriting) {
[_videoWriter startWriting];
if (_isFirstSample) {
[_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;
_isFirstSample = NO;
}

if (output == _captureVideoOutput) {
Expand Down Expand Up @@ -830,6 +831,13 @@ - (void)startVideoRecordingWithCompletion:(void (^)(FlutterError *_Nullable))com
details:nil]);
return;
}
// do not call startWriting in didOutputSampleBuffer to prevent state in which
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "do not move startWriting call to didOutputSampleBuffer... "

Because there should only be 1 place to call startWriting.

// stopVideoRecordingWithCompletion does not send completion when _isRecording is
// YES but _videoWriter.status is AVAssetWriterStatusUnknown and video lag at start
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 also explain that this happens when stop video is called immediately after start, before the first video sample is arrived (if i understand correctly)

// https://github.com/flutter/flutter/issues/132016
// https://github.com/flutter/flutter/issues/151319
[_videoWriter startWriting];
_isFirstSample = YES;
_isRecording = YES;
_isRecordingPaused = NO;
_videoTimeOffset = CMTimeMake(0, 1);
Expand All @@ -849,19 +857,17 @@ - (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]);
}
}];
}
[_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
version: 0.9.17+1

environment:
sdk: ^3.2.3
Expand Down