App foundation: introducing CameraX #23

mzorz · 2019-08-09T19:13:19Z

NOTE: please after reviewing, review and merge #25 first before merging this one 👍

This PR introduces Android Jetpack's Camera X. It's important to note the VideoCapture use case is still under the RestrictedApi annotation, so using it here ~~carefully~~ hoping they'll make it public soon.

This PR also introduces some simplification of the abstraction of a VideoRecorderFragment, which has the capabilities implemented by the interface VideoRecorderHandler:

startRecordingVideo()
stopRecordingVideo()

and interface SurfaceFragmentHandler:

activate()
deactivate()

This way, we kept both the Camera2 API implementation in Camera2BasicHandling class, and encapsulated the CameraX API implementation within the CameraXBasicHandling class, both of which are nothing else but instances of the aforementioned VideoRecorderFragment.
This also led to a simplification in the BackgroundSurfaceManager code, which is now easier to read and should pretty much take care of all the basic background surfaces handling such as turning the camera on/off, start/stop recording, and switch background surfaces (static, video player, camera preview).

Also this PR introduces a BuildConfig.USE_CAMERAX flag in gradle.properties, which is used in order to use one API or the other depending on this setting.

This way, should we need to fall back to the stable Camera2 API we can do so, by just turning the flag.

This PR introduces only the following two use cases, to just be able to match the current functionality in the app:

preview, and
VideoCapture (not documented as of yet)

In a follow up PR we'll implement the still image capture use case as well (it's coded in our Camera2 fragment handler implementation as well, but needs to be wired up).
Note there of course may be differences in handling to our own implementation, but we'll be able to choose among each of the alternatives.

To Test

start the app
go to the Composer activity (press on the floating action button)
in the overflow menu, tap on Camera preview. Check you see the camera view.
tap on the Camera preview option in the overflow menu again to START recording (note no indication that recording started is shown so, you'll have to believe)
tap on the Camera preview option in the overflow menu a third time to STOP recording. This will silently save the original recording file.
draw some emoji or text
tap on Save option (last option in the menu)
new video should have been composed.

…ntation in BackgroundSurfaceManager

…ation code for readability

…d SurfaceFragmentHandler

…e and simplified distinction in BackgroundSurfaceManager

…a on Camera2 API

…n-camerax

aforcier

Looks good overall and works well 👍 Just a few comments, some are just Kotlin FYIs as we discussed 😀

aforcier · 2019-08-13T13:09:04Z

photoeditor/src/main/java/com/automattic/photoeditor/state/BackgroundSurfaceManager.kt

    }

    fun getCurrentVideoFile(): File? {
-        return camera2BasicHandler.currentFile
+            return cameraBasicHandler.currentFile


Some weird indentation showed up here.

👍 addressed in: bf3b76a

aforcier · 2019-08-13T13:35:49Z

photoeditor/src/main/java/com/automattic/photoeditor/state/BackgroundSurfaceManager.kt

+                    // the photoEditorView layout has been recreated so, re-assign its TextureView
+                    cameraBasicHandler.textureView = photoEditorView.textureView
+                }
+            }


I'm probably missing something - could you explain why in the CAMERAX case we don't need to add a texture listener here as in the other cases?

Excellent question! It's all taken care of by the lifecycle binding CameraX uses so, we only need to tell the CameraX use case what we want from it, for example start recording video (or stopping an in-progress recording), or starting feeding the surface with the images being buffered from the camera preview, etc.

Copying from the referenced documentation in the link above for easy reading:

Instead of an application placing specific start and stop method calls in onResume() and onPause(), the application specifies a lifecycle to associate the camera with, using CameraX.bindToLifecycle(). The startup, shutdown, and production of data are governed by the specified Android Architecture Lifecycle. That lifecycle then informs CameraX when to configure the camera capture session and ensures camera state changes appropriately to match lifecycle transitions.

Ah I follow - I don't think it clicked for me that the texture listener hook was for lifecycle management. Thanks for the details, super helpful 👍

aforcier · 2019-08-13T13:36:55Z

photoeditor/src/main/java/com/automattic/photoeditor/state/BackgroundSurfaceManager.kt

@@ -23,11 +29,16 @@ class BackgroundSurfaceManager(
    private val savedInstanceState: Bundle?,


The activity val here in the constructor seems to be unused.

nice catch, I also spotted and addressed it in this other PR #26 in commits 258557f and 3c49776

aforcier · 2019-08-13T13:38:16Z

app/build.gradle

@@ -42,3 +42,40 @@ dependencies {
    androidTestImplementation 'androidx.test:runner:1.2.0'
    androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0'
 }
+
+android.buildTypes.all { buildType ->


All of this looks good, though I think we should take care of adding gradle.properties to .gitignore sooner than later and add a gradle.properties-example file. I'll open an issue to track that.

Opened: #27.

aforcier · 2019-08-13T13:49:35Z

app/build.gradle

+    def inputFile = checkGradlePropertiesFile()
+    def properties = loadPropertiesFromFile(inputFile)
+    properties.any { property ->
+        if (property.key.toLowerCase().startsWith("portkey.use.")) {


Interesting - in WPAndroid we don't have this handling and instead just use BuildConfig.VALUE.toBoolean() when accessing it. This is probably better 😀

TBH that was a TIL for me as well when I introduced / borrowed these scripts :D

aforcier · 2019-08-13T17:18:25Z

photoeditor/src/main/java/com/automattic/photoeditor/camera/VideoPlayingBasicHandling.kt

-//                setOnVideoSizeChangedListener(this)
-                setAudioStreamType(AudioManager.STREAM_MUSIC)
-                start()
+            if (currentFile != null && currentFile!!.exists()) {


We can Kotlinify this a bit and fix a nullability warning in the code (the call is actually safe but the IDE doesn't know that):

currentFile?.takeIf { it.exists() }?.let { file -> ... val inputStream = FileInputStream(file) ... }

nice! addressed in e0f25fa (plus removed some commented code)

Also TIL:

the call is actually safe but the IDE doesn't know that

aforcier · 2019-08-13T17:19:16Z

photoeditor/src/main/java/com/automattic/photoeditor/camera/VideoPlayingBasicHandling.kt

+//                val descriptor = assetManager!!.openFd("small.mp4")
+                val inputStream = FileInputStream(currentFile)
+                mediaPlayer = MediaPlayer().apply {
+                    setDataSource(inputStream.getFD())


(Kotlinification note, not a blocker)

This can use property access syntax: inputStream.fd.

aforcier · 2019-08-13T17:20:22Z

photoeditor/build.gradle

+    // CameraX core library
+    def camerax_version = "1.0.0-alpha04"
+    // CameraX view library
+    def camerax_view_version = "1.0.0-alpha01"


This one and the one below seems to be unused.

addressed in 26767fc

aforcier · 2019-08-13T17:25:55Z

photoeditor/src/main/java/com/automattic/photoeditor/camera/CameraXBasicHandling.kt

+        permissions: Array<String>,
+        grantResults: IntArray
+    ) {
+        if (!PermissionUtils.allRequiredPermissionsGranted(activity!!)) {


We can remove the !! force unwrap here by wrapping the whole function body in a null check:

activity?.let { activity -> ... }

Ended up removing this entirely in bbcf4df, to match the same thing we did in e2fe8ee , related comment here: #22 (comment)

aforcier · 2019-08-14T12:13:51Z

Awesome @mzorz, !

mzorz added 5 commits August 9, 2019 13:07

added gradle properties handling to select Camera2 or CameraX impleme…

c4d56ed

…ntation in BackgroundSurfaceManager

introduce CameraXBasicHandling fragment, simplified fragment instanti…

a2673b6

…ation code for readability

added CameraX implementation, separated concerns for VideoRecorder an…

49286a8

…d SurfaceFragmentHandler

made VideoRecorderFragment contain the minimum needed surface and fil…

ff17c99

…e and simplified distinction in BackgroundSurfaceManager

removed commented code

2e4b24d

mzorz requested a review from aforcier August 9, 2019 19:13

This was referenced Aug 9, 2019

Simplified permission handling #22

Merged

Convert mp4composer to Kotlin #21

Merged

Introduce mp4composer lib as a module #18

Merged

mzorz added 3 commits August 9, 2019 16:15

fixed style issue

30c248e

removed comment

14dee59

fixed style issues

9358e8b

mzorz mentioned this pull request Aug 10, 2019

Crash when start recording video on emulator w/ API 26 #20

Closed

mzorz added 2 commits August 10, 2019 08:47

only attempt to deactivate if already active

8cdd629

fixed switching to video player mode after recording video from camer…

4944f99

…a on Camera2 API

This was referenced Aug 10, 2019

Unable to play video after recording with CameraX #24

Closed

[do not merge] App foundations primary issue #1

Merged

aforcier self-assigned this Aug 12, 2019

aforcier added this to the Demo 1: Basic Creation milestone Aug 12, 2019

mzorz added 2 commits August 13, 2019 15:14

fixed merge conflicts

2f4c6aa

Merge branch 'app/foundation-permissions-refactor' into app/foundatio…

78054d2

…n-camerax

aforcier reviewed Aug 13, 2019

View reviewed changes

mzorz added 4 commits August 13, 2019 15:24

fixed indentation

bf3b76a

removed unused defs

26767fc

kotlinified expression plus removed commented code

e0f25fa

removed onRequestPermissionsResult handling in fragment

bbcf4df

aforcier mentioned this pull request Aug 13, 2019

Untrack gradle.properties #28

Merged

aforcier changed the base branch from app/foundation-permissions-refactor to app/foundation August 13, 2019 19:03

aforcier merged commit 447461f into app/foundation Aug 14, 2019

aforcier deleted the app/foundation-camerax branch August 14, 2019 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App foundation: introducing CameraX #23

App foundation: introducing CameraX #23

mzorz commented Aug 9, 2019 •

edited

Loading

aforcier left a comment

aforcier Aug 13, 2019

mzorz Aug 13, 2019 •

edited

Loading

aforcier Aug 13, 2019

mzorz Aug 13, 2019

aforcier Aug 14, 2019

aforcier Aug 13, 2019

mzorz Aug 13, 2019

aforcier Aug 13, 2019

aforcier Aug 13, 2019

aforcier Aug 13, 2019

mzorz Aug 13, 2019

aforcier Aug 13, 2019

mzorz Aug 13, 2019

aforcier Aug 13, 2019

aforcier Aug 13, 2019

mzorz Aug 13, 2019

aforcier Aug 13, 2019

mzorz Aug 13, 2019

aforcier commented Aug 14, 2019

		@@ -23,11 +29,16 @@ class BackgroundSurfaceManager(
		private val savedInstanceState: Bundle?,

App foundation: introducing CameraX #23

App foundation: introducing CameraX #23

Conversation

mzorz commented Aug 9, 2019 • edited Loading

To Test

aforcier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzorz Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aforcier commented Aug 14, 2019

mzorz commented Aug 9, 2019 •

edited

Loading

mzorz Aug 13, 2019 •

edited

Loading