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

Rollback to v1 #227

Merged
merged 49 commits into from
Jun 14, 2023
Merged

Rollback to v1 #227

merged 49 commits into from
Jun 14, 2023

Conversation

aler9
Copy link
Member

@aler9 aler9 commented Jun 12, 2023

Description

As discussed in #221, this reverts the library to /v1 by including only backward-compatible changes, in order to restore the release cycle of this library, that has been stuck for over a year.

In particular, i did the following:

  1. the library was reverted to the last commit before bumping to v2 (4e87540)
  2. all non-breaking commits from the v1 branch were cherry-picked
  3. all non-breaking commits from the master branch were cherry-picked
  4. the following breaking commits from master were edited and merged:
  5. the following breaking commits were left out and will be readded with a PR in a non-breaking form:

@aler9 aler9 requested review from stv0g and Sean-Der June 12, 2023 21:11
@aler9 aler9 force-pushed the new-master branch 4 times, most recently from 8b8bcd7 to ff7a88c Compare June 12, 2023 21:16
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Patch coverage: 70.00% and project coverage change: -0.29 ⚠️

Comparison is base (c5ca73a) 87.81% compared to head (5b8bbd2) 87.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
- Coverage   87.81%   87.53%   -0.29%     
==========================================
  Files          20       20              
  Lines        1863     1877      +14     
==========================================
+ Hits         1636     1643       +7     
- Misses        197      204       +7     
  Partials       30       30              
Flag Coverage Δ
go 87.53% <70.00%> (-0.29%) ⬇️
wasm 86.57% <70.00%> (-0.28%) ⬇️

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

Impacted Files Coverage Δ
codecs/av1_packet.go 100.00% <ø> (ø)
codecs/h264_packet.go 97.89% <0.00%> (-1.05%) ⬇️
codecs/opus_packet.go 88.23% <0.00%> (-11.77%) ⬇️
codecs/vp8_packet.go 89.93% <0.00%> (-1.15%) ⬇️
codecs/vp9_packet.go 94.87% <0.00%> (-0.82%) ⬇️
pkg/frame/av1.go 100.00% <ø> (ø)
packet.go 88.40% <95.00%> (+0.46%) ⬆️
packetizer.go 79.24% <100.00%> (+0.39%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

aler9 and others added 23 commits June 12, 2023 23:17
Update lint scripts and CI configs.
In RFC 7741, PictureID is not mandatory and
required only if SLI is used.
Update lint scripts and CI configs.
This was added in draft-ietf-payload-vp9-10.
Update lint scripts and CI configs.
A MTU can never be negative so this removes a class of runtime errors.
Chrome need SPS and PPS in one STAP-A, and
timestamp of STAP-A should be same with next
I-Frame. Otherwise Chrome will discard SPS and PPS
before receiving I frame.
We used to do partition head checking in PartitionHeadChecker and
partition tail checking in Depacketizer, which is confusing and
error-prone.  Move all checking into the Depacketizer itself.
WebRTC clients use padding only packets as probe
packets for bandwidth estimation. As padding was
not removed from the payload, downstream code
trying to parse padding as valid media payload
results in erroneous parsing

Testing:
--------
- Add unit tests for different padding scenarios
- Check that ion-sfu does not try to parse padding only
packets as valid video payload.
Update lint scripts and CI configs.
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.
Update lint scripts and CI configs.
MTU was changed to an unsigned integer
Update lint scripts and CI configs.
This change adds an interface and impl for zero-copy extensions.
This change makes the H265 packet satisfy the depacketizer
interface allowing H265 streams to be decoded with
SampleBuilder.
Update lint scripts and CI configs.
Update lint scripts and CI configs.
Update lint scripts and CI configs.
Marshal, MarshalTo, MarshalSize, Clone functions shouldn't have a
pointer, since they're constant. This convention is used by pion/rtcp
and the standard library, and prevents coding errors.
Implements Marshal and Unmarshal support. Marshal doesn't pack multiple
OBUs into one packet and could be improved.
kevmo314 and others added 14 commits June 12, 2023 23:17
Add AbsCaptureTimeExtension and PlayoutDelayExtension implementations.
Both of these are experimental RTP header extensions defined in
libwebrtc.
Multiple optional fields of the VP9 RTP header are read regardless of
the buffer size. This can easily lead to multiple crashes. This PR add
the necessary checks in order to avoid crashes.
All of the fields in the VP8 header except the first byte are
optional.  We used to reject VP8 packets smaller than 4 bytes,
which is incorrect.

There are two cases where such packets may appear on the wire.
GStreamer's WebRTC implementation generates VP8 streams with
no picture id, and one-byte headers.  It will occasionally
generate packets that are below 4 bytes, and which we used
to reject.

The second use case is more theoretical.  According to RFC 7741
Section 4.4, a packetizer may ignore VP8 partition boundaries.
If it splits a packet outside of a partition boundary, it may
generate a packet with S=0 and a one-byte header.
This prevents gofmt -w -s from messing up existing code.

Text with multiple spaces is now wrapped in long comments (/* */), that
are not affected by gofmt.
Remove checks since payload size can be less than 2.

Only FU-A explicitly accesses the second byte, which is not affected
because a separate boundary value check is performed.
Update lint scripts and CI configs.
Update lint scripts and CI configs.
Update lint scripts and CI configs.
@aler9 aler9 changed the title Restore /v1 Rollback to v1 Jun 13, 2023
aler9 and others added 4 commits June 13, 2023 13:12
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
@Sean-Der
Copy link
Member

Fantastic! Thank you @aler9

@Sean-Der Sean-Der merged commit 5d59489 into pion:master Jun 14, 2023
@aler9 aler9 deleted the new-master branch July 16, 2023 21:08
aler9 added a commit to aler9/rtp that referenced this pull request Jul 28, 2023
This was already fixed by pion#168 but got lost in pion#227. in SFUs, in order
to distribute a packet to all clients, MarshalTo() is called in
parallel by multiple routines, causing a race condition because the
padding flag is dynamically set inside MarshalTo(). This is particular
annoying when running automated tests. This PR fixes the issue by
removing this write operation as discussed in pion#168.
aler9 added a commit that referenced this pull request Jul 28, 2023
This was already fixed by #168 but got lost in #227. in SFUs, in order
to distribute a packet to all clients, MarshalTo() is called in
parallel by multiple routines, causing a race condition because the
padding flag is dynamically set inside MarshalTo(). This is particular
annoying when running automated tests. This PR fixes the issue by
removing this write operation as discussed in #168.
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.