Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include and use threadpool for signaling channel messages #1761

Merged
merged 9 commits into from
Jul 27, 2023
Merged

Include and use threadpool for signaling channel messages #1761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
45bc404
Include and use threadpool for signaling channel messages
jdelapla Jul 24, 2023
60471bf
Fix clang and linux gcc compile errors
jdelapla Jul 25, 2023
189abbc
Fix pointer cast
jdelapla Jul 25, 2023
45af0e4
Using SignalingClientInfo to pass information about Threadpool
jdelapla Jul 27, 2023
711b2a2
edit README.md
jdelapla Jul 27, 2023
620e802
Not necessary to change struct version
jdelapla Jul 27, 2023
2908ba2
Update struct version again, add fall-through
jdelapla Jul 27, 2023
9224a2c
clang formatting
jdelapla Jul 27, 2023
68e1c02
Clang formatting, again
jdelapla Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMake/Dependencies/libkvsCommonLws-CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ include(ExternalProject)

ExternalProject_Add(libkvsCommonLws-download
GIT_REPOSITORY https://github.com/awslabs/amazon-kinesis-video-streams-producer-c.git
GIT_TAG 8bd5adc78e8453e52a8707748c3469d8c9b74c29
GIT_TAG daf742a0a53f72341e5b2df75a05ebe6d2557811
PREFIX ${CMAKE_CURRENT_BINARY_DIR}/build
CMAKE_ARGS
-DCMAKE_INSTALL_PREFIX=${OPEN_SRC_INSTALL_PREFIX}
Expand Down
5 changes: 5 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,11 @@ When building on MacOS M1, if the build fails while trying to build OpenSSL or W
To build on a 32-bit Raspbian GNU/Linux 11 on 64-bit hardware, the OpenSSL library must be manually configured. This is due to the OpenSSL autoconfiguration script detecting 64-bit hardware and emitting 64-bit ARM assembly instructions which are not allowed in 32-bit executables. A 32-bit ARM version of OpenSSL can be configured by setting 32-bit ARM platform:
`cmake .. -DBUILD_OPENSSL_PLATFORM=linux-armv4`

### Threadpool for Signaling Channel messages
The threadpool is enabled by default, and starts with 3 threads that it can increase up to 5 if all 3 are actively in use. To change these values to better match the resources of your use case
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to make this configurable instead? Is it expected customers would not require more than 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I've pointed out the location of the defines for customers to change in this README. I would have preferred to have placed them in a more common library but that would require a different constructor for slgnaling client that allows you to pass in a given threadpool. Possibly an alternative to explore later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we can add the 2 parameters as part of SignalingClientInfo though. Having customers change the #defs and rebuild the SDK to be used as a dependency in another project gets messy. If we are allowing customers to configure this, having it as part of clientInfo is cleaner

please edit samples/Samples.h defines `KVS_SIGNALING_THREADPOOL_MIN` and `KVS_SIGNALING_THREADPOOL_MAX`. You can also disable the threadpool to instead create and detach each thread
to handle signaling messages by commenting out `KVS_USE_SIGNALING_CHANNEL_THREADPOOL`.

## Documentation
All Public APIs are documented in our [Include.h](https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/blob/master/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h), we also generate a [Doxygen](https://awslabs.github.io/amazon-kinesis-video-streams-webrtc-sdk-c/) each commit for easier navigation.

Expand Down
2 changes: 2 additions & 0 deletions samples/Common.c
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,8 @@ STATUS createSampleConfiguration(PCHAR channelName, SIGNALING_CHANNEL_ROLE_TYPE
pSampleConfiguration->clientInfo.loggingLevel = logLevel;
pSampleConfiguration->clientInfo.cacheFilePath = NULL; // Use the default path
pSampleConfiguration->clientInfo.signalingClientCreationMaxRetryAttempts = CREATE_SIGNALING_CLIENT_RETRY_ATTEMPTS_SENTINEL_VALUE;
pSampleConfiguration->clientInfo.signalingMessagesMinimumThreads = KVS_SIGNALING_THREADPOOL_MIN;
pSampleConfiguration->clientInfo.signalingMessagesMaximumThreads = KVS_SIGNALING_THREADPOOL_MAX;
pSampleConfiguration->iceCandidatePairStatsTimerId = MAX_UINT32;
pSampleConfiguration->pregenerateCertTimerId = MAX_UINT32;

Expand Down
7 changes: 7 additions & 0 deletions samples/Samples.h
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ extern "C" {
#define MASTER_DATA_CHANNEL_MESSAGE "This message is from the KVS Master"
#define VIEWER_DATA_CHANNEL_MESSAGE "This message is from the KVS Viewer"

// Signaling client threadpool for handling messages
#define KVS_SIGNALING_THREADPOOL_MIN 3
#define KVS_SIGNALING_THREADPOOL_MAX 5

// comment out this line to disable the feature
#define KVS_USE_SIGNALING_CHANNEL_THREADPOOL 1

/* Uncomment the following line in order to enable IoT credentials checks in the provided samples */
// #define IOT_CORE_ENABLE_CREDENTIALS 1

Expand Down
4 changes: 3 additions & 1 deletion src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ extern "C" {
/**
* Version of SignalingClientInfo structure
*/
#define SIGNALING_CLIENT_INFO_CURRENT_VERSION 1
#define SIGNALING_CLIENT_INFO_CURRENT_VERSION 2
Copy link
Contributor

@disa6302 disa6302 Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since, we are updating the struct (and we need to) we would need to update the version table here: https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/blob/develop/src/source/Signaling/Signaling.c#L586
Basically need to set the defaults for versions less than 2 and need internal copy. Signaling client info maintain an internal copy of all the variables, so need to do the same for this and so that the updated pSignalingClient version is passed around.


/**
* Version of SignalingClientCallbacks structure
Expand Down Expand Up @@ -1202,6 +1202,8 @@ typedef struct {
INT32 signalingClientCreationMaxRetryAttempts; //!< Max attempts to create signaling client before returning error to the caller
UINT32 stateMachineRetryCountReadOnly; //!< Retry count of state machine. Note that this **MUST NOT** be modified by the user. It is a read only
//!< field
UINT32 signalingMessagesMinimumThreads;
UINT32 signalingMessagesMaximumThreads;
} SignalingClientInfo, *PSignalingClientInfo;

/**
Expand Down
4 changes: 4 additions & 0 deletions src/source/Signaling/LwsApiCalls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2012,9 +2012,13 @@ STATUS receiveLwsMessage(PSignalingClient pSignalingClient, PCHAR pMessage, UINT
DLOGW("Failed to validate the ICE server configuration received with an Offer");
}

#ifdef KVS_USE_SIGNALING_CHANNEL_THREADPOOL
CHK_STATUS(threadpoolPush(pSignalingClient->pThreadpool, receiveLwsMessageWrapper, (PVOID) pSignalingMessageWrapper));
#else
// Issue the callback on a separate thread
CHK_STATUS(THREAD_CREATE(&receivedTid, receiveLwsMessageWrapper, (PVOID) pSignalingMessageWrapper));
CHK_STATUS(THREAD_DETACH(receivedTid));
#endif

CleanUp:

Expand Down
10 changes: 10 additions & 0 deletions src/source/Signaling/Signaling.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ STATUS createSignalingSync(PSignalingClientInfoInternal pClientInfo, PChannelInf
CHK_STATUS(createValidateChannelInfo(pChannelInfo, &pSignalingClient->pChannelInfo));
CHK_STATUS(validateSignalingCallbacks(pSignalingClient, pCallbacks));
CHK_STATUS(validateSignalingClientInfo(pSignalingClient, pClientInfo));
#ifdef KVS_USE_SIGNALING_CHANNEL_THREADPOOL
CHK_STATUS(threadpoolCreate(&pSignalingClient->pThreadpool, pClientInfo->signalingClientInfo.signalingMessagesMinimumThreads,
pClientInfo->signalingClientInfo.signalingMessagesMaximumThreads));
#endif

pSignalingClient->version = SIGNALING_CLIENT_CURRENT_VERSION;

Expand Down Expand Up @@ -222,6 +226,10 @@ STATUS freeSignaling(PSignalingClient* ppSignalingClient)

hashTableFree(pSignalingClient->diagnostics.pEndpointToClockSkewHashMap);

#ifdef KVS_USE_SIGNALING_CHANNEL_THREADPOOL
threadpoolFree(pSignalingClient->pThreadpool);
#endif

if (IS_VALID_MUTEX_VALUE(pSignalingClient->connectedLock)) {
MUTEX_FREE(pSignalingClient->connectedLock);
}
Expand Down Expand Up @@ -605,6 +613,8 @@ STATUS validateSignalingClientInfo(PSignalingClient pSignalingClient, PSignaling
break;

case 1:
// explicit-fallthrough
case 2:
// If the path is specified and not empty then we validate and copy/store
if (pSignalingClient->clientInfo.signalingClientInfo.cacheFilePath != NULL &&
pSignalingClient->clientInfo.signalingClientInfo.cacheFilePath[0] != '\0') {
Expand Down
4 changes: 4 additions & 0 deletions src/source/Signaling/Signaling.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,10 @@ typedef struct {
UINT64 getIceConfigTime;
UINT64 deleteTime;
UINT64 connectTime;

#ifdef KVS_USE_SIGNALING_CHANNEL_THREADPOOL
PThreadpool pThreadpool;
#endif
} SignalingClient, *PSignalingClient;

// Public handle to and from object converters
Expand Down