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

m_maxMessageSize should be infinity() instead of max() in RTCSctpTransport.h #28752

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented May 18, 2024

@Ahmad-S792 Ahmad-S792 added the WebRTC For bugs in WebRTC label May 18, 2024
@Ahmad-S792 Ahmad-S792 requested a review from youennf May 18, 2024 13:26
@Ahmad-S792 Ahmad-S792 self-assigned this May 18, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 18, 2024
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label May 18, 2024
Copy link
Contributor

@achristensen07 achristensen07 left a comment

Choose a reason for hiding this comment

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

Do we know what it would take to fix all the tests in that file? It looks like they all pass in other browsers, and this only fixes one of them.

@youennf
Copy link
Contributor

youennf commented May 21, 2024

Do we know what it would take to fix all the tests in that file? It looks like they all pass in other browsers, and this only fixes one of them.

We are currently relying on libwebrtc callback, which is probably not enough from looking at the code as the callback is only called in case of sctp connection state change (and not other state changes like message size).
We probably have to get the sctp state everytime we update the SDP descriptions.

That said, the change looks ok, let's go for it and I'll file another bug to fix other tests.

@youennf
Copy link
Contributor

youennf commented May 21, 2024

@youennf youennf added the merge-queue Applied to send a pull request to merge-queue label May 21, 2024
…ctpTransport.h`

https://bugs.webkit.org/show_bug.cgi?id=274025
rdar://problem/128306030

Reviewed by Youenn Fablet.

This patch aligns WebKit with web specification [1]:

[1] https://w3c.github.io/webrtc-pc/#dfn-update-the-data-max-message-size

"If both remoteMaxMessageSize and canSendSize are 0, set
[[MaxMessageSize]] to the positive Infinity value."

* Source/WebCore/Modules/mediastream/RTCSctpTransport.h:
(double m_maxMessageSize):
* LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCSctpTransport-maxMessageSize-expected.txt: Rebaselined

Canonical link: https://commits.webkit.org/279039@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/m_maxMessageSize-should-be-infinity-instead-of-max-in-RTCSctpTransport-h branch from d368c1a to 868f202 Compare May 21, 2024 07:02
@webkit-commit-queue
Copy link
Collaborator

Committed 279039@main (868f202): https://commits.webkit.org/279039@main

Reviewed commits have been landed. Closing PR #28752 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 868f202 into WebKit:main May 21, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 21, 2024
@Ahmad-S792 Ahmad-S792 deleted the eng/m_maxMessageSize-should-be-infinity-instead-of-max-in-RTCSctpTransport-h branch May 25, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebRTC For bugs in WebRTC
Projects
None yet
6 participants