Skip to content

Fix crash on rapid double-clicking video record button #541

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

Open
davidjiagoogle wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix crash on rapid double-clicking video record button #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraSession.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1224,7 +1224,10 @@ private suspend fun runVideoRecording(
for (event in videoControlEvents) {
when (event) {
is VideoCaptureControlEvent.StartRecordingEvent ->
throw IllegalStateException("A recording is already in progress")
Log.w(
TAG,
"A recording is already in progress, ignoring extra StartRecordingEvent"
)

VideoCaptureControlEvent.StopRecordingEvent -> {
recordingSettingsUpdater.cancel()
Expand Down
2 changes: 2 additions & 0 deletions ...e/src/main/java/com/google/jetpackcamera/ui/components/capture/CaptureButtonComponents.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ internal fun CaptureButton(
}

CaptureButtonUiState.Enabled.Recording.LockedRecording -> onStopRecording()
CaptureButtonUiState.Enabled.Recording.Pending,

@gemini-code-assist gemini-code-assist Bot Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If a user starts a video recording via a long-press but releases the button quickly while the UI state is still in CaptureButtonUiState.Enabled.Recording.Pending (before transitioning to PressedRecording), the release event is ignored. This is because the isLongPressing block in onKeyUp (lines 251-258) only checks for PressedRecording to trigger onStopRecording(). As a result, the recording will continue indefinitely with the UI stuck in the pressed state even though the user has released their finger.\n\nTo fix this, please update the check in onKeyUp to also handle the Pending state:\n\nkotlin\nif (!isLocked &&\n (currentUiState.value is CaptureButtonUiState.Enabled.Recording.PressedRecording ||\n currentUiState.value is CaptureButtonUiState.Enabled.Recording.Pending)\n) {\n Log.d(TAG, "Stopping recording")\n onStopRecording()\n}\n

@temcguir temcguir Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree with @gemini-code-assist's inline comment to allow stopping the recording by releasing the long-press while in the Starting state.

However, applying the bot's fix will expose a coroutine race condition in CaptureControllerImpl.kt. Because CameraSession.kt silently ignores StopRecordingEvents if it isn't actively recording yet, a rapid start/stop sequence will freeze the camera into an endless recording loop. If the UI dispatcher executes the stop coroutine before the start coroutine sends its event, the Stop event is sent to the channel first (and ignored), and then the Start event triggers the recording infinitely.

Could we also include a quick fix in CaptureControllerImpl.kt in this PR to synchronously cancel the job?

    // CaptureControllerImpl.kt
    override fun stopVideoRecording() {
        Log.d(TAG, "stopVideoRecording")
        // Cancel job synchronously before sending Stop to prevent the race condition
        recordingJob?.cancel() 
        recordingJob = null
        scope.launch {
            cameraSystem.stopVideoRecording()
        }
    }

@gemini-code-assist gemini-code-assist Bot Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

CaptureButtonUiState.Enabled.Recording.PressedRecording,
CaptureButtonUiState.Unavailable -> {
}
Expand Down Expand Up @@ -668,6 +669,7 @@ private fun CaptureButtonNucleus(
// inner circle fills white ring when locked
CaptureButtonUiState.Enabled.Recording.LockedRecording -> captureButtonSize.dp

CaptureButtonUiState.Enabled.Recording.Pending,
CaptureButtonUiState.Enabled.Recording.PressedRecording ->
(captureButtonSize * pressedVideoCaptureScale).dp

Expand Down
5 changes: 5 additions & 0 deletions ...capture/src/main/java/com/google/jetpackcamera/ui/uistate/capture/CaptureButtonUiState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ sealed interface CaptureButtonUiState {
sealed interface Recording : Enabled {
override val isEnabled: Boolean get() = true

/**
* The video recording request has been sent and is pending initialization.
*/
data object Pending : Recording

@temcguir temcguir Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given we are mapping this directly from VideoRecordingState.Starting, could we rename Pending to Starting here (and throughout the PR)? It feels like it captures the actual state of the camera backend much more accurately than "Pending".

            /**
             * The video recording request has been sent and is starting.
             */
            data object Starting : Recording


/**
* The user is actively pressing the button to record video (press-and-hold).
*/
Expand Down
3 changes: 1 addition & 2 deletions ...in/java/com/google/jetpackcamera/ui/uistateadapter/capture/CaptureButtonUiStateAdapter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ fun CaptureButtonUiState.Companion.from(
}

is VideoRecordingState.Starting ->
CaptureButtonUiState
.Enabled.Idle(captureMode = cameraAppSettings.captureMode)
CaptureButtonUiState.Enabled.Recording.Pending
}
} else {
CaptureButtonUiState
Expand Down
5 changes: 3 additions & 2 deletions ...ava/com/google/jetpackcamera/ui/uistateadapter/capture/CaptureButtonUiStateAdapterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class CaptureButtonUiStateAdapterTest {
}

@Test
fun from_cameraRunning_recordingStarting_returnsIdleAndEnabled() {
fun from_cameraRunning_recordingStarting_returnsPendingRecording() {
val cameraState = defaultCameraState.copy(
videoRecordingState = VideoRecordingState.Starting(null)
)
Expand All @@ -105,7 +105,8 @@ class CaptureButtonUiStateAdapterTest {
lockedState = false
)

assertThat(uiState).isInstanceOf(CaptureButtonUiState.Enabled.Idle::class.java)
assertThat(uiState)
.isInstanceOf(CaptureButtonUiState.Enabled.Recording.Pending::class.java)
assertThat(uiState.isEnabled).isTrue()
}
}
Loading