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

RTP and RTCP parsing needs a refactor #1233

Open
ibc opened this issue Nov 21, 2023 · 0 comments
Open

RTP and RTCP parsing needs a refactor #1233

ibc opened this issue Nov 21, 2023 · 0 comments
Assignees
Milestone

Comments

@ibc
Copy link
Member

ibc commented Nov 21, 2023

In summary: we must refactor RTCP packets in the way it's done in rtp.js library, which has super strict tests. Here some items:

Must manage unsupported packet / chunk / block types

For example, a XR packet can contain many different types of Extender Report Blocks. However, if one of those are not implemented in mediasoup then the parsed packet will ignore every block after it.

In (rtp.js](https://github.com/versatica/rtp.js) lib this is solved by having a GenericExtendedReport class.

The following XR packet is parsed with zero blocks in mediasoup since the Extended Report LRLE block type is not implemented:

0xa0, 0xcf, 0x00, 0x0E, // Padding, Type: 207 (XR), Length: 14
0x5d, 0x93, 0x15, 0x34, // Sender SSRC: 0x5d931534
// Extended Report LRLE
0x01, 0x09, 0x00, 0x04, // BT: 1 (LRLE), T: 9, Block Length: 4
0x03, 0x93, 0x2d, 0xb4, // SSRC of source: 0x03932db4
0x00, 0x11, 0x00, 0x22, // Begin Seq: 0x11, End Seq: 0x22
0b00101010, 0b10101010, // Run Lengh Chunk (zeros)
0b01101010, 0b10101010, // Run Lengh Chunk (ones)
0b11101010, 0b10101010, // Bit Vector Chunk
0b00000000, 0b00000000, // Terminating Null Chunk
// Extended Report DLRR
0x05, 0x00, 0x00, 0x06, // BT: 5 (DLRR), Block Length: 6
0x11, 0x12, 0x13, 0x14, // SSRC 1
0x00, 0x11, 0x00, 0x11, // LRR 1
0x11, 0x00, 0x11, 0x00, // DLRR 1
0x21, 0x22, 0x23, 0x24, // SSRC 2
0x00, 0x22, 0x00, 0x22, // LRR 2
0x22, 0x00, 0x22, 0x00, // DLRR 2
0x00, 0x00, 0x00, 0x04 // Padding (4 bytes)

Must properly handle RTCP padding

Having the above XR packet we can know (from the very beginning) that padding is 4 bytes. We know the RTCP length of the whole packet (14) and we know that the padding bit is set, so we can read the value of the last byte to know how many padding bytes are included and exclude them later when parsing blocks, items, subblocks, etc. In the above XR packet, some custom temporary logs show the problem:

RTC::RTCP::XR::Parse() | -- <START> len:40, offset:8
RTC::RTCP::XR::Parse() | ---- while iteration | len:40, offset:8
RTC::RTCP::XR::Parse() | ---- ExtendedReportBlock::Parse() | block type is DLRR
RTC::RTCP::XrDelaySinceLastRr::Parse() | -- <START> len:32, offset:4, reportLength:24
RTC::RTCP::XrDelaySinceLastRr::Parse() | ---- while iteration | len:32, offset:4
RTC::RTCP::XrDelaySinceLastRr::Parse() | ---- ssrcInfo parsed | size:12
RTC::RTCP::XrDelaySinceLastRr::Parse() | ---- while iteration | len:32, offset:16
RTC::RTCP::XrDelaySinceLastRr::Parse() | ---- ssrcInfo parsed | size:12
RTC::RTCP::XrDelaySinceLastRr::Parse() | ---- while iteration | len:32, offset:28
RTC::RTCP::XrDelaySinceLastRr::Parse() | ---- no ssrcInfo parsed
RTC::RTCP::XR::Parse() | ---- report block parsed | size:28
RTC::RTCP::XR::Parse() | ---- while iteration | len:40, offset:36
RTC::RTCP::XR::Parse() | ---- ExtendedReportBlock::Parse() | unknown RTCP XR block type [blockType:0]
RTC::RTCP::XR::Parse() | ---- no report block parsed

Do not include business loginc into RTCP packet classes

For example, the SDES packet class has this:

size_t GetSize() const override
{
	// A serialized packet can contain a maximum of 31 chunks.
	// If number of chunks exceeds 31 then the required number of packets
	// will be serialized which will take the size calculated below.

If the spec mandates up to 31 chunks in a SDES packet then this is valid, however it should be exposed somehow else instead of having a standard/core GetSize() returning a value that does not represent what the packet really holds.

@ibc ibc added this to the v4 milestone Nov 21, 2023
@ibc ibc self-assigned this Nov 21, 2023
@ibc ibc changed the title RTCP parsing needs a refactor RTP and RTCP parsing needs a refactor Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

1 participant