Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tsdelay and troff #367

Merged
merged 12 commits into from
Feb 7, 2024
Merged

Fix tsdelay and troff #367

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6597140
SMPTE ST 2110-10:2022: TSDELAY shall be a positive number or zero
lo-simon Jan 25, 2024
c224afe
Add testSdpParametersVideoJpegXs and update other sdp parameters test…
lo-simon Jan 25, 2024
73ddf95
Prevent GCC 4.8.4 compile error, cast data type for the bst::optional…
lo-simon Jan 25, 2024
777911c
Bump up boost to 1.83.0 to match with cpprestsdk dependencies
lo-simon Jan 26, 2024
34863b8
Remove braces around sdp_parameters's bandwidth initializer
lo-simon Jan 30, 2024
e546abd
Bump cpprestsdk to 2.10.19
lo-simon Jan 31, 2024
9b02bd7
Roll back boost and cpprestsdk, they will be bumped up from another PR
lo-simon Feb 1, 2024
f979998
Merge branch 'master' into fix_tsdelay_troff
lo-simon Feb 2, 2024
99584e3
Merge branch 'master' into fix_tsdelay_troff
lo-simon Feb 5, 2024
416f885
Apply suggestions from code review
lo-simon Feb 6, 2024
66f6784
Update sdp unit tests with @garethsb suggestions
lo-simon Feb 6, 2024
fcf8d40
Remove trailing whitespace
garethsb Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Development/cmake/NmosCppTest.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,13 @@ set(NMOS_CPP_TEST_NMOS_TEST_SOURCES
nmos/test/jwt_validation_test.cpp
nmos/test/paging_utils_test.cpp
nmos/test/query_api_test.cpp
nmos/test/sdp_test_utils.cpp
nmos/test/sdp_utils_test.cpp
nmos/test/system_resources_test.cpp
nmos/test/video_jxsv_test.cpp
)
set(NMOS_CPP_TEST_NMOS_TEST_HEADERS
nmos/test/sdp_test_utils.h
)

set(NMOS_CPP_TEST_PPLX_TEST_SOURCES
Expand Down
12 changes: 6 additions & 6 deletions Development/nmos/sdp_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,11 +793,11 @@ namespace nmos
// additional parameters introduced by SMPTE specs since then...
if (!params.range.empty()) fmtp.push_back({ sdp::fields::range, params.range.name });
if (0 != params.par) fmtp.push_back({ sdp::fields::pixel_aspect_ratio, nmos::details::make_pixel_aspect_ratio(params.par) });
if (0 != params.troff) fmtp.push_back({ sdp::fields::TROFF, utility::ostringstreamed(params.troff) });
if (params.troff) fmtp.push_back({ sdp::fields::TROFF, utility::ostringstreamed(*params.troff) });
if (0 != params.cmax) fmtp.push_back({ sdp::fields::CMAX, utility::ostringstreamed(params.cmax) });
if (0 != params.maxudp) fmtp.push_back({ sdp::fields::max_udp_packet_size, utility::ostringstreamed(params.maxudp) });
if (!params.tsmode.empty()) fmtp.push_back({ sdp::fields::timestamp_mode, params.tsmode.name });
if (0 != params.tsdelay) fmtp.push_back({ sdp::fields::timestamp_delay, utility::ostringstreamed(params.tsdelay) });
if (params.tsdelay) fmtp.push_back({ sdp::fields::timestamp_delay, utility::ostringstreamed(*params.tsdelay) });

return{ session_name, sdp::media_types::video, rtpmap, fmtp, {}, {}, {}, {}, media_stream_ids, ts_refclk };
}
Expand All @@ -814,7 +814,7 @@ namespace nmos
sdp_parameters::fmtp_t fmtp = {};
if (!params.channel_order.empty()) fmtp.push_back({ sdp::fields::channel_order, params.channel_order });
if (!params.tsmode.empty()) fmtp.push_back({ sdp::fields::timestamp_mode, params.tsmode.name });
if (0 != params.tsdelay) fmtp.push_back({ sdp::fields::timestamp_delay, utility::ostringstreamed(params.tsdelay) });
if (params.tsdelay) fmtp.push_back({ sdp::fields::timestamp_delay, utility::ostringstreamed(*params.tsdelay) });

return{ session_name, sdp::media_types::audio, rtpmap, fmtp, {}, params.packet_time, {}, {}, media_stream_ids, ts_refclk };
}
Expand All @@ -836,9 +836,9 @@ namespace nmos
if (0 != params.exactframerate) fmtp.push_back({ sdp::fields::exactframerate, nmos::details::make_exactframerate(params.exactframerate) });
if (!params.tm.empty()) fmtp.push_back({ sdp::fields::TM, params.tm.name });
if (!params.ssn.empty()) fmtp.push_back({ sdp::fields::smpte_standard_number, params.ssn.name });
if (0 != params.troff) fmtp.push_back({ sdp::fields::TROFF, utility::ostringstreamed(params.troff) });
if (params.troff) fmtp.push_back({ sdp::fields::TROFF, utility::ostringstreamed(*params.troff) });
if (!params.tsmode.empty()) fmtp.push_back({ sdp::fields::timestamp_mode, params.tsmode.name });
if (0 != params.tsdelay) fmtp.push_back({ sdp::fields::timestamp_delay, utility::ostringstreamed(params.tsdelay) });
if (params.tsdelay) fmtp.push_back({ sdp::fields::timestamp_delay, utility::ostringstreamed(*params.tsdelay) });

return{ session_name, sdp::media_types::video, rtpmap, fmtp, {}, {}, {}, {}, media_stream_ids, ts_refclk };
}
Expand All @@ -854,7 +854,7 @@ namespace nmos
// See https://tools.ietf.org/html/rfc4566#section-6
sdp_parameters::fmtp_t fmtp = {};
if (!params.tp.empty()) fmtp.push_back({ sdp::fields::type_parameter, params.tp.name });
if (0 != params.troff) fmtp.push_back({ sdp::fields::TROFF, utility::ostringstreamed(params.troff) });
if (params.troff) fmtp.push_back({ sdp::fields::TROFF, utility::ostringstreamed(*params.troff) });

return{ session_name, sdp::media_types::video, rtpmap, fmtp, {}, {}, {}, {}, media_stream_ids, ts_refclk };
}
Expand Down
24 changes: 12 additions & 12 deletions Development/nmos/sdp_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,13 +316,13 @@ namespace nmos

// additional fmtp parameters from ST 2110-21:2022
sdp::type_parameter tp;
uint32_t troff; // if omitted (zero), assume default
bst::optional<uint32_t> troff; // if omitted, assume default
garethsb marked this conversation as resolved.
Show resolved Hide resolved
uint32_t cmax; // if omitted (zero), assume max defined for tp

// additional fmtp parameters from ST 2110-10:2022
uint32_t maxudp; // if omitted (zero), assume the Standard UP Size Limit
sdp::timestamp_mode tsmode; // if omitted (empty), assume sdp::timestamp_modes::NEW
uint32_t tsdelay;
bst::optional<uint32_t> tsdelay;

video_raw_parameters() : depth(), width(), height(), interlace(), segmented(), troff(), cmax(), maxudp(), tsdelay() {}

Expand All @@ -341,11 +341,11 @@ namespace nmos
sdp::packing_mode pm,
sdp::smpte_standard_number ssn,
sdp::type_parameter tp,
uint32_t troff,
bst::optional<uint32_t> troff,
uint32_t cmax,
uint32_t maxudp,
sdp::timestamp_mode tsmode,
uint32_t tsdelay
bst::optional<uint32_t> tsdelay
)
: sampling(std::move(sampling))
, depth(depth)
Expand Down Expand Up @@ -393,7 +393,7 @@ namespace nmos

// additional fmtp parameters from ST 2110-10:2022
sdp::timestamp_mode tsmode; // if omitted (empty), assume sdp::timestamp_modes::NEW
uint32_t tsdelay;
bst::optional<uint32_t> tsdelay;

// ptime
double packet_time;
Expand All @@ -406,7 +406,7 @@ namespace nmos
uint64_t sample_rate,
utility::string_t channel_order,
sdp::timestamp_mode tsmode,
uint32_t tsdelay,
bst::optional<uint32_t> tsdelay,
double packet_time
)
: channel_count(channel_count)
Expand Down Expand Up @@ -442,11 +442,11 @@ namespace nmos
sdp::smpte_standard_number ssn;

// additional fmtp parameters from ST 2110-21:2022
uint32_t troff; // if omitted (zero), assume default
bst::optional<uint32_t> troff; // if omitted, assume default

// additional fmtp parameters from ST 2110-10:2022
sdp::timestamp_mode tsmode; // if omitted (empty), assume sdp::timestamp_modes::NEW
uint32_t tsdelay;
bst::optional<uint32_t> tsdelay;

video_smpte291_parameters() : vpid_code(), troff(), tsdelay() {}

Expand All @@ -456,9 +456,9 @@ namespace nmos
nmos::rational exactframerate,
sdp::transmission_model tm,
sdp::smpte_standard_number ssn,
uint32_t troff,
bst::optional<uint32_t> troff,
sdp::timestamp_mode tsmode,
uint32_t tsdelay
bst::optional<uint32_t> tsdelay
)
: did_sdids(std::move(did_sdids))
, vpid_code(vpid_code)
Expand All @@ -482,13 +482,13 @@ namespace nmos
{
// additional fmtp parameters from ST 2110-21:2017
sdp::type_parameter tp;
uint32_t troff; // if omitted (zero), assume default
bst::optional<uint32_t> troff; // if omitted (zero), assume default
lo-simon marked this conversation as resolved.
Show resolved Hide resolved

video_SMPTE2022_6_parameters() : troff() {}

video_SMPTE2022_6_parameters(
sdp::type_parameter tp,
uint32_t troff
bst::optional<uint32_t> troff
)
: tp(std::move(tp))
, troff(troff)
Expand Down
31 changes: 31 additions & 0 deletions Development/nmos/test/sdp_test_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@

#include "bst/test/test.h"
#include "nmos/test/sdp_test_utils.h"
lo-simon marked this conversation as resolved.
Show resolved Hide resolved
#include "nmos/sdp_utils.h"

namespace sdp_test
{
typedef std::multimap<utility::string_t, utility::string_t> comparable_fmtp_t;

inline comparable_fmtp_t comparable_fmtp(const nmos::sdp_parameters::fmtp_t& fmtp)
{
return comparable_fmtp_t{ fmtp.begin(), fmtp.end() };
}

void check_sdp_parameters(const nmos::sdp_parameters& lhs, const nmos::sdp_parameters& rhs)
{
BST_REQUIRE_EQUAL(lhs.session_name, rhs.session_name);
BST_REQUIRE_EQUAL(lhs.rtpmap.payload_type, rhs.rtpmap.payload_type);
BST_REQUIRE_EQUAL(lhs.rtpmap.encoding_name, rhs.rtpmap.encoding_name);
BST_REQUIRE_EQUAL(lhs.rtpmap.clock_rate, rhs.rtpmap.clock_rate);
if (0 != lhs.rtpmap.encoding_parameters)
BST_REQUIRE_EQUAL(lhs.rtpmap.encoding_parameters, rhs.rtpmap.encoding_parameters);
else
BST_REQUIRE((0 == rhs.rtpmap.encoding_parameters || 1 == rhs.rtpmap.encoding_parameters));
BST_REQUIRE_EQUAL(comparable_fmtp(lhs.fmtp), comparable_fmtp(rhs.fmtp));
BST_REQUIRE_EQUAL(lhs.packet_time, rhs.packet_time);
BST_REQUIRE_EQUAL(lhs.max_packet_time, rhs.max_packet_time);
BST_REQUIRE_EQUAL(lhs.bandwidth.bandwidth_type, rhs.bandwidth.bandwidth_type);
BST_REQUIRE_EQUAL(lhs.bandwidth.bandwidth, rhs.bandwidth.bandwidth);
}
}
14 changes: 14 additions & 0 deletions Development/nmos/test/sdp_test_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#ifndef NMOS_SDP_TEST_UTILS_H
#define NMOS_SDP_TEST_UTILS_H

namespace nmos
{
struct sdp_parameters;
}

namespace sdp_test
lo-simon marked this conversation as resolved.
Show resolved Hide resolved
{
void check_sdp_parameters(const nmos::sdp_parameters& lhs, const nmos::sdp_parameters& rhs);
}

#endif
66 changes: 21 additions & 45 deletions Development/nmos/test/sdp_utils_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "nmos/json_fields.h"
#include "nmos/media_type.h"
#include "nmos/random.h"
#include "nmos/test/sdp_test_utils.h"
#include "sdp/sdp.h"

////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -499,31 +500,6 @@ BST_TEST_CASE(testSdpTransportParamsMulticast)
}
}

namespace
{
typedef std::multimap<utility::string_t, utility::string_t> comparable_fmtp_t;

inline comparable_fmtp_t comparable_fmtp(const nmos::sdp_parameters::fmtp_t& fmtp)
{
return comparable_fmtp_t{ fmtp.begin(), fmtp.end() };
}

void check_sdp_parameters(const nmos::sdp_parameters& lhs, const nmos::sdp_parameters& rhs)
{
BST_REQUIRE_EQUAL(lhs.session_name, rhs.session_name);
BST_REQUIRE_EQUAL(lhs.rtpmap.payload_type, rhs.rtpmap.payload_type);
BST_REQUIRE_EQUAL(lhs.rtpmap.encoding_name, rhs.rtpmap.encoding_name);
BST_REQUIRE_EQUAL(lhs.rtpmap.clock_rate, rhs.rtpmap.clock_rate);
if (0 != lhs.rtpmap.encoding_parameters)
BST_REQUIRE_EQUAL(lhs.rtpmap.encoding_parameters, rhs.rtpmap.encoding_parameters);
else
BST_REQUIRE((0 == rhs.rtpmap.encoding_parameters || 1 == rhs.rtpmap.encoding_parameters));
BST_REQUIRE_EQUAL(comparable_fmtp(lhs.fmtp), comparable_fmtp(rhs.fmtp));
BST_REQUIRE_EQUAL(lhs.packet_time, rhs.packet_time);
BST_REQUIRE_EQUAL(lhs.max_packet_time, rhs.max_packet_time);
}
}

////////////////////////////////////////////////////////////////////////////////////////////
BST_TEST_CASE(testSdpParametersVideoRaw)
{
Expand Down Expand Up @@ -600,11 +576,11 @@ BST_TEST_CASE(testSdpParametersVideoRaw)
{ U("PM"), U("2110BPM") },
{ U("SSN"), U("ST2110-20:2022") },
{ U("TP"), U("2110TPW") },
{ U("TROFF"), U("37") },
{ U("TROFF"), U("0") },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should have these zeros as a separate test case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add new test for zero troff and tsdelay

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test for zero troff and tsdelay

{ U("CMAX"), U("42") },
{ U("MAXUDP"), U("57") },
{ U("TSMODE"), U("SAMP") },
{ U("TSDELAY"), U("82") }
{ U("TSDELAY"), U("0") }
}
},
{
Expand All @@ -622,20 +598,20 @@ BST_TEST_CASE(testSdpParametersVideoRaw)
sdp::packing_modes::block,
sdp::smpte_standard_numbers::ST2110_20_2022,
sdp::type_parameters::type_W,
37,
uint32_t(0),
Copy link
Contributor

@garethsb garethsb Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest, does a bare 0 work? Or 0U? Bare 37 or 82 used to work...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ubuntu 14.04 issue again GCC 4.8.4 compile error, when using bare 0 for the bst::optional parameters. Maybe 0U will work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes 0U does work

42,
57,
sdp::timestamp_modes::SAMP,
82
uint32_t(0)
}
};

for (auto& test : { example, wacky })
{
auto made = nmos::make_video_raw_sdp_parameters(test.first.session_name, test.second, test.first.rtpmap.payload_type);
check_sdp_parameters(test.first, made);
sdp_test::check_sdp_parameters(test.first, made);
auto roundtripped = nmos::make_video_raw_sdp_parameters(made.session_name, nmos::get_video_raw_parameters(made), made.rtpmap.payload_type);
check_sdp_parameters(test.first, roundtripped);
sdp_test::check_sdp_parameters(test.first, roundtripped);
}
}

Expand Down Expand Up @@ -681,7 +657,7 @@ BST_TEST_CASE(testSdpParametersAudioL)
{
{ U("channel-order"), U("SMPTE2110.(M,M,M,M,ST,U02)") },
{ U("TSMODE"), U("SAMP") },
{ U("TSDELAY"), U("82") }
{ U("TSDELAY"), U("0") }
},
{},
0.333
Expand All @@ -692,17 +668,17 @@ BST_TEST_CASE(testSdpParametersAudioL)
96000,
U("SMPTE2110.(M,M,M,M,ST,U02)"), // not testing nmos::make_fmtp_channel_order here
sdp::timestamp_modes::SAMP,
82,
uint32_t(0),
0.333
}
};

for (auto& test : { example, wacky })
{
auto made = nmos::make_audio_L_sdp_parameters(test.first.session_name, test.second, test.first.rtpmap.payload_type);
check_sdp_parameters(test.first, made);
sdp_test::check_sdp_parameters(test.first, made);
auto roundtripped = nmos::make_audio_L_sdp_parameters(made.session_name, nmos::get_audio_L_parameters(made), made.rtpmap.payload_type);
check_sdp_parameters(test.first, roundtripped);
sdp_test::check_sdp_parameters(test.first, roundtripped);
}
}

Expand Down Expand Up @@ -751,9 +727,9 @@ BST_TEST_CASE(testSdpParametersVideoSmpte291)
{ U("exactframerate"), U("60000/1001") },
{ U("TM"), U("CTM") },
{ U("SSN"), U("ST2110-40:2021") },
{ U("TROFF"), U("37") },
{ U("TROFF"), U("0") },
{ U("TSMODE"), U("SAMP") },
{ U("TSDELAY"), U("82") }
{ U("TSDELAY"), U("0") }
}
},
{
Expand All @@ -762,18 +738,18 @@ BST_TEST_CASE(testSdpParametersVideoSmpte291)
nmos::rates::rate59_94,
sdp::transmission_models::compatible,
sdp::smpte_standard_numbers::ST2110_40_2023,
37,
uint32_t(0),
sdp::timestamp_modes::SAMP,
82
uint32_t(0)
}
};

for (auto& test : { example, wacky })
{
auto made = nmos::make_video_smpte291_sdp_parameters(test.first.session_name, test.second, test.first.rtpmap.payload_type);
check_sdp_parameters(test.first, made);
sdp_test::check_sdp_parameters(test.first, made);
auto roundtripped = nmos::make_video_smpte291_sdp_parameters(made.session_name, nmos::get_video_smpte291_parameters(made), made.rtpmap.payload_type);
check_sdp_parameters(test.first, roundtripped);
sdp_test::check_sdp_parameters(test.first, roundtripped);
}
}

Expand Down Expand Up @@ -810,20 +786,20 @@ BST_TEST_CASE(testSdpParametersVideoSmpte2022_6)
},
{
{ U("TP"), U("2110TPW") },
{ U("TROFF"), U("37") }
{ U("TROFF"), U("0") }
}
},
{
sdp::type_parameters::type_W,
37
uint32_t(0)
}
};

for (auto& test : { example, wacky })
{
auto made = nmos::make_video_SMPTE2022_6_sdp_parameters(test.first.session_name, test.second, test.first.rtpmap.payload_type);
check_sdp_parameters(test.first, made);
sdp_test::check_sdp_parameters(test.first, made);
auto roundtripped = nmos::make_video_SMPTE2022_6_sdp_parameters(made.session_name, nmos::get_video_SMPTE2022_6_parameters(made), made.rtpmap.payload_type);
check_sdp_parameters(test.first, roundtripped);
sdp_test::check_sdp_parameters(test.first, roundtripped);
}
}
Loading
Loading