Use url > 2.1.0
nipierre opened this issue · 11 comments
Hi all,
Would it be possible to use the latest version of the url crate ?
I know they modified the parsing to abide to url parsing rules but would it be better to modify the input url when using this crate (eg. in explicitly providing the mode in the url) or to add in the parsing function a pitfall for the incriminated cases (eg. srt://:1234).
We are using this in a open source project (https://gitlab.com/media-cloud-ai/sdks/rs_mcai_worker_sdk) and we are kind of stuck as we can't use the latest development of this crate as we use the latest version of the url crate.
We would like to have some input on to how to solve this problem (we have some ideas already) and we could probably work on the solution :-).
I've made a fork adding a parsing function to circumvent the incriminated case, we're testing it in our workers atm, I'll make a PR if it is conclusive.
About the performance of the crate, we were using an older version up until now (cf. https://crates.io/crates/srt-protocol) which was not implementing several nice improvements like the connection not dropping after 80 minutes. Other than this nasty bug, the crate was working well. We principally used it in our worker SDK to allow real time communication between workers for use cases like real-time subtitling or real-time video analysis. We used it for both audio/video and data streaming. We might, in the future, use it only for data streaming as we're considering relying directly on ffmpeg to open the SRT socket (and not opening the socket with this crate and then sending data to ffmpeg buffer).
This crate is something that we will definitely use in production in 2023 for the aforementioned use-cases, thus the interest we have to keep it updated :-).
Yeah, there's been some nice improvements since the crates.io release.
We will probably need to handle the empty hostname case explicitly. I wanted to keep that case to match the reference implementation, even though there's no interfaces that we implement that are truly compatible, so it's possible we could remove it entirely.
Another potential solution is to only have that strict version requirement on srt-transmit (the commandline app) and relax the requirement for srt-tokio and srt-protocol, as I think only the commandline relies on that old behavior.
Since we took in charge (with @MarcAntoine-Arnaud) the publishing of the crate, we could probably also include a release mechanism in the repo to ease release cycle, what do you think about it ? (which would make 3 PRs from our side, counting url and dependabot)
For the moment we tried the handling of the empty hostname case with regex, leaving the rest of the crate untouched.
Your first approach is actually what I went with, you can have a look on my fork (nipierre@94817a2). I will refine it a bit and add tests if necessary after our tests are conclusive.
Unfortunately the URL parsing is a bit more complex than that, and srt://:1234 does mean something different than srt://0.0.0.0:1234, the former is a listener that binds to 0.0.0.0:1234, and the latter is a caller that binds to 0 (ephemeral) and connects to 0.0.0.0:1234 (which is a nonsense operation)
This logic also applies for UDP sockets supplied on the srt-transmit commandline
see srt-transmit/tests/stransmit_cmdline.rs for examples
I tried to cover most cases I could think of, I didn't think of any new tests though.