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.

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

IS-11 support #271

Open
wants to merge 116 commits into
base: master
Choose a base branch
from
Open

IS-11 support #271

wants to merge 116 commits into from

Conversation

N-Nagorny
Copy link
Contributor

Hello everyone!

This PR adds Stream Compatibility Management API (AMWA-TV/is-11@e98d156) implementation and IS-11 support.

Could you please review it and describe what's needed to change to be merged?

Copy link
Contributor

@garethsb garethsb left a comment

Choose a reason for hiding this comment

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

Hey, Nikita. I've just scanned the code, starting to understand it. Looks very good, just jotted a few comments as I went...

Development/nmos/api_utils.h Outdated Show resolved Hide resolved
Development/nmos/constraints.cpp Outdated Show resolved Hide resolved
Development/nmos/constraints.cpp Outdated Show resolved Hide resolved
Development/nmos/constraints.cpp Outdated Show resolved Hide resolved
Development/nmos/constraints.cpp Outdated Show resolved Hide resolved
Development/nmos/constraints.cpp Outdated Show resolved Hide resolved
Development/nmos/constraints.cpp Outdated Show resolved Hide resolved
Development/nmos/streamcompatibility_behaviour.cpp Outdated Show resolved Hide resolved
Development/nmos/streamcompatibility_behaviour.cpp Outdated Show resolved Hide resolved

try
{
validate_sdp_parameters(receiver, sdp_params);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be configurable by the user as well, I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Made validate_receiver_resources() a configurable callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly to https://github.com/sony/nmos-cpp/pull/271/files#r1028471346, I wonder if we want a make_streamcompatibility_receiver_validator function that takes the SDP parser and validator to prevent the user having to write this whole function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added make_streamcompatibility_receiver_validator that takes nmos::transport_file_parser and it has some restrictions:

  • this function is expected to parse and validate the transport file at once. Not sure if it can be split into two arguments because I can't imagine a validator that is unaware of the internal details of the transport file format.
  • functions of this type take gate but parse_rtp_transport_file (the only function of this type?) doesn't actually use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • functions of this type take gate but parse_rtp_transport_file (the only function of this type?) doesn't actually use it.

Hmm, yeah, I guess that seemed like a good idea at the time... 5bc3962 😄
We don't like making incompatible changes if we can help it, but if the gate parameter is causing problems, we should revisit.

  • this function is expected to parse and validate the transport file at once.

OK, I'll review to understand what you mean!

…move Flow Compatibility API helper functions into details namespace
N-Nagorny and others added 8 commits March 4, 2024 15:20
# Conflicts:
#	Development/cmake/NmosCppLibraries.cmake
#	Development/nmos-cpp-node/config.json
#	Development/nmos/json_schema.cpp
#	Development/nmos/json_schema.h
#	Development/nmos/node_server.cpp
#	Development/nmos/node_server.h
#	Development/nmos/settings.h
# Conflicts:
#	Development/cmake/NmosCppLibraries.cmake
#	Development/cmake/NmosCppTest.cmake
#	Development/nmos-cpp-node/config.json
#	Development/nmos-cpp-node/node_implementation.cpp
#	Development/nmos/json_fields.h
#	Development/nmos/json_schema.cpp
#	Development/nmos/json_schema.h
#	Development/nmos/model.h
#	Development/nmos/node_resources.cpp
#	Development/nmos/node_server.cpp
#	Development/nmos/node_server.h
#	Development/nmos/settings.h
Development/nmos/model.h Outdated Show resolved Hide resolved
streamcompatibility_api.support(U("/") + nmos::patterns::senderType.pattern + U("/") + nmos::patterns::resourceId.pattern + U("/constraints/active/?"), methods::PUT, [&model, validator, active_constraints_handler, effective_edid_setter, &gate_](http_request req, http_response res, const string_t&, const route_parameters& parameters)
{
nmos::api_gate gate(gate_, req, parameters);
return nmos::details::extract_json(req, gate).then([&model, req, res, parameters, &validator, &active_constraints_handler, &effective_edid_setter, gate](value data) mutable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nmos::details::extract_json(req, gate).then([&model, req, res, parameters, &validator, &active_constraints_handler, &effective_edid_setter, gate](value data) mutable
return nmos::details::extract_json(req, gate).then([&model, req, res, parameters, validator, active_constraints_handler, effective_edid_setter, gate](value data) mutable

Comment on lines +816 to +831
if (active_constraints_handler)
{
if (!active_constraints_handler(*streamcompatibility_sender, data, intersection))
{
can_adhere = false;

slog::log<slog::severities::warning>(gate, SLOG_FLF) << "Active Constraints update is requested for " << id_type << " but this sender can't adhere to these Constraints";
set_error_reply(res, status_codes::UnprocessableEntity);
}
}

if (can_adhere)
{
details::set_active_constraints(model, resourceId, nmos::fields::constraint_sets(data), intersection, effective_edid_setter);
set_reply(res, status_codes::OK, data);
}
Copy link
Contributor

@lo-simon lo-simon Mar 18, 2024

Choose a reason for hiding this comment

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

Shouldn't 500 be returned if active_constraints_handler is null?

Comment on lines +881 to +882
active_constraints_handler(*streamcompatibility_sender, active_constraints, intersection);
details::set_active_constraints(model, resourceId, nmos::fields::constraint_sets(active_constraints), intersection, effective_edid_setter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't active_constraints_handler be checked for null before use? If it is null, should it return 500?

Copy link
Contributor

@lo-simon lo-simon Mar 18, 2024

Choose a reason for hiding this comment

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

I don't think the active_constraints_handler is actually required for the Reset the Active Constraints, just set_active_constraints of the resource to reset its intersection_of_caps_and_constraints and the endpoint_active_constraints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants