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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Aug 2, 2024
Merged

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

merged 9 commits into from
Aug 2, 2024

Conversation

misos1
Copy link
Contributor

@misos1 misos1 commented Jul 7, 2024

Call to startWriting was moved to startVideoRecordingWithCompletion to fix situation when completion is not called when _isRecording is YES but _videoWriter.status is AVAssetWriterStatusUnknown in stopVideoRecordingWithCompletion which can happen if stopVideoRecordingWithCompletion is called right after startVideoRecordingWithCompletion and didOutputSampleBuffer had no chance to call startWriting in meantime and to avoid lag which happens when is called startWriting between first frame creation and its appending.

Fixes flutter/flutter#132016
Fixes flutter/flutter#151319

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@misos1 misos1 requested a review from hellohuanlin as a code owner July 7, 2024 17:19
@misos1 misos1 force-pushed the startwriting_in_startrecording branch from 1815a2a to 83cef95 Compare July 7, 2024 17:32
@misos1 misos1 mentioned this pull request Jul 7, 2024
Open
@misos1
Copy link
Contributor Author

misos1 commented Jul 7, 2024 •
edited
Loading

Btw integration tests fail on my side with flutter packages main branch because here file.path for example ends with CAP_D5507C07-56E6-48E5-96F8-7D22FF8B5AB6.jpg but fileFormat.name is jpeg so file path does not end with file format name.

flutter: 00:55 +8: Capture specific image output formats
camera_avfoundation/example/integration_test/camera_test.dart:339
> expect(file.path.endsWith(fileFormat.name), true);

@misos1 misos1 force-pushed the startwriting_in_startrecording branch from 83cef95 to ba64f20 Compare July 7, 2024 18:45
hellohuanlin
hellohuanlin reviewed Jul 8, 2024
View reviewed changes
packages/camera/camera_avfoundation/ios/Classes/FLTCam.m Outdated
@@ -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

packages/camera/camera_avfoundation/ios/Classes/FLTCam.m Outdated
@@ -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 •
edited
Loading

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.

packages/camera/camera_avfoundation/ios/Classes/FLTCam.m Outdated
// 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

@hellohuanlin
Copy link
Contributor

Btw integration tests fail on my side with flutter packages main branch because here file.path for example ends with CAP_D5507C07-56E6-48E5-96F8-7D22FF8B5AB6.jpg but fileFormat.name is jpeg so file path does not end with file format name.

Seems reasonable to allow both jpg and jpeg to pass

misos1 added 4 commits July 9, 2024 17:07
@misos1
rename variable
e2161c3
@misos1
Merge branch 'main' into startwriting_in_startrecording
f2d83af 
# Conflicts:
#	packages/camera/camera_avfoundation/CHANGELOG.md
#	packages/camera/camera_avfoundation/example/ios/RunnerTests/FLTCamSampleBufferTests.m
#	packages/camera/camera_avfoundation/ios/Classes/FLTCam.m
#	packages/camera/camera_avfoundation/pubspec.yaml
@misos1
Merge branch 'main' into startwriting_in_startrecording
165afc7
@misos1
Merge branch 'main' into startwriting_in_startrecording
05d19ce
@misos1 misos1 requested a review from hellohuanlin July 15, 2024 19:17
hellohuanlin
hellohuanlin reviewed Jul 16, 2024
View reviewed changes
Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

I think it's reasonable change. Just some nit to make it easier for future readers.

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m Outdated
@@ -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?

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m Outdated
@@ -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.

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m Outdated
@@ -830,6 +831,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
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)

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m
@@ -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

@misos1 misos1 requested a review from hellohuanlin July 17, 2024 18:16
hellohuanlin
hellohuanlin approved these changes Jul 17, 2024
View reviewed changes
...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m Outdated
}
// when _isRecording is YES startWriting was already called so _videoWriter.status
// is always either AVAssetWriterStatusWriting or AVAssetWriterStatusFailed and
// finishWritingWithCompletionHandler does not throw exception
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "so there's no need to check _videoWriter.status before finish writing"

@misos1
Copy link
Contributor Author

misos1 commented Jul 17, 2024

Btw does this also require a second review?

@hellohuanlin
Copy link
Contributor

Btw does this also require a second review?

Only first time contributors require a second review, so you are good to go. But feel free to tag more people as you see fit.

@misos1
Copy link
Contributor Author

misos1 commented Jul 17, 2024 •
edited
Loading

Oh ok (there are comments in my 2nd and 3rd PR that second reviews are required, but I did my first some year ago).

@misos1 misos1 force-pushed the startwriting_in_startrecording branch from 7c5ef79 to fb3d713 Compare July 22, 2024 18:13
@hellohuanlin
Copy link
Contributor

Only first time contributors require a second review, so you are good to go. But feel free to tag more people as you see fit.

Sorry I was terribly wrong - we need a second reviewer to land this.

@jmagman
Copy link
Member

jmagman commented Jul 24, 2024

For the curious, from https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#getting-a-code-review

Every PR must be code-reviewed before check-in, including things like rolling a dependency. Getting a review means that a regular Flutter contributor (someone with commit access; see contributor access for details) has "approved" the PR in the GitHub UI. We call this "getting an LGTM" ("looks good to me").

If you are not yourself someone with commit access, then a second person with commit access must also review and approve your PR. This ensures that two people with commit access (trusted contributors) agree on every commit.

bparrishMines
bparrishMines approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

@hellohuanlin
Copy link
Contributor

There's also a process to become a member: https://github.com/flutter/flutter/blob/master/docs/contributing/Contributor-access.md

If you are interested, I'm more than happy to sponsor it.

@bparrishMines bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 2, 2024
@auto-submit auto-submit bot merged commit 9d33722 into flutter:main Aug 2, 2024
76 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 5, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 5, 2024
@engine-flutter-autoroll
9d33722 [camera_avfoundation] fix stopVideoRecording waiting indefini…
b3eaf72 
…tely and video lag at start (flutter/packages#7065)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 5, 2024
@engine-flutter-autoroll
Roll Packages from cc9ff47 to 82e8d1e (7 revisions) (#152866)
3832823 
flutter/packages@cc9ff47...82e8d1e

2024-08-04 [email protected] Manual roll Flutter from f10a497 to cbfb222 (23 revisions) (flutter/packages#7298)
2024-08-04 [email protected] [camera_avfoundation] dealloc camera on dispose (flutter/packages#7211)
2024-08-03 [email protected] Manual roll Flutter from 4ff9462 to f10a497 (22 revisions) (flutter/packages#7297)
2024-08-03 [email protected] Manual roll Flutter from 85960d2 to 4ff9462 (45 revisions) (flutter/packages#7296)
2024-08-03 [email protected] Manual roll Flutter from 9d5ede0 to 85960d2 (24 revisions) (flutter/packages#7295)
2024-08-02 [email protected] Roll to 9d5ede0, disabling impeller for google_maps_flutter_android (flutter/packages#7294)
2024-08-02 [email protected] [camera_avfoundation] fix stopVideoRecording waiting indefinitely and video lag at start (flutter/packages#7065)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
@engine-flutter-autoroll @TytaniumDev
Roll Packages from cc9ff47 to 82e8d1e (7 revisions) (flutter#152866)
7d7190f 
flutter/packages@cc9ff47...82e8d1e

2024-08-04 [email protected] Manual roll Flutter from f10a497 to cbfb222 (23 revisions) (flutter/packages#7298)
2024-08-04 [email protected] [camera_avfoundation] dealloc camera on dispose (flutter/packages#7211)
2024-08-03 [email protected] Manual roll Flutter from 4ff9462 to f10a497 (22 revisions) (flutter/packages#7297)
2024-08-03 [email protected] Manual roll Flutter from 85960d2 to 4ff9462 (45 revisions) (flutter/packages#7296)
2024-08-03 [email protected] Manual roll Flutter from 9d5ede0 to 85960d2 (24 revisions) (flutter/packages#7295)
2024-08-02 [email protected] Roll to 9d5ede0, disabling impeller for google_maps_flutter_android (flutter/packages#7294)
2024-08-02 [email protected] [camera_avfoundation] fix stopVideoRecording waiting indefinitely and video lag at start (flutter/packages#7065)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
@engine-flutter-autoroll @Buchimi
Roll Packages from cc9ff47 to 82e8d1e (7 revisions) (flutter#152866)
6b69329 
flutter/packages@cc9ff47...82e8d1e

2024-08-04 [email protected] Manual roll Flutter from f10a497 to cbfb222 (23 revisions) (flutter/packages#7298)
2024-08-04 [email protected] [camera_avfoundation] dealloc camera on dispose (flutter/packages#7211)
2024-08-03 [email protected] Manual roll Flutter from 4ff9462 to f10a497 (22 revisions) (flutter/packages#7297)
2024-08-03 [email protected] Manual roll Flutter from 85960d2 to 4ff9462 (45 revisions) (flutter/packages#7296)
2024-08-03 [email protected] Manual roll Flutter from 9d5ede0 to 85960d2 (24 revisions) (flutter/packages#7295)
2024-08-02 [email protected] Roll to 9d5ede0, disabling impeller for google_maps_flutter_android (flutter/packages#7294)
2024-08-02 [email protected] [camera_avfoundation] fix stopVideoRecording waiting indefinitely and video lag at start (flutter/packages#7065)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@misos1
Copy link
Contributor Author

misos1 commented Sep 18, 2024

@hellohuanlin Ok so let's try it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparrishMines bparrishMines bparrishMines approved these changes

@hellohuanlin hellohuanlin hellohuanlin approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-ios platform-macos
Projects
None yet
Milestone
No milestone
4 participants
@misos1 @hellohuanlin @jmagman @bparrishMines