Skip to content

fix(rtp): reset PaddingSize when Padding bit is unset (closes #140)#142

Open
Booyaka101 wants to merge 1 commit into
emiago:mainfrom
Booyaka101:fix/rtp-padding-reset-140
Open

fix(rtp): reset PaddingSize when Padding bit is unset (closes #140)#142
Booyaka101 wants to merge 1 commit into
emiago:mainfrom
Booyaka101:fix/rtp-padding-reset-140

Conversation

@Booyaka101
Copy link
Copy Markdown

Summary

rtpUnmarshalPayload reuses the same *rtp.Packet across reads. When a packet with Padding=true is followed by one with Padding=false, the second packet's PaddingSize field is left stale at the first packet's value. The downstream payload-size calculation (rtp_packet_reader.go:153, rtp_session.go:265) subtracts pkt.PaddingSize unconditionally, so the stale value truncates the second packet's payload.

Reported and root-caused by @brandon-hc in #140.

Fix

Reset p.PaddingSize = 0 in the else branch of rtpUnmarshalPayload, matching pion/rtp's own Unmarshal behavior at packet.go:240-243.

 	end := len(buf)
 	if p.Header.Padding {
 		p.PaddingSize = buf[end-1]
 		end -= int(p.PaddingSize)
+	} else {
+		p.PaddingSize = 0
 	}

The downstream consumers at rtp_packet_reader.go:153 and rtp_session.go:265 are unchanged — the fix is at the unmarshal layer where the field gets populated.

Regression test

TestRTPUnmarshalResetsPaddingSize exercises the exact reuse scenario: marshal a padded packet, unmarshal into pkt, then marshal an unpadded packet and unmarshal into the same pkt. Asserts pkt.PaddingSize == 0 and the full payload survives.

Verification

State Platform Result
Pre-fix Windows + Go 1.26 FAIL: expected 0x0, actual 0x4 at rtp_parse_test.go:56 — confirms the test exercises the bug
Post-fix Windows + Go 1.26 PASS (regression test + full go test ./media/ suite, 1.7s)
Post-fix Linux (golang:1.26 container) PASS

Out of scope

  • The pion pkt.PaddingSize field is now deprecated in favor of pkt.Header.PaddingSize. Diago still uses the legacy field; migrating is a larger structural change and a separate concern.
  • Video support / refactoring discussed in the issue thread is orthogonal — this fix is correctness for any RTP usage, not video-specific.

)

`rtpUnmarshalPayload` reuses the *rtp.Packet across reads. When a packet
with Padding=true was followed by a packet with Padding=false, the
second packet's PaddingSize was left stale at the first packet's value.
The downstream payload-size calculation in rtp_packet_reader.go:153 and
rtp_session.go:265 subtracts pkt.PaddingSize unconditionally, so stale
values truncate the second packet's payload.

Reset p.PaddingSize = 0 in the else branch, matching pion/rtp's own
Unmarshal behavior (packet.go:240-243).

Reported and root-caused by @brandon-hc.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@emiago
Copy link
Copy Markdown
Owner

emiago commented May 4, 2026

I am having hard time to follow on this, but we can merge this as I see.

@Booyaka101
Copy link
Copy Markdown
Author

Sure — boiling it down:

Diago reuses one *rtp.Packet across reads. rtpUnmarshalPayload only writes p.PaddingSize on the padded branch, so when a padded packet is followed by an unpadded one, PaddingSize stays stuck at the previous packet's value. Downstream code (rtp_packet_reader.go:153, rtp_session.go:265) subtracts pkt.PaddingSize unconditionally, so that stale value chops bytes off the new packet's payload.

The one-line fix sets p.PaddingSize = 0 in the else branch — pion/rtp's own Unmarshal does the same thing at packet.go:240-243, which is why the field never goes stale on their side. The regression test marshals a padded packet then an unpadded one into the same pkt and asserts the payload survives; it fails on main and passes with the fix.

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.

2 participants