From 5c44b72ee20e313df47ae0ca4fe9e2f013f5b734 Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Tue, 28 Jul 2026 12:33:08 -0300 Subject: [PATCH] Fix issue with vanity interlacing frames. --- .../ActiveCallActionProcessorDelegate.java | 16 +- .../CallSetupActionProcessorDelegate.java | 5 + .../webrtc/IncomingCallActionProcessor.java | 17 +- .../service/webrtc/state/CallSetupState.kt | 1 + .../state/WebRtcServiceStateBuilder.java | 5 + .../webrtc/IncomingCallActionProcessorTest.kt | 180 ++++++++++++++++++ 6 files changed, 218 insertions(+), 6 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/service/webrtc/IncomingCallActionProcessorTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/ActiveCallActionProcessorDelegate.java b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/ActiveCallActionProcessorDelegate.java index 7027f3a3b3..81e8112004 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/ActiveCallActionProcessorDelegate.java +++ b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/ActiveCallActionProcessorDelegate.java @@ -182,7 +182,7 @@ public class ActiveCallActionProcessorDelegate extends WebRtcActionProcessor { RemotePeer activePeer = currentState.getCallInfoState().getActivePeer(); boolean remotePeerIsActive = remotePeer.callIdEquals(activePeer); boolean outgoingBeforeAccept = remotePeer.getState() == CallState.DIALING || remotePeer.getState() == CallState.REMOTE_RINGING; - boolean incomingBeforeAccept = remotePeer.getState() == CallState.ANSWERING || remotePeer.getState() == CallState.LOCAL_RINGING; + boolean incomingBeforeAccept = isIncomingBeforeAccept(currentState, remotePeer); if (remotePeerIsActive && ENDED_REMOTE_END_REASON_TO_STATE.containsKey(callEndReason)) { state = Objects.requireNonNull(ENDED_REMOTE_END_REASON_TO_STATE.get(callEndReason)); @@ -235,7 +235,7 @@ public class ActiveCallActionProcessorDelegate extends WebRtcActionProcessor { webRtcInteractor.postStateUpdate(currentState); } - if (remotePeer.getState() == CallState.ANSWERING || remotePeer.getState() == CallState.LOCAL_RINGING) { + if (isIncomingBeforeAccept(currentState, remotePeer)) { webRtcInteractor.insertMissedCall(remotePeer, remotePeer.getCallStartTimestamp(), currentState.getCallSetupState(remotePeer).isRemoteVideoOffer()); } @@ -266,7 +266,7 @@ public class ActiveCallActionProcessorDelegate extends WebRtcActionProcessor { webRtcInteractor.postStateUpdate(currentState); - if (activePeer.getState() == CallState.ANSWERING || activePeer.getState() == CallState.LOCAL_RINGING) { + if (isIncomingBeforeAccept(currentState, activePeer)) { webRtcInteractor.insertMissedCall(activePeer, activePeer.getCallStartTimestamp(), currentState.getCallSetupState(activePeer).isRemoteVideoOffer()); } @@ -280,4 +280,14 @@ public class ActiveCallActionProcessorDelegate extends WebRtcActionProcessor { return currentState; } + + /** + * Whether the given peer represents an incoming call that the user never picked up. The peer stays in + * {@link CallState#LOCAL_RINGING} from accept until the call connects, so the peer state alone cannot + * distinguish a missed call from one that was answered but ended before connecting. + */ + private static boolean isIncomingBeforeAccept(@NonNull WebRtcServiceState currentState, @NonNull RemotePeer remotePeer) { + boolean isIncoming = remotePeer.getState() == CallState.ANSWERING || remotePeer.getState() == CallState.LOCAL_RINGING; + return isIncoming && !currentState.getCallSetupState(remotePeer).isAccepted(); + } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/CallSetupActionProcessorDelegate.java b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/CallSetupActionProcessorDelegate.java index 2aa4a31f8d..f8ae68685b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/CallSetupActionProcessorDelegate.java +++ b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/CallSetupActionProcessorDelegate.java @@ -50,6 +50,11 @@ public class CallSetupActionProcessorDelegate extends WebRtcActionProcessor { activePeer.connected(); + OutgoingVideoSourceRouter router = currentState.getVideoState().getRouter(); + if (router != null) { + router.setVanitySink(null); + } + boolean localVideoEnabled = currentState.getLocalDeviceState().getCameraState().isEnabled(); boolean remoteVideoEnabled = currentState.getCallSetupState(activePeer).isRemoteVideoOffer(); webRtcInteractor.updatePhoneState(WebRtcUtil.getInCallPhoneState(context, localVideoEnabled, remoteVideoEnabled)); diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/IncomingCallActionProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/IncomingCallActionProcessor.java index 777fbe5de4..5bd6d11f24 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/IncomingCallActionProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/IncomingCallActionProcessor.java @@ -152,6 +152,7 @@ public class IncomingCallActionProcessor extends DeviceAwareActionProcessor { currentState = currentState.builder() .changeCallSetupState(activePeer.getCallId()) .acceptWithVideo(answerWithVideo) + .accepted(true) .build(); try { @@ -171,6 +172,11 @@ public class IncomingCallActionProcessor extends DeviceAwareActionProcessor { return currentState; } + if (currentState.getCallSetupState(activePeer).isAccepted()) { + Log.w(TAG, "Cannot deny after call has been accepted!"); + return currentState; + } + Log.i(TAG, "handleDenyCall():"); OutgoingVideoSourceRouter router = currentState.getVideoState().getRouter(); @@ -193,10 +199,15 @@ public class IncomingCallActionProcessor extends DeviceAwareActionProcessor { @Override protected @NonNull WebRtcServiceState handleSetIncomingRingingVanity(@NonNull WebRtcServiceState currentState, boolean enabled) { - RemotePeer activePeer = currentState.getCallInfoState().requireActivePeer(); - boolean isVideoOffer = currentState.getCallSetupState(activePeer).isRemoteVideoOffer(); + RemotePeer activePeer = currentState.getCallInfoState().requireActivePeer(); + CallSetupState callSetupState = currentState.getCallSetupState(activePeer); - if (!isVideoOffer) { + if (!callSetupState.isRemoteVideoOffer()) { + return currentState; + } + + if (callSetupState.isAccepted()) { + Log.w(TAG, "handleSetIncomingRingingVanity(): call has already been accepted, ignoring"); return currentState; } diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/state/CallSetupState.kt b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/state/CallSetupState.kt index 138666b823..647d3023f9 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/state/CallSetupState.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/state/CallSetupState.kt @@ -10,6 +10,7 @@ data class CallSetupState( var isEnableVideoOnCreate: Boolean = false, var isRemoteVideoOffer: Boolean = false, var isAcceptWithVideo: Boolean = false, + var isAccepted: Boolean = false, @get:JvmName("hasSentJoinedMessage") var sentJoinedMessage: Boolean = false, @get:JvmName("shouldRingGroup") var ringGroup: Boolean = true, var ringId: Long = NO_RING, diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/state/WebRtcServiceStateBuilder.java b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/state/WebRtcServiceStateBuilder.java index 0cbe328675..a6ae751c18 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/state/WebRtcServiceStateBuilder.java +++ b/app/src/main/java/org/thoughtcrime/securesms/service/webrtc/state/WebRtcServiceStateBuilder.java @@ -206,6 +206,11 @@ public class WebRtcServiceStateBuilder { return this; } + public @NonNull CallSetupStateBuilder accepted(boolean accepted) { + toBuild.setAccepted(accepted); + return this; + } + public @NonNull CallSetupStateBuilder sentJoinedMessage(boolean sentJoinedMessage) { toBuild.setSentJoinedMessage(sentJoinedMessage); return this; diff --git a/app/src/test/java/org/thoughtcrime/securesms/service/webrtc/IncomingCallActionProcessorTest.kt b/app/src/test/java/org/thoughtcrime/securesms/service/webrtc/IncomingCallActionProcessorTest.kt new file mode 100644 index 0000000000..1bd312d6ae --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/service/webrtc/IncomingCallActionProcessorTest.kt @@ -0,0 +1,180 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.service.webrtc + +import android.app.Application +import assertk.assertThat +import assertk.assertions.isNull +import assertk.assertions.isSameInstanceAs +import assertk.assertions.isTrue +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import org.junit.Before +import org.junit.BeforeClass +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.signal.core.util.logging.Log +import org.signal.ringrtc.CallId +import org.signal.ringrtc.CallManager +import org.thoughtcrime.securesms.components.webrtc.BroadcastVideoSink +import org.thoughtcrime.securesms.events.WebRtcViewModel +import org.thoughtcrime.securesms.recipients.RecipientId +import org.thoughtcrime.securesms.ringrtc.CameraState +import org.thoughtcrime.securesms.ringrtc.OutgoingVideoSourceRouter +import org.thoughtcrime.securesms.ringrtc.RemotePeer +import org.thoughtcrime.securesms.service.webrtc.state.WebRtcServiceState +import org.thoughtcrime.securesms.testutil.SystemOutLogger + +/** + * State transition tests for the incoming 1:1 call processor, focused on the + * accepted-but-not-yet-connected window: once a call is accepted, late vanity + * toggles and deny requests (which can arrive off stale CALL_INCOMING state) + * must be ignored. + */ +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, application = Application::class, instrumentedPackages = ["org.signal.ringrtc"]) +class IncomingCallActionProcessorTest { + + companion object { + private val CALL_ID = CallId(42L) + + @JvmStatic + @BeforeClass + fun setUpClass() { + Log.initialize(SystemOutLogger()) + } + } + + private val callManager: CallManager = mockk(relaxed = true) + private val webRtcInteractor: WebRtcInteractor = mockk(relaxed = true) + private val router: OutgoingVideoSourceRouter = mockk(relaxed = true) + private val localSink: BroadcastVideoSink = mockk() + + private val processor = IncomingCallActionProcessor(webRtcInteractor) + + @Before + fun setUp() { + every { webRtcInteractor.callManager } returns callManager + every { router.cameraState } returns CameraState(CameraState.Direction.FRONT, 2) + } + + @Test + fun `Given a ringing video call, when I handleAcceptCall, then I expect the call setup state to be accepted`() { + val state = incomingRingingCall() + + val result = processor.handleAcceptCall(state, true) + + assertThat(result.getCallSetupState(CALL_ID).isAccepted).isTrue() + verify { callManager.acceptCall(CALL_ID) } + } + + @Test + fun `Given a ringing unaccepted video call, when I enable vanity, then I expect the vanity camera to start`() { + val state = incomingRingingCall(accepted = false, cameraEnabled = false) + + processor.handleSetIncomingRingingVanity(state, true) + + verify { router.setVanitySink(localSink) } + verify { router.setEnabled(true) } + } + + @Test + fun `Given an accepted call, when I enable vanity, then I expect no change`() { + val state = incomingRingingCall(accepted = true, cameraEnabled = false) + + val result = processor.handleSetIncomingRingingVanity(state, true) + + assertThat(result).isSameInstanceAs(state) + verify(exactly = 0) { router.setVanitySink(any()) } + verify(exactly = 0) { router.setEnabled(any()) } + } + + @Test + fun `Given an accepted call with the camera on, when I disable vanity, then I expect the camera to stay on`() { + val state = incomingRingingCall(accepted = true, cameraEnabled = true) + + val result = processor.handleSetIncomingRingingVanity(state, false) + + assertThat(result).isSameInstanceAs(state) + verify(exactly = 0) { router.setEnabled(any()) } + } + + @Test + fun `Given an accepted call, when I handleDenyCall, then I expect the deny to be ignored`() { + val state = incomingRingingCall(accepted = true) + + val result = processor.handleDenyCall(state) + + assertThat(result).isSameInstanceAs(state) + verify(exactly = 0) { webRtcInteractor.sendNotAcceptedCallEventSyncMessage(any(), any(), any()) } + verify(exactly = 0) { webRtcInteractor.rejectIncomingCall(any()) } + verify(exactly = 0) { callManager.hangup() } + } + + @Test + fun `Given a ringing unaccepted call, when I handleDenyCall, then I expect the call to be rejected and terminated`() { + val state = incomingRingingCall(accepted = false) + val activePeer = state.callInfoState.requireActivePeer() + + val result = processor.handleDenyCall(state) + + verify { webRtcInteractor.sendNotAcceptedCallEventSyncMessage(activePeer, false, true) } + verify { webRtcInteractor.rejectIncomingCall(activePeer.id) } + verify { callManager.hangup() } + assertThat(result.callInfoState.activePeer).isNull() + } + + @Test + fun `Given an accepted call, when the remote hangs up before connecting, then I expect no missed call`() { + val state = incomingRingingCall(accepted = true) + val activePeer = state.callInfoState.requireActivePeer() + + processor.handleEndedRemote(state, CallManager.CallEndReason.REMOTE_HANGUP, activePeer) + + verify(exactly = 0) { webRtcInteractor.insertMissedCall(any(), any(), any()) } + } + + @Test + fun `Given an unaccepted ringing call, when the remote hangs up, then I expect a missed call`() { + val state = incomingRingingCall(accepted = false) + val activePeer = state.callInfoState.requireActivePeer() + + processor.handleEndedRemote(state, CallManager.CallEndReason.REMOTE_HANGUP, activePeer) + + verify { webRtcInteractor.insertMissedCall(activePeer, activePeer.callStartTimestamp, true) } + } + + private fun incomingRingingCall( + accepted: Boolean = false, + cameraEnabled: Boolean = false + ): WebRtcServiceState { + val peer = RemotePeer(RecipientId.from(1L), CALL_ID) + peer.answering() + peer.localRinging() + + return WebRtcServiceState(processor) + .builder() + .changeCallInfoState() + .callState(WebRtcViewModel.State.CALL_INCOMING) + .activePeer(peer) + .commit() + .changeCallSetupState(CALL_ID) + .isRemoteVideoOffer(true) + .accepted(accepted) + .commit() + .changeLocalDeviceState() + .cameraState(if (cameraEnabled) CameraState(CameraState.Direction.FRONT, 2) else CameraState.UNKNOWN) + .commit() + .changeVideoState() + .router(router) + .localSink(localSink) + .commit() + .build() + } +}