sony / nmos-cpp

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

Connection API, permissive PATCH #79

Closed pkeroulas closed 5 years ago

pkeroulas commented 5 years ago

Hello, I'm using Riedel client to perform connections between HW senders and both HW/SW receivers. HW-to-HW connections work fine but HW-to-nmos-cpp fails. When patching staged/, I get a 400 Bad request - inconsistent array size because connection_api.cpp calls merge_patch() with default permissive=false, while other APIs set that flag to true. Is there any reason for this restriction for connection API only? Regards.

garethsb commented 5 years ago

Yes. Permissive is not appropriate here. I believe the Riedel client is wrong. See https://github.com/AMWA-TV/nmos-device-connection-management/issues/68.

pkeroulas commented 5 years ago

merge_patch() is called twice in connection_api. I think you're refering to the first call which specifically deals with transport_params but the issue is located at the second call. Several examples like this one show partial json which matches the idea of patching.

Here is the patch received from Riedel Explorer:

{
      "sender_id":"ef1f136c-6fcc-3d07-8f06-40a36ba03bec",
      "transport_params":[{"ext_stream_cipher":""}]
      "transport_file":{"data":"v=0\r\no=- 1443716955 1443716955 IN IP4 192.168.39.12\r\ns=st2110 0-0-0\r\nt=0 0\r\na=group:DUP primary secondary\r\nm=video 20000 RTP/AVP 96\r\nc=IN IP4 239.0.0.12/64\r\na=source-filter: incl IN IP4 239.0.0.12 192.168.39.12\r\na=rtpmap:96 raw/90000\r\na=fmtp:96 sampling=YCbCr-4:2:2; width=1920; height=1080; exactframerate=30000/1001; depth=10; TCS=SDR; colorimetry=BT709; PM=2110GPM; SSN=ST2110-20:2017; TP=2110TPN; interlace; \r\na=mediaclk:direct=0\r\na=ts-refclk:ptp=IEEE1588-2008:08-00-11-ff-fe-22-3f-b8:127\r\na=mid:primary\r\nm=video 20000 RTP/AVP 96\r\nc=IN IP4 239.0.1.3/64\r\na=source-filter: incl IN IP4 239.0.1.3 192.168.0.1\r\na=rtpmap:96 raw/90000\r\na=fmtp:96 sampling=YCbCr-4:2:2; width=1920; height=1080; exactframerate=30000/1001; depth=10; TCS=SDR; colorimetry=BT709; PM=2110GPM; SSN=ST2110-20:2017; TP=2110TPN; interlace; \r\na=mediaclk:direct=0\r\na=ts-refclk:ptp=IEEE1588-2008:08-00-11-ff-fe-22-3f-b8:127\r\na=mid:secondary\r\n","type":"application/sdp"}
}
garethsb commented 5 years ago

Is this a redundant Sender that is being patched? Please could you post its current /staged JSON?

Without seeing it, I'm assuming so. In which case, I think Riedel Explorer should have sent:

{
      "sender_id":"ef1f136c-6fcc-3d07-8f06-40a36ba03bec",
      "transport_params":[{"ext_stream_cipher":""}, {}],
      "transport_file":{"data":"v=0\r\no=- 1443716955 1443716955 IN IP4 192.168.39.12\r\ns=st2110 0-0-0\r\nt=0 0\r\na=group:DUP primary secondary\r\nm=video 20000 RTP/AVP 96\r\nc=IN IP4 239.0.0.12/64\r\na=source-filter: incl IN IP4 239.0.0.12 192.168.39.12\r\na=rtpmap:96 raw/90000\r\na=fmtp:96 sampling=YCbCr-4:2:2; width=1920; height=1080; exactframerate=30000/1001; depth=10; TCS=SDR; colorimetry=BT709; PM=2110GPM; SSN=ST2110-20:2017; TP=2110TPN; interlace; \r\na=mediaclk:direct=0\r\na=ts-refclk:ptp=IEEE1588-2008:08-00-11-ff-fe-22-3f-b8:127\r\na=mid:primary\r\nm=video 20000 RTP/AVP 96\r\nc=IN IP4 239.0.1.3/64\r\na=source-filter: incl IN IP4 239.0.1.3 192.168.0.1\r\na=rtpmap:96 raw/90000\r\na=fmtp:96 sampling=YCbCr-4:2:2; width=1920; height=1080; exactframerate=30000/1001; depth=10; TCS=SDR; colorimetry=BT709; PM=2110GPM; SSN=ST2110-20:2017; TP=2110TPN; interlace; \r\na=mediaclk:direct=0\r\na=ts-refclk:ptp=IEEE1588-2008:08-00-11-ff-fe-22-3f-b8:127\r\na=mid:secondary\r\n","type":"application/sdp"}
}

(I'm assuming there isn't really a missing comma on the end of the "transport_params" line in the original.)

pkeroulas commented 5 years ago

You're right, I missed a comma, the sender is redundant and transmitted transport_params must be wrong.

Sender's /staged/:

{
 "master_enable": true,
 "receiver_id": null,
 "transport_params": [
  {
   "source_port": 10000,
   "destination_ip": "239.0.0.12",
   "source_ip": "192.168.39.12",
   "destination_port": 20000,
   "rtp_enabled": true
  },
  {
   "source_port": 10000,
   "destination_ip": "239.0.1.3",
   "source_ip": "192.168.0.1",
   "destination_port": 20000,
   "rtp_enabled": true
  }
 ],
 "activation": {
  "requested_time": null,
  "activation_time": null,
  "mode": null
 }
}

That said, I still don't catch why at this point, the 2 arguments should be the same size.

merged (size==5):

{
     "activation": { activation_time: null, mode: null, requested_time: null },
     "master_enable": false,
     "sender_id": null,
     "transport_file": { data: null, type: 'application/sdp' },
     "transport_params":  [
          {
           "destination_port": "auto",
           "interface_ip": "auto",
           "source_ip": null,
           "multicast_ip": null,
           "rtp_enabled": true
          },
          {
           "destination_port": "auto",
           "interface_ip": "auto",
           "source_ip": null,
           "multicast_ip": null,
           "rtp_enabled": true
          }
         ]
}

patch (size==3):

{
      "sender_id":"ef1f136c-6fcc-3d07-8f06-40a36ba03bec",
      "transport_params":[{"ext_stream_cipher":""}, {}],
      "transport_file":{"data":"v=0\r\no=- 1443716955 1443716955 IN IP4 192.168.39.12\r\ns=st2110 0-0-0\r\nt=0 0\r\na=group:DUP primary secondary\r\nm=video 20000 RTP/AVP 96\r\nc=IN IP4 239.0.0.12/64\r\na=source-filter: incl IN IP4 239.0.0.12 192.168.39.12\r\na=rtpmap:96 raw/90000\r\na=fmtp:96 sampling=YCbCr-4:2:2; width=1920; height=1080; exactframerate=30000/1001; depth=10; TCS=SDR; colorimetry=BT709; PM=2110GPM; SSN=ST2110-20:2017; TP=2110TPN; interlace; \r\na=mediaclk:direct=0\r\na=ts-refclk:ptp=IEEE1588-2008:08-00-11-ff-fe-22-3f-b8:127\r\na=mid:primary\r\nm=video 20000 RTP/AVP 96\r\nc=IN IP4 239.0.1.3/64\r\na=source-filter: incl IN IP4 239.0.1.3 192.168.0.1\r\na=rtpmap:96 raw/90000\r\na=fmtp:96 sampling=YCbCr-4:2:2; width=1920; height=1080; exactframerate=30000/1001; depth=10; TCS=SDR; colorimetry=BT709; PM=2110GPM; SSN=ST2110-20:2017; TP=2110TPN; interlace; \r\na=mediaclk:direct=0\r\na=ts-refclk:ptp=IEEE1588-2008:08-00-11-ff-fe-22-3f-b8:127\r\na=mid:secondary\r\n","type":"application/sdp"}
}
garethsb commented 5 years ago

That said, I still don't catch why at this point, the 2 arguments should be the same size.

It's not those two arguments, it's the size of the transport params array within those two objects. I suppose it might be worth merge_patch carrying a JSON Pointer 'context' through its calls so the exception message could be clearer...

garethsb commented 5 years ago

Point of fact then, I would expect the PATCH still to fail, even with the second empty 'leg' added to it, because the "ext_stream_cipher" parameter doesn't exist. Hmm 🤔

garethsb commented 5 years ago

I have got confirmation from Riedel that "ext_stream_cipher" should not have been included in a request on your Sender, and that they will also fix the "transport_params" array-size issue.

pkeroulas commented 5 years ago

Great! I made a few tests without this mysterious param and it's working fine.

That said, I still don't catch why at this point, the 2 arguments should be the same size.

It's not those two arguments, it's the size of the transport params array within those two objects. I suppose it might be worth merge_patch carrying a JSON Pointer 'context' through its calls so the exception message could be clearer...

Oh, I see, it's because of the recursive call.

You can close this. Thanks a lot!