search

webrtc-rs / webrtc

A pure Rust implementation of WebRTC
https://webrtc.rs
Apache License 2.0
4.08k stars 363 forks source link

[SCTP] Stream::read: partial read OR returning the expected size of the buffer #273

Open melekes opened 2 years ago

melekes commented 2 years ago

The problem I'm facing right now is where I'm passing a buffer to Stream::read, which is not big enough (Error::ErrShortBuffer). Note there's no indication of the expected size => you're forced to guess the number.

Also note it's different from TcpStream https://doc.rust-lang.org/std/io/trait.Read.html#tymethod.read, which partially reads the data and does not put any restrictions on the size of the buffer (i.e. you, the caller, is in control of how fast you're consuming data).

I can see that the code in Pion is identical to one here, but I nonetheless think we need to change something.

Either

  1. switch to "tcp stream" partial reading model
  2. indicate the expected buffer size by changing ErrShortBuffer to be ErrShortBuffer { min: usize }

(1) is more "rust" therefore preferred.

There's also talk about auto-tuning buffer size, which is slightly related https://github.com/pion/sctp/issues/218#issuecomment-1080008490

https://github.com/webrtc-rs/sctp/blob/ed21dae1aa7dc5f37bd40fb344fa19746b932570/src/queue/reassembly_queue.rs#L282

also, why are we subtracting n_bytes before we've actually successfully written them to client's buffer?

Migrated from https://github.com/webrtc-rs/sctp/issues/28

k0nserv commented 2 years ago

From the last issue, the agreement seems to be that we should mimic the TCP implementation. My only concern is whether it would be tricky to use an API like this for the data channels

k0nserv commented 2 years ago

If anyone is interested in looking at this the first step should be to verify that the change in SCTP would not make its use impossible in the upstream crates. If it turns out to be okay the next step is to implement this

melekes commented 2 years ago

I'm looking at this right now.

melekes commented 2 years ago

If anyone is interested in looking at this the first step should be to verify that the change in SCTP would not make its use impossible in the upstream crates. If it turns out to be okay the next step is to implement this

@k0nserv what is the best way to do it?

opened https://github.com/webrtc-rs/webrtc/pull/304

k0nserv commented 2 years ago

Well, with the merged monorepo I should think that we are okay as long as the code still compiles and the tests all pass 🤔

melekes commented 1 year ago

See https://github.com/webrtc-rs/webrtc/pull/304#issuecomment-1271652707. Requires more thinking and design.