Re: [MMUSIC] AD Evaluation of draft-ietf-mmusic-sdp-bundle-negotiation-47 - Ben's editorial comments

Christer Holmberg <christer.holmberg@ericsson.com> Sun, 21 January 2018 19:35 UTC

Return-Path: <christer.holmberg@ericsson.com>
X-Original-To: mmusic@ietfa.amsl.com
Delivered-To: mmusic@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id EAB2F12D7E5; Sun, 21 Jan 2018 11:35:31 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -4.221
X-Spam-Level:
X-Spam-Status: No, score=-4.221 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ie46ZcaZBPJh; Sun, 21 Jan 2018 11:35:29 -0800 (PST)
Received: from sesbmg23.ericsson.net (sesbmg23.ericsson.net [193.180.251.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 16EF6127023; Sun, 21 Jan 2018 11:35:28 -0800 (PST)
X-AuditID: c1b4fb25-473ff7000000341b-ef-5a64eb7fbdce
Received: from ESESSHC001.ericsson.se (Unknown_Domain [153.88.183.21]) by sesbmg23.ericsson.net (Symantec Mail Security) with SMTP id 77.C5.13339.F7BE46A5; Sun, 21 Jan 2018 20:35:27 +0100 (CET)
Received: from ESESSMB109.ericsson.se ([169.254.9.195]) by ESESSHC001.ericsson.se ([153.88.183.21]) with mapi id 14.03.0352.000; Sun, 21 Jan 2018 20:35:26 +0100
From: Christer Holmberg <christer.holmberg@ericsson.com>
To: Ben Campbell <ben@nostrum.com>, "draft-ietf-mmusic-sdp-bundle-negotiation.all@ietf.org" <draft-ietf-mmusic-sdp-bundle-negotiation.all@ietf.org>, MMUSIC <mmusic@ietf.org>
Thread-Topic: AD Evaluation of draft-ietf-mmusic-sdp-bundle-negotiation-47 - Ben's editorial comments
Thread-Index: AdOR9dB2bnAJi/LDSASEU4DilGGvkg==
Date: Sun, 21 Jan 2018 19:35:25 +0000
Message-ID: <7594FB04B1934943A5C02806D1A2204B6C13312E@ESESSMB109.ericsson.se>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [153.88.183.150]
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrDLMWRmVeSWpSXmKPExsUyM2K7qG7965Qog8dfhSzmd55mtzjx/hur xdTlj1kcmD2WLPnJ5DFr5xOWAKYoLpuU1JzMstQifbsEroxLd+8wFSxKqdhxeB57A+ONxC5G Tg4JAROJufvWs3UxcnEICRxmlLg4fSsjhLOEUWLLjlbWLkYODjYBC4nuf9ogcRGBzYwSL6+d ZQPpFhZIkzh+fi8jiC0ikC4x8fxKdghbT2L9rF9gcRYBVYnzs1vBbF4BX4mvq1vAehkFxCS+ n1rDBGIzC4hL3HoynwniIgGJJXvOM0PYohIvH/9jhbCVJFZsv8QIcg+zgKbE+l36EK2KElO6 H7JDjBeUODnzCcsERqFZSKbOQuiYhaRjFpKOBYwsqxhFi1OLk3LTjYz1Uosyk4uL8/P08lJL NjECw/zglt+qOxgvv3E8xCjAwajEw3vpWkqUEGtiWXFl7iFGCQ5mJRHeaXlAId6UxMqq1KL8 +KLSnNTiQ4zSHCxK4rwnPXmjhATSE0tSs1NTC1KLYLJMHJxSDYxtvWddNoeHKevv7w/X426w 3fm80ewYW19VzFUmG51ntb8efZyV+2SriMCKrP9SdxdOq7NyWWb8//oLW5P7GQqr7t5XTZIu Pf16pad+74a3blm+tUbi27qd4hb9l5uw8u1vQcn8NJF+zmLboOLIkAr1dQti1vwIntD9ISky KbbT3NNd6or4FSWW4oxEQy3mouJEAL4S3mxvAgAA
Archived-At: <https://mailarchive.ietf.org/arch/msg/mmusic/FXiEm6_cFhEs4YmljNWr1y4cLTI>
Subject: Re: [MMUSIC] AD Evaluation of draft-ietf-mmusic-sdp-bundle-negotiation-47 - Ben's editorial comments
X-BeenThere: mmusic@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: Multiparty Multimedia Session Control Working Group <mmusic.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/mmusic>, <mailto:mmusic-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/mmusic/>
List-Post: <mailto:mmusic@ietf.org>
List-Help: <mailto:mmusic-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/mmusic>, <mailto:mmusic-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sun, 21 Jan 2018 19:35:32 -0000

Hi,

In this reply I address the editorial comments. Please see inline.

*** Editorial Comments and nits ***

---

> - Terminology issues: I think some of the terminology here will confuse readers who are new to the concepts. 
> In particular, IIUC, “bundle address” means address + port. I think many readers will fall back to assuming that 
> address means IP address. (Especially when we talk about a “unique address” assigned to an m= section, when 
> much of the time that may just mean a unique port.)

Section 2 does define "bundle address" as an address:port combination.

But, I see your point.

Would "bundle address-port", "bundle addressport", "bundle addrprt", or something similar, be better? I don't want to say "bundle address:port combination" in every instance...

---

> I also find defining the term “Bundle Tag” to mean the first tag in the list to be confusing. It may be easier to just refer to the “first tag in the group list”.

Again, section 2 does define the meaning.

---

> — Examples: Any chance of some of the examples showing IPv6? I suspect it’s a trivial change for at least some examples, since they use domain names.

I can do that.

---

> -abstract: The abstract seems longer and more detailed than strictly necessary. The first two sentences along with a brief mention 
> of the update to 3264 and the SDES item/RTP header extension would be sufficient.

That's fine with me.

I do know that people requested certain things to be in the Abstract, but I assume they will speak up if they think something should be kept. 

---

> -Section 1,
> — first paragraph: This sort of jumps into things without much context. (e.g. that we are talking about media 
> sessions signaled using SDP offer/answer.”

I could say:

"When the SDP offer/answer mechanism [RFC3264] is used to negotiate the establishment of multimedia 
communication sessions, if separate transports (5-tuples) are negotiated for each individual media stream,
each transport consumes additional resources (especially when Interactive Connectivity Establishment
(ICE) [I-D.ietf-ice-rfc5245bis] is used).  For this reason, it is attractive to use a single transport for multiple 
media streams."

> s/consume/consumes

Will be fixed as suggested.

> — paragraphs 5-9: Most of the instances of the word “also” are unneeded.

I can remove them the unneeded ones.

> — paragraphs 7 and 9: These paragraphs both talk about updates to 3264; it would be better to keep them together.

I think they can be combined into a single paragraph.

> — 2nd to last paragraph, “The procedures in this specification apply independently to a given BUNDLE group.” : I don’t 
> understand what that sentence is trying to convey.

I says that, if your SDP contains multiple BUNDLE groups, the procedures are applied independently to each of them.

---

> - Section 2: The terminology list would be more readable as a bullet list or hanging indent list.

I will fix that.

---

> -3: There are at least one lower case “must” and a number of lower case “shoulds”. Please consider 
> using the boilerplate from RFC 8174, or verify that the lower case versions are not intended to be keywords.

I found a couple of "must" instances. Both shall be upper case.

There are 6-7 "should" instances, and it seems all of them can be modified to non-keywords.

---

> -5:
> — First paragraph: inconsistent tense. Please consider s/ “ … section will use …” / “… section uses…”

I will fix as suggested.

> — 2nd paragraph: The citation to RFC5888 looks like a reference for the “BUNDLE” semantic value, rather than
> for the grouping framework. Please consider something to the effect of the following:
> OLD:
>      The BUNDLE extension is indicated using an SDP ’group’ attribute with a "BUNDLE" semantics value [RFC5888].
> NEW:
>     The BUNDLE extension is indicated using an SDP ’group’ attribute with a semantics value [RFC5888] of "BUNDLE”.

I will fix as suggested.

---

> - Section 8.1:
> — Consider moving this section to later in the offer/answer details. I think it would be easier for a reader to 
> understand this section after they have some more general context.

The reason it is in the beginning is because it is part of the generic procedures.

Would you like to place it at the end of Section 8?

> — Title: Spell out “Multiplex” in the title.

I will fix as suggested.

> — First Paragraph: The wording could be taken to mean that all attributes with a mux category of 
> IDENTICAL or TRANSPORT must be included in the first place. I think the point is that, if they are 
> present, they must be included in each m= section. I suggest something like the following:
> OLD:
>    … IDENTICAL and TRANSPORT mux category SDP attributes MUST explicitly be included in each 
> bundled "m=“ section ...
> NEW:
>    … Any IDENTICAL and TRANSPORT mux category SDP attributes included in the BUNDLE group 
> MUST explicitly be included in each bundled "m=“ section …

I will fix as suggested.

---

> -8.2.1, first paragraph:
> — The phrase “represented by the … BUNDLE-tag” occurs repeatedly throughout the document. The use of “represented” there 
> seems awkward. I think “indicated” would be a better word choice.

This is one of the words that have been changed multiple times throughout the work of the document...

I can see whether your suggestion can be done with a search/replace, but if it requires more than that I would not like to do it.

> — should “offerer BUNDLE address” be “offerer suggested BUNDLE address”?

Yes. I will fix as suggested.

---

> -8.5.2: Should “add a bundled m= section” be “add a _new_ bundled m= section”?

Yes. I will fix as suggested.

---

> -9:
> — first paragraph: “ … different protocols on top of the transport-layer protocol …”
> Upper layer protocols?

Yes. Will fix as suggested.

> — Last paragraph: This paragraph seems mostly (or entirely) redundant to section 9.1.

The text gives an overview of what is done in this document, and what needs to be done elsewhere. I'd like to keep it.

---

-10.2:

> — " RTCP packets for which no appropriate "m=" section can be identified
>    MUST be processed as usual by the RTP layer, updating the metadata
>   associated with the corresponding RTP streams, but are not passed to
>   any "m=" section.”
>
> I don’t think things gets passed to sections of SDP. What does this really talk about?

This is another thing that it took forever to agree upon. The idea was to use different "layers" when processing received RTP/RTCP packets.

However, I do agree that the text is a little confusing.

Perhaps something like:

   "RTCP packets that cannot be associated with an appropriate "m=" section 
   MUST still be processed as usual by the RTP layer, updating the metadata
   associated with the corresponding RTP streams."

...and in the prior paragraph:

   "For each RTCP packet received (including each RTCP packet that is
   part of a compound RTCP packet), the packet is processed as usual by
   the RTP layer, then associated with the appropriate "m=" sections, and
   processed for the RTP streams represented by those "m=" sections."

> — “ Rules for additional processing of the various types of RTCP packets
>   are explained below.”
>
> Should that say “Additional rules for processing…”?

Yes. I will fix as suggested.

---

> -11, 2nd bullet: " previously selected [Section 8.3.1 ] or new suggested [ Section 8.5.1]”
> s/new/newly

Will fix as suggested.

> — last paragraph: It may be worth clarifying that this doesn't change any requirement specified by the 
> application or signaling protocol. For example, WebRTC still requires ICE even though bundle is used.

Question for clarification: Do you want to say that some applications might mandate ICE even if BUNDLE is not used, or that some applications might mandate support of ICE together with BUNDLE?

---

> 11.1:
> — 2nd paragraph: “ previously selected or new suggested “ s/new/newly

Will fix as suggested.

> — 3rd paragraph: Construct of the form of “MUST only” can be ambiguous. Please consider restating 
> in the form of a MUST NOT construction. (For example, “MUST only do foo when….” can be interpreted 
> as “MUST NOT do foo unless…” or “MUST NOT do anything but foo when…”   [Note: This construct repeats 
> several times in the next few sections. ]

What about?
 
   "When an answerer assigns a BUNDLE address to an "m=" section within a
   BUNDLE group (the "m=" section represented by the answerer BUNDLE-
   tag), the answerer onlys include SDP 'candidate' attributes (and
   other applicable ICE-related media-level SDP attributes) in that "m="
   section, following the procedures in Section 8.1. The answerer MUST NOT
   include ICE-related media-level SDP attributes in any other "m=" sections."

---

> -19, 3rd paragraph from end: It’s seems a bit odd to thank a co-author (Cullen) for text contributions :-)

Ok, I will remove him from section 19 :)

Regards,

Christer