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

RTCSctpTransport: Specify special cases for maxMessageSize #1656

Merged
merged 3 commits into from
Dec 7, 2017

Conversation

adam-be
Copy link
Member

@adam-be adam-be commented Nov 6, 2017

(including updating via O/A exchange)

Fixes #1446

@adam-be adam-be requested a review from fluffy as a code owner November 6, 2017 19:07
@adam-be
Copy link
Member Author

adam-be commented Nov 6, 2017

@lgrahl, can you take a look please?

Copy link
Contributor

@lgrahl lgrahl left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks @adam-be.

</li>
<li>
<p>Let <var>remoteMaxMessageSize</var> be the value of the
"max-message-size" SDP attribute read from the remote description,
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 need to add from the SCTP transport's SDP media section or is this clear to the reader?

Copy link
Member Author

Choose a reason for hiding this comment

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

Most of our references with specific section numbers are automatically generated to not get out of sync with the external document. But there are "hard-coded" one too, so I guess it woudn't be too bad to add a reference to a top-level section here.

<li>
<p>Let <var>canSendSize</var> be the number of bytes that this
client can send (i.e. the size of the local send buffer) or 0 if
the implementation can handle messages of any size.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: In the W3C API, sending messages of any size is impossible at the moment (the API lacks the possibility to send partial messages) but I think it's good to have it in there already to be future-proof (and as a note for implementations outside of the browser).

@alvestrand
Copy link
Contributor

@adam-be will look into writing a test for this before merging.

@adam-be
Copy link
Member Author

adam-be commented Nov 29, 2017

Test: web-platform-tests/wpt#8495

webrtc.html Outdated
<li>
<p>If either <var>remoteMaxMessageSize</var> or
<var>canSendSize</var> is 0, set <a>[[\MaxMessageSize]]</a> to the
larger of the two.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this is algorithmically fine for the both 0 case as the result of max(0, 0) == 0 is correct, it might be a bit confusing to the reader.

@alvestrand
Copy link
Contributor

API style level question: Is there an argument for why a dedicated value (0) for maxMessageSize is a more user-friendly API than maxMessageSize = Number.MAX_SAFE_INTEGER, Number.MAX_VALUE or Number.POSITIVE_INFINITY?
Using these large numbers would have the advantage of requiring no checking against zero in the common case of "is this message too large to send?".

@adam-be
Copy link
Member Author

adam-be commented Nov 30, 2017

@alvestrand, we borrow the magic number zero from the signaling. But I agree that Number.MAX_SAFE_INTEGER would make a much better API.

@lgrahl
Copy link
Contributor

lgrahl commented Nov 30, 2017

Agreed, API-wise, Number.POSITIVE_INFINITY is much nicer (I wouldn't use discrete integers such as .MAX_SAFE_INTEGER or .MAX_VALUE though). The conversion from 0 to Number.POSITIVE_INFINITY should be at the very end of the description.

Edit: Will need updating of https://github.com/webrtc/adapter/pull/702

@lgrahl
Copy link
Contributor

lgrahl commented Nov 30, 2017

Another note: This would probably require changing the type of the maxMessageSize field from unsigned long to something else?

@alvestrand
Copy link
Contributor

UNSIGNED LONG is fine for MAX_SAFE_INTEGER but not for the infinities.

@lgrahl
Copy link
Contributor

lgrahl commented Nov 30, 2017

Okay. But I'd really like to avoid those discrete integers as they semantically don't fit into the scope of arbitrary sized messages. While I agree that messages of that size are an academic problem, I'd prefer a semantically correct API.

@adam-be
Copy link
Member Author

adam-be commented Dec 5, 2017

I'll update the PR to use Number.POSITIVE_INFINITY when no size-limit in enforced.

@alvestrand alvestrand merged commit fc99b02 into master Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants