search

AMWA-TV / sdpoker

A patched version of Streampunk/sdpoker with additional source-filter testing and bug fixes, used by AMWA-TV/nmos-testing
Apache License 2.0
12 stars 8 forks source link

Make fmtp checking optional for ST.2110-40 #14

Closed peterbrightwell closed 1 year ago

peterbrightwell commented 2 years ago

See https://github.com/Streampunk/sdpoker/pull/13 which makes fmtp optional for -40. Need to confirm whether the spec actually allows this and if so cherry-pick here.

garethsb commented 2 years ago

And this follow up one - https://github.com/Streampunk/sdpoker/pull/16/files

daniel-pebble commented 2 years ago

My understanding is that VPID_Code and DID_SDID are optional but "a=fmtp" is not according to ST2110-10. https://www.rfc-editor.org/rfc/rfc8331

Here is our ST2110-40 SDP which fails test_41.

v=0 o=- 3870434320 3870434320 IN IP4 192.168.20.234 s=PBS Node: SUP-IP-DOL5 - DolphinStream-01 - PEB-S1-ANC-OUT-1 t=0 0 a=group:DUP primary secondary m=video 50040 RTP/AVP 100 c=IN IP4 239.40.139.1/128 a=source-filter: incl IN IP4 239.40.139.1 192.168.20.234 a=rtpmap:100 smpte291/90000 a=fmtp:100 a=ts-refclk:ptp=IEEE1588-2008:08-00-11-FF-FE-23-04-D8:42 a=mediaclk:direct=0 a=mid:primary m=video 50140 RTP/AVP 100 c=IN IP4 239.140.139.1/128 a=source-filter: incl IN IP4 239.140.139.1 192.168.110.235 a=rtpmap:100 smpte291/90000 a=fmtp:100 a=ts-refclk:ptp=IEEE1588-2008:08-00-11-FF-FE-23-04-D8:42 a=mediaclk:direct=0 a=mid:secondary

Running the same SDP through the latest npn Streampunk/sdpoker it doesn't generate any errors or warnings

image

garethsb commented 2 years ago

I don't yet see anything in ST 2110-40, -10, RFC 4566, or RFC 8331 that makes a=fmtp: line required. I believe your implementation with the <format> (payload type) but no <format specific parameters> is valid, but I think omitting the line could also be valid.

garethsb commented 2 years ago

However, looking at the history, the referenced PRs in streampunk/sdpoker ARE already merged in this AMWA-TV repo. So something else at play here if your file does not pass.

garethsb commented 2 years ago

Ah, @daniel-pebble, what error message are you getting, is it like the following?

For stream 1, found an 'fmtp' attribute that is not an acceptable pattern.

Does your SDP have no space after the a=fmtp:100 before the end of line?

daniel-pebble commented 2 years ago

Hi @garethsb, yeah that is the error we get. Here is a screenshot taken from within the nmos-testing container. image I can confirm when I remove the "fmtp" line from the SDP there is no error.

We do have a space at the end of the file image

It looks like adding "&& !isSkippedType" to https://github.com/AMWA-TV/sdpoker/blob/master/checkST2110.js#L446 should fix this issue.
This is what the upstream project is doing - https://github.com/Streampunk/sdpoker/blob/master/checkST2110.js#L403

garethsb commented 2 years ago

The upstream repo is out of date, that code has been changed in this repo for a reason. I'll propose what I believe is a better fix...