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

From: Harald Alvestrand <[email-masked]>
Date: Wed, 11 Feb 2015 16:44:30 +0100

On 10/22/2014 12:23 AM, Adam Roach wrote:
> 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.

Done. It's just an SDP attribute now.

>
> ----------------------------------------------------------------------
>
> 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.

I adopted Paul's suggestion instead. Seems simpler.

>
> ----------------------------------------------------------------------
>
> 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.

I think it's easiest to prohibit it. I can't think of a sensible use for 
it now.

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

Agreed. Added "MUST appear in one and only one "msid-semantic" line".
>
> ----------------------------------------------------------------------
>
> 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.
Added more language. It looks suspiciously like a negotiation; the only 
difference is that the answerer can (so far) add more semantics than the 
offerer indicates. I'm not sure this is of great value.

>
> ----------------------------------------------------------------------
>
> 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).

Good catch. Fixed.
>
> ----------------------------------------------------------------------
>
> Section 5: The mention of "recvonly" without address "sendonly" and 
> "inactive" is going to cause implementors confusion.
Mentioned them all.

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

This is a result of the many flipflops between "all tracks belong to the 
same stream" and "all tracks belong to different streams". At the latest 
TPAC meeting we flipped back to "all tracks belong to the same stream". 
Text updated.

>
> ----------------------------------------------------------------------
>
> 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.

Good catch. I used "associated with" instead of "listed in", so that the 
"*" case is covered.

>
> ----------------------------------------------------------------------
>
> 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?

No, that's not the intention. The MediaStream is not gone, it's just 
inactive - the application can still inspect the object, but no more 
media data is coming out of it.
The MediaStreamTrack isn't gone either, it's just ended.

>
> ----------------------------------------------------------------------
>
> 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?

I thought it was best to give a specific example - "can be used". 
Changed to "For example, it can be used".
>
> ----------------------------------------------------------------------
>
> 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.

Will try to catch them all.

>
> ----------------------------------------------------------------------
>
> I have not reviewed the appendices.
>
> /a
>
>
> _______________________________________________
> mmusic mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/mmusic

---

From: Harald Alvestrand <[email-masked]>
Date: Wed, 11 Feb 2015 16:44:37 +0100

On 10/21/2014 06:33 PM, Ted Hardie wrote:
> 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.

And my apologies for the even longer delay in incorporating the comments.

>
> 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.

Good question. I think "wishing to use" is appropriate - in both cases, 
if you don't signal the support, you can't use the mechanism.
>
> 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.

Replaced "legacy" with "the case of SDP generated by an entity that 
wishes to use the backwards compatible mechanism". I think that's 
correct - and there may be new devices (gateways, others) that wish to 
use the backwards-compatible mechanism despite not being "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"

Yes. Please look at the reformulation when it comes out....

> 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

I put in "Usage of the registry is expected to be low, so the expert 
should feel free to consult widely if a new request ever comes in." This 
might be enough.

> Nits:

---

From: Paul Kyzivat <[email-masked]>
Date: Wed, 11 Feb 2015 17:01:21 -0500

One comment on the -08 version:

* Section 4.2 (Answerer Processing of the Offer) says:

    If an "msid-semantic" attribute is present in the offer, and the
    answerer wishes to use the indicated semantic, the offerer MUST
    follow the procedures described for that semantic.

This makes no sense to me, since it is giving instructions to the 
*offerer*. I guess it should say "the *answerer* MUST follow ..."

	Thanks,
	Paul

---

From: Harald Alvestrand <[email-masked]>
Date: Thu, 12 Feb 2015 16:29:45 +0100

> *offerer*. I guess it should say "the *answerer* MUST follow ..."

Oops. Yes.

-- 
Surveillance is pervasive. Go Dark.

---

From: Paul Kyzivat <[email-masked]>
Date: Thu, 12 Feb 2015 12:43:42 -0500

This is really old now, but I don't know if it was ever closed out.

On 10/25/14 1:20 PM, Christer Holmberg wrote:
> 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.
>>>>>
>>>>> 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.
>>>>
>>>> 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?
>>>
>>> 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.
>>
>> 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.
>
> 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.

Section 8.2.4 says that the bulleted items must be included in the 
specification in order to be accepted for registration. It doesn't say 
the registry has to contain all of those things.

This is a checklist to be enforced by IANA and/or the expert reviewer.
The things that go into the registry are a subset of these.

So in a document you can put all the stuff in this list into an 
arbitrary place in your document that makes sense. Then in the IANA 
considerations section you can simply point to that section and ask to 
register the attribute defined there. Then IANA can extract from that 
the stuff that goes into their registry.

	Thanks,
	Paul

---

From: "Ali C. Begen (abegen)" <[email-masked]>
Date: Fri, 13 Feb 2015 05:49:14 +0000

Could you guys check whether the latest version is acceptable to you?

-----Original Message-----
From: Paul Kyzivat
Date: Thursday, February 12, 2015 at 7:43 PM
To: Christer Holmberg, Adam Roach, "[email-masked]", 
"[email-masked]"
Subject: Re: [MMUSIC] Comment on draft-ietf-mmusic-rfc4566bis-12
Resent-From: <[email-masked]>
Resent-To: "Ali C. Begen", Colin Perkins, <[email-masked]>, 
<[email-masked]>
Resent-Date: Thursday, February 12, 2015 at 7:43 PM

>>>>> 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.
>>>
>>> IANA registry doesn't need to contain all that info. It minimally 
>>>needs to contain the name of the attribute
>> Correct. And, it needs to be clear what information is required, so 
>>that expert reviewers etc know to look for it.
>>
>>> 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.
>>

---

From: Paul Kyzivat <[email-masked]>
Date: Fri, 13 Feb 2015 12:32:04 -0500

> Could you guys check whether the latest version is acceptable to you?

I checked the diffs. Found a couple of little things:

* Section 6.10

Contains:

       charset-value = mime-charset (as defined in <xref
         target="I-D.iana-charset-reg-procedure"/>)

The xref didn't get expanded (because it is in artwork.)

* Section 8.2.4:

Contains:

       ... The rule
       name SHOULD [MUST?]  NOT be defined as an Incremental Alternative
       to <att-value>.

There needs to be a decison about SHOULD or MUST. I propose to make it MUST.

Otherwise this seems fine to me.

Somebody else should double check me that the new definition of 
token-char in Section 9 is equivalent to the old one.

	Thanks,
	Paul