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

REMB - fix serialisation #1073

Merged
merged 1 commit into from Feb 24, 2024

Conversation

theimowski
Copy link
Contributor

I've found a bug in REMB packet serialisation.

For reference, here's the layout:

// 0                   1                   2                   3
// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
//+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
//|  Unique identifier 'R' 'E' 'M' 'B'                            |
//+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
//|  Num SSRC     | BR Exp    |  BR Mantissa                      |
//+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
//|   SSRC feedback                                               |

Based on sample Wireshark dissect:

image

current code reads mantissa as ..00 0110 0100 0010 0010 (first two bits are always 0)

Made following changes:

  • Mantissa is using 2 least significant bits from first byte, so it needs to be AND-ed with 3 (0000 0011 instead of 4 0000 0100) - fixed this both ways
  • Also added a roundtrip unit test - pretty much copied and adjusted the one above it.
  • The unit test showed another issue - when serialising REMB, the exponent and mantissa need to be OR-ed (instead of AND-ed) for the shared byte - otherwise that byte would always be 0, so fixed that as well

@sipsorcery
Copy link
Member

Thanks for the PR.

@sipsorcery sipsorcery merged commit 804a4c2 into sipsorcery-org:master Feb 24, 2024
1 check failed
@theimowski
Copy link
Contributor Author

Cheers! Any chance of getting a new version including the fix released any time soon?

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