Add RFC 2617 qop support and fix SETUP URL handling#58
Add RFC 2617 qop support and fix SETUP URL handling#58georgeday-firefly wants to merge 2 commits intomembraneframework:masterfrom
Conversation
lib/membrane_rtsp/request.ex
Outdated
| def process_uri(request, %URI{} = uri) do | ||
| %{uri | userinfo: nil} |
There was a problem hiding this comment.
When fixing these errors in Membrane we took an approach of both matching on the struct and using struct update syntax, like this:
| def process_uri(request, %URI{} = uri) do | |
| %{uri | userinfo: nil} | |
| def process_uri(request, %URI{} = uri) do | |
| %URI{uri | userinfo: nil} |
This gets rid of the error and keeps the visual information what is the type of variable being updated. Please apply this to the rest of the fixes in the PR and revert from map update back to struct update syntax, thanks for understanding
| if parsed.host do | ||
| %{parsed | userinfo: nil} |> URI.to_string() | ||
| else |
There was a problem hiding this comment.
What is the reason for this?
There was a problem hiding this comment.
good question, there might be a cleaner way to do it.
some cameras (like Bosch) have stream URIs with query parameters like:
rtsp://host/rtsp_tunnel?p=0&line=1&inst=1&vcd=2
and their SDP returns absolute control URLs (with full scheme and host) rather than relative paths like trackID=1 which is permitted by RFC 2326.
the original code assumed all control paths were relative, so SETUP requests broke for these cameras.
the if parsed.host do check detects whether the control path is an absolute URL by checking if it has a host after parsing so absolute URLs get used directly, relative ones get joined to the base URL as before.
the Map.put(:query, nil) in the else branch is a separate fix for cameras like Vivotek that include query params in their stream URI e.g. ?profile=Profile200 those shouldn't carry over into the SETUP request URL when joining a relative control path
also rebased onto current master and squashed into a single commit
cheers for the review and let me know if you want me to make any further changes, do I need to bump the version or anything?
There was a problem hiding this comment.
I think the solution for checking the host is reasonable, could you expand on setting the userinfo to nil?
I don't have experience with cameras that include query parametrs in their URI, but as far as I understand after establishing the connection the parameters can be discarded, correct? I'm wondering if it would be more sensible to strip them at the beginning, so that base_url never has them in the first place.
There was a problem hiding this comment.
the base uri's userinfo is already stripped in process_uri, the strip in the absolute branch handles the control url from sdp, which is a separate uri that could also contain credentials so thought we should do the same.
yeah i had the same thought originally and it would be cleaner, but the base url is used for both describe and setup, cameras like vivotek need the query params (e.g. ?profile=Profile200) in the describe request to identify the right stream, so we can only strip them when building setup uris from relative control paths
also pushed a struct syntax fix which i missed previously, cheers again
bccd0d5 to
d06bfbd
Compare
Summary
Test plan