Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
fix stopVideoRecording waiting indefinitely and video lag at start
  • Loading branch information
misos1 committed Jul 7, 2024
commit ba64f205358e7a01b254ce4f09241ba5385f0a06
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.16+1

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

## 0.9.16

* Converts Dart-to-host communcation to Pigeon.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,86 @@ - (void)testDidOutputSampleBufferIgnoreAudioSamplesBeforeVideoSamples {
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
38 changes: 22 additions & 16 deletions packages/camera/camera_avfoundation/ios/Classes/FLTCam.m
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 firstSample;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: isFirstSample

@property(assign, nonatomic) BOOL isRecording;
@property(assign, nonatomic) BOOL isRecordingPaused;
@property(assign, nonatomic) BOOL videoIsDisconnected;
Expand Down Expand Up @@ -663,15 +664,15 @@ - (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 (_firstSample && output != _captureVideoOutput) {
return;
}

CMTime currentSampleTime = CMSampleBufferGetPresentationTimeStamp(sampleBuffer);

if (_videoWriter.status != AVAssetWriterStatusWriting) {
[_videoWriter startWriting];
if (_firstSample) {
[_videoWriter startSessionAtSourceTime:currentSampleTime];
_firstSample = NO;
}

if (output == _captureVideoOutput) {
Expand Down Expand Up @@ -826,6 +827,13 @@ - (void)startVideoRecordingWithCompletion:(void (^)(FlutterError *_Nullable))com
details:nil]);
return;
}
// do not call startWriting in didOutputSampleBuffer to prevent state in which
// stopVideoRecordingWithCompletion does not send completion when _isRecording is
// YES but _videoWriter.status is AVAssetWriterStatusUnknown and video lag at start
// https://github.com/flutter/flutter/issues/132016
// https://github.com/flutter/flutter/issues/151319
[_videoWriter startWriting];
Copy link
Contributor

Choose a reason for hiding this comment

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

do you intend to remove startWriting from didOutputSampleBuffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

_firstSample = YES;
_isRecording = YES;
_isRecordingPaused = NO;
_videoTimeOffset = CMTimeMake(0, 1);
Expand All @@ -845,19 +853,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.

why is this check removed?

Copy link
Contributor Author

@misos1 misos1 Jul 9, 2024

Choose a reason for hiding this comment

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

This is where it was not sending completion (in false case) so a dart future never resolved. But now startWriting is called at the same time as _isRecording is set to YES so finishWritingWithCompletionHandler in stopVideoRecordingWithCompletion can be called only after was called startWriting which sets _videoWriter.status to AVAssetWriterStatusWriting so that check is redundant (and again it would just indicate false possibility of not calling completion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually maybe it should check whether status is AVAssetWriterStatusWriting and complete with error otherwise because it can be for example AVAssetWriterStatusFailed here (I suppose finishWritingWithCompletionHandler would just crash in that case). But this situation is nothing new introduced with this PR as before there was just check status != unknown which would pass failed to finishWritingWithCompletionHandler.

I also noticed check for AVAssetWriterStatusFailed in didOutputSampleBuffer where is called [self reportErrorMessage] but when I tried to force call reportErrorMessage I did not see this error report anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding previous comment, seems finishWritingWithCompletionHandler is ok with AVAssetWriterStatusFailed it will just pass it to handler so there is no need to check whether is status not failed before calling it, it only has problem with AVAssetWriterStatusCancelled but there is no call to cancelWriting, AVAssetWriterStatusCompleted and AVAssetWriterStatusUnknown but these two also cannot happen.

[_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.16
version: 0.9.16+1

environment:
sdk: ^3.2.3
Expand Down