Skip to content

Add RTP parameter sets and RTCP SDP support#501

Open
thangldw wants to merge 4 commits into
sony:masterfrom
thangldw:fix/is05-rtp-parameter-sets-rtcp
Open

Add RTP parameter sets and RTCP SDP support#501
thangldw wants to merge 4 commits into
sony:masterfrom
thangldw:fix/is05-rtp-parameter-sets-rtcp

Conversation

@thangldw

Copy link
Copy Markdown

Summary

  • refactor RTP Connection API resource creation into composable core, multicast, FEC, and RTCP parameter sets
  • preserve the existing sender and receiver factory overloads and their historical defaults
  • add the missing FEC and RTCP JSON field accessors
  • parse and generate the RFC 3605 a=rtcp SDP attribute, including the port-only form
  • add parameter-set, auto resolution, specification example, malformed input, and round-trip tests

Scope

This is the first reviewable slice of issue #38. It provides the Connection API parameter sets and completes RTCP SDP handling. SMPTE 2022-5 FEC SDP and SMPTE 2022-7 temporal redundancy remain follow-up work, so this PR intentionally uses Refs #38 rather than closing the issue.

Testing

  • Debug build of nmos-cpp and nmos-cpp-test
  • parameter-set tests: 41 assertions in 2 test cases
  • RTCP tests: 7 assertions in 2 test cases
  • full suite: 2060 assertions in 163 test cases

Refs #38

@garethsb

Copy link
Copy Markdown
Contributor

Thanks, the additions look pretty good to me. It's only been 7 years since I ran out of effort to finish this off. 😀
When the CI is passing, I'll try to do a full review.

@thangldw

Copy link
Copy Markdown
Author

Thanks for the feedback. I checked the current CI results: all Linux jobs pass, while all four Windows jobs fail consistently with a segfault in testInterpretationOfSdpFilesRtcp.

I will address this before any further feature work. My next update will:

  • replace the RTCP whitespace/field parsing path with a simple deterministic token parser that does not depend on regex behavior across standard-library implementations;
  • split the RTCP coverage into focused tests for the full RFC 3605 form, the port-only form, and malformed partial forms, so failures are localized;
  • run the focused tests and the full local suite, then review the diff and API compatibility before pushing the fix to this PR.

I will wait for the Windows matrix to pass after that update before asking for the full review.

@thangldw

Copy link
Copy Markdown
Author

The Windows CI fix is now pushed in 278f2e8.

The RTCP parser now tokenizes the RFC 3605 fields directly without the regex-based whitespace path, and the coverage is split into full-form, port-only, and malformed-form test cases. Locally, the focused RTCP/parameter-set tests pass, and the full suite passes with 2,062 assertions across 165 test cases. I also reviewed the final diff: it changes no public API or SDP output format, and the temporary Apple Silicon Catch compatibility changes used for local testing were fully reverted.

The new GitHub Actions run is currently marked action_required, so it appears to be waiting for repository approval before the Windows/Linux matrix can start.

@garethsb

garethsb commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Can you explain why, in the rtcp grammar, you can't just use strings_converter (as RFC 3605 uses RFC 2327 space=%d32 single character) or follow a pattern more similar to the source_filter parse function just above with a quick check mirroring the generate function for whether there are more fields after the first one?

Other than that little question, this looks really good to me - nice design, backwards compatible, good test cases. Thank you!

@thangldw

Copy link
Copy Markdown
Author

You're right—there isn't a technical reason that strings_converter cannot be used here. I introduced the separate tokenization only to enforce the two valid cardinalities (one field or four fields) while accepting horizontal whitespace, but RFC 3605 inherits space = %d32 from RFC 2327, so accepting broader whitespace was unnecessary and less consistent with the existing grammar.

I'll change the parse function to use strings_converter.parse(s), keep the explicit size check for exactly 1 or 4 fields, and then map those fields into the structured object. That will mirror the generate function and follow the existing project pattern more closely. Thanks for pointing this out.

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.

2 participants