sony / nmos-cpp

An NMOS (Networked Media Open Specifications) Registry and Node in C++ (IS-04, IS-05)
Apache License 2.0
136 stars 80 forks source link

nmos::details::get_format() and nmos::details::get_format_parameters() now reject -31 SDPs #353

Closed duncwebb closed 8 months ago

duncwebb commented 8 months ago

Good afternoon, I'm currently in the process of transitioning to the latest code in master and there's a couple of changes in nmos/sdp_utils.cpp that causes -31 SDPs (with a media type of a=rtpmap:97 AM824/48000/16) to be rejected with an sdp_processing_error("unsupported media type/encoding name"). In the (admittedly old) version in the commit I'm moving from the nmos::details::get_format() and nmos::details::get_format_parameters() methods didn't check the media type identifier...

if (sdp::media_types::audio == sdp_params.media_type) return nmos::formats::audio;

is now:

if (sdp::media_types::audio == sdp_params.media_type && U("L") == sdp_params.rtpmap.encoding_name.substr(0, 1)) return nmos::formats::audio;

Is this intentional?

garethsb commented 8 months ago

It would be great to have full support for audio/AM824 in nmos-cpp. It should be straightforward to add, but I think it should be treated separately than audio/Lnn as the required SDP format-specific parameters and NMOS resource attributes are slightly different?

duncwebb commented 8 months ago

Yes it would. I've managed to deal with it in our vendor code up until this point but the changes mean that the SDP is rejected before any of the callbacks to vendor code are made. I can patch nmos-cpp in our build system though I'll have a look at the other changes made in that commit for any potential gotchas.

garethsb commented 8 months ago

@duncwebb the intention now is that new media types can be added in vendor code - those functions should only be called by validate_sdp_parameters which is just a default implementation called by nmos-cpp if application code doesn't override it. See the example code to see how things can be extended beyond the base support for video/raw, audio/L, video/smpte291 and video/SMPTE2022-6 to include video/jxsv. The same principle should be used to add support for audio/AM824 or any other media type:

https://github.com/sony/nmos-cpp/blob/112e7ada2fa671de4b3330aefbf2e9b946abfb0e/Development/nmos-cpp-node/node_implementation.cpp#L1018

duncwebb commented 8 months ago

Thanks Gareth, I'll have a look!

duncwebb commented 8 months ago

Yep, that makes sense - I'd overlooked that in my nmos::transport_file_parser handler I was calling validate_sdp_parameters() which in turn calls the changed method. I can change that to handle the SDP appropriately. Thanks again - I'll close this issue.

garethsb commented 8 months ago

@duncwebb I'm glad that works for you. If you were able to share the differences you've encountered in handling -31 audio/AM824 vs. -30 SDP files, to be used as reference for implementation in the style of _nmos/videojxsv.h that would be great!