Fix SDP validating against media_type constraint

[N-Nagorny]

nmos::media_type represents IANA media types, sdp::media_type represents https://tools.ietf.org/html/rfc4566#section-8.2.1. This PR makes match_sdp_parameters_constraint_set() comparing apples to apples.

[garethsb]

Good catch! I'll think about a unit test as I'm working on this code right now.

[garethsb]

@N-Nagorny, I have written a basic test case. Please would you consider pulling the two commits in https://github.com/garethsb/nmos-cpp/tree/sdp-media-type-constraint-fix and pushing to your PR branch?

[garethsb]

It made me realise an issue around constraints that can't be checked...

For example, if the SDP data doesn't include ptime or maxptime but the Receiver has a parameter constraint on transport:packet_time or transport:max_packet_time, what should this validation code do - reject the SDP data because it doesn't say for sure what the value is, or accept it because it MAY be OK?

This is possibly an issue in BCP-004-01 too for Controllers, at least I don't think it's clear. "Controllers MAY ignore individual Parameter Constraints whose unique identifiers they do not recognize" maybe needs to go on to say "or for which they cannot determine the value of the parameter"?

[N-Nagorny]

I updated the PR branch. It seems reasonable to me to accept such SDP because it opens opportunity to try. If a vendor cares about strict rules, they populate the SDP and Receiver Caps with more detail to have it rejected for sure.

[garethsb]

It seems reasonable to me to accept such SDP because it opens opportunity to try.

If that's true just for some parameters we could do e.g.

<            { nmos::caps::transport::packet_time, [](CAPS_ARGS) { return nmos::match_number_constraint(sdp.packet_time, con); } },
<            { nmos::caps::transport::max_packet_time, [](CAPS_ARGS) { return nmos::match_number_constraint(sdp.max_packet_time, con); } },
<            { nmos::caps::transport::st2110_21_sender_type, [](CAPS_ARGS) { if (auto video = get_video(&format)) return nmos::match_string_constraint(video->tp.name, con); else if (auto mux = get_mux(&format)) return nmos::match_string_constraint(mux->tp.name, con); else return false; } }
---
>            { nmos::caps::transport::packet_time, [](CAPS_ARGS) { return 0 == sdp.packet_time || nmos::match_number_constraint(sdp.packet_time, con); } },
>            { nmos::caps::transport::max_packet_time, [](CAPS_ARGS) { return 0 == sdp.max_packet_time || nmos::match_number_constraint(sdp.max_packet_time, con); } },
>            { nmos::caps::transport::st2110_21_sender_type, [](CAPS_ARGS) { if (auto video = get_video(&format)) return video->tp.name.empty() || nmos::match_string_constraint(video->tp.name, con); else if (auto mux = get_mux(&format)) return mux->tp.name.empty() || nmos::match_string_constraint(mux->tp.name, con); else return false; } }

But if we want to do this for all parameters, we could modify the nmos::match_<type>_constraint functions similarly...

[garethsb]

I've added one additional fix (and a test case) for a bug introduced in the SDP refactor, #225.

a=rtpmap:96 raw/90000

was erroneously roundtripped to

a=rtpmap:96 raw/90000/1

The <encoding-parameters> after the final / should only be used for audio formats, to indicate channel count, and in that case can be omitted to indicate the default of 1 channel. Some SDP parsers will reject the /1 for other formats, so it's important to fix this.

[garethsb]

It seems reasonable to me to accept such SDP because it opens opportunity to try.

I realise that the important difference between various parameter constraints is whether or not the target parameter is required to be indicated in the SDP data or not. I suggest we look at them all in that light, and make a decision before we finalize this PR.

[garethsb]

I realise that the important difference between various parameter constraints is whether or not the target parameter is required to be indicated in the SDP data or not. I suggest we look at them all in that light, and make a decision before we finalize this PR.

Of the implemented Receiver capabilities for video/raw, audio/L{24,16} and video/SMPTE2022-6:

Capability	Target SDP Parameter	Condition
media_type	media-type and encoding-name	Required by RFC 4566
grain_rate	exactframerate	Required by ST 2110-20 (but not mentioned in RFC 4566)
frame_height	height	Required by RFC 4175
frame_width	width	Required by RFC 4175
color_sampling	sampling	Required by RFC 4175
interlace_mode	interlace and segmented	RFC 4175 interlace is optional but it's a flag and implies progressive if omitted so effectively required; segmented is optional in ST 2110-20 but implies not segmented if omitted (it's not mentioned in RFC 4175 however)
colorspace	colorimetry	Required by RFC 4175
transfer_characteristic	TCS	Optional in ST 2110-20 but implies SDR if not present (not mentioned in RFC 4175 however)
component_depth	depth	Required by RFC 4175
channel_count	<encoding parameters>	Optional but with a default of 1 in RFC 3190 (L24) and RFC 4586 (L16 and L8)
sample_rate	<clock rate>	Required by RFC 3190 and RFC 45866
sample_depth	<encoding name>	Implied by L24, L16, L8
packet_time	ptime	Optional in RFC 4586 and not mentioned in RFC 3190 (presumably because that RFC came long before RFC 4566 defined ptime); ST 2110-30 discusses packet times but doesn't mention the SDP attribute
max_packet_time	maxptime	Same situation as ptime
st2110_21_sender_type	TP	Required by ST 2110-21 (but not by RFC 4175) for video/raw and video/SMPTE2022-6

I think that analysis means:

• Constraints on packet_time and max_packet_time are the strongest cases to warrant special handling when the target parameters are omitted from the SDP data.
• transfer_characteristic ought to take account of the default in ST 2110-20.
• Finally, depending on whether we want only to handle ST 2110 or also "basic" RFC 4175, we may want to handle grain_rate when exactframerate is omitted. (Already implemented!)