Make use of the max-message-size SDP attribute in datachannels
#722
Conversation
…order to be able to reuse the logic
…ize is fed to the underlying association during the call to `start_sctp`
…`, as the only consumer of this method expects `u32`
…annel's config Given that we are no longer fixed to the 64KB max size for message sizes, the user needs a way to know the actual max message size a datachannel may send before returning an error. This information is particularly useful for applications where large messages are chunked and framed in the application itself.
| pub struct Association { | ||
| name: String, | ||
| state: Arc<AtomicU8>, | ||
| // TODO: Convert into `u32`, as there is no reason why the `max_message_size` should need to be |
There was a problem hiding this comment.
I am trying to keep refactors to the minimum in order to keep this PR as small as possible. If you do agree with this comment, I can tackle it in another PR, otherwise, I will remove said comment.
| } | ||
|
|
||
| #[derive(Clone)] | ||
| pub enum SctpMaxMessageSize { |
There was a problem hiding this comment.
I am personally not a huge fan of how the crate makes use of 0 as a sentinel value for some configuration being unset, so I decided to make use of an enum to give the user a clear distiction between having a bounded SctpMaxMessageSize, or an unbounded one (which is typically represented as a zero in this code base).
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_given_remote_max_message_size_is_none_when_data_channel_can_send_max_message_size_respected_on_send( |
There was a problem hiding this comment.
I like using the given, when, then naming pattern for certain tests. If you're not a fan of this, feel free to suggest alternative test names.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_given_remote_max_message_size_is_none_when_data_channel_can_send_max_message_size_respected_on_send( |
There was a problem hiding this comment.
The following tests whether the max-message-size attribute, or the sctp_max_message_size_can_send attribute in the SettingsEngine is actually respected (in an end to end manner).
| if let Some(local_port) = get_application_media_section_sctp_port(parsed_local) | ||
| { | ||
| self.start_sctp(local_port, remote_port).await; | ||
| // TODO: Reuse the MediaDescription retrieved when looking for the message size. |
There was a problem hiding this comment.
A small nitpick of the code from my end, if you agree with the proposal, which is retrieving the MediaDescription once, and reusing it, I can implement this in another PR.
| // so we need a dedicated field | ||
| is_started: AtomicBool, | ||
|
|
||
| // max_message_size represents the maximum size of data that can be passed to |
There was a problem hiding this comment.
This is not actually use anywhere, and cannot be properly initialized since the true max-message-size is only known once you receive the other peer's SDP.
| } | ||
|
|
||
| fn calc_message_size(remote_max_message_size: usize, can_send_size: usize) -> usize { | ||
| fn calc_message_size(remote_max_message_size: u32, can_send_size: u32) -> u32 { |
There was a problem hiding this comment.
I decided to use u32 instead of usize here, since the method is only called by the code added in this PR. It feels pretty pointless to convert all u32s into usizes and then convert the resulting usize back into a u32.
| /// have_data_channel return MediaDescription with MediaName equal application | ||
| pub(crate) fn have_data_channel( | ||
| desc: &session_description::RTCSessionDescription, | ||
| ) -> Option<&MediaDescription> { |
There was a problem hiding this comment.
I've rewritten this function in particular to use get_application_media, but I did not refactor the other functions in this file which could also make use of get_application_media in order to keep this PR to a minimum.
If you agree with the refactor, I can do it in another PR.
|
I'm also not sure what to do with |
|
Hi @rainliu , I would appreciate if you could take a look at the PR 😁 |
2 participants
Currently, webrtc-rs does not react to the
max-message-sizeattribute that may be attached to an SDP.RFC8841 introduces the
max-message-sizeSDP attribute:Browsers such as chrome typically advertise a max message size of 256KiB. Some user agents may also request a max-message-size smaller than the default 64KiB.