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

Handling of padding in TransportCC #52

Closed
LittleLightLittleFire opened this issue May 24, 2020 · 1 comment · Fixed by #53
Closed

Handling of padding in TransportCC #52

LittleLightLittleFire opened this issue May 24, 2020 · 1 comment · Fixed by #53

Comments

@LittleLightLittleFire
Copy link
Contributor

LittleLightLittleFire commented May 24, 2020

@adwpc Regarding: https://github.com/pion/rtcp/blob/master/transport_layer_cc_test.go#L597

// change last byte '0b00000001' to '0b00000000', make ci pass
0x20, 0x1, 0x94, 0x0,
// 0b00100000, 0b00000001, 0b10010100, 0b00000001,
// the 'Want []byte' came from chrome, and
// the padding byte is '0b00000001', but i think should be '0b00000000' when i read the RFC, what's wrong?
// webrtc code: https://webrtc.googlesource.com/src/webrtc/+/f54860e9ef0b68e182a01edc994626d21961bc4b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc

Even though the RFC says "zero padding", Chrome implements it as:

  if (padding_length > 0) {
    for (size_t i = 0; i < padding_length - 1; ++i) {
      packet[(*position)++] = 0;
    }
    packet[(*position)++] = padding_length;
  }

https://chromium.googlesource.com/external/webrtc/+/refs/heads/master/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc#662

That is, zero padding where the last octet is the length of the padding itself.

This is consistent with the usage of the padding bit in https://tools.ietf.org/html/rfc3550#section-6.4.1 where

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).

So if the RTCP packet has Padding = True, then last octet of the zero padding is the length of padding itself. Right now Pion sends invalid feedback packets if Padding is set to true, and Chrome is rejecting these packets based on the invalid padding.

As a workaround setting Padding = False will make Chrome accept the feedback reports since it is explictly set to allow all zero padding as backwards compatibility with "older" clients. (https://chromium.googlesource.com/external/webrtc/+/refs/heads/master/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc#475)

I have prepared a pull request #53

@adwpc
Copy link
Member

adwpc commented May 24, 2020

@LittleLightLittleFire Awsome!Will merge when the conflict is resolved.

@adwpc adwpc closed this as completed in #53 May 24, 2020
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 a pull request may close this issue.

2 participants