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.

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

[Bug]: Trickle ICE option is not included in offer when using only Data Channels in v1.8.1 #1816

Closed
2 tasks done
H-Tomoki opened this issue Sep 26, 2023 · 4 comments
Closed
2 tasks done
Labels
bug Something isn't working needs-triage

Comments

@H-Tomoki
Copy link

H-Tomoki commented Sep 26, 2023

Please confirm you have already done the following

  • I have searched the repository for related/existing bug reports
  • I have all the details the issue requires

Describe the bug

I have applied the following patch to the v1.8.1 samples to disable the media channel and use only the data channel.
When I ran kvsWebRTCClientViewer with these changes, I noticed that the offer sent to the signaling channel did not include the trickle ICE option.
In contrast, when using the media channel, the trickle ICE option always seems to be included in the offer, regardless of the Viewer's settings.

Expected Behavior

The trickle ICE option is included in the offer.

Current Behavior

No trickle ICE option included in the offer.

Here is the offer that gets sent when using only the data channel.

v=0
o=- 287633163 2 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE 0
a=msid-semantic: WMS myKvsVideoStream
m=application 9 UDP/DTLS/SCTP webrtc-datachannel
c=IN IP4 127.0.0.1
a=candidate:0 1 udp 2130706431 **.**.**.** 49934 typ host raddr 0.0.0.0 rport 0 generation 0 network-cost 999
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:****
a=ice-pwd:****
a=fingerprint:sha-256 ****
a=setup:actpass
a=mid:0
a=sctp-port:5000

And here is what gets sent when using the media channel.

v=0
o=- 565977857 2 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE 0 1 2
a=msid-semantic: WMS myKvsVideoStream
m=audio 9 UDP/TLS/RTP/SAVPF 111
c=IN IP4 127.0.0.1
a=candidate:0 1 udp 2130706431 **.**.**.** 57648 typ host raddr 0.0.0.0 rport 0 generation 0 network-cost 999
a=msid:myKvsVideoStream myAudioTrack
…
a=ice-options:trickle
…
m=video 9 UDP/TLS/RTP/SAVPF 125
c=IN IP4 127.0.0.1
a=candidate:0 1 udp 2130706431 **.**.**.** 57648 typ host raddr 0.0.0.0 rport 0 generation 0 network-cost 999
a=msid:myKvsVideoStream myVideoTrack
…
a=ice-options:trickle
…
m=application 9 UDP/DTLS/SCTP webrtc-datachannel
c=IN IP4 127.0.0.1
a=candidate:0 1 udp 2130706431 **.**.**.** 57648 typ host raddr 0.0.0.0 rport 0 generation 0 network-cost 999
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:****
a=ice-pwd:****
a=fingerprint:sha-256 ****
a=setup:actpass
a=mid:2
a=sctp-port:5000

Reproduction Steps

The patch applied to the sample application is shown below.

diff --git a/samples/Common.c b/samples/Common.c
index 97898a13d..d82931123 100644
--- a/samples/Common.c
+++ b/samples/Common.c
@@ -520,29 +520,29 @@ STATUS createSampleStreamingSession(PSampleConfiguration pSampleConfiguration, P
     CHK_STATUS(addSupportedCodec(pSampleStreamingSession->pPeerConnection, RTC_CODEC_H264_PROFILE_42E01F_LEVEL_ASYMMETRY_ALLOWED_PACKETIZATION_MODE));
     CHK_STATUS(addSupportedCodec(pSampleStreamingSession->pPeerConnection, RTC_CODEC_OPUS));
 
-    // Add a SendRecv Transceiver of type video
-    videoTrack.kind = MEDIA_STREAM_TRACK_KIND_VIDEO;
-    videoTrack.codec = RTC_CODEC_H264_PROFILE_42E01F_LEVEL_ASYMMETRY_ALLOWED_PACKETIZATION_MODE;
-    videoRtpTransceiverInit.direction = RTC_RTP_TRANSCEIVER_DIRECTION_SENDRECV;
-    STRCPY(videoTrack.streamId, "myKvsVideoStream");
-    STRCPY(videoTrack.trackId, "myVideoTrack");
-    CHK_STATUS(addTransceiver(pSampleStreamingSession->pPeerConnection, &videoTrack, &videoRtpTransceiverInit,
-                              &pSampleStreamingSession->pVideoRtcRtpTransceiver));
-
-    CHK_STATUS(transceiverOnBandwidthEstimation(pSampleStreamingSession->pVideoRtcRtpTransceiver, (UINT64) pSampleStreamingSession,
-                                                sampleBandwidthEstimationHandler));
-
-    // Add a SendRecv Transceiver of type audio
-    audioTrack.kind = MEDIA_STREAM_TRACK_KIND_AUDIO;
-    audioTrack.codec = RTC_CODEC_OPUS;
-    audioRtpTransceiverInit.direction = RTC_RTP_TRANSCEIVER_DIRECTION_SENDRECV;
-    STRCPY(audioTrack.streamId, "myKvsVideoStream");
-    STRCPY(audioTrack.trackId, "myAudioTrack");
-    CHK_STATUS(addTransceiver(pSampleStreamingSession->pPeerConnection, &audioTrack, &audioRtpTransceiverInit,
-                              &pSampleStreamingSession->pAudioRtcRtpTransceiver));
-
-    CHK_STATUS(transceiverOnBandwidthEstimation(pSampleStreamingSession->pAudioRtcRtpTransceiver, (UINT64) pSampleStreamingSession,
-                                                sampleBandwidthEstimationHandler));
+    // // Add a SendRecv Transceiver of type video
+    // videoTrack.kind = MEDIA_STREAM_TRACK_KIND_VIDEO;
+    // videoTrack.codec = RTC_CODEC_H264_PROFILE_42E01F_LEVEL_ASYMMETRY_ALLOWED_PACKETIZATION_MODE;
+    // videoRtpTransceiverInit.direction = RTC_RTP_TRANSCEIVER_DIRECTION_SENDRECV;
+    // STRCPY(videoTrack.streamId, "myKvsVideoStream");
+    // STRCPY(videoTrack.trackId, "myVideoTrack");
+    // CHK_STATUS(addTransceiver(pSampleStreamingSession->pPeerConnection, &videoTrack, &videoRtpTransceiverInit,
+    //                           &pSampleStreamingSession->pVideoRtcRtpTransceiver));
+
+    // CHK_STATUS(transceiverOnBandwidthEstimation(pSampleStreamingSession->pVideoRtcRtpTransceiver, (UINT64) pSampleStreamingSession,
+    //                                             sampleBandwidthEstimationHandler));
+
+    // // Add a SendRecv Transceiver of type audio
+    // audioTrack.kind = MEDIA_STREAM_TRACK_KIND_AUDIO;
+    // audioTrack.codec = RTC_CODEC_OPUS;
+    // audioRtpTransceiverInit.direction = RTC_RTP_TRANSCEIVER_DIRECTION_SENDRECV;
+    // STRCPY(audioTrack.streamId, "myKvsVideoStream");
+    // STRCPY(audioTrack.trackId, "myAudioTrack");
+    // CHK_STATUS(addTransceiver(pSampleStreamingSession->pPeerConnection, &audioTrack, &audioRtpTransceiverInit,
+    //                           &pSampleStreamingSession->pAudioRtcRtpTransceiver));
+
+    // CHK_STATUS(transceiverOnBandwidthEstimation(pSampleStreamingSession->pAudioRtcRtpTransceiver, (UINT64) pSampleStreamingSession,
+    //                                             sampleBandwidthEstimationHandler));
     // twcc bandwidth estimation
     CHK_STATUS(peerConnectionOnSenderBandwidthEstimation(pSampleStreamingSession->pPeerConnection, (UINT64) pSampleStreamingSession,
                                                          sampleSenderBandwidthEstimationHandler));

diff --git a/samples/kvsWebRTCClientViewer.c b/samples/kvsWebRTCClientViewer.c
index 7ed9a3c4b..ffe2ca446 100644
--- a/samples/kvsWebRTCClientViewer.c
+++ b/samples/kvsWebRTCClientViewer.c
@@ -81,7 +81,7 @@ INT32 main(INT32 argc, CHAR* argv[])
     CHK_STATUS(setLocalDescription(pSampleStreamingSession->pPeerConnection, &offerSessionDescriptionInit));
     DLOGI("[KVS Viewer] Completed setting local description");
 
-    CHK_STATUS(transceiverOnFrame(pSampleStreamingSession->pAudioRtcRtpTransceiver, (UINT64) pSampleStreamingSession, sampleAudioFrameHandler));
+    // CHK_STATUS(transceiverOnFrame(pSampleStreamingSession->pAudioRtcRtpTransceiver, (UINT64) pSampleStreamingSession, sampleAudioFrameHandler));
 
     if (!pSampleConfiguration->trickleIce) {
         DLOGI("[KVS Viewer] Non trickle ice. Wait for Candidate collection to complete");

I believe that modifying the SDK like this will fix this issue.

diff --git a/src/source/PeerConnection/SessionDescription.c b/src/source/PeerConnection/SessionDescription.c
index a3fedc2cc..5c8a37dd5 100644
--- a/src/source/PeerConnection/SessionDescription.c
+++ b/src/source/PeerConnection/SessionDescription.c
@@ -679,6 +679,10 @@ STATUS populateSessionDescriptionDataChannel(PKvsPeerConnection pKvsPeerConnecti
     STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, pKvsPeerConnection->localIcePwd);
     attributeCount++;
 
+    // STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "ice-options");
+    // STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, "trickle");
+    // attributeCount++;
+
     STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "fingerprint");
     STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, "sha-256 ");
     STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue + 8, pCertificateFingerprint);

WebRTC C SDK version being used

v1.8.1

Compiler and Version used

gcc 9.4.0

Operating System and version

Ubuntu 20.04

Platform being used

Linux

@jdelapla
Copy link
Contributor

Hello, this is resolved in #1813

@H-Tomoki
Copy link
Author

@jdelapla
Sorry for the oversight and thank you for addressing this.
However, with this fix, the trickle ICE option is always added when using the media channel. In contrast, when using only the data channel, the inclusion of the trickle ICE option depends on the Viewer's trickle ICE settings. Is this the intended behavior?

@jdelapla
Copy link
Contributor

jdelapla commented Oct 9, 2023

Currently we're only going to support our viewer in trickle-ice scenarios as it's the standard for WebRTC today. If there is significant demand for C SDK viewer non-trickle ice we can add that functionality at a later time.

@H-Tomoki
Copy link
Author

H-Tomoki commented Oct 10, 2023

@jdelapla
I have no issue with that specification. What I'm concerned about is the difference in behavior when only using the media channel versus when only using the data channel.
If it's according to that specification, then it seems inappropriate to add the trickle-ice option only when the trickle ice flag is on at the time of adding the data channel, as shown in the code below.

src/source/PeerConnection/SessionDescription.c

// Populate a SessionDescription with the current state of the KvsPeerConnection
STATUS populateSessionDescription(PKvsPeerConnection pKvsPeerConnection, PSessionDescription pRemoteSessionDescription,
                                  PSessionDescription pLocalSessionDescription)
{
    ENTERS();
…

    if (pKvsPeerConnection->canTrickleIce.value) {
        STRCPY(pLocalSessionDescription->sdpAttributes[pLocalSessionDescription->sessionAttributesCount].attributeName, "ice-options");
        STRCPY(pLocalSessionDescription->sdpAttributes[pLocalSessionDescription->sessionAttributesCount].attributeValue, "trickle");
        pLocalSessionDescription->sessionAttributesCount++;
    }
…
}

When adding the media channel, it is added regardless of the flag, as shown in the code below.

// Populate a single media section from a PKvsRtpTransceiver
STATUS populateSingleMediaSection(PKvsPeerConnection pKvsPeerConnection, PKvsRtpTransceiver pKvsRtpTransceiver,
                                  PSdpMediaDescription pSdpMediaDescription, PSessionDescription pRemoteSessionDescription,
                                  PCHAR pCertificateFingerprint, UINT32 mediaSectionId, PCHAR pDtlsRole, PHashTable pUnknownCodecPayloadTypesTable,
                                  PHashTable pUnknownCodecRtpmapTable, UINT32 unknownCodecHashTableKey)
{
    ENTERS();
…
    STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "ice-options");
    STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, "trickle");
    attributeCount++;
…
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-triage
Projects
None yet
Development

No branches or pull requests

2 participants