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

Implement elements for packetizing and depacketzing RTP packets for TCP #163

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

wkozyra95
Copy link
Contributor

@wkozyra95 wkozyra95 commented Feb 22, 2024

  • Membrane.RTP.TCP.Encapsulator - reads packet and generates bytestream that can be send over connection-oriented transport (such as TCP)
  • Membrane.RTP.TCP.Decapsulator - create RTP packets from a bytestream

Mostly based on Membrane.RTP.TCP.Depayloader (this element implements different mechanism used in RTSP)

I'm open to the name suggestions:

  • I decided to go with packetizer/depacketizer so as not to confuse it with payloaders/depayloared for RTP payload like h264
  • Other alternative could be to name it Membrane.RTP.TCP.Depayloader and Membrane.RTP.TCP.Payloader, but in that case existing Membrane.RTP.TCP.Depayloader would need to be renamed

@wkozyra95 wkozyra95 self-assigned this Feb 22, 2024
@wkozyra95 wkozyra95 force-pushed the @wkozyra95/add-rtp-tcp-packetizer branch from 8117047 to 57d18f7 Compare February 22, 2024 13:08
@wkozyra95 wkozyra95 marked this pull request as ready for review February 22, 2024 13:18
case :binary.encode_unsigned(byte_size(payload), :big) do
<<len::size(8)>> -> <<0, len>>
<<len::binary-size(2)>> -> len
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be replaced with len_bytes = <<byte_size(payload)::size(16)>> (Elixir uses big endian by default when creating binaries this way)

Copy link
Contributor

Choose a reason for hiding this comment

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

this can be further expanded to payload = <<byte_size(payload)::size(16), payload::binary>>, which creates the prefixed payload that can be directly passed to outputed buffer

@@ -0,0 +1,48 @@
defmodule Membrane.RTP.TCP.Depacketizer do
Copy link
Contributor

Choose a reason for hiding this comment

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

It was kind of counterintuitive to me that Depacketizer is reponsible for encapsulation, even though it technically is correct. What do you think about Encapsulator and Decapsulator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I would probably stay with Packetizer/Depacketizer or Payloader/Depayloader, but I don't mind changing that

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, actually is it correct? I think of depacketizing as of getting out of packets, which in this case would be removing the two-byte header with packet length…

Copy link
Member

Choose a reason for hiding this comment

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

the RFC calls it framing: https://datatracker.ietf.org/doc/html/rfc4571 however as I read it, I understand that by framing they mean adding the 2 byte header, not the other way

Copy link
Contributor Author

@wkozyra95 wkozyra95 Feb 23, 2024

Choose a reason for hiding this comment

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

packetizing = converting to packets from a continuous stream of data (TCP does not really have a concept of packets)
I already changed it either way so does not really matter

end

defp get_complete_packets(packets_binary, complete_packets) do
<<payload_length::size(16), rest::binary>> = packets_binary
Copy link
Member

Choose a reason for hiding this comment

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

you can change the function headers to the following:

defp get_complete_packets(
  <<payload_length::16, payload::binary-size(payload_length), rest::binary>>,
  complete_packets) do
# next chunk available
end
defp get_complete_packets(leftover, complete_packets) do
# next chunk unavailable
end

mat-hek
mat-hek previously approved these changes Feb 23, 2024
Noarkhh
Noarkhh previously approved these changes Feb 23, 2024
Copy link
Contributor

@Noarkhh Noarkhh left a comment

Choose a reason for hiding this comment

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

Remember to bump version in mix.exs and readme

@wkozyra95 wkozyra95 dismissed stale reviews from Noarkhh and mat-hek via 273e719 February 26, 2024 12:33
@wkozyra95 wkozyra95 force-pushed the @wkozyra95/add-rtp-tcp-packetizer branch from 8c507a0 to 273e719 Compare February 26, 2024 12:33
@wkozyra95 wkozyra95 merged commit f610958 into master Feb 26, 2024
3 checks passed
@wkozyra95 wkozyra95 deleted the @wkozyra95/add-rtp-tcp-packetizer branch February 26, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants