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

Adapt the samplebuilder to the new version of the depacketizer. #1928

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

jech
Copy link
Contributor

@jech jech commented Aug 22, 2021

Related to pion/rtp#150 and #1882.

Also fixes a bug in the depacketizer: if there was no PartitionHeadChecker, the depacketizer would treat every packet as a potential partition head, even if it was at the beginning of the buffer. Since I didn't want to tweak the tests to account for the fixed bug, I simply had the head checker return true if headBytes is nil. Ideally, the tests should be fixed to do the right thing, which I have done in https://github.com/jech/depacketizer

@jech jech force-pushed the consistent-depacketizer branch 4 times, most recently from 457e473 to 80d718d Compare August 23, 2021 13:28
@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #1928 (0db6cb8) into master (efc9500) will increase coverage by 0.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1928      +/-   ##
==========================================
+ Coverage   75.55%   75.77%   +0.22%     
==========================================
  Files          85       85              
  Lines        6123     6121       -2     
==========================================
+ Hits         4626     4638      +12     
+ Misses       1101     1092       -9     
+ Partials      396      391       -5     
Flag Coverage Δ
go 77.46% <100.00%> (+0.24%) ⬆️
wasm 70.90% <100.00%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
pkg/media/samplebuilder/samplebuilder.go 95.52% <100.00%> (-0.07%) ⬇️
icetransport.go 67.45% <0.00%> (-1.19%) ⬇️
peerconnection.go 74.87% <0.00%> (+0.25%) ⬆️
datachannel.go 82.98% <0.00%> (+1.24%) ⬆️
dtlstransport.go 65.74% <0.00%> (+1.85%) ⬆️
operations.go 94.59% <0.00%> (+2.70%) ⬆️
internal/mux/mux.go 90.47% <0.00%> (+7.93%) ⬆️

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 efc9500...0db6cb8. Read the comment docs.

@Sean-Der Sean-Der force-pushed the consistent-depacketizer branch 2 times, most recently from 1281197 to 23a436c Compare August 23, 2021 14:53
Both partition head and tail checking is now done in the
depacketizer.  For backwards compatibility, there are stubs
for PartitionHeadChecker and WithPartitionHeadChecker that do
nothing; these should be removed for 4.0.

This also fixes a bug with the depacketizer: if no head checker
was present, every packet would be considered as a potential
partition head, even if it was at the beginning of the buffer.
Since a partition head checker is now always present, the bug
cannot happen.

The tests assume the old bug, which is why the fakePacketizer
returns true if headBytes is empty.  It would be better to adapt
the tests to do the right thing, as in jech/depacketizer.
@Sean-Der Sean-Der merged commit fad7214 into pion:master Aug 23, 2021
at-wat added a commit that referenced this pull request Mar 4, 2024
Deprecated by #1928

BREAKING CHANGE: samplebuilder.WithPartitionHeadChecker
                 option is removed.
at-wat added a commit that referenced this pull request Mar 14, 2024
Deprecated by #1928

BREAKING CHANGE: samplebuilder.WithPartitionHeadChecker
                 option is removed.
ourwarmhouse added a commit to ourwarmhouse/store-backend-go that referenced this pull request May 1, 2024
Deprecated by pion/webrtc#1928

BREAKING CHANGE: samplebuilder.WithPartitionHeadChecker
                 option is removed.
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.

None yet

2 participants