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.

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

Update video recording to be controlled through the CoroutineJob #34

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update video recording to be controlled through the CoroutineJob #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e696eec
Update VideoRecording to be controlled through the CoroutineJob
yasith Aug 26, 2023
98f8d88
Merge branch 'main' into yasith/videoCaptureRefactor
yasith Sep 5, 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
7 changes: 4 additions & 3 deletions domain/camera/src/main/java/com/google/jetpackcamera/domain/camera/CameraUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import androidx.camera.core.Preview
import com.google.jetpackcamera.settings.model.AspectRatio
import com.google.jetpackcamera.settings.model.CameraAppSettings
import com.google.jetpackcamera.settings.model.FlashModeStatus
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job

/**
* Data layer for camera.
Expand All @@ -46,9 +48,7 @@ interface CameraUseCase {

suspend fun takePicture()

suspend fun startVideoRecording()

fun stopVideoRecording()
fun startVideoRecording(scope: CoroutineScope): Job

fun setZoomScale(scale: Float): Float

Expand All @@ -65,4 +65,5 @@ interface CameraUseCase {
companion object {
const val INVALID_ZOOM_SCALE = -1f
}

}
33 changes: 21 additions & 12 deletions domain/camera/src/main/java/com/google/jetpackcamera/domain/camera/CameraXCameraUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,12 @@ import com.google.jetpackcamera.settings.model.CameraAppSettings
import com.google.jetpackcamera.settings.model.FlashModeStatus
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.asExecutor
import kotlinx.coroutines.awaitCancellation
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.launch
import java.util.Date
import javax.inject.Inject

Expand Down Expand Up @@ -144,7 +147,7 @@ class CameraXCameraUseCase @Inject constructor(
})
}

override suspend fun startVideoRecording() {
override fun startVideoRecording(scope: CoroutineScope): Job {
Log.d(TAG, "recordVideo")
val captureTypeString = if(singleStreamCaptureEnabled) "SingleStream" else "MultiStream"
val name = "JCA-recording-${Date()}-$captureTypeString.mp4"
Expand All @@ -158,19 +161,25 @@ class CameraXCameraUseCase @Inject constructor(
.setContentValues(contentValues)
.build()

recording = videoCaptureUseCase.output
.prepareRecording(application, mediaStoreOutput)
.start(ContextCompat.getMainExecutor(application), Consumer { videoRecordEvent ->
run {
Log.d(TAG, videoRecordEvent.toString())
}
})
return videoCaptureUseCase.startIn(mediaStoreOutput, scope)
}

override fun stopVideoRecording() {
Log.d(TAG, "stopRecording")
recording?.stop()
}
private fun VideoCapture<Recorder>.startIn(
outputOptions: MediaStoreOutputOptions,
scope: CoroutineScope
): Job =
scope.launch {
output
.prepareRecording(application, outputOptions)
.start(ContextCompat.getMainExecutor(application), Consumer { videoRecordEvent ->
run {
Log.d(TAG, videoRecordEvent.toString())
}
})
.use {
awaitCancellation()
}
}

override fun setZoomScale(scale: Float): Float {
val zoomState = getZoomState() ?: return INVALID_ZOOM_SCALE
Expand Down
19 changes: 14 additions & 5 deletions domain/camera/src/main/java/com/google/jetpackcamera/domain/camera/test/FakeCameraUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ import com.google.jetpackcamera.domain.camera.CameraUseCase
import com.google.jetpackcamera.settings.model.AspectRatio
import com.google.jetpackcamera.settings.model.CameraAppSettings
import com.google.jetpackcamera.settings.model.FlashModeStatus
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.awaitCancellation
import kotlinx.coroutines.launch

class FakeCameraUseCase : CameraUseCase {

Expand Down Expand Up @@ -74,14 +78,19 @@ class FakeCameraUseCase : CameraUseCase {
numPicturesTaken += 1
}

override suspend fun startVideoRecording() {
recordingInProgress = true
}
override fun startVideoRecording(scope: CoroutineScope): Job {
val job = scope.launch {
recordingInProgress = true
awaitCancellation()
}
job.invokeOnCompletion {
recordingInProgress = false
}

override fun stopVideoRecording() {
recordingInProgress = false
return job
}


override fun setZoomScale(scale: Float): Float {
return -1f
}
Expand Down
20 changes: 6 additions & 14 deletions feature/preview/src/main/java/com/google/jetpackcamera/feature/preview/PreviewViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,21 +190,14 @@ class PreviewViewModel @Inject constructor(

fun startVideoRecording() {
Log.d(TAG, "startVideoRecording")
recordingJob = viewModelScope.launch {

try {
cameraUseCase.startVideoRecording()
_previewUiState.emit(
previewUiState.value.copy(
videoRecordingState = VideoRecordingState.ACTIVE
)
viewModelScope.launch {
_previewUiState.emit(
previewUiState.value.copy(
videoRecordingState = VideoRecordingState.ACTIVE
)
Log.d(TAG, "cameraUseCase.startRecording success")
} catch (exception: IllegalStateException) {
Log.d(TAG, "cameraUseCase.startVideoRecording error")
Log.d(TAG, exception.toString())
}
)
}
recordingJob = cameraUseCase.startVideoRecording(viewModelScope)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you consider keeping the stopVideoRecording function and just having the use case manage the job rather than exposing it to the view model?
From an interface / api pov I would expect a start and stop
If the use case manages the job then it could also include logic of asserting that a call to start doesn't have an active job (or cancel the job first and then start again) and similarly for stop
I'm also not sure if the job should be managed by the use case or maybe it's better suited for the view model

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some API feedback we received on the initial VideoCapture use case was that we should avoid including API that can be called from an invalid state. This exact scenario was discussed: you shouldn't be able to call a "stop" method if there isn't already a recording in progress. This is considered an API design problem with the existingMediaRecorder API. The way we implemented this in "VideoCapture" was to have a chain of methods that returned more "scoped" APIs:

  1. Recorder.prepareRecording() --> class PendingRecording
  2. PendingRecording.start() --> class Recording : AutoCloseable
  3. Recording.stop()/close().

We possibly could have taken this even further. For instance, we don't have anything like a PausedRecording when you call Recording.pause().

Kotlin and coroutines make this kind of API decomposition even easier. Instead of dealing with the clunky Java AutoCloseable, we now have coroutine scopes to handle that.

I do think we should avoid returning the Job as it's a pretty low-level detail that can probably be managed internally by the use case. We also don't need an explicit CoroutineScope argument if we keep startVideoRecording as a suspend function. We could do something like this:

suspend fun startVideoRecording(session: suspend (recording: Recording) -> RecordingCompletionStatus): RecordingCompletionStatus = coroutineScope {
  // Check if an existing job exists and throw an error if so, otherwise store `coroutineContext.job` as the active job.
  ...
  try {
    return@coroutineScope session(Recording(mediaStoreOutput))
  } finally {
    ...
  }
}

This allows us to use it something like:

viewModelScope.launch {
  val completionStatus = cameraUseCase.startVideoRecording { recording ->
    // Set up a listener that triggers from a "stop" button being pressed.
    stopSignal.addListener { recording.stop() }

    // These could potentially be broken down further into their own `coroutineScope`
    pauseSignal.addListener { recording.pause() }
    resumeSignal.addListener { recording.resume() }

    // Add listeners for recording duration, etc
    recording.addDurationListener { ... }

    // Suspend until completion
    return recording.awaitCompletion()
  }

  // Do something with the completion status
 ...
}

}

fun stopVideoRecording() {
Expand All @@ -216,7 +209,6 @@ class PreviewViewModel @Inject constructor(
)
)
}
cameraUseCase.stopVideoRecording()
recordingJob?.cancel()
}

Expand Down
1 change: 1 addition & 0 deletions ...re/preview/src/test/java/com/google/jetpackcamera/feature/preview/PreviewViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class PreviewViewModelTest {
previewViewModel.startVideoRecording()
advanceUntilIdle()
previewViewModel.stopVideoRecording()
advanceUntilIdle()
assertEquals(cameraUseCase.recordingInProgress, false)

}
Expand Down