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.

Sign up for GitHub

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

Jump to bottom

RtpPacket: Make (almost) all methods remove payload padding if present #1380

Merged
merged 6 commits into from
Apr 22, 2024
Merged

RtpPacket: Make (almost) all methods remove payload padding if present #1380

merged 6 commits into from
Apr 22, 2024

Conversation

ibc
Copy link
Member

@ibc ibc commented Apr 22, 2024 •
edited

Fixes #1379

Details

Before this PR:

  • We have inconsistent behavior: some methods in RtpPacket keep existing payload padding after mangling the payload others don't.
  • Those methods that keep existing payload do it wrong (real bug here) since they do NOT write the number of padding bytes in the last byte of the padding.

After this PR:

  • All methods (but clone()) remove payload padding if present. Period.
  • There is no real need to keep padding (AKA be padded to 4 bytes). That's just for super legazy plain RTP engines.
  • "And why don't we keep existing padding instead?". See section above "Before this PR". Lot of work for little/null benefit. I don't want to waste much energy on this topic given that (when time comes in) I'd like to focus on task RTP and RTCP parsing needs a refactor #1233 instead.

@ibc
Improve RtpPacket::SetPayloadLength()
a703ca9 
Fixes #1379

### Details

- Make `RtpPacket::SetPayloadLength()` automatically adjust padding so padding+payload is padded to 4 bytes.
@ibc ibc requested a review from jmillan April 22, 2024 09:38
@ibc ibc mentioned this pull request Apr 22, 2024
Closed
ibc
ibc commented Apr 22, 2024
View reviewed changes
worker/src/RTC/RtpPacket.cpp Outdated Show resolved Hide resolved
@ibc ibc requested a review from jmillan April 22, 2024 15:46
@ibc ibc changed the title Improve RtpPacket::SetPayloadLength() RtpPacket: Make all methods remove payload padding if present Apr 22, 2024
@ibc ibc merged commit 3d5fd76 into v3 Apr 22, 2024
35 checks passed
@ibc ibc deleted the improve-rtp-packet-set-payload-length branch April 22, 2024 18:42
@ibc ibc changed the title RtpPacket: Make all methods remove payload padding if present RtpPacket: Make (almost) all methods remove payload padding if present Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmillan jmillan jmillan approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RtpPacket::SetPayloadLength get wrong payloadLength
2 participants
@ibc @jmillan