Reduce hash table size for SDP transceivers (#2067)

* Reduce hash table size for SDP transceivers (#1914) * Modify hash table sizes * Add logs * Fix hash table size to be set according to number of m-lines and unique supported codecs * CI branch * Update actions to use node20 * Unit test for fake transceiver * Cleanup * Fix build failure * Fix unit test * Initialize to 0 * PR description check: Reduce minimum character count and print the current character count * Use PR number instead of manually constructing link, add fetch error logging * Print out the PR link * Debug the CI * Fix the extraction logic * Clang-format --------- Co-authored-by: Divya Sampath Kumar <[email protected]>
awslabs · Oct 29, 2024 · 6ba8728 · 6ba8728
1 parent 30f52d1
commit 6ba8728
Show file tree

Hide file tree

Showing 8 changed files with 266 additions and 61 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -14,7 +14,7 @@ jobs:
     runs-on: macos-12
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Install clang-format
         run: |
           brew install clang-format
@@ -33,9 +33,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -59,9 +59,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -84,9 +84,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -112,9 +112,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -140,9 +140,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -175,9 +175,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -206,7 +206,7 @@ jobs:
   #     AWS_KVS_LOG_LEVEL: 2
   #   steps:
   #     - name: Clone repository
-  #       uses: actions/checkout@v3
+  #       uses: actions/checkout@v4
   #     - name: Install dependencies
   #       run: |
   #         sudo apt clean && sudo apt update
@@ -231,9 +231,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -263,9 +263,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -304,7 +304,7 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Install dependencies
         run: |
           apk update
@@ -324,9 +324,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -360,9 +360,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -396,9 +396,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -429,9 +429,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -512,9 +512,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -542,9 +542,9 @@ jobs:
       contents: read
     steps:
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Configure AWS Credentials
-        uses: aws-actions/configure-aws-credentials@v2
+        uses: aws-actions/configure-aws-credentials@v4
         with:
           role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
           aws-region: ${{ secrets.AWS_REGION }}
@@ -592,7 +592,7 @@ jobs:
   #     AWS_KVS_LOG_LEVEL: 7
   #   steps:
   #     - name: Clone repository
-  #       uses: actions/checkout@v3
+  #       uses: actions/checkout@v4
   #     - name: Move cloned repo
   #       shell: powershell
   #       run: |
@@ -625,7 +625,7 @@ jobs:
           sudo apt clean && sudo apt update
           sudo apt-get -y install gcc-aarch64-linux-gnu g++-aarch64-linux-gnu binutils-aarch64-linux-gnu
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Build Repository
         run: |
           sudo sh -c 'echo 0 > /proc/sys/net/ipv6/conf/all/disable_ipv6'
@@ -643,7 +643,7 @@ jobs:
           sudo apt clean && sudo apt update
           sudo apt-get -y install gcc-aarch64-linux-gnu g++-aarch64-linux-gnu binutils-aarch64-linux-gnu
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Build Repository
         run: |
           sudo sh -c 'echo 0 > /proc/sys/net/ipv6/conf/all/disable_ipv6'
@@ -661,7 +661,7 @@ jobs:
           sudo apt clean && sudo apt update
           sudo apt-get -y install gcc-arm-linux-gnueabi g++-arm-linux-gnueabi binutils-arm-linux-gnueabi
       - name: Clone repository
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
       - name: Build Repository
         run: |
           sudo sh -c 'echo 0 > /proc/sys/net/ipv6/conf/all/disable_ipv6'

diff --git a/.github/workflows/pr-desc-lint.yml b/.github/workflows/pr-desc-lint.yml
@@ -23,34 +23,43 @@ jobs:
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         run: |
-          pr_description=$(gh pr view https://github.com/${GITHUB_REPOSITORY}/pull/${{ github.event.pull_request.number }} --json body -q ".body")
-          error_occurred=0
+          pr_link="https://github.com/${GITHUB_REPOSITORY}/pull/${{ github.event.pull_request.number }}"
+          
+          echo "$pr_link"
+          
+          pr_description=$(gh pr view $pr_link --json body -q ".body")
+          
+          if [ -z "$pr_description" ]; then
+            echo "Failed to fetch the PR description"
+            exit 1
+          fi
+          
           # Define minimum character count for each section
-          MIN_CHARS=25
+          MIN_CHARS=10
 
           # Extract contents
-          # Extract contents
-          what_changed=$(echo "$pr_description" | sed -n -e '/\*What was changed?\*/,/\*/p' | sed '$d' | sed '1d')
-          why_changed=$(echo "$pr_description" | sed -n -e '/\*Why was it changed?\*/,/\*/p' | sed '$d' | sed '1d')
-          how_changed=$(echo "$pr_description" | sed -n -e '/\*How was it changed?\*/,/\*/p' | sed '$d' | sed '1d')
-          testing_done=$(echo "$pr_description" | sed -n -e '/\*What testing was done for the changes?\*/,/\*/p' | sed '$d' | sed '1d')
-
+          what_changed=$(echo "$pr_description" | sed -n -e '/\*What was changed?\*/,/\*Why was it changed?\*/p' | sed '$d' | sed '1d')
+          why_changed=$(echo "$pr_description" | sed -n -e '/\*Why was it changed?\*/,/\*How was it changed?\*/p' | sed '$d' | sed '1d')
+          how_changed=$(echo "$pr_description" | sed -n -e '/\*How was it changed?\*/,/\*What testing was done for the changes?\*/p' | sed '$d' | sed '1d')
+          testing_done=$(echo "$pr_description" | sed -n -e '/\*What testing was done for the changes?\*/,/By submitting this pull request/p' | sed '$d' | sed '1d')
+  
+          error_occurred=0
           if [[ ${#what_changed} -lt $MIN_CHARS ]]; then
-            echo "PR description for what changed section is either missing or too short."
+            echo "PR description for what changed section is either missing or too short. Required: ${MIN_CHARS}, Current: ${what_changed}"
             error_occurred=1
           fi
           if [[ ${#why_changed} -lt $MIN_CHARS ]]; then
-            echo "PR description for why it changed section is either missing or too short."
+            echo "PR description for why it changed section is either missing or too short. Required: ${MIN_CHARS}, Current: ${why_changed}"
             error_occurred=1
           fi
           if [[ ${#how_changed} -lt $MIN_CHARS ]]; then
-            echo "PR description for how was it changed section is either missing or too short."
+            echo "PR description for how was it changed section is either missing or too short. Required: ${MIN_CHARS}, Current: ${how_changed}"
             error_occurred=1
           fi
           if [[ ${#testing_done} -lt $MIN_CHARS ]]; then
-            echo "PR description for testing section are either missing or too short."
+            echo "PR description for testing section are either missing or too short. Required: ${MIN_CHARS}, Current: ${testing_done}"
             error_occurred=1
           fi
           if [[ $error_occurred -eq 1 ]]; then
-          exit 1
+            exit 1
           fi
diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h
@@ -836,7 +836,6 @@ typedef enum {
     RTC_CODEC_ALAW = 5,                                                           //!< ALAW audio codec
     RTC_CODEC_UNKNOWN = 6,
     RTC_CODEC_H265 = 7, //!< H265 video codec
-
     // RTC_CODEC_MAX **MUST** be the last enum in the list **ALWAYS** and not assigned a value
     RTC_CODEC_MAX //!< Placeholder for max number of supported codecs
 } RTC_CODEC;

diff --git a/src/source/Ice/IceAgent.c b/src/source/Ice/IceAgent.c
@@ -90,7 +90,7 @@ STATUS createIceAgent(PCHAR username, PCHAR password, PIceAgentCallbacks pIceAge
 
     // Pre-allocate stun packets
 
-    // no other attribtues needed: https://tools.ietf.org/html/rfc8445#section-11
+    // no other attributes needed: https://tools.ietf.org/html/rfc8445#section-11
     CHK_STATUS(createStunPacket(STUN_PACKET_TYPE_BINDING_INDICATION, NULL, &pIceAgent->pBindingIndication));
     CHK_STATUS(hashTableCreateWithParams(ICE_HASH_TABLE_BUCKET_COUNT, ICE_HASH_TABLE_BUCKET_LENGTH, &pIceAgent->requestTimestampDiagnostics));
 

diff --git a/src/source/PeerConnection/SessionDescription.c b/src/source/PeerConnection/SessionDescription.c
@@ -958,6 +958,7 @@ STATUS populateSessionDescriptionMedia(PKvsPeerConnection pKvsPeerConnection, PS
     PCHAR pDtlsRole = NULL;
     PHashTable pUnknownCodecPayloadTypesTable = NULL, pUnknownCodecRtpmapTable = NULL;
     UINT32 unknownCodecHashTableKey = 0;
+    UINT32 unknownHashTableBucketCount = 0;
 
     CHK_STATUS(dtlsSessionGetLocalCertificateFingerprint(pKvsPeerConnection->pDtlsSession, certificateFingerprint, CERTIFICATE_FINGERPRINT_LENGTH));
 
@@ -982,8 +983,12 @@ STATUS populateSessionDescriptionMedia(PKvsPeerConnection pKvsPeerConnection, PS
         }
     } else {
         pDtlsRole = DTLS_ROLE_ACTIVE;
-        CHK_STATUS(hashTableCreate(&pUnknownCodecPayloadTypesTable));
-        CHK_STATUS(hashTableCreate(&pUnknownCodecRtpmapTable));
+        unknownHashTableBucketCount =
+            pRemoteSessionDescription->mediaCount < MIN_HASH_BUCKET_COUNT ? MIN_HASH_BUCKET_COUNT : pRemoteSessionDescription->mediaCount;
+        CHK_STATUS(hashTableCreateWithParams(unknownHashTableBucketCount, CODEC_RTPMAP_PAYLOAD_TYPES_HASH_TABLE_BUCKET_LENGTH,
+                                             &pUnknownCodecPayloadTypesTable));
+        CHK_STATUS(
+            hashTableCreateWithParams(unknownHashTableBucketCount, CODEC_RTPMAP_PAYLOAD_TYPES_HASH_TABLE_BUCKET_LENGTH, &pUnknownCodecRtpmapTable));
 
         // this function creates a list of transceivers corresponding to each m-line and adds it answerTransceivers
         // if an m-line does not have a corresponding transceiver created by the user, we create a fake transceiver
@@ -1184,18 +1189,21 @@ STATUS findTransceiversByRemoteDescription(PKvsPeerConnection pKvsPeerConnection
     PRtcMediaStreamTrack pRtcMediaStreamTrack;
     RtcMediaStreamTrack track;
 
-    CHK_STATUS(hashTableCreate(&pSeenTransceivers)); // to be used by findCodecInTransceivers
+    // pSeenTranceivers is populated only with codec types supported by the SDK. And if already populated, it is not added again.
+    // Hence, it is sufficient if the hash table count is set to minimum or required transceivers
+    UINT32 seenTranceiversHashBucketCnt = RTC_CODEC_MAX < MIN_HASH_BUCKET_COUNT ? MIN_HASH_BUCKET_COUNT : RTC_CODEC_MAX;
+    CHK_STATUS(hashTableCreateWithParams(seenTranceiversHashBucketCnt, CODEC_RTPMAP_PAYLOAD_TYPES_HASH_TABLE_BUCKET_LENGTH, &pSeenTransceivers));
 
     // sample m-lines
     // m=audio 9 UDP/TLS/RTP/SAVPF 111 63 103 104 9 0 8 106 105 13 110 112 113 126
     // m=video 9 UDP/TLS/RTP/SAVPF 96 97 98 99 100 101 102 121 127 120 125 107 108 109 124 119 123 117 35 36 114 115 116 62 118
     // this loop iterates over all the m-lines
+
     for (currentMedia = 0; currentMedia < pRemoteSessionDescription->mediaCount; currentMedia++) {
         pMediaDescription = &(pRemoteSessionDescription->mediaDescriptions[currentMedia]);
         foundMediaSectionWithCodec = FALSE;
         count = 0;
         MEMSET(firstCodec, 0x00, MAX_PAYLOAD_TYPE_LENGTH);
-
         // Scan the media section name for any codecs we support
         // sample attributeValue=audio 9 UDP/TLS/RTP/SAVPF 111 63 103 104 9 0 8 106 105 13 110 112 113 126
         attributeValue = pMediaDescription->mediaName;
@@ -1244,7 +1252,6 @@ STATUS findTransceiversByRemoteDescription(PKvsPeerConnection pKvsPeerConnection
                 attributeValue = end + 1;
             }
         } while (end != NULL && !foundMediaSectionWithCodec);
-
         // get the first payload type from codecs in case we need to use it to generate a fake transceiver to respond to an m-line
         // if we don't have a user-created one corresponding to an m-line
         // we can respond to an m-line by including any one codec the offer had
@@ -1261,7 +1268,6 @@ STATUS findTransceiversByRemoteDescription(PKvsPeerConnection pKvsPeerConnection
         for (currentAttribute = 0; currentAttribute < pMediaDescription->mediaAttributesCount && !foundMediaSectionWithCodec; currentAttribute++) {
             attributeValue = pMediaDescription->sdpAttributes[currentAttribute].attributeValue;
             rtcCodec = RTC_CODEC_UNKNOWN;
-
             // check for supported codec in rtpmap values only if an a-line contains the keyword "rtpmap" to save string comparisons
             if (STRNCMP(RTPMAP_VALUE, pMediaDescription->sdpAttributes[currentAttribute].attributeName, 6) == 0) {
                 if (STRSTR(attributeValue, H264_VALUE) != NULL) {
@@ -1324,7 +1330,6 @@ STATUS findTransceiversByRemoteDescription(PKvsPeerConnection pKvsPeerConnection
             CHK_STATUS(doubleListInsertItemTail(pKvsPeerConnection->pAnswerTransceivers, (UINT64) pKvsRtpFakeTransceiver));
 
             CHK_STATUS(STRTOUI32(firstCodec, firstCodec + tokenLen, 10, &codec));
-
             // Insert (int)(firstCodec) and rtpMapValue into the hashtables with the same key so they can be retrieved later during serialization
             CHK_STATUS(hashTableContains(pUnknownCodecPayloadTypesTable, (UINT64) codec, &containsPayloadType));
             CHK_STATUS(hashTableContains(pUnknownCodecRtpmapTable, (UINT64) rtpMapValue, &containsRtpMap));
@@ -1339,7 +1344,6 @@ STATUS findTransceiversByRemoteDescription(PKvsPeerConnection pKvsPeerConnection
     }
 
 CleanUp:
-
     CHK_STATUS(hashTableFree(pSeenTransceivers));
     CHK_LOG_ERR(retStatus);
 

diff --git a/src/source/PeerConnection/SessionDescription.h b/src/source/PeerConnection/SessionDescription.h
@@ -83,6 +83,8 @@ extern "C" {
 #define TWCC_SDP_ATTR "transport-cc"
 #define TWCC_EXT_URL  "http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01"
 
+#define CODEC_RTPMAP_PAYLOAD_TYPES_HASH_TABLE_BUCKET_LENGTH 2
+
 STATUS setPayloadTypesFromOffer(PHashTable, PHashTable, PSessionDescription);
 STATUS setPayloadTypesForOffer(PHashTable);