Conversation
garethsb
left a comment
garethsb
left a comment
There was a problem hiding this comment.
Hey, Nikita. I've just scanned the code, starting to understand it. Looks very good, just jotted a few comments as I went...
|
|
||
| try | ||
| { | ||
| validate_sdp_parameters(receiver, sdp_params); |
There was a problem hiding this comment.
This needs to be configurable by the user as well, I think?
There was a problem hiding this comment.
Yes. Made validate_receiver_resources() a configurable callback.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
gatebutparse_rtp_transport_file(the only function of this type?) doesn't actually use it.
There was a problem hiding this comment.
- functions of this type take
gatebutparse_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
…raint Set is a subset of another one
Co-authored-by: Simon Lo <simon.lo@sony.com>
Co-authored-by: Simon Lo <simon.lo@sony.com>
| 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 |
There was a problem hiding this comment.
| 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 |
| 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); | ||
| } |
There was a problem hiding this comment.
Shouldn't 500 be returned if active_constraints_handler is null?
| active_constraints_handler(*streamcompatibility_sender, active_constraints, intersection); | ||
| details::set_active_constraints(model, resourceId, nmos::fields::constraint_sets(active_constraints), intersection, effective_edid_setter); |
There was a problem hiding this comment.
Shouldn't active_constraints_handler be checked for null before use? If it is null, should it return 500?
There was a problem hiding this comment.
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.
|
Hi, we would love to see IS-11 support in NVNMOS. Thanks, 7thSense team! |
|
Hi @N-Nagorny, I hope you are doing well. We are now looking into this PR. I just merged it locally to master, but when I try to push to your repository, I am getting a permission denied error. Can you add me in as a collaborator to your nmos-cpp repository. So I can complete the review and make any necessary changes. Many thanks. |
|
This is superseded by #474 |
4 participants
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?