Support SMPTE 2022-7 temporal redundancy SDP#502
Conversation
garethsb
left a comment
garethsb
left a comment
There was a problem hiding this comment.
Also looks pretty good, thank you. Just a few comments so far!
| const web::json::field_as_string ssrc_attribute{ U("ssrc_attribute") }; | ||
| const web::json::field_as_value_or ssrc_attribute_value{ U("ssrc_attribute_value"), {} }; |
There was a problem hiding this comment.
I'm wondering whether it would be possible/nice in the JSON model to represent the "source attribute" exactly the same as a top-level attribute is represented but nested here?
I.e. something like:
{ sdp::fields::ssrc_id, id },
{ sdp::fields::attribute, value_of({
{ sdp::fields::name, U("cname") },
{ sdp::fields::value, cname }
}) }There was a problem hiding this comment.
Agreed. Representing the SSRC source attribute as a nested name/value object reuses the same JSON shape as a top-level SDP attribute and avoids baking cname-specific fields into the model. I will replace the separate SSRC attribute name/value fields with an attribute object, then update the grammar, generation/parsing paths, and round-trip tests accordingly.
| return sdp::group_semantics::duplication == sdp::group_semantics_type{ sdp::fields::semantics(value) } | ||
| && 2 == sdp::fields::ssrc_ids(value).size() | ||
| ? 2 | ||
| : 0; |
There was a problem hiding this comment.
why not return the actual number of ssrc_ids here, and if necessary bail out higher up if it's not a supported number (0 or 2)?
| return sdp::group_semantics::duplication == sdp::group_semantics_type{ sdp::fields::semantics(value) } | |
| && 2 == sdp::fields::ssrc_ids(value).size() | |
| ? 2 | |
| : 0; | |
| return sdp::group_semantics::duplication == sdp::group_semantics_type{ sdp::fields::semantics(value) } | |
| ? sdp::fields::ssrc_ids(value).size() | |
| : 0; |
There was a problem hiding this comment.
Agreed. The detection code should preserve the actual SSRC cardinality, and the NMOS interpretation layer should own the supported-count decision. In combination with the later comment about the helper having only one call site, I will remove the helper, compute the actual SSRC ID array size at that call site, and keep the two-source requirement only where temporal redundancy is interpreted.
| const auto& ssrc_group_value = sdp::fields::value(*ssrc_group); | ||
| if (sdp::group_semantics::duplication != sdp::group_semantics_type{ sdp::fields::semantics(ssrc_group_value) }) continue; | ||
| if (2 != sdp::fields::ssrc_ids(ssrc_group_value).size()) continue; |
There was a problem hiding this comment.
This duplicates some of the logic of get_ssrc_duplication_count... but given that function isn't directly usable here, and that means get_ssrc_duplication_count is only used in one place, maybe better to drop that function and inline its code into the single call-site?
There was a problem hiding this comment.
Agreed. Since the helper has a single call site and the temporal-redundancy parser necessarily performs the same group lookup for additional data, keeping the helper does not buy enough reuse. I will remove it and inline the DUP SSRC-group lookup at the transport-parameter call site, while leaving the richer temporal-redundancy parsing logic in its existing layer.
|
The review updates are now pushed in commit 87f9ee2. This update:
The focused temporal-redundancy tests pass with 18 assertions across 2 test cases, the existing separate-source/destination tests pass, and the full local suite passes with 2,030 assertions across 161 test cases. The temporary Apple Silicon Catch compatibility patch used by the local test build was fully reverted before committing. The new GitHub Actions suite is currently marked action_required and appears to be waiting for repository approval. |
2 participants
Summary
ssrc,ssrc-group, andduplication-delaysdp_parameters, including SSRC IDs, CNAMEs, delay, and MIDTesting
This PR is independent of and complementary to #501, so the two remaining modes can be reviewed separately.
Refs #38