Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

Fails to send with high bitrate (or large messages) #144

Closed
marcus-pousette opened this issue Apr 27, 2023 · 9 comments
Closed

Fails to send with high bitrate (or large messages) #144

marcus-pousette opened this issue Apr 27, 2023 · 9 comments
Labels
question Further information is requested

Comments

@marcus-pousette
Copy link
Contributor

marcus-pousette commented Apr 27, 2023

I am running into issues when using WebRTC instead of Websockets from a browser.

Using WebRTC transport seem to fail if the bitrate is too high, and the failure is kind of "silent", which makes it hard to detect.

I am able to reproduce in the browser-to-browser example,

1).

Set

streamMuxers: [mplex({ maxMsgSize: 15 * 1e3 })], 

Based on the message size limitation

2).
Instead of sending a message of text, send 1mb of data ( sender.push(new Uint8array(1e6)))
Log the results on the other side.

Sending data works when the message is small (isch). < 200 kb. Fails otherwise. I am running a Chrome browser

In my own application I can run the exact same setup with Websockets and a relay, with mplex message size limitation above and it works as expected. But it does not work with WebRTC transport when the bitrate becomes too large (or the message sizes).

@marcus-pousette marcus-pousette changed the title Fails to send high bitrate (or large messages) Fails to send with high bitrate (or large messages) Apr 27, 2023
@marcus-pousette
Copy link
Contributor Author

marcus-pousette commented Apr 27, 2023

Okey, so this transport already has a muxer (which means the providedstreamMuxers will be ignored). Looks like it does not support chunking like mplex, with 'maxMsgSize'. I guess it is still possible to do here since the data messages are length prefixed.

@maschad maschad added the question Further information is requested label May 2, 2023
@maschad maschad added this to js-libp2p May 2, 2023
@maschad maschad moved this to 🥞Weekly Candidates/Discuss🎙 in js-libp2p May 2, 2023
@maschad
Copy link
Member

maschad commented May 2, 2023

Currently, it's not practical to use RTCDataChannel for messages larger than 64 KiB (16 KiB if you want to support cross-browser exchange of data - That is why in the spec states that encoded messages including their length prefix MUST NOT exceed 16kiB to support all major browsers.)

The problem arises from the fact that SCTP—the protocol used for sending and receiving data on an RTCDataChannel—was originally designed for use as a signaling protocol. There is a proposal which will make it possible to send messages with essentially no size limitations, but for now perhaps we should just set that max limit to 16Kb to avoid silent failures for consumers.

@marcus-pousette
Copy link
Contributor Author

marcus-pousette commented May 2, 2023

See my pr #147 . I am using that currently and I am able to send any message sizes at any rate. There is also an issue (with the current release), it seems like that if you send 1000 messages of size 1kb in a rapid succession you can also run into the same problem as the underlying Datachannel will not "send" you message right away, but will buffer many pieces of messages to a large message. The PR also does work around this by making sure that the buffer size never exceeds the threshold.

@maschad
Copy link
Member

maschad commented May 9, 2023

Thanks for your PR, there definitely is credence to your proposed solution, but I think there ought to be a conversation around changing the spec first to allow the length prefix to exceed 16kb, before we implement this change as it will have ramifications for other implementations

I think the work related to ensuring that the message size exceeds 16kb and subsequently ensuring the buffer never exceeds the threshold is appropriate for now.

@mxinden
Copy link
Member

mxinden commented May 10, 2023

@marcus-pousette what do you gain from sending large messages over many small messages? What is your motivation here?

The PR also does work around this by making sure that the buffer size never exceeds the threshold.

👍 linking the corresponding tracking issue #40 here.


As far as I can tell, we should also set this option to a=max-message-size:16384, see specification.

a=max-message-size:100000

@ckousik was it a deliberate choice not to set this?


Also related, to prevent head-of-line-blocking we recommend a limit of 1204 bytes, see head-of-line blocking section:

Thus for payloads that would suffer from head-of-line blocking, implementations SHOULD choose a message size equal or below 1204 bytes.

As far as I can tell, our _sink implementation does not chunk messages from the user on the way down to the browser's WebRTC stack.

private async _sink (src: Source<Uint8ArrayList | Uint8Array>): Promise<void> {
const closeWrite = this._closeWriteIterable()
for await (const buf of merge(closeWrite, src)) {
if (this.streamState.isWriteClosed()) {
return
}
const msgbuf = pb.Message.toBinary({ message: buf.subarray() })
const sendbuf = lengthPrefixed.encode.single(msgbuf)
this.channel.send(sendbuf.subarray())
}
}

@ckousik could you comment here as well? Do I understand correctly that this implementation does not chunk the messages?

@marcus-pousette
Copy link
Contributor Author

marcus-pousette commented May 10, 2023

@maschad

Thanks for your PR, there definitely is credence to your proposed solution, but I think there ought to be a conversation around changing the spec first to allow the length prefix to exceed 16kb, before we implement this change as it will have ramifications for libp2p/specs#475

I see. I guess I don't agree that the specification is optimal here then, since if we are doing ordered data-channels and are doing length-prefixed messages, we kind of have all the conditions to do any sized message.

I think the work related to ensuring that the message size exceeds 16kb and subsequently ensuring the buffer never exceeds the threshold is appropriate for now.

Good!

@mxinden

@marcus-pousette what do you gain from sending large messages over many small messages? What is your motivation here?

I am doing video-streaming over this and you can not control message sizes from the encoder and the throughput could be something like 10 mbit/s even though you only send small messages. I also do other high-throughput database replication work, which requires this.

max-message-size:16384

I red somewhere that max-message-size is not respected for DataChannels, but I might be wrong here. Perhaps relevant. In the future, it would also maybe make sense to create a custom sdp message where one leaks what kind of client one is using and then chooses the max message size option based on what combination it is, firefox-chrome, chrome-chrome, etc.

Like

a=client:firefox-?.?.?

then

transports = [webRTC({maxMsgSize: (client: string) => /* choose wisely */ } )]

@mxinden
Copy link
Member

mxinden commented May 16, 2023

Thanks for your PR, there definitely is credence to your proposed solution, but I think there ought to be a conversation around changing the spec first to allow the length prefix to exceed 16kb, before we implement this change as it will have ramifications for libp2p/specs#475

I see. I guess I don't agree that the specification is optimal here then, since if we are doing ordered data-channels and are doing length-prefixed messages, we kind of have all the conditions to do any sized message.

Agreed. From a library user perspective, arbitrary size messages should be possible. Though within js-libp2p-webrtc we should chunk messages, thus allowing messages from other streams and control messages to be interleaved, thus reducing the impact of head-of-line blocking.

@mxinden
Copy link
Member

mxinden commented May 16, 2023

@marcus-pousette what do you gain from sending large messages over many small messages? What is your motivation here?

I am doing video-streaming over this and you can not control message sizes from the encoder and the throughput could be something like 10 mbit/s even though you only send small messages. I also do other high-throughput database replication work, which requires this.

In terms of performance, I would be surprised if smaller message size has a significant impact. Given that the IP + SCTP + libp2p + xxx overhead is small compared to the overall message size the larger the message gets, I don't expect the impact to be large. See also first entry on the FAQ on multiplexing.

@marcus-pousette
Copy link
Contributor Author

Here is some benchmarking

https://viblast.com/blog/2015/2/5/webrtc-data-channel-message-size/

I don't think the performance impact is huge, but I think it also depends on how the messages is handled on the other end, whether we have to allocate memory in order to join multiple small messages into a large one.

@github-project-automation github-project-automation bot moved this from 🥞Weekly Candidates/Discuss🎙 to 🎉Done in js-libp2p May 17, 2023
github-actions bot pushed a commit that referenced this issue May 17, 2023
## [2.0.3](v2.0.2...v2.0.3) (2023-05-17)

### Bug Fixes

* restrict message sizes to 16kb ([#147](#147)) ([aca4422](aca4422)), closes [#144](#144) [#158](#158)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
Archived in project
Development

No branches or pull requests

3 participants