Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand length verification #37

Merged
merged 6 commits into from
May 6, 2024
Merged

Expand length verification #37

merged 6 commits into from
May 6, 2024

Conversation

Noarkhh
Copy link
Contributor

@Noarkhh Noarkhh commented Apr 15, 2024

Make verify_content_length recognize and inform if body length is too long

@gBillal
Copy link
Contributor

gBillal commented Apr 19, 2024

Hi @Noarkhh
Since you're working on fixing the verifiy_content_length, there's a bug that occurs sometimes because of this function in (https://github.com/membraneframework/membrane_rtsp/blob/master/lib/membrane_rtsp/transport/tcp_socket.ex#L56).

When sending a play request using the same session for rtp data. Some IP cameras send an RTSP response along an RTP packet on one network packet which causes the function above to loop forever.

@Noarkhh
Copy link
Contributor Author

Noarkhh commented Apr 19, 2024

Hi @gBillal

I think when this function was written it was assumed that the only traffic on the TCP connection would be RTSP communication. In my experience with RTP over TCP with RTSP some servers start streaming and send some RTCP and RTP packets even before the response to PLAY request. I think the best solution to this is to use RTSP.play_no_response and capture the response using RTP.RTSP.Decapsulator.

Purpose of this PR was essentially to expand the verification to include cases, where there is no Content-Length header, but there was more data anyway. I needed it specifically for membraneframework/membrane_rtp_plugin#166, so that capturing RTSP responses in cases of interleaved RTP would work correctly.

By the way I was getting ready to start implementing a RTSP Source and creating membrane_rtsp_plugin, since the RTSP functionality is kind of repetetive, but then I randomly stumpled upon your implementation 😅 I forked and started adapting it, so you can expect a PR soon, since we'd be happy if we could make it work for our use cases too :)

@gBillal
Copy link
Contributor

gBillal commented Apr 19, 2024

Using play_no_response make sense in this case since the rtsp_decapsulator will parse the rtsp response at some point.

For membrane_rtsp_plugin I also found myself repeating the same code over to use RTSP, this is why I decided to create an element to reuse it. I had a hunch that you guys will create a similar element, this is why I haven't published it on hex.

@Noarkhh Noarkhh requested a review from mat-hek April 25, 2024 10:25
@@ -128,7 +129,7 @@ defmodule Membrane.RTSP.Response do
if byte_size(body) == 0 do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, can you change this if to when (a guard), to reduce nesting?

@Noarkhh Noarkhh merged commit e1b1856 into master May 6, 2024
3 checks passed
@Noarkhh Noarkhh deleted the expand-length-verification branch May 6, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants