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.

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

merged 6 commits into from
Nov 15, 2024

Conversation

stefankiesz
Copy link
Contributor

@stefankiesz stefankiesz commented Nov 13, 2024

Issue #, if available:

What was changed?

Why was it changed?

  • To reduce excess memory allocated through char[length].

How was it changed?

  • Copying the relevant changes from the above PR and adding additional improvements.

What testing was done for the changes?

  • Allowing for CI to pass.

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

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
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.

@stefankiesz stefankiesz marked this pull request as ready for review November 14, 2024 22:25
src/source/Ice/IceAgent.h Outdated Show resolved Hide resolved
src/source/Ice/IceAgent.h Outdated Show resolved Hide resolved
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[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

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[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.

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.

@stefankiesz stefankiesz merged commit a7e67dd into develop Nov 15, 2024
27 checks passed
@stefankiesz stefankiesz deleted the shorten-string-lengths branch November 15, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants