From c06fb07e684f36c7ed873b97cf42acd0f0e09fd0 Mon Sep 17 00:00:00 2001 From: Gareth Sylvester-Bradley Date: Tue, 7 Jan 2025 14:41:26 +0000 Subject: [PATCH 1/4] Refactor handling of required format-specific parameters --- Development/nmos/sdp_utils.cpp | 45 +++++++++++++++++++-------------- Development/nmos/sdp_utils.h | 9 +++++++ Development/nmos/video_jxsv.cpp | 13 +++++++--- 3 files changed, 45 insertions(+), 22 deletions(-) diff --git a/Development/nmos/sdp_utils.cpp b/Development/nmos/sdp_utils.cpp index 22f693b9..621d236f 100644 --- a/Development/nmos/sdp_utils.cpp +++ b/Development/nmos/sdp_utils.cpp @@ -1284,34 +1284,33 @@ namespace nmos } // Get additional "video/raw" parameters from the SDP parameters - video_raw_parameters get_video_raw_parameters(const sdp_parameters& sdp_params) + template + video_raw_parameters get_video_raw_parameters(const sdp_parameters& sdp_params, MissingRequiredParameter missing = MissingRequiredParameter{}) { video_raw_parameters params; - if (sdp_params.fmtp.empty()) throw details::sdp_processing_error("missing attribute: fmtp"); - // See SMPTE ST 2110-20:2017 Section 7.2 Required Media Type Parameters // and Section 7.3 Media Type Parameters with default values const auto sampling = details::find_fmtp(sdp_params.fmtp, sdp::fields::sampling); - if (sdp_params.fmtp.end() == sampling) throw details::sdp_processing_error("missing format parameter: sampling"); - params.sampling = sdp::sampling{ sampling->second }; + if (sdp_params.fmtp.end() == sampling) missing(sdp::fields::sampling); + else params.sampling = sdp::sampling{ sampling->second }; const auto depth = details::find_fmtp(sdp_params.fmtp, sdp::fields::depth); - if (sdp_params.fmtp.end() == depth) throw details::sdp_processing_error("missing format parameter: depth"); - params.depth = utility::istringstreamed(depth->second); + if (sdp_params.fmtp.end() == depth) missing(sdp::fields::depth); + else params.depth = utility::istringstreamed(depth->second); const auto width = details::find_fmtp(sdp_params.fmtp, sdp::fields::width); - if (sdp_params.fmtp.end() == width) throw details::sdp_processing_error("missing format parameter: width"); - params.width = utility::istringstreamed(width->second); + if (sdp_params.fmtp.end() == width) missing(sdp::fields::width); + else params.width = utility::istringstreamed(width->second); const auto height = details::find_fmtp(sdp_params.fmtp, sdp::fields::height); - if (sdp_params.fmtp.end() == height) throw details::sdp_processing_error("missing format parameter: height"); - params.height = utility::istringstreamed(height->second); + if (sdp_params.fmtp.end() == height) missing(sdp::fields::height); + else params.height = utility::istringstreamed(height->second); const auto exactframerate = details::find_fmtp(sdp_params.fmtp, sdp::fields::exactframerate); - if (sdp_params.fmtp.end() == exactframerate) throw details::sdp_processing_error("missing format parameter: exactframerate"); - params.exactframerate = nmos::details::parse_exactframerate(exactframerate->second); + if (sdp_params.fmtp.end() == exactframerate) missing(sdp::fields::exactframerate); + else params.exactframerate = nmos::details::parse_exactframerate(exactframerate->second); // optional const auto interlace = details::find_fmtp(sdp_params.fmtp, sdp::fields::interlace); @@ -1326,8 +1325,8 @@ namespace nmos if (sdp_params.fmtp.end() != tcs) params.tcs = sdp::transfer_characteristic_system{ tcs->second }; const auto colorimetry = details::find_fmtp(sdp_params.fmtp, sdp::fields::colorimetry); - if (sdp_params.fmtp.end() == colorimetry) throw details::sdp_processing_error("missing format parameter: colorimetry"); - params.colorimetry = sdp::colorimetry{ colorimetry->second }; + if (sdp_params.fmtp.end() == colorimetry) missing(sdp::fields::colorimetry); + else params.colorimetry = sdp::colorimetry{ colorimetry->second }; // optional const auto range = details::find_fmtp(sdp_params.fmtp, sdp::fields::range); @@ -1338,12 +1337,12 @@ namespace nmos if (sdp_params.fmtp.end() != par) params.par = nmos::details::parse_pixel_aspect_ratio(par->second); const auto pm = details::find_fmtp(sdp_params.fmtp, sdp::fields::packing_mode); - if (sdp_params.fmtp.end() == pm) throw details::sdp_processing_error("missing format parameter: PM"); - params.pm = sdp::packing_mode{ pm->second }; + if (sdp_params.fmtp.end() == pm) missing(sdp::fields::packing_mode); + else params.pm = sdp::packing_mode{ pm->second }; const auto ssn = details::find_fmtp(sdp_params.fmtp, sdp::fields::smpte_standard_number); - if (sdp_params.fmtp.end() == ssn) throw details::sdp_processing_error("missing format parameter: SSN"); - params.ssn = sdp::smpte_standard_number{ ssn->second }; + if (sdp_params.fmtp.end() == ssn) missing(sdp::fields::smpte_standard_number); + else params.ssn = sdp::smpte_standard_number{ ssn->second }; // "Senders and Receivers compliant to [ST 2110-20] shall comply with the provisions of SMPTE ST 2110-21." // See SMPTE ST 2110-20:2017 Section 6.1.1 @@ -1378,6 +1377,14 @@ namespace nmos return params; } + // Get additional "video/raw" parameters from the SDP parameters + video_raw_parameters get_video_raw_parameters(const sdp_parameters& sdp_params) + { + if (sdp_params.fmtp.empty()) throw details::sdp_processing_error("missing attribute: fmtp"); + + return get_video_raw_parameters(sdp_params); + } + // Get additional "audio/L" parameters from the SDP parameters audio_L_parameters get_audio_L_parameters(const sdp_parameters& sdp_params) { diff --git a/Development/nmos/sdp_utils.h b/Development/nmos/sdp_utils.h index c4ce30e6..aeb9be61 100644 --- a/Development/nmos/sdp_utils.h +++ b/Development/nmos/sdp_utils.h @@ -7,6 +7,7 @@ #include #include "bst/any.h" #include "bst/optional.h" +#include "cpprest/basic_utils.h" #include "sdp/json.h" #include "sdp/ntp.h" #include "nmos/did_sdid.h" @@ -582,6 +583,14 @@ namespace nmos return std::runtime_error{ "sdp processing error - " + message }; } + struct throw_missing_fmtp + { + void operator()(const utility::string_t& name) const + { + throw details::sdp_processing_error("missing format parameter: " + utility::us2s(name)); + } + }; + inline sdp_parameters::fmtp_t::const_iterator find_fmtp(const sdp_parameters::fmtp_t& fmtp, const utility::string_t& name) { return std::find_if(fmtp.begin(), fmtp.end(), [&](const sdp_parameters::fmtp_t::value_type& param) diff --git a/Development/nmos/video_jxsv.cpp b/Development/nmos/video_jxsv.cpp index bde7df1e..ca0c40fb 100644 --- a/Development/nmos/video_jxsv.cpp +++ b/Development/nmos/video_jxsv.cpp @@ -186,13 +186,14 @@ namespace nmos } // Get additional "video/jxsv" parameters from the SDP parameters - video_jxsv_parameters get_video_jxsv_parameters(const sdp_parameters& sdp_params) + template + video_jxsv_parameters get_video_jxsv_parameters(const sdp_parameters& sdp_params, MissingRequiredParameter missing = MissingRequiredParameter{}) { video_jxsv_parameters params; const auto packetmode = details::find_fmtp(sdp_params.fmtp, sdp::video_jxsv::fields::packetmode); - if (sdp_params.fmtp.end() == packetmode) throw details::sdp_processing_error("missing format parameter: packetmode"); - params.packetmode = (sdp::video_jxsv::packetization_mode)utility::istringstreamed(packetmode->second); + if (sdp_params.fmtp.end() == packetmode) missing(sdp::video_jxsv::fields::packetmode); + else params.packetmode = (sdp::video_jxsv::packetization_mode)utility::istringstreamed(packetmode->second); // optional const auto transmode = details::find_fmtp(sdp_params.fmtp, sdp::video_jxsv::fields::transmode); @@ -286,6 +287,12 @@ namespace nmos return params; } + // Get additional "video/jxsv" parameters from the SDP parameters + video_jxsv_parameters get_video_jxsv_parameters(const sdp_parameters& sdp_params) + { + return get_video_jxsv_parameters(sdp_params); + } + // Calculate the format bit rate (kilobits/second) from the specified frame rate, dimensions and bits per pixel uint64_t get_video_jxsv_bit_rate(const nmos::rational& grain_rate, uint32_t frame_width, uint32_t frame_height, double bits_per_pixel) { From 973ec88fdcb49e6ef16a5f1cf8fd192e772de79f Mon Sep 17 00:00:00 2001 From: Gareth Sylvester-Bradley Date: Tue, 7 Jan 2025 14:44:05 +0000 Subject: [PATCH 2/4] Remove potential misleading error message Since an 'empty' fmtp attribute would appear the same at this abstraction, it's better to rely on reporting omission of specific required format-specific parameters --- Development/nmos/sdp_utils.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/Development/nmos/sdp_utils.cpp b/Development/nmos/sdp_utils.cpp index 621d236f..e1459ff9 100644 --- a/Development/nmos/sdp_utils.cpp +++ b/Development/nmos/sdp_utils.cpp @@ -1380,8 +1380,6 @@ namespace nmos // Get additional "video/raw" parameters from the SDP parameters video_raw_parameters get_video_raw_parameters(const sdp_parameters& sdp_params) { - if (sdp_params.fmtp.empty()) throw details::sdp_processing_error("missing attribute: fmtp"); - return get_video_raw_parameters(sdp_params); } From 382f77c5aebebc215dabcf1caabb9a390f044f5b Mon Sep 17 00:00:00 2001 From: Gareth Sylvester-Bradley Date: Tue, 7 Jan 2025 14:52:50 +0000 Subject: [PATCH 3/4] Switch if-else branches for consistency between required and optional parameters --- Development/nmos/sdp_utils.cpp | 32 ++++++++++++++++---------------- Development/nmos/video_jxsv.cpp | 4 ++-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/Development/nmos/sdp_utils.cpp b/Development/nmos/sdp_utils.cpp index e1459ff9..3f4a382b 100644 --- a/Development/nmos/sdp_utils.cpp +++ b/Development/nmos/sdp_utils.cpp @@ -1293,24 +1293,24 @@ namespace nmos // and Section 7.3 Media Type Parameters with default values const auto sampling = details::find_fmtp(sdp_params.fmtp, sdp::fields::sampling); - if (sdp_params.fmtp.end() == sampling) missing(sdp::fields::sampling); - else params.sampling = sdp::sampling{ sampling->second }; + if (sdp_params.fmtp.end() != sampling) params.sampling = sdp::sampling{ sampling->second }; + else missing(sdp::fields::sampling); const auto depth = details::find_fmtp(sdp_params.fmtp, sdp::fields::depth); - if (sdp_params.fmtp.end() == depth) missing(sdp::fields::depth); - else params.depth = utility::istringstreamed(depth->second); + if (sdp_params.fmtp.end() != depth) params.depth = utility::istringstreamed(depth->second); + else missing(sdp::fields::depth); const auto width = details::find_fmtp(sdp_params.fmtp, sdp::fields::width); - if (sdp_params.fmtp.end() == width) missing(sdp::fields::width); - else params.width = utility::istringstreamed(width->second); + if (sdp_params.fmtp.end() != width) params.width = utility::istringstreamed(width->second); + else missing(sdp::fields::width); const auto height = details::find_fmtp(sdp_params.fmtp, sdp::fields::height); - if (sdp_params.fmtp.end() == height) missing(sdp::fields::height); - else params.height = utility::istringstreamed(height->second); + if (sdp_params.fmtp.end() != height) params.height = utility::istringstreamed(height->second); + else missing(sdp::fields::height); const auto exactframerate = details::find_fmtp(sdp_params.fmtp, sdp::fields::exactframerate); - if (sdp_params.fmtp.end() == exactframerate) missing(sdp::fields::exactframerate); - else params.exactframerate = nmos::details::parse_exactframerate(exactframerate->second); + if (sdp_params.fmtp.end() != exactframerate) params.exactframerate = nmos::details::parse_exactframerate(exactframerate->second); + else missing(sdp::fields::exactframerate); // optional const auto interlace = details::find_fmtp(sdp_params.fmtp, sdp::fields::interlace); @@ -1325,8 +1325,8 @@ namespace nmos if (sdp_params.fmtp.end() != tcs) params.tcs = sdp::transfer_characteristic_system{ tcs->second }; const auto colorimetry = details::find_fmtp(sdp_params.fmtp, sdp::fields::colorimetry); - if (sdp_params.fmtp.end() == colorimetry) missing(sdp::fields::colorimetry); - else params.colorimetry = sdp::colorimetry{ colorimetry->second }; + if (sdp_params.fmtp.end() != colorimetry) params.colorimetry = sdp::colorimetry{ colorimetry->second }; + else missing(sdp::fields::colorimetry); // optional const auto range = details::find_fmtp(sdp_params.fmtp, sdp::fields::range); @@ -1337,12 +1337,12 @@ namespace nmos if (sdp_params.fmtp.end() != par) params.par = nmos::details::parse_pixel_aspect_ratio(par->second); const auto pm = details::find_fmtp(sdp_params.fmtp, sdp::fields::packing_mode); - if (sdp_params.fmtp.end() == pm) missing(sdp::fields::packing_mode); - else params.pm = sdp::packing_mode{ pm->second }; + if (sdp_params.fmtp.end() != pm) params.pm = sdp::packing_mode{ pm->second }; + else missing(sdp::fields::packing_mode); const auto ssn = details::find_fmtp(sdp_params.fmtp, sdp::fields::smpte_standard_number); - if (sdp_params.fmtp.end() == ssn) missing(sdp::fields::smpte_standard_number); - else params.ssn = sdp::smpte_standard_number{ ssn->second }; + if (sdp_params.fmtp.end() != ssn) params.ssn = sdp::smpte_standard_number{ ssn->second }; + else missing(sdp::fields::smpte_standard_number); // "Senders and Receivers compliant to [ST 2110-20] shall comply with the provisions of SMPTE ST 2110-21." // See SMPTE ST 2110-20:2017 Section 6.1.1 diff --git a/Development/nmos/video_jxsv.cpp b/Development/nmos/video_jxsv.cpp index ca0c40fb..f178cb40 100644 --- a/Development/nmos/video_jxsv.cpp +++ b/Development/nmos/video_jxsv.cpp @@ -192,8 +192,8 @@ namespace nmos video_jxsv_parameters params; const auto packetmode = details::find_fmtp(sdp_params.fmtp, sdp::video_jxsv::fields::packetmode); - if (sdp_params.fmtp.end() == packetmode) missing(sdp::video_jxsv::fields::packetmode); - else params.packetmode = (sdp::video_jxsv::packetization_mode)utility::istringstreamed(packetmode->second); + if (sdp_params.fmtp.end() != packetmode) params.packetmode = (sdp::video_jxsv::packetization_mode)utility::istringstreamed(packetmode->second); + else missing(sdp::video_jxsv::fields::packetmode); // optional const auto transmode = details::find_fmtp(sdp_params.fmtp, sdp::video_jxsv::fields::transmode); From cffd35edb25d23bfb4884dc2ce0bafb61cd7c225 Mon Sep 17 00:00:00 2001 From: Gareth Sylvester-Bradley Date: Tue, 7 Jan 2025 15:47:07 +0000 Subject: [PATCH 4/4] Add _or_defaults versions of the functions that normally throw when required parameters are omitted - these functions still throw if they encounter invalid values --- Development/nmos/sdp_utils.cpp | 6 ++++++ Development/nmos/sdp_utils.h | 1 + Development/nmos/video_jxsv.cpp | 6 ++++++ Development/nmos/video_jxsv.h | 1 + 4 files changed, 14 insertions(+) diff --git a/Development/nmos/sdp_utils.cpp b/Development/nmos/sdp_utils.cpp index 3f4a382b..eb9b378c 100644 --- a/Development/nmos/sdp_utils.cpp +++ b/Development/nmos/sdp_utils.cpp @@ -1383,6 +1383,12 @@ namespace nmos return get_video_raw_parameters(sdp_params); } + // Get additional "video/raw" parameters from the SDP parameters + video_raw_parameters get_video_raw_parameters_or_defaults(const sdp_parameters& sdp_params) + { + return get_video_raw_parameters<>(sdp_params, [](const utility::string_t&) {}); + } + // Get additional "audio/L" parameters from the SDP parameters audio_L_parameters get_audio_L_parameters(const sdp_parameters& sdp_params) { diff --git a/Development/nmos/sdp_utils.h b/Development/nmos/sdp_utils.h index aeb9be61..9d0568af 100644 --- a/Development/nmos/sdp_utils.h +++ b/Development/nmos/sdp_utils.h @@ -507,6 +507,7 @@ namespace nmos sdp_parameters make_video_raw_sdp_parameters(const utility::string_t& session_name, const video_raw_parameters& params, uint64_t payload_type, const std::vector& media_stream_ids = {}, const std::vector& ts_refclk = {}); // Get additional "video/raw" parameters from the SDP parameters video_raw_parameters get_video_raw_parameters(const sdp_parameters& sdp_params); + video_raw_parameters get_video_raw_parameters_or_defaults(const sdp_parameters& sdp_params); // Construct additional "audio/L" parameters from the IS-04 resources, using default values for unspecified items audio_L_parameters make_audio_L_parameters(const web::json::value& node, const web::json::value& source, const web::json::value& flow, const web::json::value& sender, bst::optional packet_time); diff --git a/Development/nmos/video_jxsv.cpp b/Development/nmos/video_jxsv.cpp index f178cb40..6da8b3a0 100644 --- a/Development/nmos/video_jxsv.cpp +++ b/Development/nmos/video_jxsv.cpp @@ -293,6 +293,12 @@ namespace nmos return get_video_jxsv_parameters(sdp_params); } + // Get additional "video/jxsv" parameters from the SDP parameters + video_jxsv_parameters get_video_jxsv_parameters_or_defaults(const sdp_parameters& sdp_params) + { + return get_video_jxsv_parameters<>(sdp_params, [](const utility::string_t&) {}); + } + // Calculate the format bit rate (kilobits/second) from the specified frame rate, dimensions and bits per pixel uint64_t get_video_jxsv_bit_rate(const nmos::rational& grain_rate, uint32_t frame_width, uint32_t frame_height, double bits_per_pixel) { diff --git a/Development/nmos/video_jxsv.h b/Development/nmos/video_jxsv.h index 90d0b740..947b8a49 100644 --- a/Development/nmos/video_jxsv.h +++ b/Development/nmos/video_jxsv.h @@ -353,6 +353,7 @@ namespace nmos sdp_parameters make_video_jxsv_sdp_parameters(const utility::string_t& session_name, const video_jxsv_parameters& params, uint64_t payload_type, const std::vector& media_stream_ids = {}, const std::vector& ts_refclk = {}); // Get additional "video/jxsv" parameters from the SDP parameters video_jxsv_parameters get_video_jxsv_parameters(const sdp_parameters& sdp_params); + video_jxsv_parameters get_video_jxsv_parameters_or_defaults(const sdp_parameters& sdp_params); // Construct SDP parameters for "video/jxsv" inline sdp_parameters make_sdp_parameters(const utility::string_t& session_name, const video_jxsv_parameters& params, uint64_t payload_type, const std::vector& media_stream_ids = {}, const std::vector& ts_refclk = {})