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

Shorten Stats String Lengths #2079

Merged
merged 6 commits into from
Nov 15, 2024
Merged

Shorten Stats String Lengths #2079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5cd414f
Update data types for ice agent stats
stefankiesz Nov 13, 2024
c67b3bb
Fix missing semicolon
stefankiesz Nov 13, 2024
9496a55
Add more string length reductions, clang format
stefankiesz Nov 13, 2024
73c07e3
Address comments
stefankiesz Nov 14, 2024
a5e7829
Clang format
stefankiesz Nov 14, 2024
8395f27
Address comments
stefankiesz Nov 15, 2024
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
5 changes: 0 additions & 5 deletions 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 @@ -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
*/
Expand Down
46 changes: 28 additions & 18 deletions src/include/com/amazonaws/kinesis/video/webrtcclient/Stats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ extern "C" {
*/
#define MAX_CANDIDATE_ID_LENGTH 9U

/**
* Maximum allowed ICE URI length
*/
#define MAX_ICE_CONFIG_URI_LEN 127
stefankiesz marked this conversation as resolved.
Show resolved Hide resolved

/**
* Maximum allowed relay protocol length
*/
Expand Down Expand Up @@ -52,7 +57,7 @@ extern "C" {
/**
* Maximum allowed maximum protocol length (allowed values: tcp, udp)
*/
#define MAX_PROTOCOL_LENGTH 8U
#define MAX_PROTOCOL_LENGTH 7U

/**
* Maximum allowed length of IP address string
Expand All @@ -65,6 +70,12 @@ extern "C" {
#define MAX_STATS_STRING_LENGTH 255U
/*!@} */

/**
* Maximum length of candidate type (host, srflx, relay, prflx, unknown)
*/
#define MAX_CANDIDATE_TYPE_LENGTH 7U
/*!@} */

/**
* @brief DOMString type is used to store strings of size 256 bytes (inclusive of '\0' character
*
Expand Down Expand Up @@ -233,14 +244,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;

/**
Expand All @@ -267,15 +278,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;

/**
Expand Down
29 changes: 14 additions & 15 deletions src/source/Ice/IceAgent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
CHAR url[MAX_ICE_CONFIG_URI_LEN + 1]; //!< 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
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to transportId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those were removed from two structs in the original PR, likely to save memory as they are not used anywhere.
https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/pull/1947/files

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw IP_ADDR_STR_LENGTH + 1 in another part of this PR, is this storing something different? The comment is the same as in RtcIceCandidateStats.

Since these strings use a lot of the same types, suggest to add and use typedefs for these char arrays with optimized lengths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither IP_ADDR_STR_LENGTH nor KVS_IP_ADDRESS_STRING_BUFFER_LEN were modified or used differently in this PR. Will leave this comment unresolved for visibility for future improvements.

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
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
} RtcIceCandidateDiagnostics, *PRtcIceCandidateDiagnostics;

typedef struct {
Expand Down
Loading