Subject: [MMUSIC] Review of draft-ietf-mmusic-msid-07

From: Ted Hardie <[email-masked]>
Date: Tue, 21 Oct 2014 09:33:28 -0700

Some time ago I promised a review of this draft, wrote one, and then never
sent it; Ari kindly reminded me that I owed to the group, without properly
berating me for the idiocy.  I've done a quick review of the updates since
the original and updated it.  I followed the template for the Apps Area
Directorate reviews, but this is not a review from that team (and not part
of their archives, etc.)

My apologies for the delay.

Ted

Document: draft-ietf-mmusic-msid
Title: WebRTC MediaStream Identification in the Session Description Protocol

Reviewer: Ted Hardie

Summary: No basic concerns with the mechanism described.
Comments:
Because the document currently describes the WebRTC semantics and makes
specific
references to JSEP, it's not clear that it can safely go forward with JSEP
not yet approved.
JSEP, on the other hand, needs this and BUNDLE to be stable.  Rather than
deadlock on
this at the WG level, I suggest that this document (and Bundle) go forward
even if JSEP is not yet
approved, but with an explicit note that they are not requesting downrefs.
That will
hold them for a final check by authors at the RFC Editor

Major Issues:

Minor Issues:

Section 4 consistently describes the requirements in terms of what the
entity implements, e.g.:

   An entity implmementing an MSID semantic MUST add one or more "msid-
   semantic" attributes to its session level attributes, indicating the
   MSID semantic it supports.

It's not clear this formulation is correct, as there may be cases
where an implementation
supports a specific semantic,but does not wish to use it.  This
hinges, I guess, on what
"entity" means here (the implementation or the instance in which it is
used).  I'd suggest
changing these formulations to something like "An implementation
supporting an MSID semantic
for a specific offer/answer MUST add..." unless there is a strong
reason to believe that
it must be offered if implemented.

Later, in section 5.1, the document says:

  "If an entity implementing the WMS semantic sends a description,
   it MUST include the msid-semantic:WMS attribute, even if no media
   streams are sent. This allows us to distinguish between the case
   of no media streams at the moment and the case of legacy SDP generation."

It's not clear what legacy SDP generation means here. It seems that the
presence of any msid-semantic session level attribute would indicate
that it was not "legacy SDP generation", for at least some values of legacy.

I think the point here is that you want to know at beginning of the session
whether or not WMS semantics will be used, even if there no media to bear
the MSIDs yet.  I'd suggest recasting this to say that, with something like

"In an entity intends to use WMS semantics in a session, it should the
msid-semantic
on sending a description, even if no media are sent"

In the IANA considerations, the document says:

   This document requests IANA to create a new registry called
   "Semantics for the msid-semantic SDP attribute", which should have
   exactly the same rules as for the "Semantics for the ssrc-group SDP
   attribute" registry (Expert Review), and to register the "WMS"
   semantic within this new registry.

Generally, IANA seems to be asking for guidance to the expert when
you create a registry with expert review; if the intent is to repeat
the guidance given for ssrc-group SDP attribute registry, then a pointer
to the guidance may be necessary.  Alternatively, it may be enough to
suggest that the working group expects this to be a sparsely used registry

Nits:

---

From: Adam Roach <[email-masked]>
Date: Tue, 21 Oct 2014 17:23:23 -0500

On 10/21/14 11:33, Ted Hardie wrote:
> Some time ago I promised a review of this draft, wrote one, and then 
> never sent it; Ari kindly reminded me that I owed to the group, 
> without properly berating me for the idiocy.

I, too, promised a review; and, like Ted, didn't ever send one. I don't 
even have the excuse of writing one and forgetting about it. I just 
didn't make the time to get to it. My comments below are largely in 
document order.

I find no major issues in this document, but I believe that it requires 
quite a few editorial (and a few slightly-more-than editorial) changes 
before it's ready to progress.

----------------------------------------------------------------------

Section 1.2 starts with an assertion that the document describes a new 
RFC 5888 grouping type. This no longer appears to eb true, and it should 
be removed.

----------------------------------------------------------------------

Section 2: The sentence " The attribute also allows application-specific 
information to the association" appears to be missing some words.

Implementors are likely to find the statements " The identifier... MUST 
be unique among the identifier values used in the same SDP session" and 
" There may... be multiple media descriptions that have the same value 
for identifier" to be confusing, when taken together. I think the 
conceptual thing that must be unique here needs to be more clearly 
defined. In fact, I'm a bit confused myself -- the way the mechanism is 
defined, I believe it is impossible to violate this MUST: using the same 
ID simply puts things in the same group. Is this intended to be some 
kind of temporal constraint?

----------------------------------------------------------------------

Section 2:

   msid-attr = "msid:" msid-id

There's some unfortunate interaction between ABNF and SDP here.

RFC 4566 states "In general, <value> is either a number of fields 
delimited by a single space character or a free format string, and is 
CASE-SIGNIFICANT unless a specific field defines otherwise." Section 
5.13, which defines the syntax for a=, does not carve out an exception. 
So, attribute names are case sensitive.

RFC 5234 states "ABNF strings are case insensitive and the character set 
for these strings is US-ASCII." So, the definition above is overly lax 
in defining the syntax.

Historically, the solution has been to do something like:

   msid-attr = %x6D.73.69.64 ":" msid-id

But that's pretty horrible. Luckily, this is in the RFC Editors' queue: 
http://datatracker.ietf.org/doc/draft-kyzivat-case-sensitive-abnf/

If you're willing to cite it in this document, it makes things much 
easier to read:

   msid-attr = %s"msid:" msid-id

This comment also applies to "msid-semantic:" in section 3.

----------------------------------------------------------------------

In section 3, there's not a clear prohibition against an MSID appearing 
in more than one "msid-semantic" line, but I'm pretty sure that's what 
we want. In any case, we need to be explicit about whether this is 
allowed, or you _will_ have implementors arguing about it.

Similarly, I think we want a statement that each MSID MUST appear in an 
msid-semantic line somewhere, right? Otherwise, they're pretty useless.

----------------------------------------------------------------------

Section 4 should probably go into a bit more detail about what is mean 
by the fact that MSID is not negotiated; in particular, the document 
needs to point out somewhere the consequences that (a) the session has 
to "work" (for some definition of "work") if the remote party does not 
understand the MSID semantic being advertised, and (b) you won't 
necessarily know if the remote party has applied the semantics you 
indicate in an msid-semantic attribute.

----------------------------------------------------------------------

Section 4.2 is titled "Answerer processing of the offer," but appears to 
describe the offerer processing of the answer (in fact, its text is 
identical to section 4.4).

----------------------------------------------------------------------

Section 5: The mention of "recvonly" without address "sendonly" and 
"inactive" is going to cause implementors confusion.

----------------------------------------------------------------------

Section 5.1: I honestly don't know what is meant in the final paragraph 
by "default media stream". This needs to be clarified.

----------------------------------------------------------------------

Section 5.2.2 leaves out a step: the recipient needs to only do these 
things for MSIDs that are listed in the session-level 
"a=msid-semantic:WMS" line.

----------------------------------------------------------------------

Section 5.2.5 doesn't talk about signaling to the application that a 
MediaStream is gone once it no longer appears in any MSID lines. 
Certainly that's the intention, right?

----------------------------------------------------------------------

The MSID purpose in section 6 seems oddly specific ("It can be used to 
signal the relationship between a WebRTC MediaStream and a set of 
m-lines") for a mechanism that otherwise seems to be intended to be 
generic. Perhaps this can be made more general?

----------------------------------------------------------------------

General, tiny nit: tools.ietf.org requires citations to have a space 
before the bracket; otherwise, they're not turned into hotlinks. So, 
e.g., "Grouping[RFC5888]" won't be linked because there is no space 
after the "g". This happens at least twice in the document.

----------------------------------------------------------------------

I have not reviewed the appendices.

/a

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 21 Oct 2014 22:10:07 -0400

> This comment also applies to "msid-semantic:" in section 3.

Please take a look at draft-ietf-mmusic-rfc4566bis-12, where I have 
redone the definitions of all the attributes defined there.

I have adopted a definition style that doesn't use abnf to specify the 
*name* of the attribute. So specifying the name in a case-sensitive way 
isn't an issue. I suggest you follow that style.

And, if you object to that style, please provide comments on that draft.

	Thanks,
	Paul

---

From: Adam Roach <[email-masked]>
Date: Fri, 24 Oct 2014 13:29:04 -0500

On 10/21/14 21:10, Paul Kyzivat wrote:
> *name* of the attribute. So specifying the name in a case-sensitive 
> way isn't an issue. I suggest you follow that style.
>

That does seem cleaner, and I would think the MSID work should build on it.

I do admit that I'm confused by draft-ietf-mmusic-rfc4566bis-12 section 
8.2.4. The third bullet calls for an ABNF syntax rule defining for the 
syntax of the value. The seventh bullet calls for an ABNF definition for 
the syntax of the value.

If this isn't straight-up duplication, then I think the document needs 
to make a clearer distinction between what it means by these two 
bullets. I certainly wouldn't know how to conform to these requirements 
if I needed to register a new attribute.

/a

---

From: Paul Kyzivat <[email-masked]>
Date: Fri, 24 Oct 2014 14:59:53 -0400

> if I needed to register a new attribute.

The third bullet asks for the *name* of an ABNF rule.

The 7th bullet asks for the *definition* of that rule. I thought it was 
clear. If not, can you suggest a change to improve it?

Also note, that the third bullet says to use [] to indicate an optional 
value. This isn't ABNF, though it looks like it. Attributes with 
optional values are rare. (There aren't any defined in 4566.) The thing 
about an optional value is that the ":" separator is only present if the 
value is. This is a way to say that without including the attribute name 
in the syntax of the value.

(I forget which one, but I reviewed a proposal for a new attribute that 
did have an optional value, and it had initially put the ":" in when 
there was no value. It is important that we are consistent about how 
this is done.)

	Thanks,
	Paul

---

From: Christer Holmberg <[email-masked]>
Date: Sat, 25 Oct 2014 14:52:33 +0000

Hi,

>> I do admit that I'm confused by draft-ietf-mmusic-rfc4566bis-12 
>> section 8.2.4. The third bullet calls for an ABNF syntax rule defining 
>> for the syntax of the value. The seventh bullet calls for an ABNF 
>> definition for the syntax of the value.
>>
>> bullets. I certainly wouldn't know how to conform to these 
>> requirements if I needed to register a new attribute.
>
> The 7th bullet asks for the *definition* of that rule. I thought it was clear. If not, can > you suggest a change to improve it?

My suggestion would be to remove the 3rd bullet. IANA does not need that information, and there is no need to put it in any registry.

The 7th bullet would ask for a reference to where the syntax, offer/answer considerations etc, are defined.

Regards,

Christer

---

From: Paul Kyzivat <[email-masked]>
Date: Sat, 25 Oct 2014 13:04:31 -0400

> The 7th bullet would ask for a reference to where the syntax, offer/answer considerations etc, are defined.

This isn't just a matter of what IANA needs to know.

4566bis must define the complete set of information that must be 
provided about an attribute. The IANA registry doesn't need to contain 
all that info. It minimally needs to contain the name of the attribute 
and a reference to the document that contains the rest. We put more of 
the info in the registry for convenience.

In particular, it isn't practical to put the abnf for the value into the 
registry. Nor the semantics for the attribute, beyond some basics like 
session/media/source level.

	Thanks,
	Paul

	Thanks,
	Paul

---

From: Christer Holmberg <[email-masked]>
Date: Sat, 25 Oct 2014 17:20:43 +0000

Hi,

>>>> section 8.2.4. The third bullet calls for an ABNF syntax rule 
>>>> defining for the syntax of the value. The seventh bullet calls for 
>>>> an ABNF definition for the syntax of the value.
>>>>
>>>> If this isn't straight-up duplication, then I think the document 
>>>> needs to make a clearer distinction between what it means by these 
>>>> two bullets. I certainly wouldn't know how to conform to these 
> 4566bis must define the complete set of information that must be provided about an attribute. The 
> IANA registry doesn't need to contain all that info. It minimally needs to contain the name of the attribute 
> and a reference to the document that contains the rest.

Correct. And, it needs to be clear what information is required, so that expert reviewers etc know to look for it.

> We put more of the info in the registry for convenience.
>
>In particular, it isn't practical to put the abnf for the value into the registry. Nor the semantics for the attribute, beyond some basics like session/media/source level.

Exactly - and I think that is the issue me and Adam are having, as bullet 3 seems to require the value ABNF to be put in the registry. For that reason I suggest to remove bullet 3, and then in bullet 7 (or somewhere else) inform that the value ABNF needs to be present in the document defining the attribute.

The name of the attribute obviously need to be in the registry.

Regards,

Christer