Default stack size change and Rolling buffer config rework #2011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2011 +/- ##
===========================================
- Coverage 88.68% 88.66% -0.02%
===========================================
Files 48 48
Lines 12726 12741 +15
===========================================
+ Hits 11286 11297 +11
- Misses 1440 1444 +4 ☔ View full report in Codecov by Sentry. |
disa6302
changed the title
niyatim23
reviewed
Jun 17, 2024
niyatim23
reviewed
Jun 17, 2024
niyatim23
reviewed
Jun 17, 2024
stefankiesz
reviewed
Jun 18, 2024
sirknightj
reviewed
Jun 18, 2024
stefankiesz
previously approved these changes
Jun 18, 2024
sirknightj
reviewed
Jun 19, 2024
sirknightj
reviewed
Jun 19, 2024
sirknightj
reviewed
Jun 19, 2024
sirknightj
reviewed
Jun 19, 2024
sirknightj
reviewed
Jun 19, 2024
disa6302
force-pushed
the
add-logs
branch
3 times, most recently
from
4c1e457 to
c589984
Compare
sirknightj
reviewed
Jul 1, 2024
| pKvsRtpTransceiver->pRollingBufferConfig->rollingBufferDurationSec = rollingBufferDurationSec; | ||
| } else { | ||
| DLOGW("Rolling buffer duration does not fit range (%lf sec - %lf sec). Setting to default %lf sec", MIN_ROLLING_BUFFER_DURATION_IN_SECONDS, | ||
| MAX_ROLLING_BUFFER_DURATION_IN_SECONDS, DEFAULT_ROLLING_BUFFER_DURATION_IN_SECONDS); |
There was a problem hiding this comment.
You should add what the current value is set to. Also, should add "audio" and "video" for the duration lines.
The below logs are missing that info..
2024-06-28 23:40:41.721 WARN setUpRollingBufferConfigInternal(): Rolling buffer duration does not fit range (0.100000 sec - 10.000000 sec). Setting to default 3.000000 sec
2024-06-28 23:40:41.721 WARN setUpRollingBufferConfigInternal(): Rolling buffer bitrate does not fit range (104857.600000 bps - 251658240.000000 bps) for audio. Setting to default 1024000.000000 bps
2024-06-28 23:40:41.722 WARN setUpRollingBufferConfigInternal(): Rolling buffer duration does not fit range (0.100000 sec - 10.000000 sec). Setting to default 3.000000 sec
2024-06-28 23:40:41.722 WARN setUpRollingBufferConfigInternal(): Rolling buffer bitrate does not fit range (104857.600000 bps - 251658240.000000 bps) for video. Setting to default 5242880.000000 bps
sirknightj
reviewed
Jul 1, 2024
| if (pKvsRtpTransceiver != NULL) { | ||
| if (pKvsRtpTransceiver->pRollingBufferConfig == NULL) { | ||
| // Passing in 0,0. The default values will be set up since application has not set up rolling buffer config with the | ||
| // createRollingBufferConfig() call |
There was a problem hiding this comment.
It will also print out a bunch of extra logs (WARN)?
There was a problem hiding this comment.
(making it look like an error of some kind)
sirknightj
reviewed
Jul 1, 2024
| @@ -1633,6 +1630,19 @@ typedef struct { | |||
| * @{ | |||
| */ | |||
|
|
|||
| /** | |||
| * @brief Set up rolling buffer configuration - max duration of media to store (sec) and expected max bitrate (bips) of the encoded media | |||
There was a problem hiding this comment.
In some areas, bips is used. Others, bps is used. Please adjust for consistency
remote timerQueueKick as it is causing TURN failures due to lock contention
remove timerQueueKick as it is causing TURN failures due to lock contention
stefankiesz
reviewed
Oct 8, 2024
|
|
||
| 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: |
There was a problem hiding this comment.
Suggested change
| 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: |
|
Superseded by the linked PRs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
What was changed?
RtcRtpTransceiverInitstruct (which causes garbage values for config if the application does not memset the struct, leading to a crash) to surfacing an API to set up the values. The parameters are now internal to the SDK and can only be set up via this API call.Why was it changed?
How was it changed?
createRollingBufferConfigAPIWhat testing was done for the changes?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.