Skip to content
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

[camerax] Make fixes required to swap camera_android_camerax for camera_android #6697

Merged
merged 9 commits into from
May 10, 2024

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented May 8, 2024

Makes changes needed to land #6629. Specifically:

  • Fixes timing issue with stopVideoRecording such that the Future it returns will only complete when CameraX reports that the recording is finalized (via listening for the finalized video recording event)
  • Modifies startVideoCapturing such that the Future it returns will only complete when CameraX reports that video capturing has started (via listening for the started video recording event)
  • Adds empty implementation and TODO for implementing setDescriptionWhileRecording

Pre-launch Checklist

@camsim99 camsim99 changed the title [camerax] Make fixes to swap camera_android_camerax for camera_android [camerax] Make fixes required to swap camera_android_camerax for camera_android May 8, 2024
@camsim99 camsim99 marked this pull request as ready for review May 8, 2024 22:57
@camsim99 camsim99 requested a review from a team May 8, 2024 22:58
Comment on lines 198 to 202
sleep(const Duration(seconds: 2));

final XFile file = await controller.stopVideoRecording();
final int recordingTime =
DateTime.now().millisecondsSinceEpoch - recordingStart;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make this test a bit better by comparing on both sides of the stopRecording call.

Suggested change
sleep(const Duration(seconds: 2));
final XFile file = await controller.stopVideoRecording();
final int recordingTime =
DateTime.now().millisecondsSinceEpoch - recordingStart;
// Test should pass with this at any non zero value.
sleep(const Duration(seconds: 2));
final int preStopTime =
DateTime.now().millisecondsSinceEpoch - recordingStart;
final XFile file = await controller.stopVideoRecording();
final int postStopTime =
DateTime.now().millisecondsSinceEpoch - recordingStart;
....
expect(duration, greaterThan(preStopTime));
expect(duration, lessThan(postStopTime));

Note:
We should be using a package like https://pub.dev/packages/system_clock because DateTime.now is vulnerable to the clock changing while the test is running but that feels out of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The preStopTime check didn't quite work I assume because stopping video recording takes so long but I kept the naming of postStopTime for clarity

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't matter how long stopping recording takes.

The timeline:
start recording
record start time
delay
record prestop time
trigger stop

The comparison for prestop does not depend on the length of time it takes to stop recording.
At minimum the recording should be longer than the pause duration. What is the value of duration when the test fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe the start recording is slow I mean? The duration was 1448 ms (which is also below the 2000 ms of recording), prestop was 2002 ms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we could also fix this by listening for the VideoRecordEvent.Start event to return. Let me give it a go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reidbaker Ok so I added the fix but it didn't quite work still :/ the duration increased but guessing there still some lag time in starting the video. Still left it in though as it is a better approximation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to land this as-is since I've done all I can to remedy this issue. If we feel like we need this fixed, I can file a bug and escalate to CameraX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed flutter/flutter#148138 for this.

///
/// Currently unsupported, so is a no-op.
@override
Future<void> setDescriptionWhileRecording(CameraDescription description) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we have a feature we know will silently stop working when we modify the default to camerax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does mean that :/ but the fact that it will be a breaking change can mitigate that fact; it will be noted in the README here but I can also add it to the camera/camera CHANGELOG and camera/camera README.

@stuartmorgan any thoughts on this? I realized recently that this was not implemented for camera_android_camerax.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would definitely note the loss of functionality in the CHANGELOG for the camera breaking change.

This is a pretty new feature IIRC, and I would expect relatively niche, so losing it in a breaking change doesn't seem like a major issue. People will always have the possibility of using camera_android (or a fork of it, if/when we stop maintaining it) if they need it.

camsim99 added a commit to camsim99/packages that referenced this pull request May 9, 2024
@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2024
@auto-submit auto-submit bot merged commit 1ab2f5b into flutter:main May 10, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 13, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 13, 2024
flutter/packages@6c4482a...1412041

2024-05-13 stuartmorgan@google.com [in_app_purchase] Update country code Android example (flutter/packages#6722)
2024-05-13 engine-flutter-autoroll@skia.org Roll Flutter from 1dfb46e to 1255435 (6 revisions) (flutter/packages#6723)
2024-05-13 43054281+camsim99@users.noreply.github.com [camera] Change default Android implementation from `camera_android` to `camera_android_camerax` (flutter/packages#6629)
2024-05-13 15619084+vashworth@users.noreply.github.com [image_picker_ios] Make all headers public for SwiftPM to keep inline with CocoaPods (flutter/packages#6707)
2024-05-12 engine-flutter-autoroll@skia.org Roll Flutter from 2aa05c1 to 1dfb46e (3 revisions) (flutter/packages#6715)
2024-05-11 engine-flutter-autoroll@skia.org Roll Flutter from 2bfb1b0 to 2aa05c1 (26 revisions) (flutter/packages#6713)
2024-05-10 stuartmorgan@google.com [quick_actions] Add Swift Package Manager support (flutter/packages#6682)
2024-05-10 stuartmorgan@google.com [url_launcher] Add Swift Package Manager support (flutter/packages#6677)
2024-05-10 43054281+camsim99@users.noreply.github.com [Android][webview_flutter] Run integration tests on emulators running Android 34 (flutter/packages#6499)
2024-05-10 50643541+Mairramer@users.noreply.github.com [image_picker_android] - will fix crash on Android 12+ devices (flutter/packages#6691)
2024-05-10 43054281+camsim99@users.noreply.github.com [camerax] Make fixes required to swap camera_android_camerax for camera_android (flutter/packages#6697)
2024-05-10 737941+loic-sharma@users.noreply.github.com Update .gitignore for Swift Package Manager (flutter/packages#6705)
2024-05-10 34871572+gmackall@users.noreply.github.com [quick_actions_android] Switch to `Compat` version of `ShortcutManager` to support Google surfaces (flutter/packages#6638)
2024-05-10 737941+loic-sharma@users.noreply.github.com [local_auth_darwin] Adds Swift Package Manager compatibility (flutter/packages#6708)
2024-05-10 engine-flutter-autoroll@skia.org Roll Flutter from 00f4066 to 2bfb1b0 (9 revisions) (flutter/packages#6706)
2024-05-09 38299943+VictorOhashi@users.noreply.github.com [go_router] Feat add route redirect shellroutes (#114559) (flutter/packages#6432)

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 flutter-ecosystem@google.com,rmistry@google.com 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
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
…ra_android (flutter#6697)

Makes changes needed to land flutter#6629. Specifically:

- Fixes timing issue with `stopVideoRecording` such that the `Future` it returns will only complete when CameraX reports that the recording is finalized (via listening for the [finalized video recording event](https://developer.android.com/reference/androidx/camera/video/VideoRecordEvent.Finalize))
- Modifies `startVideoCapturing` such that the `Future` it returns will only complete when CameraX reports that video capturing has started (via listening for the [started video recording event](https://developer.android.com/reference/androidx/camera/video/VideoRecordEvent.Start))
- Adds empty implementation and TODO for implementing `setDescriptionWhileRecording`
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…ra_android (flutter#6697)

Makes changes needed to land flutter#6629. Specifically:

- Fixes timing issue with `stopVideoRecording` such that the `Future` it returns will only complete when CameraX reports that the recording is finalized (via listening for the [finalized video recording event](https://developer.android.com/reference/androidx/camera/video/VideoRecordEvent.Finalize))
- Modifies `startVideoCapturing` such that the `Future` it returns will only complete when CameraX reports that video capturing has started (via listening for the [started video recording event](https://developer.android.com/reference/androidx/camera/video/VideoRecordEvent.Start))
- Adds empty implementation and TODO for implementing `setDescriptionWhileRecording`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants