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 WithTailChecker option to depacketizer #1882

Closed
wants to merge 1 commit into from

Conversation

jech
Copy link
Member

@jech jech commented Jul 13, 2021

While Galene is now using its own samplebuilder, it would be good
to avoid splitting the community by having the public interfaces
diverge too much. This means preventing internal details of the
samplebuilder from leaking into other parts of Pion.

An earlier commit sneaked in the IsDetectedFinalPacketInSequence
method to the rtp.Depacketizer interface. Not only does this
break compatibility with earlier versions, it requires every single
implementation of rtp.Depacketizer to implement essentially the
same code. It is also conceptually wrong, as the job of finding
the end of the packet belongs in the samplebuilder, not in the
depacketizer.

In this commit, we treat partition ends similarly to partition
starts: we add a new callback to the samplebuilder whose role
is to determine whether a packet ends a partition. A new option
WithPartitionTailChecker, analogous to WithPartitionHeadChecker,
can be used to set that field. Tests are adapted.

@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #1882 (6a0dd9b) into master (36cf395) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1882      +/-   ##
==========================================
- Coverage   75.61%   75.55%   -0.06%     
==========================================
  Files          84       84              
  Lines        6049     6051       +2     
==========================================
- Hits         4574     4572       -2     
- Misses       1084     1088       +4     
  Partials      391      391              
Flag Coverage Δ
go 77.24% <100.00%> (-0.07%) ⬇️
wasm 71.33% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
pkg/media/samplebuilder/samplebuilder.go 94.20% <100.00%> (+0.08%) ⬆️
sctptransport.go 76.11% <0.00%> (-3.34%) ⬇️
dtlstransport.go 63.88% <0.00%> (-1.86%) ⬇️
peerconnection.go 74.95% <0.00%> (+0.26%) ⬆️
datachannel.go 82.98% <0.00%> (+1.24%) ⬆️

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 36cf395...6a0dd9b. Read the comment docs.

An earlier commit sneaked in the IsDetectedFinalPacketInSequence
method to the rtp.Depacketizer interface.  Not only does this
break compatibility with earlier versions, it requires every single
implementation of rtp.Depacketizer to implement essentially the
same code.  It is also conceptually wrong, as the job of finding
the end of the packet belongs in the samplebuilder, not in the
depacketizer.

In this commit, we treat partition ends similarly to partition
starts: we add a new callback to the samplebuilder whose role
is to determine whether a packet ends a partition.  A new option
WithPartitionTailChecker, analogous to WithPartitionHeadChecker,
can be used to set that field.  Tests are adapted.
@Sean-Der
Copy link
Member

Since we have IsDetectedFinalPacketInSequence I am going to close this as wontfix for v3.*

The API disagreement is unfortunate, but I think we need to stick with it until v4. @jech If you feel strongly I am in support of completely reworking the public API for v4 we have a couple other things for it as well.

@Sean-Der Sean-Der closed this Aug 14, 2021
@jech
Copy link
Member Author

jech commented Aug 15, 2021

@Sean-Der in that case, I suppose we move the PartitionHeadChecker into the codec too, in order to avoid having an inconsistent API. I'll see if I have time to submit a PR in time for 3.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants