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

Add tcp depayloader #161

Merged
merged 16 commits into from
Jan 31, 2024
Merged

Add tcp depayloader #161

merged 16 commits into from
Jan 31, 2024

Conversation

Noarkhh
Copy link
Contributor

@Noarkhh Noarkhh commented Jan 10, 2024

membraneframework/membrane_core#697
Add an element for depayloading a RTP stream received through TCP and encapsulated according to RFC 7826 Section 14.

@Noarkhh Noarkhh marked this pull request as draft January 10, 2024 16:20
@Noarkhh Noarkhh force-pushed the add-tcp-depayloader branch 2 times, most recently from b579d3c to f250ab7 Compare January 10, 2024 17:23
@Noarkhh Noarkhh force-pushed the add-tcp-depayloader branch from f250ab7 to 07093c9 Compare January 10, 2024 17:26
@Noarkhh Noarkhh linked an issue Jan 18, 2024 that may be closed by this pull request
3 tasks
@Noarkhh Noarkhh force-pushed the add-tcp-depayloader branch from 6777b6c to b321cf0 Compare January 22, 2024 14:43
@Noarkhh Noarkhh force-pushed the add-tcp-depayloader branch from 7844264 to ccfa6d1 Compare January 22, 2024 16:43
@Noarkhh Noarkhh marked this pull request as ready for review January 22, 2024 16:48
@Noarkhh Noarkhh requested a review from varsill January 22, 2024 16:49
README.md Show resolved Hide resolved
lib/membrane/rtp/tcp_depayloader.ex Show resolved Hide resolved
lib/membrane/rtp/tcp_depayloader.ex Show resolved Hide resolved
lib/membrane/rtp/tcp_depayloader.ex Outdated Show resolved Hide resolved
lib/membrane/rtp/tcp_depayloader.ex Outdated Show resolved Hide resolved
lib/membrane/rtp/tcp_depayloader.ex Outdated Show resolved Hide resolved
get_complete_packets(packets_binary, state.rtp_channel_id)

packets_buffers =
Enum.map(complete_packets_binaries, &%Buffer{payload: &1, metadata: metadata})
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a guarantee that the metadata is rewritten properly? What if the buffer's payload is put in unprocessed_data, and the metadata is attached to the data corresponding to the previous input buffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only metadata that is in buffers coming from TCP Source are tcp_source_address, tcp_source_port and arrival_ts. The first two don't change and the third one can be interpreted as in when the packet has fully arrived, which then works in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it makes sense, however, I am slightly worried that in case we were about to extend the metadata sent from the TCP Source, we would need to remember that the metadata is translated by one buffer when passed through the depayloader

lib/membrane/rtp/tcp_depayloader.ex Outdated Show resolved Hide resolved
@Noarkhh Noarkhh requested a review from varsill January 23, 2024 13:07
varsill
varsill previously approved these changes Jan 25, 2024
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

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

I've left one comment unresolved where I have described my doubts, but generally speaking - LGTM ;)

@Noarkhh Noarkhh requested a review from mat-hek January 31, 2024 12:33
@Noarkhh Noarkhh merged commit a70eee2 into master Jan 31, 2024
3 checks passed
@Noarkhh Noarkhh deleted the add-tcp-depayloader branch January 31, 2024 12:36
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.

Allow for streaming RTP with TCP when using RTSP
3 participants