From c996442d751eb2b8167c5ddfcb6f89b6c3f8a8ff Mon Sep 17 00:00:00 2001 From: Divya Sampath Kumar Date: Mon, 18 Mar 2024 13:04:14 -0700 Subject: [PATCH 01/17] Ice memory reduction - enable ice stats flag (#1947) * Change params size * Use dyanmic allocation and flag for ice stats * Debug 1 * Revert "Debug 1" This reverts commit ad7d02f10b3bb153640b38b0f0722eef902a9434. * Revert "Use dyanmic allocation and flag for ice stats" This reverts commit bf9a2ee4613bb94c07bc6fcccbcb550f16097afa. * Working version * enable flag in samples * Add unit test * Fix bug * README for the flag * Update readme --- README.md | 41 +++ samples/Common.c | 40 +-- samples/Samples.h | 1 + .../kinesis/video/webrtcclient/Include.h | 6 +- .../kinesis/video/webrtcclient/Stats.h | 43 +-- src/source/Ice/IceAgent.c | 286 +++++++++++++----- src/source/Ice/IceAgent.h | 37 ++- src/source/Metrics/Metrics.c | 92 +++--- tst/MetricsApiTest.cpp | 34 ++- 9 files changed, 401 insertions(+), 179 deletions(-) diff --git a/README.md b/README.md index 7527802653..a6718f86b8 100644 --- a/README.md +++ b/README.md @@ -589,6 +589,47 @@ Let us look into when each of these could be changed: 3. `iceConnectionCheckTimeout`: It is useful to increase this timeout in unstable/slow network where the packet exchange takes time and hence the binding request/response. Essentially, increasing it will allow atleast one candidate pair to be tried for nomination by the other peer. 4. `iceConnectionCheckPollingInterval`: This value is set to a default of 50 ms per [spec](https://datatracker.ietf.org/doc/html/rfc8445#section-14.2). Changing this would change the frequency of connectivity checks and essentially, the ICE state machine transitions. Decreasing the value could help in faster connection establishment in a reliable high performant network setting with good system resources. Increasing the value could help in reducing the network load, however, the connection establishment could slow down. Unless there is a strong reasoning, it is **NOT** recommended to deviate from spec/default. +### Enable ICE agent stats + +The SDK calculates 4 different stats: +1. ICE server stats - stats for ICE servers the SDK is using +2. [Local candidate stats](https://www.w3.org/TR/webrtc-stats/#dom-rtcstatstype-local-candidate) - stats for the selected local candidate +3. [Remote candidate stats](https://www.w3.org/TR/webrtc-stats/#dom-rtcstatstype-remote-candidate) - stats for the selected remote candidate +4. [Candidate pair stats](https://www.w3.org/TR/webrtc-stats/#dom-rtcstatstype-candidate-pair) - stats for the selected candidate pair + +For more information on these stats, refer to [AWS Docs](https://docs.aws.amazon.com/kinesisvideostreams-webrtc-dg/latest/devguide/kvswebrtc-reference.html) + +The SDK disables generating these stats by default. In order to be enable the SDK to calculate these stats, the application needs to set the following field: +`configuration.kvsRtcConfiguration.enableIceStats = TRUE`. + +### Controlling RTP rolling buffer capacity + +The SDK maintains an RTP rolling buffer to hold the RTP packets. This is useful to respond to NACKs and even in case of JitterBuffer. The rolling buffer size is controlled by 3 parameters: +1. MTU: This is set to a default of 1200 bytes +2. Buffer duration: This is the amount of time of media that you would like the rolling buffer to accommodate before it is overwritten due to buffer overflow. By default, the SDK sets this to 1 second +3. Highest expected bitrate: This is the expected bitrate of the media in question. The typical bitrates could vary based on resolution and codec. By default, the SDK sets this to 5 mibps for video and 1 mibps for audio + +The rolling buffer capacity is calculated as follows: +``` +Capacity = Buffer duration * highest expected bitrate (in bps) / 8 / MTU + +With buffer duration = 1 second, Highest expected bitrate = 5 mibps and MTU 1200 bytes, capacity = 546 RTP packets +``` + +The rolling buffer size can be configured per transceiver through the following fields: +``` +RtcRtpTransceiverInit.rollingBufferDurationSec = pPeerConnection, &pSampleStreamingSession->peerConnectionMetrics)); CHK_STATUS(iceAgentGetMetrics(pSampleStreamingSession->pPeerConnection, &pSampleStreamingSession->iceMetrics)); - if (STATUS_FAILED(retStatus = logSelectedIceCandidatesInformation(pSampleStreamingSession))) { - DLOGW("Failed to get information about selected Ice candidates: 0x%08x", retStatus); + if (pSampleConfiguration->enableIceStats) { + CHK_LOG_ERR(logSelectedIceCandidatesInformation(pSampleStreamingSession)); } break; case RTC_PEER_CONNECTION_STATE_FAILED: @@ -114,21 +114,21 @@ STATUS logSelectedIceCandidatesInformation(PSampleStreamingSession pSampleStream CHK(pSampleStreamingSession != NULL, STATUS_NULL_ARG); rtcMetrics.requestedTypeOfStats = RTC_STATS_TYPE_LOCAL_CANDIDATE; CHK_STATUS(rtcPeerConnectionGetMetrics(pSampleStreamingSession->pPeerConnection, NULL, &rtcMetrics)); - DLOGD("Local Candidate IP Address: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.address); - DLOGD("Local Candidate type: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.candidateType); - DLOGD("Local Candidate port: %d", rtcMetrics.rtcStatsObject.localIceCandidateStats.port); - DLOGD("Local Candidate priority: %d", rtcMetrics.rtcStatsObject.localIceCandidateStats.priority); - DLOGD("Local Candidate transport protocol: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.protocol); - DLOGD("Local Candidate relay protocol: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.relayProtocol); - DLOGD("Local Candidate Ice server source: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.url); + DLOGI("Local Candidate IP Address: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.address); + DLOGI("Local Candidate type: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.candidateType); + DLOGI("Local Candidate port: %d", rtcMetrics.rtcStatsObject.localIceCandidateStats.port); + DLOGI("Local Candidate priority: %d", rtcMetrics.rtcStatsObject.localIceCandidateStats.priority); + DLOGI("Local Candidate transport protocol: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.protocol); + DLOGI("Local Candidate relay protocol: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.relayProtocol); + DLOGI("Local Candidate Ice server source: %s", rtcMetrics.rtcStatsObject.localIceCandidateStats.url); rtcMetrics.requestedTypeOfStats = RTC_STATS_TYPE_REMOTE_CANDIDATE; CHK_STATUS(rtcPeerConnectionGetMetrics(pSampleStreamingSession->pPeerConnection, NULL, &rtcMetrics)); - DLOGD("Remote Candidate IP Address: %s", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.address); - DLOGD("Remote Candidate type: %s", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.candidateType); - DLOGD("Remote Candidate port: %d", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.port); - DLOGD("Remote Candidate priority: %d", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.priority); - DLOGD("Remote Candidate transport protocol: %s", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.protocol); + DLOGI("Remote Candidate IP Address: %s", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.address); + DLOGI("Remote Candidate type: %s", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.candidateType); + DLOGI("Remote Candidate port: %d", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.port); + DLOGI("Remote Candidate priority: %d", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.priority); + DLOGI("Remote Candidate transport protocol: %s", rtcMetrics.rtcStatsObject.remoteIceCandidateStats.protocol); CleanUp: LEAVES(); return retStatus; @@ -365,6 +365,8 @@ STATUS initializePeerConnection(PSampleConfiguration pSampleConfiguration, PRtcP // Set the ICE mode explicitly configuration.iceTransportPolicy = ICE_TRANSPORT_POLICY_ALL; + configuration.kvsRtcConfiguration.enableIceStats = pSampleConfiguration->enableIceStats; + // Set the STUN server PCHAR pKinesisVideoStunUrlPostFix = KINESIS_VIDEO_STUN_URL_POSTFIX; // If region is in CN, add CN region uri postfix @@ -501,6 +503,9 @@ STATUS createSampleStreamingSession(PSampleConfiguration pSampleConfiguration, P pSampleStreamingSession->twccMetadata.updateLock = MUTEX_CREATE(TRUE); } + // Flag to enable SDK to calculate selected ice server, local, remote and candidate pair stats. + pSampleConfiguration->enableIceStats = FALSE; + CHK_STATUS(initializePeerConnection(pSampleConfiguration, &pSampleStreamingSession->pPeerConnection)); CHK_STATUS(peerConnectionOnIceCandidate(pSampleStreamingSession->pPeerConnection, (UINT64) pSampleStreamingSession, onIceCandidateHandler)); CHK_STATUS( @@ -1208,9 +1213,8 @@ STATUS freeSampleConfiguration(PSampleConfiguration* ppSampleConfiguration) } for (i = 0; i < pSampleConfiguration->streamingSessionCount; ++i) { - retStatus = gatherIceServerStats(pSampleConfiguration->sampleStreamingSessionList[i]); - if (STATUS_FAILED(retStatus)) { - DLOGW("Failed to ICE Server Stats for streaming session %d: %08x", i, retStatus); + if (pSampleConfiguration->enableIceStats) { + CHK_LOG_ERR(gatherIceServerStats(pSampleConfiguration->sampleStreamingSessionList[i])); } freeSampleStreamingSession(&pSampleConfiguration->sampleStreamingSessionList[i]); } @@ -1542,7 +1546,7 @@ STATUS signalingMessageReceived(UINT64 customData, PReceivedSignalingMessage pRe MUTEX_UNLOCK(pSampleConfiguration->sampleConfigurationObjLock); locked = FALSE; - if (startStats && + if (pSampleConfiguration->enableIceStats && startStats && STATUS_FAILED(retStatus = timerQueueAddTimer(pSampleConfiguration->timerQueueHandle, SAMPLE_STATS_DURATION, SAMPLE_STATS_DURATION, getIceCandidatePairStatsCallback, (UINT64) pSampleConfiguration, &pSampleConfiguration->iceCandidatePairStatsTimerId))) { diff --git a/samples/Samples.h b/samples/Samples.h index f99de6224d..ed3f614666 100644 --- a/samples/Samples.h +++ b/samples/Samples.h @@ -171,6 +171,7 @@ typedef struct { PCHAR rtspUri; UINT32 logLevel; BOOL enableTwcc; + BOOL enableIceStats; } SampleConfiguration, *PSampleConfiguration; typedef struct { diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h index 8e4a239001..88922c56ba 100644 --- a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h +++ b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h @@ -460,11 +460,6 @@ extern "C" { */ #define MAX_SIGNALING_ENDPOINT_URI_LEN 512 -/** - * Maximum allowed ICE URI length - */ -#define MAX_ICE_CONFIG_URI_LEN 256 - /** * Maximum allowed correlation ID length */ @@ -1204,6 +1199,7 @@ typedef struct { BOOL disableSenderSideBandwidthEstimation; //!< Disable TWCC feedback based sender bandwidth estimation, enabled by default. //!< You want to set this to TRUE if you are on a very stable connection and want to save 1.2MB of //!< memory + BOOL enableIceStats; //!< Enable ICE stats to be calculated } KvsRtcConfiguration, *PKvsRtcConfiguration; /** diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h index 72fffb67ba..ce03a869a8 100644 --- a/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h +++ b/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h @@ -24,6 +24,11 @@ extern "C" { */ #define MAX_CANDIDATE_ID_LENGTH 9U +/** + * Maximum allowed ICE URI length + */ +#define MAX_ICE_CONFIG_URI_LEN 256 + /** * Maximum allowed relay protocol length */ @@ -63,6 +68,11 @@ extern "C" { * Maximum allowed generic length used in DOMString */ #define MAX_STATS_STRING_LENGTH 255U + +/** + * Maximum length of candidate type (host, srflx, relay, prflx, unknown) + */ +#define MAX_CANDIDATE_TYPE_LENGTH 10U /*!@} */ /** @@ -233,14 +243,14 @@ typedef struct { * Reference: https://www.w3.org/TR/webrtc-stats/#ice-server-dict* */ typedef struct { - DOMString url; //!< STUN/TURN server URL - DOMString protocol; //!< Valid values: UDP, TCP - UINT32 iceServerIndex; //!< Ice server index to get stats from. Not available in spec! Needs to be - //!< populated by the application to get specific server stats - INT32 port; //!< Port number used by client - UINT64 totalRequestsSent; //!< Total amount of requests that have been sent to the server - UINT64 totalResponsesReceived; //!< Total number of responses received from the server - UINT64 totalRoundTripTime; //!< Sum of RTTs of all the requests for which response has been received + CHAR url[MAX_ICE_CONFIG_URI_LEN + 1]; //!< STUN/TURN server URL + CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP + UINT32 iceServerIndex; //!< Ice server index to get stats from. Not available in spec! Needs to be + //!< populated by the application to get specific server stats + INT32 port; //!< Port number used by client + UINT64 totalRequestsSent; //!< Total amount of requests that have been sent to the server + UINT64 totalResponsesReceived; //!< Total number of responses received from the server + UINT64 totalRoundTripTime; //!< Sum of RTTs of all the requests for which response has been received } RtcIceServerStats, *PRtcIceServerStats; /** @@ -267,15 +277,14 @@ typedef struct { */ typedef struct { - DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained - DOMString transportId; //!< Not used currently. ID of object that was inspected for RTCTransportStats - CHAR address[IP_ADDR_STR_LENGTH + 1]; //!< IPv4 or IPv6 address of the candidate - DOMString protocol; //!< Valid values: UDP, TCP - DOMString relayProtocol; //!< Protocol used by endpoint to communicate with TURN server. (Only for local candidate) - //!< Valid values: UDP, TCP, TLS - INT32 priority; //!< Computed using the formula in https://tools.ietf.org/html/rfc5245#section-15.1 - INT32 port; //!< Port number of the candidate - DOMString candidateType; //!< Type of local/remote ICE candidate + DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained + CHAR address[IP_ADDR_STR_LENGTH + 1]; //!< IPv4 or IPv6 address of the candidate + CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP + CHAR relayProtocol[MAX_PROTOCOL_LENGTH + 1]; //!< Protocol used by endpoint to communicate with TURN server. (Only for local candidate) + //!< Valid values: UDP, TCP, TLS + INT32 priority; //!< Computed using the formula in https://tools.ietf.org/html/rfc5245#section-15.1 + INT32 port; //!< Port number of the candidate + CHAR candidateType[MAX_CANDIDATE_TYPE_LENGTH + 1]; //!< Type of local/remote ICE candidate } RtcIceCandidateStats, *PRtcIceCandidateStats; /** diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index f9e78b8748..4388eaec55 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -40,7 +40,6 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge CHK(NULL != (pIceAgent = (PIceAgent) MEMCALLOC(1, SIZEOF(IceAgent))), STATUS_NOT_ENOUGH_MEMORY); STRNCPY(pIceAgent->localUsername, username, MAX_ICE_CONFIG_USER_NAME_LEN); STRNCPY(pIceAgent->localPassword, password, MAX_ICE_CONFIG_CREDENTIAL_LEN); - ATOMIC_STORE_BOOL(&pIceAgent->remoteCredentialReceived, FALSE); ATOMIC_STORE_BOOL(&pIceAgent->agentStartGathering, FALSE); ATOMIC_STORE_BOOL(&pIceAgent->stopGathering, FALSE); @@ -107,18 +106,22 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge (PCHAR) pRtcConfiguration->iceServers[i].username, (PCHAR) pRtcConfiguration->iceServers[i].credential), pIceAgent->iceAgentProfileDiagnostics.iceServerParsingTime[i], "ICE server parsing"); if (STATUS_SUCCEEDED(retStatus)) { - pIceAgent->rtcIceServerDiagnostics[i].port = (INT32) getInt16(pIceAgent->iceServers[i].ipAddress.port); - switch (pIceAgent->iceServers[pIceAgent->iceServersCount].transport) { - case KVS_SOCKET_PROTOCOL_UDP: - STRCPY(pIceAgent->rtcIceServerDiagnostics[i].protocol, ICE_TRANSPORT_TYPE_UDP); - break; - case KVS_SOCKET_PROTOCOL_TCP: - STRCPY(pIceAgent->rtcIceServerDiagnostics[i].protocol, ICE_TRANSPORT_TYPE_TCP); - break; - default: - MEMSET(pIceAgent->rtcIceServerDiagnostics[i].protocol, 0, SIZEOF(pIceAgent->rtcIceServerDiagnostics[i].protocol)); + if (pIceAgent->kvsRtcConfiguration.enableIceStats) { + CHK(NULL != (pIceAgent->pRtcIceServerDiagnostics[i] = (PRtcIceServerDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceServerDiagnostics))), + STATUS_NOT_ENOUGH_MEMORY); + pIceAgent->pRtcIceServerDiagnostics[i]->port = (INT32) getInt16(pIceAgent->iceServers[i].ipAddress.port); + switch (pIceAgent->iceServers[pIceAgent->iceServersCount].transport) { + case KVS_SOCKET_PROTOCOL_UDP: + STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, ICE_TRANSPORT_TYPE_UDP); + break; + case KVS_SOCKET_PROTOCOL_TCP: + STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, ICE_TRANSPORT_TYPE_TCP); + break; + default: + MEMSET(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, 0, SIZEOF(pIceAgent->pRtcIceServerDiagnostics[i]->protocol)); + } + STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->url, pRtcConfiguration->iceServers[i].urls); } - STRCPY(pIceAgent->rtcIceServerDiagnostics[i].url, pRtcConfiguration->iceServers[i].urls); pIceAgent->iceServersCount++; } else { DLOGE("Failed to parse ICE servers"); @@ -126,6 +129,16 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge } } + if (pIceAgent->kvsRtcConfiguration.enableIceStats) { + CHK(NULL != + (pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics = + (PRtcIceCandidateDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidateDiagnostics))), + STATUS_NOT_ENOUGH_MEMORY); + CHK(NULL != + (pIceAgent->pRtcSelectedLocalIceCandidateDiagnostics = + (PRtcIceCandidateDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidateDiagnostics))), + STATUS_NOT_ENOUGH_MEMORY); + } CleanUp: if (STATUS_FAILED(retStatus) && pIceAgent != NULL) { @@ -154,6 +167,7 @@ STATUS freeIceAgent(PIceAgent* ppIceAgent) PIceAgent pIceAgent = NULL; PDoubleListNode pCurNode = NULL; UINT64 data; + UINT32 i; PIceCandidatePair pIceCandidatePair = NULL; PIceCandidate pIceCandidate = NULL; @@ -163,8 +177,6 @@ STATUS freeIceAgent(PIceAgent* ppIceAgent) pIceAgent = *ppIceAgent; - hashTableFree(pIceAgent->requestTimestampDiagnostics); - if (pIceAgent->localCandidates != NULL) { CHK_STATUS(doubleListGetHeadNode(pIceAgent->localCandidates, &pCurNode)); while (pCurNode != NULL) { @@ -254,6 +266,12 @@ STATUS freeIceAgent(PIceAgent* ppIceAgent) freeTransactionIdStore(&pIceAgent->pStunBindingRequestTransactionIdStore); } + hashTableFree(pIceAgent->requestTimestampDiagnostics); + SAFE_MEMFREE(pIceAgent->pRtcSelectedLocalIceCandidateDiagnostics); + SAFE_MEMFREE(pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics); + for (i = 0; i < MAX_ICE_SERVERS_COUNT; i++) { + SAFE_MEMFREE(pIceAgent->pRtcIceServerDiagnostics[i]); + } MEMFREE(pIceAgent); *ppIceAgent = NULL; @@ -264,6 +282,83 @@ STATUS freeIceAgent(PIceAgent* ppIceAgent) return retStatus; } +STATUS iceAgentAddConfig(PIceAgent pIceAgent, PIceConfigInfo pIceConfigInfo) +{ + STATUS retStatus = STATUS_SUCCESS; + UINT32 i = 0; + // used in PROFILE macro + UINT64 startTimeInMacro = 0; + BOOL locked = FALSE; + + CHK(pIceAgent != NULL && pIceConfigInfo != NULL, STATUS_NULL_ARG); + + for (i = 0; i < pIceConfigInfo->uriCount; i++) { + MUTEX_LOCK(pIceAgent->lock); + locked = TRUE; + PROFILE_CALL_WITH_T_OBJ(retStatus = parseIceServer(&pIceAgent->iceServers[pIceAgent->iceServersCount], (PCHAR) pIceConfigInfo->uris[i], + (PCHAR) pIceConfigInfo->userName, (PCHAR) pIceConfigInfo->password), + pIceAgent->iceAgentProfileDiagnostics.iceServerParsingTime[i], "ICE server parsing"); + MUTEX_UNLOCK(pIceAgent->lock); + locked = FALSE; + + if (STATUS_SUCCEEDED(retStatus)) { + MUTEX_LOCK(pIceAgent->lock); + locked = TRUE; + if (pIceAgent->pRtcIceServerDiagnostics[i] != NULL) { + pIceAgent->pRtcIceServerDiagnostics[i]->port = (INT32) getInt16(pIceAgent->iceServers[i].ipAddress.port); + switch (pIceAgent->iceServers[pIceAgent->iceServersCount].transport) { + case KVS_SOCKET_PROTOCOL_UDP: + STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, ICE_TRANSPORT_TYPE_UDP); + break; + case KVS_SOCKET_PROTOCOL_TCP: + STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, ICE_TRANSPORT_TYPE_TCP); + break; + default: + MEMSET(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, 0, SIZEOF(pIceAgent->pRtcIceServerDiagnostics[i]->protocol)); + } + STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->url, pIceConfigInfo->uris[i]); + } + MUTEX_UNLOCK(pIceAgent->lock); + locked = FALSE; + + // important to unlock iceAgent lock before calling init relay candidate, since iceAgent APIs are thread safe + // if you don't unlock this can lead to a deadlock with the timerqueue. + // init candidate && pairs + if (pIceAgent->iceServers[pIceAgent->iceServersCount].isTurn) { + if (pIceAgent->iceServers[pIceAgent->iceServersCount].transport == KVS_SOCKET_PROTOCOL_UDP || + pIceAgent->iceServers[pIceAgent->iceServersCount].transport == KVS_SOCKET_PROTOCOL_NONE) { + CHK_STATUS(iceAgentInitRelayCandidate(pIceAgent, pIceAgent->iceServersCount, KVS_SOCKET_PROTOCOL_UDP)); + } + + if (pIceAgent->iceServers[pIceAgent->iceServersCount].transport == KVS_SOCKET_PROTOCOL_TCP || + pIceAgent->iceServers[pIceAgent->iceServersCount].transport == KVS_SOCKET_PROTOCOL_NONE) { + CHK_STATUS(iceAgentInitRelayCandidate(pIceAgent, pIceAgent->iceServersCount, KVS_SOCKET_PROTOCOL_TCP)); + } + } + + MUTEX_LOCK(pIceAgent->lock); + locked = TRUE; + + pIceAgent->iceServersCount++; + + MUTEX_UNLOCK(pIceAgent->lock); + locked = FALSE; + + } else { + DLOGE("Failed to parse ICE servers"); + } + } + +CleanUp: + CHK_LOG_ERR(retStatus); + + if (locked) { + MUTEX_UNLOCK(pIceAgent->lock); + } + + return retStatus; +} + STATUS iceAgentValidateKvsRtcConfig(PKvsRtcConfiguration pKvsRtcConfiguration) { STATUS retStatus = STATUS_SUCCESS; @@ -696,13 +791,15 @@ STATUS iceAgentSendPacket(PIceAgent pIceAgent, PBYTE pBuffer, UINT32 bufferLen) CleanUp: if (STATUS_SUCCEEDED(retStatus) && pIceAgent->pDataSendingIceCandidatePair != NULL) { - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.packetsDiscardedOnSend += packetsDiscarded; - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.bytesDiscardedOnSend += bytesDiscarded; - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.state = pIceAgent->pDataSendingIceCandidatePair->state; - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.lastPacketSentTimestamp = - pIceAgent->pDataSendingIceCandidatePair->lastDataSentTime; - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.bytesSent += bytesSent; - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.packetsSent += packetsSent; + if (pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->packetsDiscardedOnSend += packetsDiscarded; + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->bytesDiscardedOnSend += bytesDiscarded; + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->state = pIceAgent->pDataSendingIceCandidatePair->state; + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->lastPacketSentTimestamp = + pIceAgent->pDataSendingIceCandidatePair->lastDataSentTime; + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->bytesSent += bytesSent; + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->packetsSent += packetsSent; + } } if (locked) { @@ -1075,6 +1172,12 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, if (pCurrentIceCandidate->state == ICE_CANDIDATE_STATE_VALID && pCurrentIceCandidate->ipAddress.family == pIceCandidate->ipAddress.family) { pIceCandidatePair = (PIceCandidatePair) MEMCALLOC(1, SIZEOF(IceCandidatePair)); CHK(pIceCandidatePair != NULL, STATUS_NOT_ENOUGH_MEMORY); + if (pIceAgent->kvsRtcConfiguration.enableIceStats) { + CHK(NULL != + (pIceCandidatePair->pRtcIceCandidatePairDiagnostics = + (PRtcIceCandidatePairDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidatePairDiagnostics))), + STATUS_NOT_ENOUGH_MEMORY); + } if (isRemoteCandidate) { // Since we pick local candidate list @@ -1094,19 +1197,23 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, CHK_STATUS(hashTableCreateWithParams(ICE_HASH_TABLE_BUCKET_COUNT, ICE_HASH_TABLE_BUCKET_LENGTH, &pIceCandidatePair->requestSentTime)); pIceCandidatePair->lastDataSentTime = 0; - STRNCPY(pIceCandidatePair->rtcIceCandidatePairDiagnostics.localCandidateId, pIceCandidatePair->local->id, - ARRAY_SIZE(pIceCandidatePair->rtcIceCandidatePairDiagnostics.localCandidateId)); - STRNCPY(pIceCandidatePair->rtcIceCandidatePairDiagnostics.remoteCandidateId, pIceCandidatePair->remote->id, - ARRAY_SIZE(pIceCandidatePair->rtcIceCandidatePairDiagnostics.remoteCandidateId)); - pIceCandidatePair->rtcIceCandidatePairDiagnostics.state = pIceCandidatePair->state; - pIceCandidatePair->rtcIceCandidatePairDiagnostics.nominated = pIceCandidatePair->nominated; - pIceCandidatePair->rtcIceCandidatePairDiagnostics.lastPacketSentTimestamp = pIceCandidatePair->lastDataSentTime; pIceCandidatePair->firstStunRequest = TRUE; pIceCandidatePair->priority = computeCandidatePairPriority(pIceCandidatePair, pIceAgent->isControlling); - pIceCandidatePair->rtcIceCandidatePairDiagnostics.totalRoundTripTime = 0.0; - pIceCandidatePair->rtcIceCandidatePairDiagnostics.currentRoundTripTime = 0.0; - // Set data sending ICE candidate pair stats - NULLABLE_SET_EMPTY(pIceCandidatePair->rtcIceCandidatePairDiagnostics.circuitBreakerTriggerCount); + + if (pIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + STRNCPY(pIceCandidatePair->pRtcIceCandidatePairDiagnostics->localCandidateId, pIceCandidatePair->local->id, + ARRAY_SIZE(pIceCandidatePair->pRtcIceCandidatePairDiagnostics->localCandidateId)); + STRNCPY(pIceCandidatePair->pRtcIceCandidatePairDiagnostics->remoteCandidateId, pIceCandidatePair->remote->id, + ARRAY_SIZE(pIceCandidatePair->pRtcIceCandidatePairDiagnostics->remoteCandidateId)); + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->state = pIceCandidatePair->state; + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->nominated = pIceCandidatePair->nominated; + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->lastPacketSentTimestamp = pIceCandidatePair->lastDataSentTime; + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->totalRoundTripTime = 0.0; + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->currentRoundTripTime = 0.0; + // Set data sending ICE candidate pair stats + NULLABLE_SET_EMPTY(pIceCandidatePair->pRtcIceCandidatePairDiagnostics->circuitBreakerTriggerCount); + } + CHK_STATUS(insertIceCandidatePair(pIceAgent->iceCandidatePairs, pIceCandidatePair)); freeObjOnFailure = FALSE; } @@ -1137,6 +1244,7 @@ STATUS freeIceCandidatePair(PIceCandidatePair* ppIceCandidatePair) CHK_LOG_ERR(freeTransactionIdStore(&pIceCandidatePair->pTransactionIdStore)); CHK_LOG_ERR(hashTableFree(pIceCandidatePair->requestSentTime)); + SAFE_MEMFREE(pIceCandidatePair->pRtcIceCandidatePairDiagnostics); SAFE_MEMFREE(pIceCandidatePair); CleanUp: @@ -1280,16 +1388,19 @@ STATUS iceCandidatePairCheckConnection(PStunPacket pStunBindingRequest, PIceAgen CHK_STATUS(hashTableUpsert(pIceCandidatePair->requestSentTime, checkSum, GETTIME())); CHK_STATUS(hashTableUpsert(pIceAgent->requestTimestampDiagnostics, checkSum, GETTIME())); - if (pIceCandidatePair->local->iceCandidateType == ICE_CANDIDATE_TYPE_RELAYED) { - pIceAgent->rtcIceServerDiagnostics[pIceCandidatePair->local->iceServerIndex].totalRequestsSent++; + if (pIceCandidatePair->local->iceCandidateType == ICE_CANDIDATE_TYPE_RELAYED && + pIceAgent->pRtcIceServerDiagnostics[pIceCandidatePair->local->iceServerIndex] != NULL) { + pIceAgent->pRtcIceServerDiagnostics[pIceCandidatePair->local->iceServerIndex]->totalRequestsSent++; } CHK_STATUS(iceAgentSendStunPacket(pStunBindingRequest, (PBYTE) pIceAgent->remotePassword, (UINT32) STRLEN(pIceAgent->remotePassword) * SIZEOF(CHAR), pIceAgent, pIceCandidatePair->local, &pIceCandidatePair->remote->ipAddress)); - pIceCandidatePair->rtcIceCandidatePairDiagnostics.lastRequestTimestamp = GETTIME(); - pIceCandidatePair->rtcIceCandidatePairDiagnostics.requestsSent++; + if (pIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->lastRequestTimestamp = GETTIME(); + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->requestsSent++; + } CleanUp: CHK_LOG_ERR(retStatus); @@ -1334,7 +1445,9 @@ STATUS iceAgentSendStunPacket(PStunPacket pStunPacket, PBYTE password, UINT32 pa &pIceCandidatePair)); if (pIceCandidatePair != NULL && pIceCandidatePair == pIceAgent->pDataSendingIceCandidatePair && pIceAgent->pDataSendingIceCandidatePair->firstStunRequest) { - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.firstRequestTimestamp = GETTIME(); + if (pIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->firstRequestTimestamp = GETTIME(); + } pIceAgent->pDataSendingIceCandidatePair->firstStunRequest = FALSE; } } @@ -1414,8 +1527,10 @@ STATUS iceAgentSendSrflxCandidateRequest(PIceAgent pIceAgent) transactionIdStoreInsert(pIceAgent->pStunBindingRequestTransactionIdStore, pBindingRequest->header.transactionId); checkSum = COMPUTE_CRC32(pBindingRequest->header.transactionId, ARRAY_SIZE(pBindingRequest->header.transactionId)); CHK_STATUS(iceAgentSendStunPacket(pBindingRequest, NULL, 0, pIceAgent, pCandidate, &pIceServer->ipAddress)); - pIceAgent->rtcIceServerDiagnostics[pCandidate->iceServerIndex].totalRequestsSent++; - CHK_STATUS(hashTableUpsert(pIceAgent->requestTimestampDiagnostics, checkSum, GETTIME())); + if (pIceAgent->pRtcIceServerDiagnostics[pCandidate->iceServerIndex] != NULL) { + pIceAgent->pRtcIceServerDiagnostics[pCandidate->iceServerIndex]->totalRequestsSent++; + CHK_STATUS(hashTableUpsert(pIceAgent->requestTimestampDiagnostics, checkSum, GETTIME())); + } } break; @@ -1966,10 +2081,11 @@ STATUS updateCandidateStats(PIceAgent pIceAgent, BOOL isRemote) STATUS retStatus = STATUS_SUCCESS; CHK(pIceAgent != NULL && pIceAgent->pDataSendingIceCandidatePair != NULL, STATUS_NULL_ARG); PIceCandidate pIceCandidate = pIceAgent->pDataSendingIceCandidatePair->remote; - PRtcIceCandidateDiagnostics pRtcIceCandidateDiagnostics = &pIceAgent->rtcSelectedRemoteIceCandidateDiagnostics; + PRtcIceCandidateDiagnostics pRtcIceCandidateDiagnostics = pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics; if (!isRemote) { pIceCandidate = pIceAgent->pDataSendingIceCandidatePair->local; - pRtcIceCandidateDiagnostics = &pIceAgent->rtcSelectedLocalIceCandidateDiagnostics; + pRtcIceCandidateDiagnostics = pIceAgent->pRtcSelectedLocalIceCandidateDiagnostics; + CHK(pRtcIceCandidateDiagnostics != NULL, STATUS_SUCCESS); STRNCPY(pRtcIceCandidateDiagnostics->url, STATS_NOT_APPLICABLE_STR, ARRAY_SIZE(pRtcIceCandidateDiagnostics->url)); // URL and relay protocol are populated only for local candidate by spec. // If candidate type is host, there is no URL and is set to N/A @@ -1988,13 +2104,14 @@ STATUS updateCandidateStats(PIceAgent pIceAgent, BOOL isRemote) break; case KVS_SOCKET_PROTOCOL_TCP: STRNCPY(pRtcIceCandidateDiagnostics->relayProtocol, ICE_TRANSPORT_TYPE_TCP, - ARRAY_SIZE(pIceAgent->rtcSelectedLocalIceCandidateDiagnostics.relayProtocol)); + ARRAY_SIZE(pIceAgent->pRtcSelectedLocalIceCandidateDiagnostics->relayProtocol)); break; default: MEMSET(pRtcIceCandidateDiagnostics->relayProtocol, 0, SIZEOF(pRtcIceCandidateDiagnostics->relayProtocol)); } } } + CHK(pRtcIceCandidateDiagnostics != NULL, STATUS_SUCCESS); getIpAddrStr(&pIceCandidate->ipAddress, pRtcIceCandidateDiagnostics->address, ARRAY_SIZE(pRtcIceCandidateDiagnostics->address)); pRtcIceCandidateDiagnostics->port = (UINT16) getInt16(pIceCandidate->ipAddress.port); pRtcIceCandidateDiagnostics->priority = pIceCandidate->priority; @@ -2002,8 +2119,8 @@ STATUS updateCandidateStats(PIceAgent pIceAgent, BOOL isRemote) ARRAY_SIZE(pRtcIceCandidateDiagnostics->candidateType)); STRNCPY(pRtcIceCandidateDiagnostics->protocol, ICE_TRANSPORT_TYPE_UDP, ARRAY_SIZE(pRtcIceCandidateDiagnostics->protocol)); - if (pIceCandidate->iceCandidateType == ICE_CANDIDATE_TYPE_RELAYED) { - STRNCPY(pRtcIceCandidateDiagnostics->protocol, pIceAgent->rtcIceServerDiagnostics[pIceCandidate->iceServerIndex].protocol, + if (pIceCandidate->iceCandidateType == ICE_CANDIDATE_TYPE_RELAYED && pIceAgent->pRtcIceServerDiagnostics[pIceCandidate->iceServerIndex] != NULL) { + STRNCPY(pRtcIceCandidateDiagnostics->protocol, pIceAgent->pRtcIceServerDiagnostics[pIceCandidate->iceServerIndex]->protocol, ARRAY_SIZE(pRtcIceCandidateDiagnostics->protocol)); } CleanUp: @@ -2068,10 +2185,6 @@ STATUS iceAgentConnectedStateSetup(PIceAgent pIceAgent) if (pIceCandidatePair->state == ICE_CANDIDATE_PAIR_STATE_SUCCEEDED && pIceCandidatePair->nominated) { pIceAgent->pDataSendingIceCandidatePair = pIceCandidatePair; - retStatus = updateSelectedLocalRemoteCandidateStats(pIceAgent); - if (STATUS_FAILED(retStatus)) { - DLOGW("Failed to update candidate stats with status code 0x%08x", retStatus); - } break; } } @@ -2165,7 +2278,9 @@ STATUS iceAgentReadyStateSetup(PIceAgent pIceAgent) pCurNode = pCurNode->pNext; if (pIceCandidatePair->nominated && pIceCandidatePair->state == ICE_CANDIDATE_PAIR_STATE_SUCCEEDED) { pNominatedAndValidCandidatePair = pIceCandidatePair; - pNominatedAndValidCandidatePair->rtcIceCandidatePairDiagnostics.nominated = pIceCandidatePair->nominated; + if (pNominatedAndValidCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pNominatedAndValidCandidatePair->pRtcIceCandidatePairDiagnostics->nominated = pIceCandidatePair->nominated; + } break; } } @@ -2183,6 +2298,9 @@ STATUS iceAgentReadyStateSetup(PIceAgent pIceAgent) iceAgentGetCandidateTypeStr(pIceAgent->pDataSendingIceCandidatePair->remote->iceCandidateType), pIceAgent->pDataSendingIceCandidatePair->roundTripTime / HUNDREDS_OF_NANOS_IN_A_MILLISECOND, pIceAgent->pDataSendingIceCandidatePair->local->priority, pIceAgent->pDataSendingIceCandidatePair->priority); + + CHK_LOG_ERR(updateSelectedLocalRemoteCandidateStats(pIceAgent)); + /* no state timeout for ready state */ pIceAgent->stateEndTime = INVALID_TIMESTAMP_VALUE; @@ -2372,10 +2490,12 @@ STATUS incomingDataHandler(UINT64 customData, PSocketConnection pSocketConnectio pIceAgent->pDataSendingIceCandidatePair->remote->ipAddress.family == pSrc->family && MEMCMP(pIceAgent->pDataSendingIceCandidatePair->remote->ipAddress.address, pSrc->address, addrLen) == 0 && (pIceAgent->pDataSendingIceCandidatePair->remote->ipAddress.port == pSrc->port)) { - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.lastPacketReceivedTimestamp = GETTIME(); - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.bytesReceived += bufferLen; - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics - .packetsReceived++; // Since every byte buffer translates to a single RTP packet + if (pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->lastPacketReceivedTimestamp = GETTIME(); + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->bytesReceived += bufferLen; + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics + ->packetsReceived++; // Since every byte buffer translates to a single RTP packet + } } } else { if (ATOMIC_LOAD_BOOL(&pIceAgent->processStun)) { @@ -2491,7 +2611,7 @@ STATUS handleStunPacket(PIceAgent pIceAgent, PBYTE pBuffer, UINT32 bufferLen, PS if (!pIceCandidatePair->nominated) { CHK_STATUS(getStunAttribute(pStunPacket, STUN_ATTRIBUTE_TYPE_USE_CANDIDATE, &pStunAttr)); if (pStunAttr != NULL) { - DLOGD("received candidate with USE_CANDIDATE flag, local candidate type %s(%s:%s).", + DLOGI("received candidate with USE_CANDIDATE flag, local candidate type %s(%s:%s).", iceAgentGetCandidateTypeStr(pIceCandidatePair->local->iceCandidateType), pIceCandidatePair->local->id, pIceCandidatePair->remote->id); pIceCandidatePair->nominated = TRUE; @@ -2505,9 +2625,11 @@ STATUS handleStunPacket(PIceAgent pIceAgent, PBYTE pBuffer, UINT32 bufferLen, PS } if (pIceCandidatePair == pIceAgent->pDataSendingIceCandidatePair) { - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.requestsReceived += connectivityCheckRequestsReceived; - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.responsesSent += connectivityCheckResponsesSent; - pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics.nominated = pIceCandidatePair->nominated; + if (pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->requestsReceived += connectivityCheckRequestsReceived; + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->responsesSent += connectivityCheckResponsesSent; + pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics->nominated = pIceCandidatePair->nominated; + } } else { DLOGD("going to change the data sending ice candidate pair."); } @@ -2522,14 +2644,16 @@ STATUS handleStunPacket(PIceAgent pIceAgent, PBYTE pBuffer, UINT32 bufferLen, PS CHK_WARN(pIceCandidate != NULL, retStatus, "Local candidate with socket %d not found. Dropping STUN binding success response", pSocketConnection->localSocket); - // Update round trip time for serial reflexive candidate - pIceAgent->rtcIceServerDiagnostics[pIceCandidate->iceServerIndex].totalResponsesReceived++; - // Transaction ID count be same for candidates coming from same interface, which means there would only - // be one entry. It is not necessary to update a return sttaus since it is not indicative of a failure - if ((hashTableGet(pIceAgent->requestTimestampDiagnostics, checkSum, &requestSentTime)) == STATUS_SUCCESS) { - pIceAgent->rtcIceServerDiagnostics[pIceCandidate->iceServerIndex].totalRoundTripTime += GETTIME() - requestSentTime; - CHK_STATUS(hashTableRemove(pIceAgent->requestTimestampDiagnostics, checkSum)); - hashTableGetCount(pIceAgent->requestTimestampDiagnostics, &count); + if (pIceAgent->pRtcIceServerDiagnostics[pIceCandidate->iceServerIndex] != NULL) { + // Update round trip time for server reflexive candidate + pIceAgent->pRtcIceServerDiagnostics[pIceCandidate->iceServerIndex]->totalResponsesReceived++; + // Transaction ID count be same for candidates coming from same interface, which means there would only + // be one entry. It is not necessary to update a return sttaus since it is not indicative of a failure + if ((hashTableGet(pIceAgent->requestTimestampDiagnostics, checkSum, &requestSentTime)) == STATUS_SUCCESS) { + pIceAgent->pRtcIceServerDiagnostics[pIceCandidate->iceServerIndex]->totalRoundTripTime += GETTIME() - requestSentTime; + CHK_STATUS(hashTableRemove(pIceAgent->requestTimestampDiagnostics, checkSum)); + hashTableGetCount(pIceAgent->requestTimestampDiagnostics, &count); + } } CHK_STATUS(deserializeStunPacket(pBuffer, bufferLen, NULL, 0, &pStunPacket)); @@ -2557,8 +2681,10 @@ STATUS handleStunPacket(PIceAgent pIceAgent, PBYTE pBuffer, UINT32 bufferLen, PS DLOGD("Pair binding response! %s %s", pIceCandidatePair->local->id, pIceCandidatePair->remote->id); if (hashTableGet(pIceCandidatePair->requestSentTime, checkSum, &requestSentTime) == STATUS_SUCCESS) { pIceCandidatePair->roundTripTime = GETTIME() - requestSentTime; - pIceCandidatePair->rtcIceCandidatePairDiagnostics.currentRoundTripTime = - (DOUBLE) (pIceCandidatePair->roundTripTime) / HUNDREDS_OF_NANOS_IN_A_SECOND; + if (pIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->currentRoundTripTime = + (DOUBLE) (pIceCandidatePair->roundTripTime) / HUNDREDS_OF_NANOS_IN_A_SECOND; + } } else { DLOGW("Unable to fetch request Timestamp from the hash table. No update to RTT for the pair (error code: 0x%08x)", retStatus); } @@ -2567,11 +2693,14 @@ STATUS handleStunPacket(PIceAgent pIceAgent, PBYTE pBuffer, UINT32 bufferLen, PS // Update round trip time and responses received only for relay candidates. if (pIceCandidatePair->local->iceCandidateType == ICE_CANDIDATE_TYPE_RELAYED) { - pIceAgent->rtcIceServerDiagnostics[pIceCandidatePair->local->iceServerIndex].totalResponsesReceived++; - retStatus = hashTableGet(pIceAgent->requestTimestampDiagnostics, checkSum, &requestSentTime); - if (hashTableGet(pIceAgent->requestTimestampDiagnostics, checkSum, &requestSentTime) == STATUS_SUCCESS) { - pIceAgent->rtcIceServerDiagnostics[pIceCandidatePair->local->iceServerIndex].totalRoundTripTime += GETTIME() - requestSentTime; - CHK_STATUS(hashTableRemove(pIceAgent->requestTimestampDiagnostics, checkSum)); + if (pIceAgent->pRtcIceServerDiagnostics[pIceCandidatePair->local->iceServerIndex] != NULL) { + pIceAgent->pRtcIceServerDiagnostics[pIceCandidatePair->local->iceServerIndex]->totalResponsesReceived++; + retStatus = hashTableGet(pIceAgent->requestTimestampDiagnostics, checkSum, &requestSentTime); + if (hashTableGet(pIceAgent->requestTimestampDiagnostics, checkSum, &requestSentTime) == STATUS_SUCCESS) { + pIceAgent->pRtcIceServerDiagnostics[pIceCandidatePair->local->iceServerIndex]->totalRoundTripTime += + GETTIME() - requestSentTime; + CHK_STATUS(hashTableRemove(pIceAgent->requestTimestampDiagnostics, checkSum)); + } } } CHK_STATUS(deserializeStunPacket(pBuffer, bufferLen, (PBYTE) pIceAgent->remotePassword, @@ -2603,17 +2732,20 @@ STATUS handleStunPacket(PIceAgent pIceAgent, PBYTE pBuffer, UINT32 bufferLen, PS pIceCandidatePair->roundTripTime = GETTIME() - requestSentTime; DLOGD("Ice candidate pair %s_%s is connected. Round trip time: %" PRIu64 "ms", pIceCandidatePair->local->id, pIceCandidatePair->remote->id, pIceCandidatePair->roundTripTime / HUNDREDS_OF_NANOS_IN_A_MILLISECOND); - pIceCandidatePair->rtcIceCandidatePairDiagnostics.totalRoundTripTime += - (DOUBLE) (pIceCandidatePair->roundTripTime) / HUNDREDS_OF_NANOS_IN_A_SECOND; - + if (pIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->totalRoundTripTime += + (DOUBLE) (pIceCandidatePair->roundTripTime) / HUNDREDS_OF_NANOS_IN_A_SECOND; + } CHK_STATUS(hashTableRemove(pIceCandidatePair->requestSentTime, checkSum)); } else { DLOGW("Unable to fetch request Timestamp from the hash table. No update to RTT for the pair (error code: 0x%08x)", retStatus); } } - pIceCandidatePair->rtcIceCandidatePairDiagnostics.responsesReceived += connectivityCheckResponsesReceived; - pIceCandidatePair->rtcIceCandidatePairDiagnostics.lastResponseTimestamp = GETTIME(); + if (pIceCandidatePair->pRtcIceCandidatePairDiagnostics != NULL) { + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->responsesReceived += connectivityCheckResponsesReceived; + pIceCandidatePair->pRtcIceCandidatePairDiagnostics->lastResponseTimestamp = GETTIME(); + } break; case STUN_PACKET_TYPE_BINDING_INDICATION: diff --git a/src/source/Ice/IceAgent.h b/src/source/Ice/IceAgent.h index a069ecb1aa..f1b90e2a4d 100644 --- a/src/source/Ice/IceAgent.h +++ b/src/source/Ice/IceAgent.h @@ -80,24 +80,23 @@ typedef struct __IceAgent* PIceAgent; * Internal structure tracking ICE server parameters for diagnostics and metrics/stats */ typedef struct { - CHAR url[MAX_STATS_STRING_LENGTH + 1]; //!< STUN/TURN server URL - CHAR protocol[MAX_STATS_STRING_LENGTH + 1]; //!< Valid values: UDP, TCP - INT32 port; //!< Port number used by client - UINT64 totalRequestsSent; //!< Total amount of requests that have been sent to the server - UINT64 totalResponsesReceived; //!< Total number of responses received from the server - UINT64 totalRoundTripTime; //!< Sum of RTTs of all the requests for which response has been received + CHAR url[MAX_ICE_CONFIG_URI_LEN + 1]; //!< STUN/TURN server URL + CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP + INT32 port; //!< Port number used by client + UINT64 totalRequestsSent; //!< Total amount of requests that have been sent to the server + UINT64 totalResponsesReceived; //!< Total number of responses received from the server + UINT64 totalRoundTripTime; //!< Sum of RTTs of all the requests for which response has been received } RtcIceServerDiagnostics, *PRtcIceServerDiagnostics; typedef struct { - DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained - DOMString transportId[MAX_STATS_STRING_LENGTH + 1]; //!< ID of object that was inspected for RTCTransportStats - CHAR address[KVS_IP_ADDRESS_STRING_BUFFER_LEN]; //!< IPv4 or IPv6 address of the candidate - DOMString protocol; //!< Valid values: UDP, TCP - DOMString relayProtocol; //!< Protocol used by endpoint to communicate with TURN server. - //!< Valid values: UDP, TCP, TLS - INT32 priority; //!< Computed using the formula in https://tools.ietf.org/html/rfc5245#section-15.1 - INT32 port; //!< Port number of the candidate - DOMString candidateType; //!< Type of local/remote ICE candidate + DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained + CHAR address[KVS_IP_ADDRESS_STRING_BUFFER_LEN]; //!< IPv4 or IPv6 address of the candidate + CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP + CHAR relayProtocol[MAX_PROTOCOL_LENGTH + 1]; //!< Protocol used by endpoint to communicate with TURN server. + //!< Valid values: UDP, TCP, TLS + CHAR candidateType[MAX_CANDIDATE_TYPE_LENGTH + 1]; //!< Type of local/remote ICE candidate + INT32 priority; //!< Computed using the formula in https://tools.ietf.org/html/rfc5245#section-15.1 + INT32 port; //!< Port number of the candidate } RtcIceCandidateDiagnostics, *PRtcIceCandidateDiagnostics; typedef struct { @@ -183,7 +182,7 @@ typedef struct { PHashTable requestSentTime; UINT64 roundTripTime; UINT64 responsesReceived; - RtcIceCandidatePairDiagnostics rtcIceCandidatePairDiagnostics; + PRtcIceCandidatePairDiagnostics pRtcIceCandidatePairDiagnostics; } IceCandidatePair, *PIceCandidatePair; typedef struct { @@ -213,9 +212,9 @@ struct __IceAgent { CHAR remotePassword[MAX_ICE_CONFIG_CREDENTIAL_LEN + 1]; CHAR combinedUserName[(MAX_ICE_CONFIG_USER_NAME_LEN + 1) << 1]; //!< the combination of remote user name and local user name. - RtcIceServerDiagnostics rtcIceServerDiagnostics[MAX_ICE_SERVERS_COUNT]; - RtcIceCandidateDiagnostics rtcSelectedLocalIceCandidateDiagnostics; - RtcIceCandidateDiagnostics rtcSelectedRemoteIceCandidateDiagnostics; + PRtcIceServerDiagnostics pRtcIceServerDiagnostics[MAX_ICE_SERVERS_COUNT]; + PRtcIceCandidateDiagnostics pRtcSelectedLocalIceCandidateDiagnostics; + PRtcIceCandidateDiagnostics pRtcSelectedRemoteIceCandidateDiagnostics; IceAgentProfileDiagnostics iceAgentProfileDiagnostics; PHashTable requestTimestampDiagnostics; diff --git a/src/source/Metrics/Metrics.c b/src/source/Metrics/Metrics.c index f39c3e0292..7d4acbc9d9 100644 --- a/src/source/Metrics/Metrics.c +++ b/src/source/Metrics/Metrics.c @@ -13,37 +13,40 @@ STATUS getIceCandidatePairStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceCa pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; MUTEX_LOCK(pIceAgent->lock); locked = TRUE; + CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); CHK(pIceAgent->pDataSendingIceCandidatePair != NULL, STATUS_SUCCESS); - PRtcIceCandidatePairDiagnostics pRtcIceCandidatePairDiagnostics = &pIceAgent->pDataSendingIceCandidatePair->rtcIceCandidatePairDiagnostics; - STRCPY(pRtcIceCandidatePairStats->localCandidateId, pRtcIceCandidatePairDiagnostics->localCandidateId); - STRCPY(pRtcIceCandidatePairStats->remoteCandidateId, pRtcIceCandidatePairDiagnostics->remoteCandidateId); - pRtcIceCandidatePairStats->state = pRtcIceCandidatePairDiagnostics->state; - pRtcIceCandidatePairStats->nominated = pRtcIceCandidatePairDiagnostics->nominated; + PRtcIceCandidatePairDiagnostics pRtcIceCandidatePairDiagnostics = pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics; + if (pRtcIceCandidatePairDiagnostics != NULL) { + STRCPY(pRtcIceCandidatePairStats->localCandidateId, pRtcIceCandidatePairDiagnostics->localCandidateId); + STRCPY(pRtcIceCandidatePairStats->remoteCandidateId, pRtcIceCandidatePairDiagnostics->remoteCandidateId); + pRtcIceCandidatePairStats->state = pRtcIceCandidatePairDiagnostics->state; + pRtcIceCandidatePairStats->nominated = pRtcIceCandidatePairDiagnostics->nominated; - // Note: circuitBreakerTriggerCount This is set to NULL currently - pRtcIceCandidatePairStats->circuitBreakerTriggerCount = pRtcIceCandidatePairDiagnostics->circuitBreakerTriggerCount; + // Note: circuitBreakerTriggerCount This is set to NULL currently + pRtcIceCandidatePairStats->circuitBreakerTriggerCount = pRtcIceCandidatePairDiagnostics->circuitBreakerTriggerCount; - pRtcIceCandidatePairStats->packetsDiscardedOnSend = pRtcIceCandidatePairDiagnostics->packetsDiscardedOnSend; - pRtcIceCandidatePairStats->packetsSent = pRtcIceCandidatePairDiagnostics->packetsSent; - pRtcIceCandidatePairStats->packetsReceived = pRtcIceCandidatePairDiagnostics->packetsReceived; + pRtcIceCandidatePairStats->packetsDiscardedOnSend = pRtcIceCandidatePairDiagnostics->packetsDiscardedOnSend; + pRtcIceCandidatePairStats->packetsSent = pRtcIceCandidatePairDiagnostics->packetsSent; + pRtcIceCandidatePairStats->packetsReceived = pRtcIceCandidatePairDiagnostics->packetsReceived; - pRtcIceCandidatePairStats->bytesDiscardedOnSend = pRtcIceCandidatePairDiagnostics->bytesDiscardedOnSend; - pRtcIceCandidatePairStats->bytesSent = pRtcIceCandidatePairDiagnostics->bytesSent; - pRtcIceCandidatePairStats->bytesReceived = pRtcIceCandidatePairDiagnostics->bytesReceived; + pRtcIceCandidatePairStats->bytesDiscardedOnSend = pRtcIceCandidatePairDiagnostics->bytesDiscardedOnSend; + pRtcIceCandidatePairStats->bytesSent = pRtcIceCandidatePairDiagnostics->bytesSent; + pRtcIceCandidatePairStats->bytesReceived = pRtcIceCandidatePairDiagnostics->bytesReceived; - pRtcIceCandidatePairStats->lastPacketSentTimestamp = pRtcIceCandidatePairDiagnostics->lastPacketSentTimestamp; - pRtcIceCandidatePairStats->lastPacketReceivedTimestamp = pRtcIceCandidatePairDiagnostics->lastPacketReceivedTimestamp; - pRtcIceCandidatePairStats->lastRequestTimestamp = pRtcIceCandidatePairDiagnostics->lastRequestTimestamp; - pRtcIceCandidatePairStats->firstRequestTimestamp = pRtcIceCandidatePairDiagnostics->firstRequestTimestamp; - pRtcIceCandidatePairStats->lastResponseTimestamp = pRtcIceCandidatePairDiagnostics->lastResponseTimestamp; + pRtcIceCandidatePairStats->lastPacketSentTimestamp = pRtcIceCandidatePairDiagnostics->lastPacketSentTimestamp; + pRtcIceCandidatePairStats->lastPacketReceivedTimestamp = pRtcIceCandidatePairDiagnostics->lastPacketReceivedTimestamp; + pRtcIceCandidatePairStats->lastRequestTimestamp = pRtcIceCandidatePairDiagnostics->lastRequestTimestamp; + pRtcIceCandidatePairStats->firstRequestTimestamp = pRtcIceCandidatePairDiagnostics->firstRequestTimestamp; + pRtcIceCandidatePairStats->lastResponseTimestamp = pRtcIceCandidatePairDiagnostics->lastResponseTimestamp; - pRtcIceCandidatePairStats->totalRoundTripTime = pRtcIceCandidatePairDiagnostics->totalRoundTripTime; - pRtcIceCandidatePairStats->currentRoundTripTime = pRtcIceCandidatePairDiagnostics->currentRoundTripTime; + pRtcIceCandidatePairStats->totalRoundTripTime = pRtcIceCandidatePairDiagnostics->totalRoundTripTime; + pRtcIceCandidatePairStats->currentRoundTripTime = pRtcIceCandidatePairDiagnostics->currentRoundTripTime; - pRtcIceCandidatePairStats->requestsReceived = pRtcIceCandidatePairDiagnostics->requestsReceived; - pRtcIceCandidatePairStats->requestsSent = pRtcIceCandidatePairDiagnostics->requestsSent; - pRtcIceCandidatePairStats->responsesReceived = pRtcIceCandidatePairDiagnostics->responsesReceived; - pRtcIceCandidatePairStats->responsesSent = pRtcIceCandidatePairDiagnostics->responsesSent; + pRtcIceCandidatePairStats->requestsReceived = pRtcIceCandidatePairDiagnostics->requestsReceived; + pRtcIceCandidatePairStats->requestsSent = pRtcIceCandidatePairDiagnostics->requestsSent; + pRtcIceCandidatePairStats->responsesReceived = pRtcIceCandidatePairDiagnostics->responsesReceived; + pRtcIceCandidatePairStats->responsesSent = pRtcIceCandidatePairDiagnostics->responsesSent; + } CleanUp: if (locked) { MUTEX_UNLOCK(pIceAgent->lock); @@ -59,17 +62,20 @@ STATUS getIceCandidateStats(PRtcPeerConnection pRtcPeerConnection, BOOL isRemote CHK((pRtcPeerConnection != NULL || pRtcIceCandidateStats != NULL), STATUS_NULL_ARG); MUTEX_LOCK(pIceAgent->lock); locked = TRUE; - PRtcIceCandidateDiagnostics pRtcIceCandidateDiagnostics = &pIceAgent->rtcSelectedRemoteIceCandidateDiagnostics; - if (!isRemote) { - pRtcIceCandidateDiagnostics = &pIceAgent->rtcSelectedLocalIceCandidateDiagnostics; - STRCPY(pRtcIceCandidateStats->relayProtocol, pRtcIceCandidateDiagnostics->relayProtocol); - STRCPY(pRtcIceCandidateStats->url, pRtcIceCandidateDiagnostics->url); + CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); + PRtcIceCandidateDiagnostics pRtcIceCandidateDiagnostics = pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics; + if (pRtcIceCandidateDiagnostics != NULL) { + if (!isRemote) { + pRtcIceCandidateDiagnostics = pIceAgent->pRtcSelectedLocalIceCandidateDiagnostics; + STRCPY(pRtcIceCandidateStats->relayProtocol, pRtcIceCandidateDiagnostics->relayProtocol); + STRCPY(pRtcIceCandidateStats->url, pRtcIceCandidateDiagnostics->url); + } + STRCPY(pRtcIceCandidateStats->address, pRtcIceCandidateDiagnostics->address); + STRCPY(pRtcIceCandidateStats->candidateType, pRtcIceCandidateDiagnostics->candidateType); + pRtcIceCandidateStats->port = pRtcIceCandidateDiagnostics->port; + pRtcIceCandidateStats->priority = pRtcIceCandidateDiagnostics->priority; + STRCPY(pRtcIceCandidateStats->protocol, pRtcIceCandidateDiagnostics->protocol); } - STRCPY(pRtcIceCandidateStats->address, pRtcIceCandidateDiagnostics->address); - STRCPY(pRtcIceCandidateStats->candidateType, pRtcIceCandidateDiagnostics->candidateType); - pRtcIceCandidateStats->port = pRtcIceCandidateDiagnostics->port; - pRtcIceCandidateStats->priority = pRtcIceCandidateDiagnostics->priority; - STRCPY(pRtcIceCandidateStats->protocol, pRtcIceCandidateDiagnostics->protocol); CleanUp: if (locked) { MUTEX_UNLOCK(pIceAgent->lock); @@ -83,16 +89,20 @@ STATUS getIceServerStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceServerSta BOOL locked = FALSE; PIceAgent pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; CHK((pRtcPeerConnection != NULL && pRtcIceServerStats != NULL), STATUS_NULL_ARG); - CHK(pRtcIceServerStats->iceServerIndex < pIceAgent->iceServersCount, STATUS_ICE_SERVER_INDEX_INVALID); MUTEX_LOCK(pIceAgent->lock); locked = TRUE; - pRtcIceServerStats->port = pIceAgent->rtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex].port; - STRCPY(pRtcIceServerStats->protocol, pIceAgent->rtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex].protocol); - STRCPY(pRtcIceServerStats->url, pIceAgent->rtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex].url); - pRtcIceServerStats->totalRequestsSent = pIceAgent->rtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex].totalRequestsSent; - pRtcIceServerStats->totalResponsesReceived = pIceAgent->rtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex].totalResponsesReceived; - pRtcIceServerStats->totalRoundTripTime = pIceAgent->rtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex].totalRoundTripTime; + CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); + CHK(pRtcIceServerStats->iceServerIndex < pIceAgent->iceServersCount, STATUS_ICE_SERVER_INDEX_INVALID); + + if (pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex] != NULL) { + pRtcIceServerStats->port = pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex]->port; + STRCPY(pRtcIceServerStats->protocol, pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex]->protocol); + STRCPY(pRtcIceServerStats->url, pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex]->url); + pRtcIceServerStats->totalRequestsSent = pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex]->totalRequestsSent; + pRtcIceServerStats->totalResponsesReceived = pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex]->totalResponsesReceived; + pRtcIceServerStats->totalRoundTripTime = pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex]->totalRoundTripTime; + } CleanUp: if (locked) { MUTEX_UNLOCK(pIceAgent->lock); diff --git a/tst/MetricsApiTest.cpp b/tst/MetricsApiTest.cpp index 7484d2db94..46db2c245a 100644 --- a/tst/MetricsApiTest.cpp +++ b/tst/MetricsApiTest.cpp @@ -56,7 +56,7 @@ TEST_F(MetricsApiTest, webRtcIceServerGetMetrics) STRNCPY(configuration.iceServers[1].urls, (PCHAR) "turns:54.202.170.151:443?transport=tcp", MAX_ICE_CONFIG_URI_LEN); STRNCPY(configuration.iceServers[1].credential, (PCHAR) "username", MAX_ICE_CONFIG_CREDENTIAL_LEN); STRNCPY(configuration.iceServers[1].username, (PCHAR) "password", MAX_ICE_CONFIG_USER_NAME_LEN); - + configuration.kvsRtcConfiguration.enableIceStats = TRUE; ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); EXPECT_EQ(STATUS_ICE_SERVER_INDEX_INVALID, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); @@ -89,7 +89,7 @@ TEST_F(MetricsApiTest, webRtcIceCandidateGetMetrics) STRNCPY(configuration.iceServers[0].urls, (PCHAR) "stun:stun.kinesisvideo.us-west-2.amazonaws.com:443", MAX_ICE_CONFIG_URI_LEN); STRNCPY(configuration.iceServers[0].credential, (PCHAR) "", MAX_ICE_CONFIG_CREDENTIAL_LEN); STRNCPY(configuration.iceServers[0].username, (PCHAR) "", MAX_ICE_CONFIG_USER_NAME_LEN); - + configuration.kvsRtcConfiguration.enableIceStats = TRUE; ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; @@ -156,6 +156,36 @@ TEST_F(MetricsApiTest, webRtcIceCandidateGetMetrics) EXPECT_EQ(STATUS_SUCCESS, freePeerConnection(&pRtcPeerConnection)); } +TEST_F(MetricsApiTest, webRtcIceServerGetMetrics_IceStatsDisabled) +{ + RtcConfiguration configuration; + PRtcPeerConnection pRtcPeerConnection; + RtcStats rtcIceMetrics; + rtcIceMetrics.requestedTypeOfStats = RTC_STATS_TYPE_ICE_SERVER; // Supplying a type that is unavailable + rtcIceMetrics.rtcStatsObject.iceServerStats.iceServerIndex = 5; + + MEMSET(&configuration, 0x00, SIZEOF(RtcConfiguration)); + MEMSET(&rtcIceMetrics.rtcStatsObject.iceServerStats, 0x00, SIZEOF(RtcIceServerStats)); + STRNCPY(configuration.iceServers[0].urls, (PCHAR) "stun:stun.kinesisvideo.us-west-2.amazonaws.com:443", MAX_ICE_CONFIG_URI_LEN); + STRNCPY(configuration.iceServers[0].credential, (PCHAR) "", MAX_ICE_CONFIG_CREDENTIAL_LEN); + STRNCPY(configuration.iceServers[0].username, (PCHAR) "", MAX_ICE_CONFIG_USER_NAME_LEN); + + STRNCPY(configuration.iceServers[1].urls, (PCHAR) "turns:54.202.170.151:443?transport=tcp", MAX_ICE_CONFIG_URI_LEN); + STRNCPY(configuration.iceServers[1].credential, (PCHAR) "username", MAX_ICE_CONFIG_CREDENTIAL_LEN); + STRNCPY(configuration.iceServers[1].username, (PCHAR) "password", MAX_ICE_CONFIG_USER_NAME_LEN); + configuration.kvsRtcConfiguration.enableIceStats = FALSE; + ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); + + rtcIceMetrics.rtcStatsObject.iceServerStats.iceServerIndex = 1; + EXPECT_EQ(STATUS_SUCCESS, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); + EXPECT_EQ(0, rtcIceMetrics.rtcStatsObject.iceServerStats.port); + EXPECT_EQ('\0', rtcIceMetrics.rtcStatsObject.iceServerStats.url[0]); + EXPECT_EQ('\0', rtcIceMetrics.rtcStatsObject.iceServerStats.protocol[0]); + + EXPECT_EQ(STATUS_SUCCESS, closePeerConnection(pRtcPeerConnection)); + EXPECT_EQ(STATUS_SUCCESS, freePeerConnection(&pRtcPeerConnection)); +} + } // namespace webrtcclient } // namespace video } // namespace kinesis From fc9555e370d0af4deb4330d776cc00914f78e6e0 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Fri, 1 Nov 2024 15:06:33 -0700 Subject: [PATCH 02/17] Fix readme typos --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index a6718f86b8..bb8ed6107e 100644 --- a/README.md +++ b/README.md @@ -599,7 +599,7 @@ The SDK calculates 4 different stats: For more information on these stats, refer to [AWS Docs](https://docs.aws.amazon.com/kinesisvideostreams-webrtc-dg/latest/devguide/kvswebrtc-reference.html) -The SDK disables generating these stats by default. In order to be enable the SDK to calculate these stats, the application needs to set the following field: +The SDK disables generating these stats by default. In order to enable the SDK to calculate these stats, the application needs to set the following field: `configuration.kvsRtcConfiguration.enableIceStats = TRUE`. ### Controlling RTP rolling buffer capacity @@ -622,12 +622,12 @@ RtcRtpTransceiverInit.rollingBufferDurationSec = Date: Fri, 1 Nov 2024 17:06:50 -0700 Subject: [PATCH 03/17] clang format --- samples/Common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samples/Common.c b/samples/Common.c index bbb882b1ef..c1497705be 100644 --- a/samples/Common.c +++ b/samples/Common.c @@ -505,7 +505,7 @@ STATUS createSampleStreamingSession(PSampleConfiguration pSampleConfiguration, P // Flag to enable SDK to calculate selected ice server, local, remote and candidate pair stats. pSampleConfiguration->enableIceStats = FALSE; - + CHK_STATUS(initializePeerConnection(pSampleConfiguration, &pSampleStreamingSession->pPeerConnection)); CHK_STATUS(peerConnectionOnIceCandidate(pSampleStreamingSession->pPeerConnection, (UINT64) pSampleStreamingSession, onIceCandidateHandler)); CHK_STATUS( From a46d6d9aeb9d73730cb019c74e5c51925b3b2744 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Mon, 4 Nov 2024 16:45:39 -0800 Subject: [PATCH 04/17] Add ENABLE_STATS_CALCULATION_CONTROL CMake option and compiler flag. Update ReadMe. --- CMakeLists.txt | 5 ++++ README.md | 6 +++-- samples/Common.c | 5 +++- .../kinesis/video/webrtcclient/Include.h | 3 ++- src/source/Ice/IceAgent.c | 21 +++++++++++++--- src/source/Metrics/Metrics.c | 24 ++++++++++++++++--- 6 files changed, 54 insertions(+), 10 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 38b7e2ac16..bc3508db3d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -21,6 +21,7 @@ option(ENABLE_DATA_CHANNEL "Enable support for data channel" ON) option(ENABLE_KVS_THREADPOOL "Enable support for KVS thread pool in signaling" OFF) option(INSTRUMENTED_ALLOCATORS "Enable memory instrumentation" OFF) option(ENABLE_AWS_SDK_IN_TESTS "Enable support for compiling AWS SDKs for tests" ON) +option(ENABLE_STATS_CALCULATION_CONTROL "Enable support for runtime control of ice agent stat calculations." OFF) # Developer Flags option(BUILD_TEST "Build the testing tree." OFF) @@ -115,6 +116,10 @@ if (ENABLE_KVS_THREADPOOL) add_definitions(-DENABLE_KVS_THREADPOOL) endif() +if (ENABLE_STATS_CALCULATION_CONTROL) + add_definitions(-DENABLE_STATS_CALCULATION_CONTROL) +endif() + if(USE_OPENSSL) add_definitions(-DKVS_USE_OPENSSL) elseif(USE_MBEDTLS) diff --git a/README.md b/README.md index bb8ed6107e..c3d14bfc58 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,7 @@ You can pass the following options to `cmake ..`. * `-DLINK_PROFILER` -- Link with gperftools (available profiler options are listed [here](https://github.com/gperftools/gperftools)) * `-DPKG_CONFIG_EXECUTABLE` -- Set pkg config path. This might be required to find gstreamer's pkg config specifically on Windows. * `-DENABLE_KVS_THREADPOOL` -- Enable the KVS threadpool which is off by default. +* `-DENABLE_STATS_CALCULATION_CONTROL` -- Enable the runtime control of ICE agent stats calculations. To clean up the `open-source` and `build` folders from previous build, use `cmake --build . --target clean` from the `build` folder @@ -599,8 +600,9 @@ The SDK calculates 4 different stats: For more information on these stats, refer to [AWS Docs](https://docs.aws.amazon.com/kinesisvideostreams-webrtc-dg/latest/devguide/kvswebrtc-reference.html) -The SDK disables generating these stats by default. In order to enable the SDK to calculate these stats, the application needs to set the following field: -`configuration.kvsRtcConfiguration.enableIceStats = TRUE`. +The SDK enables generating these stats by default. To control whether the SDK calculates these stats, the ENABLE_STATS_CALCULATION_CONTROL CMake option must be set, enabling the use of the following field: +`configuration.kvsRtcConfiguration.enableIceStats = FALSE`. +Disabling these stats may lead to reductions in memory use. ### Controlling RTP rolling buffer capacity diff --git a/samples/Common.c b/samples/Common.c index c1497705be..9cd07e2f30 100644 --- a/samples/Common.c +++ b/samples/Common.c @@ -503,8 +503,11 @@ STATUS createSampleStreamingSession(PSampleConfiguration pSampleConfiguration, P pSampleStreamingSession->twccMetadata.updateLock = MUTEX_CREATE(TRUE); } - // Flag to enable SDK to calculate selected ice server, local, remote and candidate pair stats. + // Flag to enable/disable SDK calculations of selected ice server, local, remote and candidate pair stats. + // Note: enableIceStats only has an effect if compiler flag ENABLE_STATS_CALCULATION_CONTROL is defined. +#ifdef ENABLE_STATS_CALCULATION_CONTROL pSampleConfiguration->enableIceStats = FALSE; +#endif CHK_STATUS(initializePeerConnection(pSampleConfiguration, &pSampleStreamingSession->pPeerConnection)); CHK_STATUS(peerConnectionOnIceCandidate(pSampleStreamingSession->pPeerConnection, (UINT64) pSampleStreamingSession, onIceCandidateHandler)); diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h index 88922c56ba..2b57e954df 100644 --- a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h +++ b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h @@ -1199,7 +1199,8 @@ typedef struct { BOOL disableSenderSideBandwidthEstimation; //!< Disable TWCC feedback based sender bandwidth estimation, enabled by default. //!< You want to set this to TRUE if you are on a very stable connection and want to save 1.2MB of //!< memory - BOOL enableIceStats; //!< Enable ICE stats to be calculated + BOOL enableIceStats; //!< Control whether ICE agent stats are to be calculated. ENABLE_STATS_CALCULATION_CONTROL compiler flag must be defined + //!< to use this member, else stats are enabled by default. } KvsRtcConfiguration, *PKvsRtcConfiguration; /** diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index 4388eaec55..1783cab2a6 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -30,6 +30,13 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge PIceAgent pIceAgent = NULL; UINT32 i; UINT64 startTimeInMacro = 0; + BOOL statsControlEnabled = FALSE; + + // Ice agent stats calculations are on by default. + // Runtime control for turning stats calculations on/off can be activated with this compiler flag. + #ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; + #endif CHK(ppIceAgent != NULL && username != NULL && password != NULL && pConnectionListener != NULL, STATUS_NULL_ARG); CHK(STRNLEN(username, MAX_ICE_CONFIG_USER_NAME_LEN + 1) <= MAX_ICE_CONFIG_USER_NAME_LEN && @@ -106,7 +113,7 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge (PCHAR) pRtcConfiguration->iceServers[i].username, (PCHAR) pRtcConfiguration->iceServers[i].credential), pIceAgent->iceAgentProfileDiagnostics.iceServerParsingTime[i], "ICE server parsing"); if (STATUS_SUCCEEDED(retStatus)) { - if (pIceAgent->kvsRtcConfiguration.enableIceStats) { + if (!statsControlEnabled || (statsControlEnabled && pIceAgent->kvsRtcConfiguration.enableIceStats)) { CHK(NULL != (pIceAgent->pRtcIceServerDiagnostics[i] = (PRtcIceServerDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceServerDiagnostics))), STATUS_NOT_ENOUGH_MEMORY); pIceAgent->pRtcIceServerDiagnostics[i]->port = (INT32) getInt16(pIceAgent->iceServers[i].ipAddress.port); @@ -129,7 +136,8 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge } } - if (pIceAgent->kvsRtcConfiguration.enableIceStats) { + // Note: The conditional is emphasizing that statsControlEnabled must be TRUE in order to use kvsRtcConfiguration.enableIceStats. + if (!statsControlEnabled || (statsControlEnabled && pIceAgent->kvsRtcConfiguration.enableIceStats)) { CHK(NULL != (pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics = (PRtcIceCandidateDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidateDiagnostics))), @@ -1153,6 +1161,13 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, PIceCandidatePair pIceCandidatePair = NULL; BOOL freeObjOnFailure = TRUE; PIceCandidate pCurrentIceCandidate = NULL; + BOOL statsControlEnabled = FALSE; + + // Ice agent stats calculations are on by default. + // Runtime control for turning stats calculations on/off can be activated with this compiler flag. + #ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; + #endif CHK(pIceAgent != NULL && pIceCandidate != NULL, STATUS_NULL_ARG); CHK_WARN(pIceCandidate->state == ICE_CANDIDATE_STATE_VALID, retStatus, "New ice candidate need to be valid to form pairs"); @@ -1172,7 +1187,7 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, if (pCurrentIceCandidate->state == ICE_CANDIDATE_STATE_VALID && pCurrentIceCandidate->ipAddress.family == pIceCandidate->ipAddress.family) { pIceCandidatePair = (PIceCandidatePair) MEMCALLOC(1, SIZEOF(IceCandidatePair)); CHK(pIceCandidatePair != NULL, STATUS_NOT_ENOUGH_MEMORY); - if (pIceAgent->kvsRtcConfiguration.enableIceStats) { + if (!statsControlEnabled || (statsControlEnabled && pIceAgent->kvsRtcConfiguration.enableIceStats)) { CHK(NULL != (pIceCandidatePair->pRtcIceCandidatePairDiagnostics = (PRtcIceCandidatePairDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidatePairDiagnostics))), diff --git a/src/source/Metrics/Metrics.c b/src/source/Metrics/Metrics.c index 7d4acbc9d9..f514dc955d 100644 --- a/src/source/Metrics/Metrics.c +++ b/src/source/Metrics/Metrics.c @@ -9,11 +9,17 @@ STATUS getIceCandidatePairStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceCa STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; PIceAgent pIceAgent = NULL; + BOOL statsControlEnabled = FALSE; + + #ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; + #endif + CHK((pRtcPeerConnection != NULL || pRtcIceCandidatePairStats != NULL), STATUS_NULL_ARG); pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; MUTEX_LOCK(pIceAgent->lock); locked = TRUE; - CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); + CHK_WARN(!statsControlEnabled || pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats are not enabled."); CHK(pIceAgent->pDataSendingIceCandidatePair != NULL, STATUS_SUCCESS); PRtcIceCandidatePairDiagnostics pRtcIceCandidatePairDiagnostics = pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics; if (pRtcIceCandidatePairDiagnostics != NULL) { @@ -59,10 +65,16 @@ STATUS getIceCandidateStats(PRtcPeerConnection pRtcPeerConnection, BOOL isRemote STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; PIceAgent pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; + BOOL statsControlEnabled = FALSE; + + #ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; + #endif + CHK((pRtcPeerConnection != NULL || pRtcIceCandidateStats != NULL), STATUS_NULL_ARG); MUTEX_LOCK(pIceAgent->lock); locked = TRUE; - CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); + CHK_WARN(!statsControlEnabled || pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); PRtcIceCandidateDiagnostics pRtcIceCandidateDiagnostics = pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics; if (pRtcIceCandidateDiagnostics != NULL) { if (!isRemote) { @@ -88,11 +100,17 @@ STATUS getIceServerStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceServerSta STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; PIceAgent pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; + BOOL statsControlEnabled = FALSE; + + #ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; + #endif + CHK((pRtcPeerConnection != NULL && pRtcIceServerStats != NULL), STATUS_NULL_ARG); MUTEX_LOCK(pIceAgent->lock); locked = TRUE; - CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); + CHK_WARN(!statsControlEnabled || pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); CHK(pRtcIceServerStats->iceServerIndex < pIceAgent->iceServersCount, STATUS_ICE_SERVER_INDEX_INVALID); if (pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex] != NULL) { From 81d87b2d46a252edbaebfbddbd2df3c3fa9e955c Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Mon, 4 Nov 2024 16:55:06 -0800 Subject: [PATCH 05/17] Add IceStatsControlOn tests --- tst/MetricsApiTest.cpp | 122 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 121 insertions(+), 1 deletion(-) diff --git a/tst/MetricsApiTest.cpp b/tst/MetricsApiTest.cpp index 46db2c245a..3340666560 100644 --- a/tst/MetricsApiTest.cpp +++ b/tst/MetricsApiTest.cpp @@ -156,7 +156,9 @@ TEST_F(MetricsApiTest, webRtcIceCandidateGetMetrics) EXPECT_EQ(STATUS_SUCCESS, freePeerConnection(&pRtcPeerConnection)); } -TEST_F(MetricsApiTest, webRtcIceServerGetMetrics_IceStatsDisabled) +#ifdef ENABLE_STATS_CALCULATION_CONTROL + +TEST_F(MetricsApiTest, webRtcIceServerGetMetrics_IceStatsControlOn_Disabled) { RtcConfiguration configuration; PRtcPeerConnection pRtcPeerConnection; @@ -186,6 +188,124 @@ TEST_F(MetricsApiTest, webRtcIceServerGetMetrics_IceStatsDisabled) EXPECT_EQ(STATUS_SUCCESS, freePeerConnection(&pRtcPeerConnection)); } +TEST_F(MetricsApiTest, webRtcIceCandidateGetMetrics_IceStatsControlOn_Enabled) +{ + RtcConfiguration configuration; + PRtcPeerConnection pRtcPeerConnection = NULL; + PIceAgent pIceAgent; + RtcStats rtcIceMetrics; + + MEMSET(&configuration, 0x00, SIZEOF(RtcConfiguration)); + STRNCPY(configuration.iceServers[0].urls, (PCHAR) "stun:stun.kinesisvideo.us-west-2.amazonaws.com:443", MAX_ICE_CONFIG_URI_LEN); + STRNCPY(configuration.iceServers[0].credential, (PCHAR) "", MAX_ICE_CONFIG_CREDENTIAL_LEN); + STRNCPY(configuration.iceServers[0].username, (PCHAR) "", MAX_ICE_CONFIG_USER_NAME_LEN); + configuration.kvsRtcConfiguration.enableIceStats = TRUE; + ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); + + pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; + + if(pIceAgent == NULL) { + DLOGI("ICE Agent null"); + } else { + DLOGI("ICE agent not null"); + } + IceCandidate localCandidate; + IceCandidate remoteCandidate; + IceCandidatePair iceCandidatePair; + + MEMSET(&localCandidate, 0x00, SIZEOF(IceCandidate)); + localCandidate.state = ICE_CANDIDATE_STATE_VALID; + localCandidate.ipAddress.family = KVS_IP_FAMILY_TYPE_IPV4; + localCandidate.iceCandidateType = ICE_CANDIDATE_TYPE_SERVER_REFLEXIVE; + localCandidate.isRemote = FALSE; + localCandidate.ipAddress.address[0] = 0x7f; + localCandidate.ipAddress.address[1] = 0x00; + localCandidate.ipAddress.address[2] = 0x00; + localCandidate.ipAddress.address[3] = 0x01; + localCandidate.ipAddress.port = getInt16(1234); + localCandidate.priority = 1; + + MEMSET(&remoteCandidate, 0x00, SIZEOF(IceCandidate)); + remoteCandidate.state = ICE_CANDIDATE_STATE_VALID; + remoteCandidate.ipAddress.family = KVS_IP_FAMILY_TYPE_IPV4; + remoteCandidate.iceCandidateType = ICE_CANDIDATE_TYPE_HOST; + remoteCandidate.isRemote = FALSE; + remoteCandidate.ipAddress.address[0] = 0x0a; + remoteCandidate.ipAddress.address[1] = 0x01; + remoteCandidate.ipAddress.address[2] = 0xff; + remoteCandidate.ipAddress.address[3] = 0xff; + remoteCandidate.ipAddress.port = getInt16(1111); + remoteCandidate.priority = 3; + + iceCandidatePair.local = &localCandidate; + iceCandidatePair.remote = &remoteCandidate; + pIceAgent->pDataSendingIceCandidatePair = &iceCandidatePair; + + EXPECT_EQ(STATUS_SUCCESS, updateSelectedLocalRemoteCandidateStats(pIceAgent)); + + rtcIceMetrics.requestedTypeOfStats = RTC_STATS_TYPE_LOCAL_CANDIDATE; + EXPECT_EQ(STATUS_SUCCESS, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); + + EXPECT_EQ(1234, rtcIceMetrics.rtcStatsObject.localIceCandidateStats.port); + EXPECT_EQ(1, rtcIceMetrics.rtcStatsObject.localIceCandidateStats.priority); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "127.0.0.1", rtcIceMetrics.rtcStatsObject.localIceCandidateStats.address); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "srflx", rtcIceMetrics.rtcStatsObject.localIceCandidateStats.candidateType); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "", rtcIceMetrics.rtcStatsObject.localIceCandidateStats.protocol); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "stun.kinesisvideo.us-west-2.amazonaws.com", rtcIceMetrics.rtcStatsObject.localIceCandidateStats.url); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "", rtcIceMetrics.rtcStatsObject.localIceCandidateStats.relayProtocol); + + rtcIceMetrics.requestedTypeOfStats = RTC_STATS_TYPE_REMOTE_CANDIDATE; + EXPECT_EQ(STATUS_SUCCESS, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); + EXPECT_EQ(1111, rtcIceMetrics.rtcStatsObject.remoteIceCandidateStats.port); + EXPECT_EQ(3, rtcIceMetrics.rtcStatsObject.remoteIceCandidateStats.priority); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "10.1.255.255", rtcIceMetrics.rtcStatsObject.remoteIceCandidateStats.address); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "host", rtcIceMetrics.rtcStatsObject.remoteIceCandidateStats.candidateType); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "", rtcIceMetrics.rtcStatsObject.remoteIceCandidateStats.protocol); + + EXPECT_EQ(STATUS_SUCCESS, closePeerConnection(pRtcPeerConnection)); + EXPECT_EQ(STATUS_SUCCESS, freePeerConnection(&pRtcPeerConnection)); +} + +TEST_F(MetricsApiTest, webRtcIceServerGetMetrics_IceStatsControlOn_Enabled) +{ + RtcConfiguration configuration; + PRtcPeerConnection pRtcPeerConnection; + RtcStats rtcIceMetrics; + rtcIceMetrics.requestedTypeOfStats = RTC_STATS_TYPE_ICE_SERVER; // Supplying a type that is unavailable + rtcIceMetrics.rtcStatsObject.iceServerStats.iceServerIndex = 5; + + MEMSET(&configuration, 0x00, SIZEOF(RtcConfiguration)); + STRNCPY(configuration.iceServers[0].urls, (PCHAR) "stun:stun.kinesisvideo.us-west-2.amazonaws.com:443", MAX_ICE_CONFIG_URI_LEN); + STRNCPY(configuration.iceServers[0].credential, (PCHAR) "", MAX_ICE_CONFIG_CREDENTIAL_LEN); + STRNCPY(configuration.iceServers[0].username, (PCHAR) "", MAX_ICE_CONFIG_USER_NAME_LEN); + + STRNCPY(configuration.iceServers[1].urls, (PCHAR) "turns:54.202.170.151:443?transport=tcp", MAX_ICE_CONFIG_URI_LEN); + STRNCPY(configuration.iceServers[1].credential, (PCHAR) "username", MAX_ICE_CONFIG_CREDENTIAL_LEN); + STRNCPY(configuration.iceServers[1].username, (PCHAR) "password", MAX_ICE_CONFIG_USER_NAME_LEN); + configuration.kvsRtcConfiguration.enableIceStats = TRUE; + ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); + + EXPECT_EQ(STATUS_ICE_SERVER_INDEX_INVALID, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); + + rtcIceMetrics.rtcStatsObject.iceServerStats.iceServerIndex = 0; + EXPECT_EQ(STATUS_SUCCESS, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); + + EXPECT_EQ(443, rtcIceMetrics.rtcStatsObject.iceServerStats.port); + EXPECT_PRED_FORMAT2(testing::IsSubstring, configuration.iceServers[0].urls, rtcIceMetrics.rtcStatsObject.iceServerStats.url); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "", rtcIceMetrics.rtcStatsObject.iceServerStats.protocol); + + rtcIceMetrics.rtcStatsObject.iceServerStats.iceServerIndex = 1; + EXPECT_EQ(STATUS_SUCCESS, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); + EXPECT_EQ(443, rtcIceMetrics.rtcStatsObject.iceServerStats.port); + EXPECT_PRED_FORMAT2(testing::IsSubstring, configuration.iceServers[1].urls, rtcIceMetrics.rtcStatsObject.iceServerStats.url); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "tcp", rtcIceMetrics.rtcStatsObject.iceServerStats.protocol); + + EXPECT_EQ(STATUS_SUCCESS, closePeerConnection(pRtcPeerConnection)); + EXPECT_EQ(STATUS_SUCCESS, freePeerConnection(&pRtcPeerConnection)); +} + +#endif + } // namespace webrtcclient } // namespace video } // namespace kinesis From 5ca50e9a7a9af6ec28495592436cdb83968f3d6d Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Mon, 4 Nov 2024 17:35:27 -0800 Subject: [PATCH 06/17] Add CI test forENABLE_STATS_CALCULATION_CONTROL=TRUE --- .github/workflows/ci.yml | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6a56d15900..508576327f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -533,6 +533,36 @@ jobs: run: | cd build timeout --signal=SIGABRT 60m ./tst/webrtc_client_test + ubuntu-os-build-stats-calc-control: + runs-on: ubuntu-20.04 + env: + AWS_KVS_LOG_LEVEL: 2 + permissions: + id-token: write + contents: read + steps: + - name: Clone repository + uses: actions/checkout@v4 + - name: Configure AWS Credentials + uses: aws-actions/configure-aws-credentials@v4 + with: + role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }} + aws-region: ${{ secrets.AWS_REGION }} + - name: Install dependencies + run: | + sudo apt clean && sudo apt update + sudo apt-get -y install libcurl4-openssl-dev + - name: Build repository + run: | + # TODO: Remove the following line. This is only a workaround for enabling IPv6, https://github.com/travis-ci/travis-ci/issues/8891. + sudo sh -c 'echo 0 > /proc/sys/net/ipv6/conf/all/disable_ipv6' + mkdir build && cd build + cmake .. -DBUILD_TEST=TRUE -DENABLE_STATS_CALCULATION_CONTROL=TRUE + make + - name: Run tests + run: | + cd build + timeout --signal=SIGABRT 60m ./tst/webrtc_client_test windows-msvc-openssl: runs-on: windows-2022 env: From be0e273391724d65623ceade8c2eea7917ce40c7 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Mon, 4 Nov 2024 17:38:09 -0800 Subject: [PATCH 07/17] Clang formatting --- .../kinesis/video/webrtcclient/Include.h | 4 ++-- src/source/Ice/IceAgent.c | 24 +++++++++---------- src/source/Metrics/Metrics.c | 24 +++++++++---------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h index 2b57e954df..408ec30a35 100644 --- a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h +++ b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h @@ -1199,8 +1199,8 @@ typedef struct { BOOL disableSenderSideBandwidthEstimation; //!< Disable TWCC feedback based sender bandwidth estimation, enabled by default. //!< You want to set this to TRUE if you are on a very stable connection and want to save 1.2MB of //!< memory - BOOL enableIceStats; //!< Control whether ICE agent stats are to be calculated. ENABLE_STATS_CALCULATION_CONTROL compiler flag must be defined - //!< to use this member, else stats are enabled by default. + BOOL enableIceStats; //!< Control whether ICE agent stats are to be calculated. ENABLE_STATS_CALCULATION_CONTROL compiler flag must be defined + //!< to use this member, else stats are enabled by default. } KvsRtcConfiguration, *PKvsRtcConfiguration; /** diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index 1783cab2a6..1f87957c08 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -31,12 +31,12 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge UINT32 i; UINT64 startTimeInMacro = 0; BOOL statsControlEnabled = FALSE; - - // Ice agent stats calculations are on by default. - // Runtime control for turning stats calculations on/off can be activated with this compiler flag. - #ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; - #endif + +// Ice agent stats calculations are on by default. +// Runtime control for turning stats calculations on/off can be activated with this compiler flag. +#ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; +#endif CHK(ppIceAgent != NULL && username != NULL && password != NULL && pConnectionListener != NULL, STATUS_NULL_ARG); CHK(STRNLEN(username, MAX_ICE_CONFIG_USER_NAME_LEN + 1) <= MAX_ICE_CONFIG_USER_NAME_LEN && @@ -1162,12 +1162,12 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, BOOL freeObjOnFailure = TRUE; PIceCandidate pCurrentIceCandidate = NULL; BOOL statsControlEnabled = FALSE; - - // Ice agent stats calculations are on by default. - // Runtime control for turning stats calculations on/off can be activated with this compiler flag. - #ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; - #endif + +// Ice agent stats calculations are on by default. +// Runtime control for turning stats calculations on/off can be activated with this compiler flag. +#ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; +#endif CHK(pIceAgent != NULL && pIceCandidate != NULL, STATUS_NULL_ARG); CHK_WARN(pIceCandidate->state == ICE_CANDIDATE_STATE_VALID, retStatus, "New ice candidate need to be valid to form pairs"); diff --git a/src/source/Metrics/Metrics.c b/src/source/Metrics/Metrics.c index f514dc955d..0b5df752fe 100644 --- a/src/source/Metrics/Metrics.c +++ b/src/source/Metrics/Metrics.c @@ -10,10 +10,10 @@ STATUS getIceCandidatePairStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceCa BOOL locked = FALSE; PIceAgent pIceAgent = NULL; BOOL statsControlEnabled = FALSE; - - #ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; - #endif + +#ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; +#endif CHK((pRtcPeerConnection != NULL || pRtcIceCandidatePairStats != NULL), STATUS_NULL_ARG); pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; @@ -66,10 +66,10 @@ STATUS getIceCandidateStats(PRtcPeerConnection pRtcPeerConnection, BOOL isRemote BOOL locked = FALSE; PIceAgent pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; BOOL statsControlEnabled = FALSE; - - #ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; - #endif + +#ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; +#endif CHK((pRtcPeerConnection != NULL || pRtcIceCandidateStats != NULL), STATUS_NULL_ARG); MUTEX_LOCK(pIceAgent->lock); @@ -101,10 +101,10 @@ STATUS getIceServerStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceServerSta BOOL locked = FALSE; PIceAgent pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; BOOL statsControlEnabled = FALSE; - - #ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; - #endif + +#ifdef ENABLE_STATS_CALCULATION_CONTROL + statsControlEnabled = TRUE; +#endif CHK((pRtcPeerConnection != NULL && pRtcIceServerStats != NULL), STATUS_NULL_ARG); From 3e1d5399de786dac70cb1af537bda2703e21dd4d Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Tue, 5 Nov 2024 11:04:19 -0800 Subject: [PATCH 08/17] Remove unused iceAgentAddConfig --- src/source/Ice/IceAgent.c | 77 --------------------------------------- 1 file changed, 77 deletions(-) diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index 1f87957c08..b698edde59 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -290,83 +290,6 @@ STATUS freeIceAgent(PIceAgent* ppIceAgent) return retStatus; } -STATUS iceAgentAddConfig(PIceAgent pIceAgent, PIceConfigInfo pIceConfigInfo) -{ - STATUS retStatus = STATUS_SUCCESS; - UINT32 i = 0; - // used in PROFILE macro - UINT64 startTimeInMacro = 0; - BOOL locked = FALSE; - - CHK(pIceAgent != NULL && pIceConfigInfo != NULL, STATUS_NULL_ARG); - - for (i = 0; i < pIceConfigInfo->uriCount; i++) { - MUTEX_LOCK(pIceAgent->lock); - locked = TRUE; - PROFILE_CALL_WITH_T_OBJ(retStatus = parseIceServer(&pIceAgent->iceServers[pIceAgent->iceServersCount], (PCHAR) pIceConfigInfo->uris[i], - (PCHAR) pIceConfigInfo->userName, (PCHAR) pIceConfigInfo->password), - pIceAgent->iceAgentProfileDiagnostics.iceServerParsingTime[i], "ICE server parsing"); - MUTEX_UNLOCK(pIceAgent->lock); - locked = FALSE; - - if (STATUS_SUCCEEDED(retStatus)) { - MUTEX_LOCK(pIceAgent->lock); - locked = TRUE; - if (pIceAgent->pRtcIceServerDiagnostics[i] != NULL) { - pIceAgent->pRtcIceServerDiagnostics[i]->port = (INT32) getInt16(pIceAgent->iceServers[i].ipAddress.port); - switch (pIceAgent->iceServers[pIceAgent->iceServersCount].transport) { - case KVS_SOCKET_PROTOCOL_UDP: - STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, ICE_TRANSPORT_TYPE_UDP); - break; - case KVS_SOCKET_PROTOCOL_TCP: - STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, ICE_TRANSPORT_TYPE_TCP); - break; - default: - MEMSET(pIceAgent->pRtcIceServerDiagnostics[i]->protocol, 0, SIZEOF(pIceAgent->pRtcIceServerDiagnostics[i]->protocol)); - } - STRCPY(pIceAgent->pRtcIceServerDiagnostics[i]->url, pIceConfigInfo->uris[i]); - } - MUTEX_UNLOCK(pIceAgent->lock); - locked = FALSE; - - // important to unlock iceAgent lock before calling init relay candidate, since iceAgent APIs are thread safe - // if you don't unlock this can lead to a deadlock with the timerqueue. - // init candidate && pairs - if (pIceAgent->iceServers[pIceAgent->iceServersCount].isTurn) { - if (pIceAgent->iceServers[pIceAgent->iceServersCount].transport == KVS_SOCKET_PROTOCOL_UDP || - pIceAgent->iceServers[pIceAgent->iceServersCount].transport == KVS_SOCKET_PROTOCOL_NONE) { - CHK_STATUS(iceAgentInitRelayCandidate(pIceAgent, pIceAgent->iceServersCount, KVS_SOCKET_PROTOCOL_UDP)); - } - - if (pIceAgent->iceServers[pIceAgent->iceServersCount].transport == KVS_SOCKET_PROTOCOL_TCP || - pIceAgent->iceServers[pIceAgent->iceServersCount].transport == KVS_SOCKET_PROTOCOL_NONE) { - CHK_STATUS(iceAgentInitRelayCandidate(pIceAgent, pIceAgent->iceServersCount, KVS_SOCKET_PROTOCOL_TCP)); - } - } - - MUTEX_LOCK(pIceAgent->lock); - locked = TRUE; - - pIceAgent->iceServersCount++; - - MUTEX_UNLOCK(pIceAgent->lock); - locked = FALSE; - - } else { - DLOGE("Failed to parse ICE servers"); - } - } - -CleanUp: - CHK_LOG_ERR(retStatus); - - if (locked) { - MUTEX_UNLOCK(pIceAgent->lock); - } - - return retStatus; -} - STATUS iceAgentValidateKvsRtcConfig(PKvsRtcConfiguration pKvsRtcConfiguration) { STATUS retStatus = STATUS_SUCCESS; From d3a13f904e7d32b23e5bcc3fa8c4451a207d2d44 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Tue, 5 Nov 2024 11:32:21 -0800 Subject: [PATCH 09/17] Remove unintended readme merge changes "Controlling RTP rolling buffer capacity" --- README.md | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/README.md b/README.md index c3d14bfc58..0c1b4ef690 100644 --- a/README.md +++ b/README.md @@ -604,34 +604,6 @@ The SDK enables generating these stats by default. To control whether the SDK ca `configuration.kvsRtcConfiguration.enableIceStats = FALSE`. Disabling these stats may lead to reductions in memory use. -### Controlling RTP rolling buffer capacity - -The SDK maintains an RTP rolling buffer to hold the RTP packets. This is useful to respond to NACKs and even in case of JitterBuffer. The rolling buffer size is controlled by 3 parameters: -1. MTU: This is set to a default of 1200 bytes -2. Buffer duration: This is the amount of time of media that you would like the rolling buffer to accommodate before it is overwritten due to buffer overflow. By default, the SDK sets this to 1 second -3. Highest expected bitrate: This is the expected bitrate of the media in question. The typical bitrates could vary based on resolution and codec. By default, the SDK sets this to 5 mibps for video and 1 mibps for audio - -The rolling buffer capacity is calculated as follows: -``` -Capacity = Buffer duration * highest expected bitrate (in bps) / 8 / MTU - -With buffer duration = 1 second, Highest expected bitrate = 5 mibps and MTU 1200 bytes, capacity = 546 RTP packets -``` - -The rolling buffer size can be configured per transceiver through the following fields: -``` -RtcRtpTransceiverInit.rollingBufferDurationSec = Date: Wed, 6 Nov 2024 10:37:56 -0800 Subject: [PATCH 10/17] Address comments, add null check --- samples/Common.c | 4 +-- .../kinesis/video/webrtcclient/Include.h | 2 ++ src/source/Ice/IceAgent.c | 19 ++++++----- src/source/Metrics/Metrics.c | 32 +++++++++---------- tst/MetricsApiTest.cpp | 2 -- 5 files changed, 28 insertions(+), 31 deletions(-) diff --git a/samples/Common.c b/samples/Common.c index 9cd07e2f30..6235d605e9 100644 --- a/samples/Common.c +++ b/samples/Common.c @@ -365,7 +365,9 @@ STATUS initializePeerConnection(PSampleConfiguration pSampleConfiguration, PRtcP // Set the ICE mode explicitly configuration.iceTransportPolicy = ICE_TRANSPORT_POLICY_ALL; +#ifdef ENABLE_STATS_CALCULATION_CONTROL configuration.kvsRtcConfiguration.enableIceStats = pSampleConfiguration->enableIceStats; +#endif // Set the STUN server PCHAR pKinesisVideoStunUrlPostFix = KINESIS_VIDEO_STUN_URL_POSTFIX; @@ -505,9 +507,7 @@ STATUS createSampleStreamingSession(PSampleConfiguration pSampleConfiguration, P // Flag to enable/disable SDK calculations of selected ice server, local, remote and candidate pair stats. // Note: enableIceStats only has an effect if compiler flag ENABLE_STATS_CALCULATION_CONTROL is defined. -#ifdef ENABLE_STATS_CALCULATION_CONTROL pSampleConfiguration->enableIceStats = FALSE; -#endif CHK_STATUS(initializePeerConnection(pSampleConfiguration, &pSampleStreamingSession->pPeerConnection)); CHK_STATUS(peerConnectionOnIceCandidate(pSampleStreamingSession->pPeerConnection, (UINT64) pSampleStreamingSession, onIceCandidateHandler)); diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h index 408ec30a35..535e34509f 100644 --- a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h +++ b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h @@ -1199,8 +1199,10 @@ typedef struct { BOOL disableSenderSideBandwidthEstimation; //!< Disable TWCC feedback based sender bandwidth estimation, enabled by default. //!< You want to set this to TRUE if you are on a very stable connection and want to save 1.2MB of //!< memory +#ifdef ENABLE_STATS_CALCULATION_CONTROL BOOL enableIceStats; //!< Control whether ICE agent stats are to be calculated. ENABLE_STATS_CALCULATION_CONTROL compiler flag must be defined //!< to use this member, else stats are enabled by default. +#endif } KvsRtcConfiguration, *PKvsRtcConfiguration; /** diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index b698edde59..5d75603f1f 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -30,12 +30,12 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge PIceAgent pIceAgent = NULL; UINT32 i; UINT64 startTimeInMacro = 0; - BOOL statsControlEnabled = FALSE; + BOOL disableStats = FALSE; // Ice agent stats calculations are on by default. // Runtime control for turning stats calculations on/off can be activated with this compiler flag. #ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; + disableStats = !pIceAgent->kvsRtcConfiguration.enableIceStats; #endif CHK(ppIceAgent != NULL && username != NULL && password != NULL && pConnectionListener != NULL, STATUS_NULL_ARG); @@ -113,7 +113,7 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge (PCHAR) pRtcConfiguration->iceServers[i].username, (PCHAR) pRtcConfiguration->iceServers[i].credential), pIceAgent->iceAgentProfileDiagnostics.iceServerParsingTime[i], "ICE server parsing"); if (STATUS_SUCCEEDED(retStatus)) { - if (!statsControlEnabled || (statsControlEnabled && pIceAgent->kvsRtcConfiguration.enableIceStats)) { + if (!disableStats) { CHK(NULL != (pIceAgent->pRtcIceServerDiagnostics[i] = (PRtcIceServerDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceServerDiagnostics))), STATUS_NOT_ENOUGH_MEMORY); pIceAgent->pRtcIceServerDiagnostics[i]->port = (INT32) getInt16(pIceAgent->iceServers[i].ipAddress.port); @@ -136,8 +136,7 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge } } - // Note: The conditional is emphasizing that statsControlEnabled must be TRUE in order to use kvsRtcConfiguration.enableIceStats. - if (!statsControlEnabled || (statsControlEnabled && pIceAgent->kvsRtcConfiguration.enableIceStats)) { + if (!disableStats) { CHK(NULL != (pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics = (PRtcIceCandidateDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidateDiagnostics))), @@ -1084,12 +1083,12 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, PIceCandidatePair pIceCandidatePair = NULL; BOOL freeObjOnFailure = TRUE; PIceCandidate pCurrentIceCandidate = NULL; - BOOL statsControlEnabled = FALSE; + BOOL disableStats = FALSE; // Ice agent stats calculations are on by default. // Runtime control for turning stats calculations on/off can be activated with this compiler flag. #ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; + disableStats = !pIceAgent->kvsRtcConfiguration.enableIceStats; #endif CHK(pIceAgent != NULL && pIceCandidate != NULL, STATUS_NULL_ARG); @@ -1110,7 +1109,7 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, if (pCurrentIceCandidate->state == ICE_CANDIDATE_STATE_VALID && pCurrentIceCandidate->ipAddress.family == pIceCandidate->ipAddress.family) { pIceCandidatePair = (PIceCandidatePair) MEMCALLOC(1, SIZEOF(IceCandidatePair)); CHK(pIceCandidatePair != NULL, STATUS_NOT_ENOUGH_MEMORY); - if (!statsControlEnabled || (statsControlEnabled && pIceAgent->kvsRtcConfiguration.enableIceStats)) { + if (!disableStats) { CHK(NULL != (pIceCandidatePair->pRtcIceCandidatePairDiagnostics = (PRtcIceCandidatePairDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidatePairDiagnostics))), @@ -2587,10 +2586,10 @@ STATUS handleStunPacket(PIceAgent pIceAgent, PBYTE pBuffer, UINT32 bufferLen, PS pIceAgent->pRtcIceServerDiagnostics[pIceCandidate->iceServerIndex]->totalResponsesReceived++; // Transaction ID count be same for candidates coming from same interface, which means there would only // be one entry. It is not necessary to update a return sttaus since it is not indicative of a failure - if ((hashTableGet(pIceAgent->requestTimestampDiagnostics, checkSum, &requestSentTime)) == STATUS_SUCCESS) { + if (STATUS_SUCCEEDED(hashTableGet(pIceAgent->requestTimestampDiagnostics, checkSum, &requestSentTime))) { pIceAgent->pRtcIceServerDiagnostics[pIceCandidate->iceServerIndex]->totalRoundTripTime += GETTIME() - requestSentTime; CHK_STATUS(hashTableRemove(pIceAgent->requestTimestampDiagnostics, checkSum)); - hashTableGetCount(pIceAgent->requestTimestampDiagnostics, &count); + CHK_STATUS(hashTableGetCount(pIceAgent->requestTimestampDiagnostics, &count)); } } diff --git a/src/source/Metrics/Metrics.c b/src/source/Metrics/Metrics.c index 0b5df752fe..5dc10abf11 100644 --- a/src/source/Metrics/Metrics.c +++ b/src/source/Metrics/Metrics.c @@ -19,7 +19,9 @@ STATUS getIceCandidatePairStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceCa pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; MUTEX_LOCK(pIceAgent->lock); locked = TRUE; - CHK_WARN(!statsControlEnabled || pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats are not enabled."); +#ifdef ENABLE_STATS_CALCULATION_CONTROL + CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_INVALID_OPERATION, "ICE stats not enabled"); +#endif CHK(pIceAgent->pDataSendingIceCandidatePair != NULL, STATUS_SUCCESS); PRtcIceCandidatePairDiagnostics pRtcIceCandidatePairDiagnostics = pIceAgent->pDataSendingIceCandidatePair->pRtcIceCandidatePairDiagnostics; if (pRtcIceCandidatePairDiagnostics != NULL) { @@ -64,18 +66,16 @@ STATUS getIceCandidateStats(PRtcPeerConnection pRtcPeerConnection, BOOL isRemote { STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; - PIceAgent pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; - BOOL statsControlEnabled = FALSE; - -#ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; -#endif - + PIceAgent pIceAgent = NULL; + PRtcIceCandidateDiagnostics pRtcIceCandidateDiagnostics = NULL; CHK((pRtcPeerConnection != NULL || pRtcIceCandidateStats != NULL), STATUS_NULL_ARG); + pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; MUTEX_LOCK(pIceAgent->lock); locked = TRUE; - CHK_WARN(!statsControlEnabled || pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); - PRtcIceCandidateDiagnostics pRtcIceCandidateDiagnostics = pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics; +#ifdef ENABLE_STATS_CALCULATION_CONTROL + CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_INVALID_OPERATION, "ICE stats not enabled"); +#endif + pRtcIceCandidateDiagnostics = pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics; if (pRtcIceCandidateDiagnostics != NULL) { if (!isRemote) { pRtcIceCandidateDiagnostics = pIceAgent->pRtcSelectedLocalIceCandidateDiagnostics; @@ -100,17 +100,15 @@ STATUS getIceServerStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceServerSta STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; PIceAgent pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; - BOOL statsControlEnabled = FALSE; - -#ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; -#endif - CHK((pRtcPeerConnection != NULL && pRtcIceServerStats != NULL), STATUS_NULL_ARG); MUTEX_LOCK(pIceAgent->lock); locked = TRUE; - CHK_WARN(!statsControlEnabled || pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_SUCCESS, "ICE stats not enabled"); + +#ifdef ENABLE_STATS_CALCULATION_CONTROL + CHK_WARN(pIceAgent->kvsRtcConfiguration.enableIceStats, STATUS_INVALID_OPERATION, "ICE stats not enabled"); +#endif + CHK(pRtcIceServerStats->iceServerIndex < pIceAgent->iceServersCount, STATUS_ICE_SERVER_INDEX_INVALID); if (pIceAgent->pRtcIceServerDiagnostics[pRtcIceServerStats->iceServerIndex] != NULL) { diff --git a/tst/MetricsApiTest.cpp b/tst/MetricsApiTest.cpp index 3340666560..ada13a3863 100644 --- a/tst/MetricsApiTest.cpp +++ b/tst/MetricsApiTest.cpp @@ -56,7 +56,6 @@ TEST_F(MetricsApiTest, webRtcIceServerGetMetrics) STRNCPY(configuration.iceServers[1].urls, (PCHAR) "turns:54.202.170.151:443?transport=tcp", MAX_ICE_CONFIG_URI_LEN); STRNCPY(configuration.iceServers[1].credential, (PCHAR) "username", MAX_ICE_CONFIG_CREDENTIAL_LEN); STRNCPY(configuration.iceServers[1].username, (PCHAR) "password", MAX_ICE_CONFIG_USER_NAME_LEN); - configuration.kvsRtcConfiguration.enableIceStats = TRUE; ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); EXPECT_EQ(STATUS_ICE_SERVER_INDEX_INVALID, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); @@ -89,7 +88,6 @@ TEST_F(MetricsApiTest, webRtcIceCandidateGetMetrics) STRNCPY(configuration.iceServers[0].urls, (PCHAR) "stun:stun.kinesisvideo.us-west-2.amazonaws.com:443", MAX_ICE_CONFIG_URI_LEN); STRNCPY(configuration.iceServers[0].credential, (PCHAR) "", MAX_ICE_CONFIG_CREDENTIAL_LEN); STRNCPY(configuration.iceServers[0].username, (PCHAR) "", MAX_ICE_CONFIG_USER_NAME_LEN); - configuration.kvsRtcConfiguration.enableIceStats = TRUE; ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; From 20e6b29e37e29fce3b1eb6eaae9dca1c8a940e75 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 6 Nov 2024 10:59:03 -0800 Subject: [PATCH 11/17] Cleanup unused variable --- src/source/Metrics/Metrics.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/source/Metrics/Metrics.c b/src/source/Metrics/Metrics.c index 5dc10abf11..93a8cb030e 100644 --- a/src/source/Metrics/Metrics.c +++ b/src/source/Metrics/Metrics.c @@ -9,11 +9,6 @@ STATUS getIceCandidatePairStats(PRtcPeerConnection pRtcPeerConnection, PRtcIceCa STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; PIceAgent pIceAgent = NULL; - BOOL statsControlEnabled = FALSE; - -#ifdef ENABLE_STATS_CALCULATION_CONTROL - statsControlEnabled = TRUE; -#endif CHK((pRtcPeerConnection != NULL || pRtcIceCandidatePairStats != NULL), STATUS_NULL_ARG); pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent; From e9159aacbf4b5044b6352af92b2d78e29fb54669 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 6 Nov 2024 15:48:02 -0800 Subject: [PATCH 12/17] Rename and negate variable --- src/source/Ice/IceAgent.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index 5d75603f1f..910cbbd7ce 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -30,12 +30,12 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge PIceAgent pIceAgent = NULL; UINT32 i; UINT64 startTimeInMacro = 0; - BOOL disableStats = FALSE; + BOOL doStatCalcs = TRUE; // Ice agent stats calculations are on by default. // Runtime control for turning stats calculations on/off can be activated with this compiler flag. #ifdef ENABLE_STATS_CALCULATION_CONTROL - disableStats = !pIceAgent->kvsRtcConfiguration.enableIceStats; + doStatCalcs = pIceAgent->kvsRtcConfiguration.enableIceStats; #endif CHK(ppIceAgent != NULL && username != NULL && password != NULL && pConnectionListener != NULL, STATUS_NULL_ARG); @@ -113,7 +113,7 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge (PCHAR) pRtcConfiguration->iceServers[i].username, (PCHAR) pRtcConfiguration->iceServers[i].credential), pIceAgent->iceAgentProfileDiagnostics.iceServerParsingTime[i], "ICE server parsing"); if (STATUS_SUCCEEDED(retStatus)) { - if (!disableStats) { + if (doStatCalcs) { CHK(NULL != (pIceAgent->pRtcIceServerDiagnostics[i] = (PRtcIceServerDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceServerDiagnostics))), STATUS_NOT_ENOUGH_MEMORY); pIceAgent->pRtcIceServerDiagnostics[i]->port = (INT32) getInt16(pIceAgent->iceServers[i].ipAddress.port); @@ -136,7 +136,7 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge } } - if (!disableStats) { + if (doStatCalcs) { CHK(NULL != (pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics = (PRtcIceCandidateDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidateDiagnostics))), @@ -1083,12 +1083,12 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, PIceCandidatePair pIceCandidatePair = NULL; BOOL freeObjOnFailure = TRUE; PIceCandidate pCurrentIceCandidate = NULL; - BOOL disableStats = FALSE; + BOOL doStatCalcs = TRUE; // Ice agent stats calculations are on by default. // Runtime control for turning stats calculations on/off can be activated with this compiler flag. #ifdef ENABLE_STATS_CALCULATION_CONTROL - disableStats = !pIceAgent->kvsRtcConfiguration.enableIceStats; + doStatCalcs = pIceAgent->kvsRtcConfiguration.enableIceStats; #endif CHK(pIceAgent != NULL && pIceCandidate != NULL, STATUS_NULL_ARG); @@ -1109,7 +1109,7 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, if (pCurrentIceCandidate->state == ICE_CANDIDATE_STATE_VALID && pCurrentIceCandidate->ipAddress.family == pIceCandidate->ipAddress.family) { pIceCandidatePair = (PIceCandidatePair) MEMCALLOC(1, SIZEOF(IceCandidatePair)); CHK(pIceCandidatePair != NULL, STATUS_NOT_ENOUGH_MEMORY); - if (!disableStats) { + if (doStatCalcs) { CHK(NULL != (pIceCandidatePair->pRtcIceCandidatePairDiagnostics = (PRtcIceCandidatePairDiagnostics) MEMCALLOC(1, SIZEOF(RtcIceCandidatePairDiagnostics))), From 283d7788498a6b1e56d7a21f1d8a1ac10015f89b Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 6 Nov 2024 16:10:06 -0800 Subject: [PATCH 13/17] Move setting of enableIceStats to fix segfault --- src/source/Ice/IceAgent.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index 910cbbd7ce..0533bcf3ef 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -32,12 +32,6 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge UINT64 startTimeInMacro = 0; BOOL doStatCalcs = TRUE; -// Ice agent stats calculations are on by default. -// Runtime control for turning stats calculations on/off can be activated with this compiler flag. -#ifdef ENABLE_STATS_CALCULATION_CONTROL - doStatCalcs = pIceAgent->kvsRtcConfiguration.enableIceStats; -#endif - CHK(ppIceAgent != NULL && username != NULL && password != NULL && pConnectionListener != NULL, STATUS_NULL_ARG); CHK(STRNLEN(username, MAX_ICE_CONFIG_USER_NAME_LEN + 1) <= MAX_ICE_CONFIG_USER_NAME_LEN && STRNLEN(password, MAX_ICE_CONFIG_CREDENTIAL_LEN + 1) <= MAX_ICE_CONFIG_CREDENTIAL_LEN, @@ -89,6 +83,12 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge pIceAgent->relayCandidateCount = 0; + // Ice agent stats calculations are on by default. + // Runtime control for turning stats calculations on/off can be activated with this compiler flag. + #ifdef ENABLE_STATS_CALCULATION_CONTROL + doStatCalcs = pIceAgent->kvsRtcConfiguration.enableIceStats; + #endif + CHK_STATUS(doubleListCreate(&pIceAgent->localCandidates)); CHK_STATUS(doubleListCreate(&pIceAgent->remoteCandidates)); CHK_STATUS(doubleListCreate(&pIceAgent->iceCandidatePairs)); From 2e2cc6d176ac5d9162ac2a0ad82044d955d34b6c Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 6 Nov 2024 16:37:37 -0800 Subject: [PATCH 14/17] Revert member size savings changes (to be applied in separate PR) --- .../kinesis/video/webrtcclient/Include.h | 5 +++ .../kinesis/video/webrtcclient/Stats.h | 43 ++++++++----------- src/source/Ice/IceAgent.h | 29 +++++++------ 3 files changed, 37 insertions(+), 40 deletions(-) diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h index 535e34509f..5d3da7659c 100644 --- a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h +++ b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h @@ -460,6 +460,11 @@ extern "C" { */ #define MAX_SIGNALING_ENDPOINT_URI_LEN 512 +/** + * Maximum allowed ICE URI length + */ +#define MAX_ICE_CONFIG_URI_LEN 256 + /** * Maximum allowed correlation ID length */ diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h index ce03a869a8..72fffb67ba 100644 --- a/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h +++ b/src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h @@ -24,11 +24,6 @@ extern "C" { */ #define MAX_CANDIDATE_ID_LENGTH 9U -/** - * Maximum allowed ICE URI length - */ -#define MAX_ICE_CONFIG_URI_LEN 256 - /** * Maximum allowed relay protocol length */ @@ -68,11 +63,6 @@ extern "C" { * Maximum allowed generic length used in DOMString */ #define MAX_STATS_STRING_LENGTH 255U - -/** - * Maximum length of candidate type (host, srflx, relay, prflx, unknown) - */ -#define MAX_CANDIDATE_TYPE_LENGTH 10U /*!@} */ /** @@ -243,14 +233,14 @@ typedef struct { * Reference: https://www.w3.org/TR/webrtc-stats/#ice-server-dict* */ typedef struct { - CHAR url[MAX_ICE_CONFIG_URI_LEN + 1]; //!< STUN/TURN server URL - CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP - UINT32 iceServerIndex; //!< Ice server index to get stats from. Not available in spec! Needs to be - //!< populated by the application to get specific server stats - INT32 port; //!< Port number used by client - UINT64 totalRequestsSent; //!< Total amount of requests that have been sent to the server - UINT64 totalResponsesReceived; //!< Total number of responses received from the server - UINT64 totalRoundTripTime; //!< Sum of RTTs of all the requests for which response has been received + DOMString url; //!< STUN/TURN server URL + DOMString protocol; //!< Valid values: UDP, TCP + UINT32 iceServerIndex; //!< Ice server index to get stats from. Not available in spec! Needs to be + //!< populated by the application to get specific server stats + INT32 port; //!< Port number used by client + UINT64 totalRequestsSent; //!< Total amount of requests that have been sent to the server + UINT64 totalResponsesReceived; //!< Total number of responses received from the server + UINT64 totalRoundTripTime; //!< Sum of RTTs of all the requests for which response has been received } RtcIceServerStats, *PRtcIceServerStats; /** @@ -277,14 +267,15 @@ typedef struct { */ typedef struct { - DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained - CHAR address[IP_ADDR_STR_LENGTH + 1]; //!< IPv4 or IPv6 address of the candidate - CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP - CHAR relayProtocol[MAX_PROTOCOL_LENGTH + 1]; //!< Protocol used by endpoint to communicate with TURN server. (Only for local candidate) - //!< Valid values: UDP, TCP, TLS - INT32 priority; //!< Computed using the formula in https://tools.ietf.org/html/rfc5245#section-15.1 - INT32 port; //!< Port number of the candidate - CHAR candidateType[MAX_CANDIDATE_TYPE_LENGTH + 1]; //!< Type of local/remote ICE candidate + DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained + DOMString transportId; //!< Not used currently. ID of object that was inspected for RTCTransportStats + CHAR address[IP_ADDR_STR_LENGTH + 1]; //!< IPv4 or IPv6 address of the candidate + DOMString protocol; //!< Valid values: UDP, TCP + DOMString relayProtocol; //!< Protocol used by endpoint to communicate with TURN server. (Only for local candidate) + //!< Valid values: UDP, TCP, TLS + INT32 priority; //!< Computed using the formula in https://tools.ietf.org/html/rfc5245#section-15.1 + INT32 port; //!< Port number of the candidate + DOMString candidateType; //!< Type of local/remote ICE candidate } RtcIceCandidateStats, *PRtcIceCandidateStats; /** diff --git a/src/source/Ice/IceAgent.h b/src/source/Ice/IceAgent.h index f1b90e2a4d..098b5ca222 100644 --- a/src/source/Ice/IceAgent.h +++ b/src/source/Ice/IceAgent.h @@ -80,23 +80,24 @@ typedef struct __IceAgent* PIceAgent; * Internal structure tracking ICE server parameters for diagnostics and metrics/stats */ typedef struct { - CHAR url[MAX_ICE_CONFIG_URI_LEN + 1]; //!< STUN/TURN server URL - CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP - INT32 port; //!< Port number used by client - UINT64 totalRequestsSent; //!< Total amount of requests that have been sent to the server - UINT64 totalResponsesReceived; //!< Total number of responses received from the server - UINT64 totalRoundTripTime; //!< Sum of RTTs of all the requests for which response has been received + CHAR url[MAX_STATS_STRING_LENGTH + 1]; //!< STUN/TURN server URL + CHAR protocol[MAX_STATS_STRING_LENGTH + 1]; //!< Valid values: UDP, TCP + INT32 port; //!< Port number used by client + UINT64 totalRequestsSent; //!< Total amount of requests that have been sent to the server + UINT64 totalResponsesReceived; //!< Total number of responses received from the server + UINT64 totalRoundTripTime; //!< Sum of RTTs of all the requests for which response has been received } RtcIceServerDiagnostics, *PRtcIceServerDiagnostics; typedef struct { - DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained - CHAR address[KVS_IP_ADDRESS_STRING_BUFFER_LEN]; //!< IPv4 or IPv6 address of the candidate - CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Valid values: UDP, TCP - CHAR relayProtocol[MAX_PROTOCOL_LENGTH + 1]; //!< Protocol used by endpoint to communicate with TURN server. - //!< Valid values: UDP, TCP, TLS - CHAR candidateType[MAX_CANDIDATE_TYPE_LENGTH + 1]; //!< Type of local/remote ICE candidate - INT32 priority; //!< Computed using the formula in https://tools.ietf.org/html/rfc5245#section-15.1 - INT32 port; //!< Port number of the candidate + DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained + DOMString transportId[MAX_STATS_STRING_LENGTH + 1]; //!< ID of object that was inspected for RTCTransportStats + CHAR address[KVS_IP_ADDRESS_STRING_BUFFER_LEN]; //!< IPv4 or IPv6 address of the candidate + DOMString protocol; //!< Valid values: UDP, TCP + DOMString relayProtocol; //!< Protocol used by endpoint to communicate with TURN server. + //!< Valid values: UDP, TCP, TLS + INT32 priority; //!< Computed using the formula in https://tools.ietf.org/html/rfc5245#section-15.1 + INT32 port; //!< Port number of the candidate + DOMString candidateType; //!< Type of local/remote ICE candidate } RtcIceCandidateDiagnostics, *PRtcIceCandidateDiagnostics; typedef struct { From 85f89a8f3ed010c7686e6dc2fe01b8d779e5d068 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 6 Nov 2024 17:10:35 -0800 Subject: [PATCH 15/17] Move other instance setting of enableIceStats, clang format. --- src/source/Ice/IceAgent.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index 0533bcf3ef..0a92851efa 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -83,11 +83,11 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge pIceAgent->relayCandidateCount = 0; - // Ice agent stats calculations are on by default. - // Runtime control for turning stats calculations on/off can be activated with this compiler flag. - #ifdef ENABLE_STATS_CALCULATION_CONTROL - doStatCalcs = pIceAgent->kvsRtcConfiguration.enableIceStats; - #endif +// Ice agent stats calculations are on by default. +// Runtime control for turning stats calculations on/off can be activated with this compiler flag. +#ifdef ENABLE_STATS_CALCULATION_CONTROL + doStatCalcs = pIceAgent->kvsRtcConfiguration.enableIceStats; +#endif CHK_STATUS(doubleListCreate(&pIceAgent->localCandidates)); CHK_STATUS(doubleListCreate(&pIceAgent->remoteCandidates)); @@ -1085,15 +1085,16 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, PIceCandidate pCurrentIceCandidate = NULL; BOOL doStatCalcs = TRUE; + CHK(pIceAgent != NULL && pIceCandidate != NULL, STATUS_NULL_ARG); + CHK_WARN(pIceCandidate->state == ICE_CANDIDATE_STATE_VALID, retStatus, + "New ice candidate need to be valid to form pairs"); // Ice agent stats calculations are on by default. + // Ice agent stats calculations are on by default. // Runtime control for turning stats calculations on/off can be activated with this compiler flag. #ifdef ENABLE_STATS_CALCULATION_CONTROL doStatCalcs = pIceAgent->kvsRtcConfiguration.enableIceStats; #endif - CHK(pIceAgent != NULL && pIceCandidate != NULL, STATUS_NULL_ARG); - CHK_WARN(pIceCandidate->state == ICE_CANDIDATE_STATE_VALID, retStatus, "New ice candidate need to be valid to form pairs"); - // if pIceCandidate is a remote candidate, then form pairs with every single valid local candidate. Otherwise, // form pairs with every single valid remote candidate pDoubleList = isRemoteCandidate ? pIceAgent->localCandidates : pIceAgent->remoteCandidates; @@ -1109,6 +1110,7 @@ STATUS createIceCandidatePairs(PIceAgent pIceAgent, PIceCandidate pIceCandidate, if (pCurrentIceCandidate->state == ICE_CANDIDATE_STATE_VALID && pCurrentIceCandidate->ipAddress.family == pIceCandidate->ipAddress.family) { pIceCandidatePair = (PIceCandidatePair) MEMCALLOC(1, SIZEOF(IceCandidatePair)); CHK(pIceCandidatePair != NULL, STATUS_NOT_ENOUGH_MEMORY); + if (doStatCalcs) { CHK(NULL != (pIceCandidatePair->pRtcIceCandidatePairDiagnostics = From c37e24cda79870bbcd13e6c2b0929bc28f8bebd9 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 6 Nov 2024 19:59:46 -0800 Subject: [PATCH 16/17] Fix setting of enableIceStats in tests --- tst/MetricsApiTest.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tst/MetricsApiTest.cpp b/tst/MetricsApiTest.cpp index ada13a3863..656d45a68c 100644 --- a/tst/MetricsApiTest.cpp +++ b/tst/MetricsApiTest.cpp @@ -21,6 +21,9 @@ TEST_F(MetricsApiTest, webRtcGetMetrics) EXPECT_EQ(STATUS_NULL_ARG, rtcPeerConnectionGetMetrics(NULL, NULL, &rtcMetrics)); MEMSET(&configuration, 0x00, SIZEOF(RtcConfiguration)); +#ifdef ENABLE_STATS_CALCULATION_CONTROL + configuration.kvsRtcConfiguration.enableIceStats = TRUE; +#endif ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); @@ -49,6 +52,9 @@ TEST_F(MetricsApiTest, webRtcIceServerGetMetrics) rtcIceMetrics.rtcStatsObject.iceServerStats.iceServerIndex = 5; MEMSET(&configuration, 0x00, SIZEOF(RtcConfiguration)); +#ifdef ENABLE_STATS_CALCULATION_CONTROL + configuration.kvsRtcConfiguration.enableIceStats = TRUE; +#endif STRNCPY(configuration.iceServers[0].urls, (PCHAR) "stun:stun.kinesisvideo.us-west-2.amazonaws.com:443", MAX_ICE_CONFIG_URI_LEN); STRNCPY(configuration.iceServers[0].credential, (PCHAR) "", MAX_ICE_CONFIG_CREDENTIAL_LEN); STRNCPY(configuration.iceServers[0].username, (PCHAR) "", MAX_ICE_CONFIG_USER_NAME_LEN); @@ -85,6 +91,9 @@ TEST_F(MetricsApiTest, webRtcIceCandidateGetMetrics) RtcStats rtcIceMetrics; MEMSET(&configuration, 0x00, SIZEOF(RtcConfiguration)); +#ifdef ENABLE_STATS_CALCULATION_CONTROL + configuration.kvsRtcConfiguration.enableIceStats = TRUE; +#endif STRNCPY(configuration.iceServers[0].urls, (PCHAR) "stun:stun.kinesisvideo.us-west-2.amazonaws.com:443", MAX_ICE_CONFIG_URI_LEN); STRNCPY(configuration.iceServers[0].credential, (PCHAR) "", MAX_ICE_CONFIG_CREDENTIAL_LEN); STRNCPY(configuration.iceServers[0].username, (PCHAR) "", MAX_ICE_CONFIG_USER_NAME_LEN); @@ -177,7 +186,7 @@ TEST_F(MetricsApiTest, webRtcIceServerGetMetrics_IceStatsControlOn_Disabled) ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection)); rtcIceMetrics.rtcStatsObject.iceServerStats.iceServerIndex = 1; - EXPECT_EQ(STATUS_SUCCESS, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); + EXPECT_EQ(STATUS_INVALID_OPERATION, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics)); EXPECT_EQ(0, rtcIceMetrics.rtcStatsObject.iceServerStats.port); EXPECT_EQ('\0', rtcIceMetrics.rtcStatsObject.iceServerStats.url[0]); EXPECT_EQ('\0', rtcIceMetrics.rtcStatsObject.iceServerStats.protocol[0]); From f1e3dd6e224447705cc6c512cdd149c8d130ac77 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Fri, 8 Nov 2024 10:37:13 -0800 Subject: [PATCH 17/17] Address comment --- src/source/Ice/IceAgent.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c index 0a92851efa..a9d3a066a8 100644 --- a/src/source/Ice/IceAgent.c +++ b/src/source/Ice/IceAgent.c @@ -273,7 +273,7 @@ STATUS freeIceAgent(PIceAgent* ppIceAgent) freeTransactionIdStore(&pIceAgent->pStunBindingRequestTransactionIdStore); } - hashTableFree(pIceAgent->requestTimestampDiagnostics); + CHK_LOG_ERR(hashTableFree(pIceAgent->requestTimestampDiagnostics)); SAFE_MEMFREE(pIceAgent->pRtcSelectedLocalIceCandidateDiagnostics); SAFE_MEMFREE(pIceAgent->pRtcSelectedRemoteIceCandidateDiagnostics); for (i = 0; i < MAX_ICE_SERVERS_COUNT; i++) {