From 717c83753f24ffbccabe17c23100268f9146040a Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Thu, 11 Nov 2021 21:02:15 +0100 Subject: [PATCH] delay discovered ice candidates until JingleRtpConnection is ready to receive otherwise setLocalDescription and the arrival of first candidates might race the rtpContentDescription being set --- .../xmpp/jingle/JingleRtpConnection.java | 39 +++++++++++++++++-- .../xmpp/jingle/WebRTCWrapper.java | 28 ++++++++++++- 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java index 37c51bfdc..4796d0c58 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java @@ -34,6 +34,7 @@ import java.util.List; import java.util.Map; import java.util.Queue; import java.util.Set; +import java.util.concurrent.ExecutionException; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -567,6 +568,7 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web final SessionDescription sessionDescription = SessionDescription.parse(webRTCSessionDescription.description); final RtpContentMap respondingRtpContentMap = RtpContentMap.of(sessionDescription); this.responderRtpContentMap = respondingRtpContentMap; + webRTCWrapper.setIsReadyToReceiveIceCandidates(true); final ListenableFuture outgoingContentMapFuture = prepareOutgoingContentMap(respondingRtpContentMap); Futures.addCallback(outgoingContentMapFuture, new FutureCallback() { @@ -872,6 +874,7 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web final SessionDescription sessionDescription = SessionDescription.parse(webRTCSessionDescription.description); final RtpContentMap rtpContentMap = RtpContentMap.of(sessionDescription); this.initiatorRtpContentMap = rtpContentMap; + this.webRTCWrapper.setIsReadyToReceiveIceCandidates(true); final ListenableFuture outgoingContentMapFuture = encryptSessionInitiate(rtpContentMap); Futures.addCallback(outgoingContentMapFuture, new FutureCallback() { @Override @@ -1339,7 +1342,7 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web @Override public void onConnectionChange(final PeerConnection.PeerConnectionState newState) { - Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": PeerConnectionState changed to" + newState); + Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": PeerConnectionState changed to " + newState); this.stateHistory.add(newState); if (newState == PeerConnection.PeerConnectionState.CONNECTED) { this.sessionDuration.start(); @@ -1348,7 +1351,10 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web } final boolean neverConnected = !this.stateHistory.contains(PeerConnection.PeerConnectionState.CONNECTED); - final boolean failedOrDisconnected = Arrays.asList(PeerConnection.PeerConnectionState.FAILED, PeerConnection.PeerConnectionState.DISCONNECTED).contains(newState); + final boolean failedOrDisconnected = Arrays.asList( + PeerConnection.PeerConnectionState.FAILED, + PeerConnection.PeerConnectionState.DISCONNECTED + ).contains(newState); if (neverConnected && failedOrDisconnected) { @@ -1356,7 +1362,7 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": not sending session-terminate after connectivity error because session is already in state " + this.state); return; } - new Thread(this::closeWebRTCSessionAfterFailedConnection).start(); + webRTCWrapper.execute(this::closeWebRTCSessionAfterFailedConnection); } else if (newState == PeerConnection.PeerConnectionState.FAILED) { Log.d(Config.LOGTAG, "attempting to restart ICE"); webRTCWrapper.restartIce(); @@ -1367,6 +1373,33 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web @Override public void onRenegotiationNeeded() { Log.d(Config.LOGTAG, "onRenegotiationNeeded()"); + this.webRTCWrapper.execute(this::renegotiate); + } + + private void renegotiate() { + this.webRTCWrapper.setIsReadyToReceiveIceCandidates(false); + try { + final SessionDescription sessionDescription = setLocalSessionDescription(); + final RtpContentMap rtpContentMap = RtpContentMap.of(sessionDescription); + setRenegotiatedContentMap(rtpContentMap); + this.webRTCWrapper.setIsReadyToReceiveIceCandidates(true); + } catch (final Exception e) { + Log.d(Config.LOGTAG, "failed to renegotiate", e); + //TODO send some sort of failure (comparable to when initiating) + } + } + + private void setRenegotiatedContentMap(final RtpContentMap rtpContentMap) { + if (isInitiator()) { + this.initiatorRtpContentMap = rtpContentMap; + } else { + this.responderRtpContentMap = rtpContentMap; + } + } + + private SessionDescription setLocalSessionDescription() throws ExecutionException, InterruptedException { + final org.webrtc.SessionDescription sessionDescription = this.webRTCWrapper.setLocalDescription().get(); + return SessionDescription.parse(sessionDescription.description); } private void closeWebRTCSessionAfterFailedConnection() { diff --git a/src/main/java/eu/siacs/conversations/xmpp/jingle/WebRTCWrapper.java b/src/main/java/eu/siacs/conversations/xmpp/jingle/WebRTCWrapper.java index 5ee6c5d5e..9ea4cd389 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/WebRTCWrapper.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/WebRTCWrapper.java @@ -45,8 +45,13 @@ import org.webrtc.voiceengine.WebRtcAudioEffects; import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedList; import java.util.List; +import java.util.Queue; import java.util.Set; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -59,6 +64,8 @@ public class WebRTCWrapper { private static final String EXTENDED_LOGGING_TAG = WebRTCWrapper.class.getSimpleName(); + private final ExecutorService executorService = Executors.newSingleThreadExecutor(); + //we should probably keep this in sync with: https://github.com/signalapp/Signal-Android/blob/master/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java#L296 private static final Set HARDWARE_AEC_BLACKLIST = new ImmutableSet.Builder() .add("Pixel") @@ -79,6 +86,8 @@ public class WebRTCWrapper { private static final int CAPTURING_MAX_FRAME_RATE = 30; private final EventCallback eventCallback; + private final AtomicBoolean readyToReceivedIceCandidates = new AtomicBoolean(false); + private final Queue iceCandidates = new LinkedList<>(); private final AppRTCAudioManager.AudioManagerEvents audioManagerEvents = new AppRTCAudioManager.AudioManagerEvents() { @Override public void onAudioDeviceChanged(AppRTCAudioManager.AudioDevice selectedAudioDevice, Set availableAudioDevices) { @@ -125,7 +134,11 @@ public class WebRTCWrapper { @Override public void onIceCandidate(IceCandidate iceCandidate) { - eventCallback.onIceCandidate(iceCandidate); + if (readyToReceivedIceCandidates.get()) { + eventCallback.onIceCandidate(iceCandidate); + } else { + iceCandidates.add(iceCandidate); + } } @Override @@ -294,7 +307,14 @@ public class WebRTCWrapper { } void restartIce() { - requirePeerConnection().restartIce(); + executorService.execute(()-> requirePeerConnection().restartIce()); + } + + public void setIsReadyToReceiveIceCandidates(final boolean ready) { + readyToReceivedIceCandidates.set(ready); + while(ready && iceCandidates.peek() != null) { + eventCallback.onIceCandidate(iceCandidates.poll()); + } } synchronized void close() { @@ -528,6 +548,10 @@ public class WebRTCWrapper { return appRTCAudioManager; } + void execute(final Runnable command) { + executorService.execute(command); + } + public interface EventCallback { void onIceCandidate(IceCandidate iceCandidate);