nmos::details::make_sdp_parameters in sdp_utils in case of audio always reprting ptime 1 and payload id 97

[aholzinger]

I'm currently stumpling upon this one (sdp_utils.cpp line 190):

params.packet_time = 1;

return{ sender.at(nmos::fields::label).as_string(), params, 97, media_stream_ids, details::make_ts_refclk(node, source, sender, ptp_domain) };

Wouldn't it be nice if the function would correctly report packet time and payload id?

The problem seems to be that in the data provided in the function parameters neither ptime nor pid are contained.

[garethsb]

Those values are not found in the NMOS resources. If your app knows them, it can set them. Like the example app, https://github.com/sony/nmos-cpp/blob/master/Development/nmos-cpp-node/node_implementation.cpp#L911

[aholzinger]

That's how I worked around it.

Wouldn't it then be better to return invalid values for the two parameters so that the user will immediately be aware to update these parameters?

[garethsb]

The dynamic payload types 96, 97, 98 seem to be defacto defaults for video, audio, ancillary data. They're used in many spec examples at least.

Perhaps it's worth adding the extra function parameters with default arguments or as another overload?

[aholzinger]

That would be a good idea, So a caller could pre-set the values with whatever he/she likes (invalid values or pre-fetched correct values from the SDP).

[garethsb]

It's a little awkward because the format-specific functions in nmos::details are exposed to the user only via:

nmos-cpp/Development/nmos/sdp_utils.h

Line 23 in 4fba3ab

sdp_parameters make_sdp_parameters(const web::json::value& node, const web::json::value& source, const web::json::value& flow, const web::json::value& sender, const std::vector<utility::string_t>& media_stream_ids, bst::optional<int> ptp_domain);

Adding an optional<uint64_t> payload_type = nullopt to that function declaration would be OK, I guess.

But it feels a little wrong to add the (basically) audio-specific double packet_time to this function's parameter list.

So I'm not sure of the best approach...

pre-fetched correct values from the SDP

For what it's worth, depending on the structure of your app, the get_session_description_sdp_parameters function and therefore also parse_session_description can do this for you.

[aholzinger]

It's true that in the wild ptime normally is only seen in m=audio sections while I don't see anything in the SDP RFC that would prohibit it to show up in a m=video section.

So it could default to a negative/invalid value, couldn't it?

Thanks, I already use parse_session_description for this.

[garethsb]

I agree the ptime attribute is permitted by RFC 4566 on other media types besides audio... sdp::make_session_description can of course handle this attribute in any media description. However, since it's only commonly used for audio, the high-level representation of the SDP file, nmos::sdp_parameters, has it as audio.packet_time.

If packet_time were added as an argument to nmos::make_sdp_parameters, that would suggest to users that the ptime attribute could be written for all media types. To support that consistently, the member variable should also be moved to the "top level" of sdp_parameters, and the parser should also read it for all media types... Or would it be better for the make_sdp_parameters function to silently ignore packet_time if specified for other media types, or throw an exception? None of these seem ideal.

The three-step process, nmos::make_sdp_parameters -> nmos::make_session_description -> sdp::make_session_description, is intended to allow you to make changes to defaults, add to or modify the session description, between each step (via sdp_parameters in the first case, json in the second case), on the way to generating the text format.

Both rtpmap.payload_type and audio.packet_time can already be changed after nmos::make_sdp_parameters.

A little bit more discussion required to determine the best strategy!