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 3 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
21 changes: 8 additions & 13 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 Expand Up @@ -1505,14 +1500,14 @@ typedef struct {
* Reference: https://www.w3.org/TR/webrtc/#dom-rtcdatachannelinit
*/
typedef struct {
BOOL ordered; //!< Decides the order in which data is sent. If true, data is sent in order
NullableUint16 maxPacketLifeTime; //!< Limits the time (in milliseconds) during which the channel will (re)transmit
//!< data if not acknowledged. This value may be clamped if it exceeds the maximum
//!< value supported by the user agent.
NullableUint16 maxRetransmits; //!< Control number of times a channel retransmits data if not delivered successfully
CHAR protocol[MAX_DATA_CHANNEL_PROTOCOL_LEN + 1]; //!< Sub protocol name for the channel
BOOL negotiated; //!< If set to true, it is up to the application to negotiate the channel and create an
//!< RTCDataChannel object with the same id as the other peer.
BOOL ordered; //!< Decides the order in which data is sent. If true, data is sent in order
NullableUint16 maxPacketLifeTime; //!< Limits the time (in milliseconds) during which the channel will (re)transmit
//!< data if not acknowledged. This value may be clamped if it exceeds the maximum
//!< value supported by the user agent.
NullableUint16 maxRetransmits; //!< Control number of times a channel retransmits data if not delivered successfully
CHAR protocol[MAX_PROTOCOL_LENGTH + 1]; //!< Sub protocol name for the channel
stefankiesz marked this conversation as resolved.
Show resolved Hide resolved
BOOL negotiated; //!< If set to true, it is up to the application to negotiate the channel and create an
//!< RTCDataChannel object with the same id as the other peer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for later. I noticed that the data channel init struct doesn't have a label member. It seems it's actually called "name" and instead, passed into the createDataChannel() method which takes in this struct + a few additional params one of them is this name.

} RtcDataChannelInit, *PRtcDataChannelInit;
/*!@} */

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 10U
stefankiesz marked this conversation as resolved.
Show resolved Hide resolved
/*!@} */

/**
* @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_STATS_STRING_LENGTH + 1]; //!< STUN/TURN server URL
stefankiesz marked this conversation as resolved.
Show resolved Hide resolved
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
DOMString url; //!< For local candidates this is the URL of the ICE server from which the candidate was obtained
stefankiesz marked this conversation as resolved.
Show resolved Hide resolved
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