Update video recording to be controlled through the CoroutineJob

[yasith]

This is a clean up PR, to clean up the video recording code. Video start/stop is now controlled through the coroutine job spawned from viewModel scope

[jsaund]

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

[temcguir]

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
 ...
}

[jsaund]

overall lgtm
just curious about whether we should keep the stop function