Skip to content

[camerax] Implement video capture for CameraX android camera re-write #3467

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

Merged
camsim99 merged 19 commits into from
May 9, 2023
Merged

[camerax] Implement video capture for CameraX android camera re-write #3467

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f876e73
remaking video capture pr to fix CLA issue
May 3, 2023
ca2a353
accepting documentation change to createTempFile
May 3, 2023
22eea7a
making video recording tests into a group, and random typo
May 3, 2023
77dfba1
Merge branch 'main' into video_capture
gmackall May 4, 2023
daf385b
Merge branch 'flutter:main' into video_capture
gmackall May 8, 2023
167f9c2
documentation changes from review comments
May 8, 2023
23ec03e
changing test name
May 8, 2023
034a508
Merge branch 'flutter:main' into video_capture
gmackall May 8, 2023
d5affdd
changing a use of RuntimeException to a FlutterError
May 8, 2023
9797067
format
May 8, 2023
c76d1a4
adding tests for error cases in stopVideoRecording
May 8, 2023
3141b02
formatting
May 8, 2023
92a51b8
Making explicit that we do nothing on pressing record twice, and addi…
May 8, 2023
5e776a5
formatting new tests
May 8, 2023
0ba0dfc
review comments around documentation and an assert
May 8, 2023
89d7936
removing use of the word 'we' per the style guide
May 8, 2023
9eecbed
removing second 'assert' that was copied over from first instance of …
May 8, 2023
f8eee31
Merge branch 'main' into video_capture
gmackall May 9, 2023
fb4770b
Merge branch 'main' into video_capture
gmackall May 9, 2023
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
Prev Previous commit
Next Next commit
accepting documentation change to createTempFile
  • Loading branch information
Gray Mackall committed May 3, 2023
commit ca2a35360168ce46ecfacf04fc630244249b9175
6 changes: 4 additions & 2 deletions packages/camera/camera_android_camerax/lib/src/system_services.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,10 @@ class SystemServices {
api.stopListeningForDeviceOrientationChange();
}

/// Gets a file path to use to create a temp file at. It is created by calling
/// File.createTempFile(prefix, suffix, cacheDir), on the native side, where
/// Returns a file path which was used to create a temporary file.
///
/// The file and path constraints are determined by the implementation of
/// File.createTempFile(prefix, suffix, cacheDir), on the android side, where
/// where cacheDir is the cache directory identified by the current application
/// context using context.getCacheDir().
static Future<String> getTempFilePath(String prefix, String suffix,
Copy link
Contributor

@reidbaker reidbaker May 4, 2023

Choose a reason for hiding this comment

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

I think from the tests I read prefix is actually the file name and suffix is the extension. That came as a bit of a suprise from reading the documentation I would have expected prefix<something the caller does not control>suffix for the file name.

For example it means as a caller I think this means I need to ensure that I pass something different each time or risk overwriting the previous file.

Copy link
Member Author

@gmackall gmackall May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll clarify the documentation before merging, the way it actually works is that the file ends up being
prefix<something the caller does not control>.suffix, i.e. 'suffix' is the file extension, and prefix is part of the file name, but more gets added by createTempFile (a random sequence of numbers).

So getTempFile('MOV', '.temp'), is something like <cachedir>/MOV1231242244.temp (notably this means you do not have to pass different things to avoid overwriting).

FWIW I chose prefix and suffix because those are the variable names used by the native methods implementation/documentation.

Copy link
Contributor

@reidbaker reidbaker May 8, 2023

Choose a reason for hiding this comment

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

OK in that case it means the documentation is correct but that is not what the tests are testing for. I think using the same names as the native code is a reasonable choice.

Specifically in packages/camera/camera_android_camerax/test/system_services_test.dart and in packages/camera/camera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/SystemServicesTest.java you are testing for the result to be equal totestPath + testPrefix + testSuffix (or the file for prefix and suffix). Instead of testing that the output had a path of testPath a file prefix of testPrefix and an extension of testSuffix. Which is fine it is just confusing when trying to understand what to expect from the tests.

Expand Down