search

scottlamb / retina

High-level RTSP multimedia streaming library, in Rust
https://crates.io/crates/retina
Apache License 2.0
237 stars 47 forks source link

unsafe bytes #24

Closed lattice0 closed 3 years ago

lattice0 commented 3 years ago

I see that you're using https://docs.rs/bytes/1.0.1/bytes/ for zero-copy, but it uses a lot of unsafe code. Shouldn't we at least give a feature option that swaps bytes by a safe version that does some copies? I'd prefer to use the safe version to minimize the amount of unsafe calls in my app.

I'm very concerned about this :(

scottlamb commented 3 years ago

What IO library are you using? If it's tokio (as Retina currently requires, though I'm open to changing this), using bytes is inevitable.

lattice0 commented 3 years ago

Didn't know tokio used bytes. It's god because it's best reviewed but would be nice in the future to not rely on it maybe? But this is for the far future, not now. I guess it would be a library with the same API but that does deep copies on the calls, I don't know.

lattice0 commented 3 years ago

my guess is that zero-copy is not a big deal for encoded packets cause they are small, or maybe most applications wouldn't need it. So lowering the attack surface would be great. By having a crate clone of the bytes library but that does copy instead of keeping a reference to a memory would be great. I really hate unsafe calls.

Also, is it possible to make this crate independent of the async runtime? Because tokio does some unsafe trade-offs (like using bytes) for performance, so it can be used to process lots of requests, but for an rtsp client this is just not needed (for the majority of cases like an NVR with 16 cameras), I guess.

scottlamb commented 3 years ago

It's already not zero-copy per se, more like "one copy", as discussed at #4.

What bytes gives us now is the ability to accumulate keep between h264::Depacketizer::push and h264::Depacketizer::pull without introducing an additional copy. I'm tempted to switch to using a ring buffer for this purpose. Personally I'm not worried about the unsafe in bytes (and my application will pull it anyway via the tokio ecosystem, including hyper), but this would also improve performance by reducing allocations. #6 It's not at the top of my priority list though.

Yes, it's possible to make this crate independent of the async runtime: support tokio, async-std, and plain synchronous std. Basically everything in src/client/mod.rs has to be either refactored for reusability across runtimes or duplicated. The other stuff (src/client/parse.rs, src/codec/h264.rs, etc.) is runtime-independent already.

I don't need runtime independence, as my application is using tokio anyway. So it's also not at the top of my priority list. But this would make retina more widely usable, so I think it'd be worth doing.