Subject: [rtcweb] partial review of draft-ietf-rtcweb-sdp-01

From: Paul Kyzivat <[email-masked]>
Date: Mon, 30 May 2016 18:14:53 -0400

Ted asked if I would do a review of draft-ietf-rtcweb-sdp-01. I started, 
but it is taking a long time. I'm sending what I have so far to give you 
something to consider/work on.

In order to simplify this review for both me and readers, I will in 
general only mention an issue once, the first time I find it, and be 
silent on future occurrences of the same issue.

* In general in the examples it would be very helpful if you would 
include a blank line prior to each m-line. When reading these it is 
currently hard to find the m-lines, which is something needed when 
trying understand and compare things.

* Section 3:

I realize this is for a webrtc oriented reader, so I am trying to 
understand it from that perspective. Even so, I have trouble with 
classifying a=ice* as "Security Descriptions". Given the existing 
categories I would think they better belong in "Stream Description".

* Section 5.1:

    o  The term "Session" is used rather loosely in this document to
       refer to either a "Communication Session" or a "RTP Session" or a
       "RTP Stream" depending on the context.

This seems like a bad idea. It may be forgivable to mix communication 
session and RTP session in examples where a single bundle is used for 
all media, or there is only one m-line. But not if multiple m-lines 
aren't bundled, and not with RTP Stream.

OTOH, so far I haven't noticed any place where this is an issue.

* Section 5.2.1:

The SDP line-wrapping convention described in 5.1 isn't being used in 
the example.

Is there a reason for including the IP address in the a=rtcp line, when 
it is the same as the address in the c= line, which is the default?

Is there any expectation that a=ptime:60 only applies to opus? IIUC this 
should be interpreted ad applying to all codecs mentioned in the m-line. 
(There is no order-dependence among attributes within a single media 
section. AFAIK there is no way of specifying a separate ptime for 
individual codecs in an m-line.)

Use of sha-1 in a=fingerprint is about to become incorrect according to 
draft-ietf-mmusic-4572-update-03. That work is being driven by rtcweb, 
so I would think you would want to reflect it here.

The way tables are formatted in RFC format can make the examples 
confusing to readers: the label for table 1 falls exactly between the 
bottom of the table it describes and the following table. When viewed on 
a screen it is difficult to tell what each table is. I suggest tweaking 
the contents of the tables to make this clearer. E.g.,

    +----------------------------------+--------------------------------+
    | SDP Contents - Offer             | RFC#/Notes                     |
    +----------------------------------+--------------------------------+

In Table 2, the o-line has two spaces after the "-". It should only be one.

* Section 5.2.3:

Table 5 has ice and fingerprint attributes prior to the first m-line.

The m-line and associated attributes for the data channel is 
inconsistent with draft-ietf-mmusic-sctp-sdp-16. I think it ought to be 
(more or less):

m=application 56966 UDP/DTLS/SCTP webrtc-datachannel
c=IN IP4 24.23.204.141
a=mid:data
a=sctp-port:5000
a=setup:actpass
...

If you really want to negotiate the usage of channel 1, then it also 
needs to include:

a=dcmap:1 subprotocol="chat";label="channel 1"

(Note: this presumes that "chat" is a registered subprotocol. It isn't!)

* Section 5.2.4:

This seems fine if Bob intends to send audio (e.g. MoH). If not then it 
would be better to answer a=inactive.

* Section 5.2.5:

Has there been any discussion of pros/cons of having DTMF on a separate 
m-line and a separate ssrc? If one or both ends actually think of DTMF 
as being part of another audio stream then this might present problems. 
This could especially be true for interoperation with telephony devices.

Is there anything in rtcweb that prevents having these on the same 
m-line and ssrc?

* Section 5.2.7:

Note that the BAS concept has been removed from Bundle, so you at least 
shouldn't refer to it. (You can still do the O/A if you want.)

In Table 14 (answer), why are ice attributes and fingerprints given 
separately (and differently) for the two m-lines? (Even though the ports 
and addresses are the same.) This is contrary to current bundle (-29) 
procedures.

In the 2nd (BAS) offer there is a gratuitous change in ordering of 
several of the attributes. (Makes it difficult to see what has changed.) 
I also don't understand the other differences between the ice and 
fingerprints in audio and video, and between this and the first offer.

* Section 5.2.8:

This example *assumes* bundle support, and so uses the same addr/port 
for bundled m-lines. But this will fail badly if the assumption turns 
out to be wrong. When making this assumption you ought to use 
bundle-only in the initial offer to ensure nothing bad happens if the 
assumption of bundle support by answerer turns out to be wrong. AFAIK 
there is no downside to doing this.

Again this doesn't follow current bundle rules.

=======

That's as far as I have gotten. (Slow going.) I'll try to get you more 
later.

	Thanks,
	Paul

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 31 May 2016 15:09:35 -0400

Another general issue:

I have been *trying* to verify the consistency in use of ports, IP 
addresses, fingerprints, ice ufrag info, ice passwords, ssrcs, cnames, etc.

Of course this is excruciating to do. (For a reviewer like me, and also 
for a potential implementer trying to understand the examples.)

My preliminary take is that there are a number of errors. A lot of them 
are probably cut/paste errors. With sufficient analysis it is sometimes 
possible to figure out what was intended vs. what is written. But it is 
hard with this much stuff.

It would be easier if, instead of using random sample values, the values 
were replaced with semantically relevant and easily recognized values. 
(E.g., "ALICE-SSRC-1", "TED-CNAME-2") Unfortunately I don't think this 
can be done while also having those values by syntactically valid.

I think I will end up making such a replacement myself, in a local copy, 
simply to assist in analyzing what is there.

	Thanks,
	Paul

On 5/30/16 6:14 PM, Paul Kyzivat wrote:
> _______________________________________________
> rtcweb mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/rtcweb
>

---

From: Suhas Nandakumar <[email-masked]>
Date: Tue, 31 May 2016 13:25:35 -0700

Thanks Paul for reviewing the draft.  We will go through your comments and
get back to you. In the meanwhile, I suggest people to HTML rendering of
the draft for ease of readability.

Here is the link: https://tools.ietf.org/id/draft-ietf-rtcweb-sdp-01.html

Thanks
Suhas

On Tue, May 31, 2016 at 12:09 PM, Paul Kyzivat <[email-masked]>
wrote:

> (E.g., "ALICE-SSRC-1", "TED-CNAME-2") Unfortunately I don't think this can
> be done while also having those values by syntactically valid.
>