ebu / pi-list

Live IP Software Toolkit to assist EBU members in the implementation of IP based facilities
GNU General Public License v3.0
108 stars 30 forks source link

Several little issues in exported SDP files for ancillary data #51

Closed garethsb closed 5 years ago

garethsb commented 5 years ago

Example exported SDP file for ancillary data:

v=0
o=- 1 1 IN IP4 0.0.0.0
s=LIST Generated SDP
i=Ancillary flow, GENERATED by EBU-LIST (some params like media clock are hardcoded).
t=0 0
a=recvonly
m=ancillary 50040 RTP/AVP 100
c=IN IP4 239.43.24.1/32
a=mediaclk:direct=0
a=ts-refclk:ptp=IEEE1588-2008:00-00-00-00-00-00-00-00:00
a=rtpmap:100 smpte291/90000
a=source-filter:incl IN IP4 239.43.24.1 192.168.23.24
a=fmtp:100 DID_SDID={0x96,0x96};
  1. Shouldn't it have m=video... not ancillary, as the correct media type is video/smpte291?
    Fix presumably needed here: https://github.com/ebu/pi-list/blob/9c500b2ffdbb16b2b3e83c7ed3d4c181ca7a6d6b/cpp/libs/rtp/lib/src/ebu/list/sdp/sdp_writer.cpp#L49-L50
  2. a=ts-refclk: format isn't valid according to RFC 7273 Section 4.8, is it?
    If a domain number is intended, perhaps the fix would be to change trailing :00 into :domain-nmbr=0 at: https://github.com/ebu/pi-list/blob/9c500b2ffdbb16b2b3e83c7ed3d4c181ca7a6d6b/cpp/libs/rtp/lib/src/ebu/list/sdp/sdp_writer.cpp#L67
  3. DID_SDID looks plausible but this stream had SMPTE ST 12 timecode (60h, 60h), so I think the number formatting is incorrect.
    Fix presumably needed here because std::to_string only does decimal output: https://github.com/ebu/pi-list/blob/9c500b2ffdbb16b2b3e83c7ed3d4c181ca7a6d6b/cpp/libs/st2110/lib/src/ebu/list/st2110/d40/anc_description.cpp#L76-L78
garethsb commented 5 years ago
  1. a=ts-refclk: format isn't valid according to RFC 7273 Section 4.8, is it? If a domain number is intended, perhaps the fix would be to change trailing :00 into :domain-nmbr=0 at: https://github.com/ebu/pi-list/blob/9c500b2ffdbb16b2b3e83c7ed3d4c181ca7a6d6b/cpp/libs/rtp/lib/src/ebu/list/sdp/sdp_writer.cpp#L67

I just remembered I'd seen errata for RFC 7273 that changed the ABNF to match the examples.

Based on https://www.rfc-editor.org/errata/eid4450 the fix for this issue is only to change the trailing :00 into :0 (single digit).

pedro-alves-ferreira commented 5 years ago

Hello Gareth,

Many thanks for pointing this out and for the thorough feedback. We'll correct these issues soon.

Regards,

Pedro

pedro-alves-ferreira commented 5 years ago

Hi Gareth,

These should have been fixed now. I'd appreciate if you could check it and close the issue.

Thanks!

Pedro