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.

Sign up for GitHub

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

Jump to bottom

Fixes transportcc padding #53

Merged
merged 2 commits into from
May 24, 2020
Merged

Fixes transportcc padding #53

merged 2 commits into from
May 24, 2020

Conversation

LittleLightLittleFire
Copy link
Contributor

@LittleLightLittleFire LittleLightLittleFire commented May 24, 2020 •
edited
Loading

If padding is set in the header, then the last octet of padding is set
to be the length of padding itself. This is consistent with the usage of
"zero padding" in SR.

If the padding bit is set, this individual RTCP packet contains
some additional padding octets at the end which are not part of
the control information but are included in the length field. The
last octet of the padding is a count of how many padding octets
should be ignored, including itself (it will be a multiple of
four).

https://tools.ietf.org/html/rfc3550#section-6.4.1

Reference issue

Fixes #52

@LittleLightLittleFire LittleLightLittleFire mentioned this pull request May 24, 2020
Closed
@Sean-Der
Copy link
Member

@LittleLightLittleFire nice work! This LGTM, if you just add a body to your commit message it should pass the linter!

Sorry for the frustration, but it helps debugging in the future to have the verbose git log.

@LittleLightLittleFire LittleLightLittleFire force-pushed the 52-fixes-padding-in-transport-cc branch 2 times, most recently from cf02853 to ebd045f Compare May 24, 2020 09:41
@LittleLightLittleFire LittleLightLittleFire changed the title Fixes tranportcc padding Fixes tranportscc padding May 24, 2020
@LittleLightLittleFire LittleLightLittleFire changed the title Fixes tranportscc padding Fixes transportcc padding May 24, 2020
@codecov
Copy link

codecov bot commented May 24, 2020 •
edited
Loading

Codecov Report

Merging #53 into master will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   71.58%   71.50%   -0.08%     
==========================================
  Files          17       17              
  Lines        1070     1067       -3     
==========================================
- Hits          766      763       -3     
  Misses        248      248              
  Partials       56       56
Flag Coverage Δ
#go 71.50% <100.00%> (-0.08%) ⬇️
#wasm 71.50% <100.00%> (-0.08%) ⬇️
Impacted Files Coverage Δ
util.go 100.00% <ø> (ø)
transport_layer_cc.go 73.50% <100.00%> (+0.54%) ⬆️

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 a4af353...48780f0. Read the comment docs.

@LittleLightLittleFire
Copy link
Contributor Author

@Sean-Der Cheers, all done.

adwpc
adwpc approved these changes May 24, 2020
View reviewed changes
Copy link
Member

@adwpc adwpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM！Thanks!

@adwpc
Copy link
Member

adwpc commented May 24, 2020

@LittleLightLittleFire This branch cannot be rebased due to conflicts

@LittleLightLittleFire
Fixes transportcc padding
27e7666 
If padding is set in the header, then the last octet of padding is set
to be the length of padding itself. This is consistent with the usage of
"zero padding" in SR.

> If the padding bit is set, this individual RTCP packet contains
> some additional padding octets at the end which are not part of
> the control information but are included in the length field.  The
> last octet of the padding is a count of how many padding octets
> should be ignored, including itself (it will be a multiple of
> four).

https://tools.ietf.org/html/rfc3550#section-6.4.1
@LittleLightLittleFire
Removes dumpBinary debug function
48780f0 
CI is complaining of deadcode.
@LittleLightLittleFire
Copy link
Contributor Author

@adwpc Looks like CI doesn't like your commit message (no body) on the merge. I force pushed it again after rebasing, would make cleaner history anyway.

@LittleLightLittleFire
Copy link
Contributor Author

LGTM

@adwpc adwpc merged commit 7dd7bdc into pion:master May 24, 2020
@LittleLightLittleFire LittleLightLittleFire deleted the 52-fixes-padding-in-transport-cc branch May 24, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adwpc adwpc adwpc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handling of padding in TransportCC
3 participants
@LittleLightLittleFire @Sean-Der @adwpc