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

Add PaddingSize to rtp.Packet #155

Merged
merged 1 commit into from
Nov 22, 2021
Merged

Add PaddingSize to rtp.Packet #155

merged 1 commit into from
Nov 22, 2021

Conversation

boks1971
Copy link
Contributor

When a packet has padding, the padding size information
is lost during unmarshal. Add a PaddingSize field to
Packet structure.

Also support marshalling on an RTP packet with padding.

Testing:

Modified/added unit tests.

When a packet has padding, the padding size information
is lost during unmarshal. Add a PaddingSize field to
Packet structure.

Also support marshalling on an RTP packet with padding.

Testing:
--------
Modified/added unit tests.
@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #155 (762664c) into master (45d5015) will increase coverage by 0.24%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
+ Coverage   93.06%   93.31%   +0.24%     
==========================================
  Files          14       14              
  Lines         937      942       +5     
==========================================
+ Hits          872      879       +7     
+ Misses         46       45       -1     
+ Partials       19       18       -1     
Flag Coverage Δ
go 93.31% <88.88%> (+0.24%) ⬆️
wasm 92.88% <88.88%> (+0.25%) ⬆️

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

Impacted Files Coverage Δ
packet.go 87.71% <88.88%> (+1.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45d5015...762664c. Read the comment docs.

@boks1971 boks1971 merged commit db6ce1f into master Nov 22, 2021
@boks1971 boks1971 deleted the raja_padding_size branch December 19, 2021 04:02
@aler9 aler9 mentioned this pull request Mar 8, 2022
aler9 added a commit to aler9/rtp that referenced this pull request Mar 8, 2022
A Marshal function shouldn't change any property of the object
that is being marshaled. Otherwise, Marshal() is non-thread safe
and can't be called by multiple goroutines in parallel.

PR pion#155 makes Packet.Marshal() set the Packet.Header.Padding bit
when Packet.PaddingSize is non-zero; while this is preferable
from the user point of view, it doesn't allow thread safety.

This patch fixes the issue.
aler9 added a commit to aler9/rtp that referenced this pull request Mar 8, 2022
A Marshal function shouldn't change any property of the object that is
being marshaled. Otherwise, Marshal() is non-thread safe and can't be
called by multiple goroutines in parallel.

PR pion#155 makes Packet.Marshal() set the Packet.Header.Padding bit when
Packet.PaddingSize is non-zero; while this is preferable from the user
point of view, it doesn't allow thread safety.

This patch fixes the issue.
aler9 added a commit to aler9/rtp that referenced this pull request Mar 8, 2022
A Marshal function shouldn't change any property of the object that is
being marshaled. Otherwise, Marshal() is non-thread safe and can't be
called by multiple goroutines in parallel.

PR pion#155 makes Packet.Marshal() set the Packet.Header.Padding bit when
Packet.PaddingSize is non-zero; while this is preferable from the user
point of view, it doesn't allow thread safety.

This patch fixes the issue.
aler9 added a commit to aler9/rtp that referenced this pull request Mar 17, 2022
A Marshal function shouldn't change any property of the object that is
being marshaled. Otherwise, Marshal() is non-thread safe and can't be
called by multiple goroutines in parallel.

PR pion#155 makes Packet.Marshal() set the Packet.Header.Padding bit when
Packet.PaddingSize is non-zero; while this is preferable from the user
point of view, it doesn't allow thread safety.

This patch fixes the issue.
aler9 added a commit that referenced this pull request Mar 17, 2022
A Marshal function shouldn't change any property of the object that is
being marshaled. Otherwise, Marshal() is non-thread safe and can't be
called by multiple goroutines in parallel.

PR #155 makes Packet.Marshal() set the Packet.Header.Padding bit when
Packet.PaddingSize is non-zero; while this is preferable from the user
point of view, it doesn't allow thread safety.

This patch fixes the issue.
@bixycler
Copy link

bixycler commented Mar 5, 2023

Also support marshalling on an RTP packet with padding.

But even with marshalled packet, the plaintext will always be unmarshalled and its padding info is thrown away: both webrtc.TrackLocalStaticRTP.Write(b []byte) and srtp.SessionSRTP.write(b []byte) just p.Unmarshal(b) then call the "writeRTP()" counterparts, where the packet is not passed down as a whole but only its header and payload (missing PaddingSize) are passed down via b.writeStream.WriteRTP(&p.Header, p.Payload), to TrackLocalWriter,... and to srtp.Context.encryptRTP(dst []byte, header *rtp.Header, payload []byte). See pion/webrtc#2403 for details.

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.

3 participants