Moved PaddingSize field from Packet to Header

[sirzooro]

Parts of pion code passes RTP header and payload via separate parameters instead of using Packet struct to pass them together. As a result value of padding size field may be lost. This change adds PaddingSize field to the Header struct and marks one in Packet as a deprecated.

This is part of fix for pion/webrtc#2403 . Another change is needed for pion/srtp to resolve it fully.

[codecov]

Codecov Report

Attention: Patch coverage is 80.76923% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.85%. Comparing base (ea0d786) to head (a8b82f7).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
packet.go	80.76%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   86.88%   86.85%   -0.03%     
==========================================
  Files          26       26              
  Lines        3087     3104      +17     
==========================================
+ Hits         2682     2696      +14     
- Misses        348      350       +2     
- Partials       57       58       +1     

Flag	Coverage Δ
go	86.85% <80.76%> (-0.03%)	⬇️
wasm	86.30% <80.76%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JoTurk]

Thank you for taking care of this project, This is something that was on my todo list, I think adding it to the header is the best approach, just few nitpicks

[JoTurk]

You have my vote and I agree with this change, padding size should belong in the header, But please don't merge it until we hear from others.
CC @Sean-Der @jech what do you think?

[aler9]

Hello, this change doesn't fix anything inside this specific library (pion/rtp) and makes it more difficult to use since the resulting public interface is less clear and doesn't correspond to a real RTP packet structure anymore, since the padding is moved inside the header while it doesn't belong there (as written inside the code too).

Personally i would have fixed things inside pion/webrtc in order to route PaddingSize together with Header, creating there (not here) a dedicated structure, like:

type headerPaddingSizePair struct {
    header rtp.Header
    paddingSize int
}

instead of making this, that feels like a workaround.

Said that, if there are no alternatives for fixing an important issue, this can be merged since it doesn't cause any conflict with existing code.

[JoTurk]

instead of making this, that feels like a workaround.

Yes this is a workaround because we can't change WriteRTP(header *rtp.Header, payload []byte), It's true that it's not related to RTP, If we went to do it right, I suggest we fix it in webrtc and srtp and break changes or introduce new APIs, But I think it's safer to do a workaround and wait for pion 5.

We can make a list for the stuff we should fix / break for pion 5, This seems like a good fit!

[sirzooro]

Hello, this change doesn't fix anything inside this specific library (pion/rtp) [...]

Yes. Full fix also requires change to pion/srtp, where padding has to be appended to payload before encryption. pion/srtp has to know padding size to do this. Public API has to be backward compatible, so this looked like a best approach.

[...] and makes it more difficult to use since the resulting public interface is less clear and doesn't correspond to a real RTP packet structure anymore, since the padding is moved inside the header while it doesn't belong there (as written inside the code too).

Size fields usually are in protocol headers. But RTP was designed to be as compact as possible, so designers did not want to waste extra bytes to send packet size and padding size in RTP header. Using one bit to mark presence of padding and sending padding size in last byte of the packet allowed to reduce packet size, even that this approach is a bit hacky.

Personally i would have fixed things inside pion/webrtc in order to route PaddingSize together with Header, creating there (not here) a dedicated structure, like:

type headerPaddingSizePair struct {
    header rtp.Header
    paddingSize int
}

instead of making this, that feels like a workaround.

Said that, if there are no alternatives for fixing an important issue, this can be merged since it doesn't cause any conflict with existing code.

This would break public API, so this is something to consider for pion v5.0.x. I think that it should be changed to send RTP header and raw (marshaled) RTP packet in one struct. SRTP ciphers works with buffers, so having RTP packet as raw bytes would simplify things there. This is especially important for Cryptex (full header encryption, RFC 9335) support. There is already WebRTC extension defined for it, so after some time it will became part of the standard and browsers will start using it: https://w3c.github.io/webrtc-extensions/#rtp-header-extension-encryption

BTW, I am going to implement Cryptex in pion/srtp.

[at-wat]

LGTM!
Let's have a full fix in the next major update

[at-wat]

The function name looks like an immutable method but have a side effect. (Clone() as well)
I think it's reasonable for now but maybe better to note this as a godoc comment.

[sirzooro]

Thanks. I have updated code to remove this side effect.

[sirzooro]

@joeturki there are no new issues, are you ok with merging this?

[JoTurk]

looks good to me, thank you for fixing this <3

[sirzooro]

Thanks! To fully fix this, I need to create two more PRs, for srtp and webtc.