Refactor TWCC #1934

disa6302 · 2024-02-23T20:27:48Z

Issue #, if available:

What was changed?

Refactored TWCC implementation to reduce memory consumption.

Why was it changed?

The current TWCC implementation consumes 1.5MB of memory and is enabled by default. While this provides a good cache locality, the SDK need not store 65536 RTP packet information in memory since TWCC reports come in periodically (every few milliseconds based on tests). Even if TWCC reports are not received, the RTP packet information should be removed from memory on a rolling time window basis.

Before this PR, the max heap allocation observed is 4.05 MB

After this PR, the max heap allocation observed is 2.53MB

As seen, the implementation change leads to almost getting rid of all the allocation (1.5MB) in the best case scenario.

How was it changed?

Switched to using a hash map of TWCC info packets from an array for 65536 TWCC packet. The changes also make sure the TWCC info packets are deleted once the packet is confirmed as received as per TWCC reports. Current implementation did not need that
The rolling window in the earlier implementation used a stack queue. However, since the TWCC info packets always lived in memory, this was not necessary. The current implementation uses a variable to track last checked packet in the rolling window.

What testing was done for the changes?

Ran the sample by disabling rolling window related changes to confirm deletion of TWCC info packets as and when TWCC packets are received.
Ran the sample by disabling the deletion post receive changes to confirm rolling window approach works as expected.
Modified the unit test to accommodate for the new implementation.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov-commenter · 2024-02-23T20:56:37Z

Codecov Report

Attention: Patch coverage is 59.29204% with 46 lines in your changes are missing coverage. Please review.

Project coverage is 75.53%. Comparing base (7be31c1) to head (8a5f4c1).

❗ Current head 8a5f4c1 differs from pull request most recent head 3794c3b. Consider uploading reports for the commit 3794c3b to get more accurate results

Files	Patch %	Lines
src/source/PeerConnection/Rtcp.c	39.28%	34 Missing ⚠️
src/source/PeerConnection/PeerConnection.c	81.81%	10 Missing ⚠️
samples/Common.c	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1934      +/-   ##
===========================================
+ Coverage    75.28%   75.53%   +0.25%     
===========================================
  Files           49       49              
  Lines        13871    13928      +57     
===========================================
+ Hits         10443    10521      +78     
+ Misses        3428     3407      -21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/source/PeerConnection/PeerConnection.c

jdelapla · 2024-03-04T21:13:08Z

After this PR, the max heap allocation observed is 2.53MB

Is this with TWCC enabled?

disa6302 · 2024-03-04T21:16:09Z

After this PR, the max heap allocation observed is 2.53MB

Is this with TWCC enabled?

Yes. It is with TWCC enabled. Not disabling TWCC by default in the PR.

README.md

src/source/PeerConnection/PeerConnection.c

src/source/PeerConnection/Rtcp.c

src/source/PeerConnection/PeerConnection.c

README.md

hassanctech

lgtm

src/source/PeerConnection/Rtcp.c

tst/RtcpFunctionalityTest.cpp

hassanctech

nice!

hassanctech

lgtm

* Remove stackqueue usage * Use hashtable instead..working logic * Cleanup, increase hash table size, fix loop bug * method 2, array of pointers * Add rolling window * rolling window with hashtable * hash table with rw * Fix bug * Fix twcc unit test * Cleanup rw logic * Cleanup * Cleanup logic * Update README * unused var fix * Use defines for hash table size * Address comments, disable TWCC by default * readme * Fix windows gst issue * Comments

* Refactor TWCC (#1934) * Remove stackqueue usage * Use hashtable instead..working logic * Cleanup, increase hash table size, fix loop bug * method 2, array of pointers * Add rolling window * rolling window with hashtable * hash table with rw * Fix bug * Fix twcc unit test * Cleanup rw logic * Cleanup * Cleanup logic * Update README * unused var fix * Use defines for hash table size * Address comments, disable TWCC by default * readme * Fix windows gst issue * Comments * Remove enableIceStats merge conflict * Address comments * Fix left over typo * Address comments, refactor updaing of TWCC hash table in onRtcpTwccPacket for testing * Clang format, fix compiler Werror * Correct typos * Fix "expression result unused" Werrors * Remove unused variable * Address comments * Fix endless loop * Address comment * Add removal of null hashTable items, add test for null item in hashTable * Address comments * Cleanup freeing of KvsPeerConnection * Clang format --------- Co-authored-by: Divya Sampath Kumar <disa6302@colorado.edu>

* 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 <disa6302@colorado.edu> * Add job to run the sample and check for memory leak (#2071) * Add job to run the sample and check for memory leak * Simulate a memory leak * MEMALLOC instead of MALLOC * Remove simulated memory leak * Print the exit statuses in case of failure * Move offer from stack to heap (#2070) * Ensure remote description does not live in memory for viewer duration (#1915) * Ensure remote description does not live in memory for viewer duration * Fix memory leak * Free the allocated memory upon failure, also add missing enter, leave and chk_log_error in peerconnection * Working reduced stack sizes. Adjusted default stack size to 64kb, moved Offer/Answer messages to be on heap instead of stack * Revert setting the stack sizes by default; Samples: revert disableSenderSideBandwidthEstimation to TRUE by default * Add missing malloc out of memory checks * Undo samples/Common.c changes * Add chk_log_err to freeJitterBuffer and freeKvsRtpTransceiver * Address comments: add additional null checks and adjust comments * Clang-format --------- Co-authored-by: Divya Sampath Kumar <disa6302@colorado.edu> Co-authored-by: James Delaplane <delaplan@amazon.com> * Memory Optimization - ICE Agent Stats (#1947) (#2074) * 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 ad7d02f. * Revert "Use dyanmic allocation and flag for ice stats" This reverts commit bf9a2ee. * Working version * enable flag in samples * Add unit test * Fix bug * README for the flag * Update readme * Fix readme typos * clang format * Add ENABLE_STATS_CALCULATION_CONTROL CMake option and compiler flag. Update ReadMe. * Add IceStatsControlOn tests * Add CI test forENABLE_STATS_CALCULATION_CONTROL=TRUE * Clang formatting * Remove unused iceAgentAddConfig * Remove unintended readme merge changes "Controlling RTP rolling buffer capacity" * Address comments, add null check * Cleanup unused variable * Rename and negate variable * Move setting of enableIceStats to fix segfault * Revert member size savings changes (to be applied in separate PR) * Move other instance setting of enableIceStats, clang format. * Fix setting of enableIceStats in tests * Address comment --------- Co-authored-by: Divya Sampath Kumar <disa6302@colorado.edu> * Memory Optimization - Refactor TWCC (#1934) (#2075) * Refactor TWCC (#1934) * Remove stackqueue usage * Use hashtable instead..working logic * Cleanup, increase hash table size, fix loop bug * method 2, array of pointers * Add rolling window * rolling window with hashtable * hash table with rw * Fix bug * Fix twcc unit test * Cleanup rw logic * Cleanup * Cleanup logic * Update README * unused var fix * Use defines for hash table size * Address comments, disable TWCC by default * readme * Fix windows gst issue * Comments * Remove enableIceStats merge conflict * Address comments * Fix left over typo * Address comments, refactor updaing of TWCC hash table in onRtcpTwccPacket for testing * Clang format, fix compiler Werror * Correct typos * Fix "expression result unused" Werrors * Remove unused variable * Address comments * Fix endless loop * Address comment * Add removal of null hashTable items, add test for null item in hashTable * Address comments * Cleanup freeing of KvsPeerConnection * Clang format --------- Co-authored-by: Divya Sampath Kumar <disa6302@colorado.edu> * Shorten Stats String Lengths (#2079) * Memory Optimization - Rolling Buffer (#2077) * Rb size config (#1921) * Reduce default size * Rolling buffer configurability * Add configurability * Allow setting parameter through trx instead of config * Add UT, fix the members to DOUBLE * Remove unnecessary fields * Rolling buffer readme update * Use enums for min values * Add usage example in samples * Fix unit test * Memset init in UTs * Modify the bitrates in the samples based on codec used in our samples * Modify unit tests, add video/audio specific defaults * Add missing null check * Readme update * Address nits * Clang fix * Logs for RB * set explicit type cast * add max values * Refactor RB logic, readme update, sample cleanup * nits * Fix unit tests * return * update description in header * fix unused var * minor changes * typo fix * review comments * sdp, stun, rtp * comments * cancelled builds * log status * sctp * Revert unrelated sample changes * Revert unrelated CMake changes * Revert unrelated test changes * Revert unrelated ReadMe changes * Fix DEFAULT_MTU_SIZE variable name * Cleanup merge changes * Fixup and comment on Rtp.c * Fixup Rtp.h * Fixup ReadMe * Clang format * Revert all sample changes * Address comments * Add back accidentally deleted line * Address comment * Update README.md --------- Co-authored-by: Divya Sampath Kumar <disa6302@colorado.edu> * Add option to specify stack size to pthread_create (#2073) * Thread stack sizes (#1924) * Working reduced stack sizes. Adjusted default stack size to 64kb, moved Offer/Answer messages to be on heap instead of stack * Custom stack sizes in Common.c * CMake flag working and added to README * Adjust connection listener stack size for datachannels * Change stack size for media related threads and connection listener again. Add README notes to highlight this potential problem threads for other applications * Unnecessary memset * nit picks * Update CMakeLists.txt fix for ZLIB since it is not found on Windows, that way we can verify the stack side change on Windows platform, previously it was not running at all due to build fail. * Update CMake dependency back to develop since the other PRs have merged * README update --------- Co-authored-by: Hassan Sahibzada <hsahibza@amazon.com> Co-authored-by: Divya Sampath Kumar <disa6302@colorado.edu> * Stack size configurablility * Revert setting specific thread stack sizes in the sample and SDK * Check develop branch * Check stack size is forwarded to PIC correctly * Set e * Remove duplicate mkdir build * Print out CMake logs in case of error * Missing fi * Remove square brackets * Dont build samples * Build samples * Pass the stack size down and dont build samples * Remove constrained device * Bump kvsCommonLws from 1.5.4RC to 1.5.4 * Add v * Update README.md --------- Co-authored-by: jdelapla <delaplan@amazon.com> Co-authored-by: Hassan Sahibzada <hsahibza@amazon.com> Co-authored-by: Divya Sampath Kumar <disa6302@colorado.edu> * SDP Attribute Fixes (#2082) * fix nack pli overwrite bug (#2035) * fix nack pli overwrite bug * create generic strings instead of hard coding * remove stale comment in tests * Check that the sdp lines are not missing, append SDP_LINE_SEPARATOR to lines checked * Remove duplicate session attribute lines, add missing attribute name * Clang format --------- Co-authored-by: Hassan Sahibzada <hsahibza@amazon.com> * ReadMe Fixes (#2083) * Add copy code buttons to github clone commands (#1928) * Add copy code buttons to github clone commands * Add language specifier * build -> built (#1925) * Update clone command * Remove Duplicate ReadMe Lines (#2085) * Update README.md * Update make commad * Bump project version in CMake (#2086) * Upgrade to macos-13 and 15, use DRY for Mac jobs (#2090) * Upgrade to macos-13 and 15, use DRY for Mac jobs, use docker for ubuntu jobs * Revert Ubuntu docker changes * Remove Mac+GCC+Tests combo * Install GStreamer for the mac jobs to check the GStreamer sample builds * Remove unnecessary installs * Build and run the tests without AWS SDK * Add D * Samples: Fit the RTP transceiver rolling buffers to fit the set of sample frames (#2089) * Properly fit the rolling buffers to fit the set of sample frames * Clang-format * Adjust the comment for the rolling buffer configuration for the sample frames * Bump the version from 1.11.1 to 1.12.0 (#2092) * Specify configureTransceiverRollingBuffer as a Public API --------- Co-authored-by: sirknightj <jggunawa@amazon.com> Co-authored-by: Divya Sampath Kumar <disa6302@colorado.edu> Co-authored-by: James Delaplane <delaplan@amazon.com> Co-authored-by: Hassan Sahibzada <hsahibza@amazon.com>

disa6302 force-pushed the twcc-resize branch 3 times, most recently from 028a453 to 3bc1961 Compare February 23, 2024 22:51

disa6302 marked this pull request as ready for review February 23, 2024 23:56

sirknightj added memory-optimization enhancement New feature or request labels Feb 24, 2024

stefankiesz reviewed Mar 4, 2024

View reviewed changes

src/source/PeerConnection/PeerConnection.c Outdated Show resolved Hide resolved

src/source/PeerConnection/PeerConnection.c Outdated Show resolved Hide resolved

jdelapla suggested changes Mar 4, 2024

View reviewed changes

disa6302 force-pushed the twcc-resize branch from e2c3d05 to 33cc801 Compare March 4, 2024 23:01

hassanctech reviewed Mar 5, 2024

View reviewed changes

src/source/PeerConnection/PeerConnection.c Outdated Show resolved Hide resolved

disa6302 force-pushed the twcc-resize branch from 33cc801 to cf13ace Compare March 5, 2024 17:38

disa6302 requested review from hassanctech, jdelapla and stefankiesz March 5, 2024 18:08

sirknightj reviewed Mar 8, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

jdelapla previously approved these changes Mar 11, 2024

View reviewed changes

disa6302 added 11 commits March 18, 2024 16:09

Remove stackqueue usage

0f447ae

Use hashtable instead..working logic

70d05ac

Cleanup, increase hash table size, fix loop bug

1e37769

method 2, array of pointers

773369e

Add rolling window

04672cc

rolling window with hashtable

9ae17d7

hash table with rw

025fb71

Fix bug

0a91aa8

Fix twcc unit test

16516ab

Cleanup rw logic

5b05689

Cleanup

339cf07

disa6302 added 4 commits March 18, 2024 16:09

Update README

70e209f

unused var fix

aaea649

Use defines for hash table size

093e9b5

Address comments, disable TWCC by default

68df911

disa6302 dismissed jdelapla’s stale review via 68df911 March 18, 2024 23:11

disa6302 force-pushed the twcc-resize branch from 4dfda81 to 68df911 Compare March 18, 2024 23:11

hassanctech reviewed Mar 20, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

hassanctech previously approved these changes Mar 20, 2024

View reviewed changes

readme

4bc5816

disa6302 dismissed hassanctech’s stale review via 4bc5816 March 20, 2024 17:26

Fix windows gst issue

cbf7acf

hassanctech reviewed Mar 21, 2024

View reviewed changes

src/source/PeerConnection/Rtcp.c Outdated Show resolved Hide resolved

hassanctech reviewed Mar 21, 2024

View reviewed changes

tst/RtcpFunctionalityTest.cpp Show resolved Hide resolved

hassanctech reviewed Mar 21, 2024

View reviewed changes

tst/RtcpFunctionalityTest.cpp Show resolved Hide resolved

hassanctech reviewed Mar 21, 2024

View reviewed changes

tst/RtcpFunctionalityTest.cpp Outdated Show resolved Hide resolved

hassanctech previously approved these changes Mar 21, 2024

View reviewed changes

disa6302 dismissed hassanctech’s stale review via 2ef577f March 21, 2024 15:47

disa6302 force-pushed the twcc-resize branch 3 times, most recently from e0e5170 to cdbab98 Compare March 21, 2024 16:12

Comments

3794c3b

disa6302 force-pushed the twcc-resize branch from cdbab98 to 3794c3b Compare March 21, 2024 16:47

hassanctech approved these changes Mar 21, 2024

View reviewed changes

jdelapla approved these changes Mar 21, 2024

View reviewed changes

disa6302 merged commit f6bc9fc into develop Mar 21, 2024
30 checks passed

disa6302 deleted the twcc-resize branch March 21, 2024 18:12

stefankiesz mentioned this pull request Nov 5, 2024

Memory Optimization - Refactor TWCC (#1934) #2075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor TWCC #1934

Refactor TWCC #1934

disa6302 commented Feb 23, 2024

codecov-commenter commented Feb 23, 2024 •

edited

Loading

jdelapla commented Mar 4, 2024

disa6302 commented Mar 4, 2024

hassanctech left a comment

hassanctech left a comment

hassanctech left a comment

Refactor TWCC #1934

Refactor TWCC #1934

Conversation

disa6302 commented Feb 23, 2024

codecov-commenter commented Feb 23, 2024 • edited Loading

Codecov Report

jdelapla commented Mar 4, 2024

disa6302 commented Mar 4, 2024

hassanctech left a comment

Choose a reason for hiding this comment

hassanctech left a comment

Choose a reason for hiding this comment

hassanctech left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 23, 2024 •

edited

Loading