Subject: [MMUSIC] draft-ietf-mmusic-sdp-mux-attributes-05 review

From: Ari Keränen <[email-masked]>
Date: Wed, 10 Dec 2014 16:57:01 +0200

Hi Suhas,

Here are my review comments on the SDP mux draft. Technically I think 
the doc is in pretty good shape, so these are mostly editorial issues.

Cheers,
Ari

Abstract

    In the RTCWEB WG, there is a need to use a
    single 5-tuple for sending and receiving media associated with
    multiple media descriptions ("m=" lines).

Bundling is not RTCWEB-only feature, so I would rather drop the RTCWEB 
reference at least from the abstract, and write it in more general way 
(e.g., "in order to save resources and decrease connection setup times, 
these is a need..." like in section 3)

    Such a requirement has
    raised concerns over the semantic implications of the SDP attributes
    associated with the RTP Media Streams multiplexed over a single
    underlying transport layer flow.

I think we are beyond "raised concerns" and have actually found out it 
was a real issue, so I would update the wording here.

    The scope of this specification is to provide a framework for
    analyzing the multiplexing characteristics of SDP attributes.

s/scope/purpose/

1.  Introduction

    Real-Time Communication in WEB-browsers (Rtcweb) framework requires
    Real-time Transport Protocol (RTP) as the media transport protocol
    and Session Description Protocol (SDP) [RFC4566] for describing and
    negotiating multi-media communication sessions.

This (and the following paragraph) should also be more general. Here 
perhaps you can use RTCWEB as an example of where such feature is 
required, but adding an RTCWEB reference (e.g. the use case doc) would 
be good here for context.

I think it would make sense also to reference the bundle draft already 
here and writing a sentence or two about the relationship with this draft.

also:
  s/requires/uses/
  s/multi-media/multimedia/

3.  Motivation

    This imposes further restrictions on applicability of
    these SDP attributes as they are defined today.

Not sure where "these" here refers to. I guess that word can either be 
dropped, or clarify which attributes you mean.

4.  SDP Attribute Analysis Framework

Would it make sense to have "before" and "after" in all the SDP examples 
(now there's only the "before" part)?

4.2.  Category: NOT RECOMMENDED

Can/should we recommend here what to do in this case? Not multiplex? Not 
add such attributes? Either way?

4.3.  Category: IDENTICAL

    Attributes that MUST be identical across all the media descriptions
    being multiplexed.

Maybe clarify that attributes and their *values* (if exist) must be 
identical.

         <allOneLine>
         a=zrtp-hash:1.10 fe30efd02423cb054e50efd0248742ac7a52c8f91bc2
         df881ae642c371ba46df
         </allOneLine>

The draft uses now 3 different ways of presenting too long SDP lines 
(see e.g., 4.5, 4.7 and 14.3.1.1). I guess they are from the defining 
RFCs, but I would rather see uniform style across this document. 
Personally I prefer the style of section 4.5 (2 char indent).

4.4.  Category: SUM

Should clarify where do you put the SUM'ed value.

4.5.  Category: TRANSPORT

    Attributes that can be set normally for multiple items in a
    multiplexed group but the software MUST pick just one of the
    attribute of the given type for use.  The one chosen is the attribute
    associated with the "m=" line that represents the information being
    used for the transport of the RTP.

How do you "pick" the one; by repeating the same value for each bundled 
"m="-line?

Also, do we need to consider non-RTP cases too?

4.8.  Category: SPECIAL

    Attributes where the text in the source draft must be consulted for
    further handling when multiplexed.

s/source draft/specification defining the attribute/

5.9.  RFC6773 - DCCP-UDP Encapsulation

    Since RFC6773 is about tunneling DCCP in UDP, with the UDP port being
    the port of the DCCP en-/de-capsulation service.

Can't parse the sentence. Something missing here? Or extra "since"?

    This MAY or MAY NOT work depending on the
    nature of DCCP encapsulations and ports choses thus rendering it to
    be very application dependent.

Lower-case (non-RFC2119) "may" and "may not" would be appropriate here. 
Also s/ports choses/port choices/?

5.12.  RFC5245 - Interactive Connectivity Establishment (ICE)

Since the ICE candidate behavior is already specified in the bundle 
draft at least reference there would be good? Also please make sure that 
these stay in sync with what bundle has until it's an RFC.

5.21.  RFC6230 - Media Control Channel Framework

            | cfw-id  | Not Applicable  | M     | NORMAL       |

Should that be "Not Impacted" as in other NORMAL cases?

5.39.  RFC6189 - ZRTP

    | zrtp-hash  | Complicates if all the        | M     | NOT          |
    |            | m=lines are not authenticated |       | RECOMMENDED  |
    |            | as given in the example below |       |              |

Where's the example? You mean the one in section 4.2?

5.46.  RFC2326 - Real Time Streaming Protocol

Could mention here briefly why RTSP is not supported (similar to the 
next section).

5.50.  3GPP TS 183.063

    | PSCid               | Not Impacted | Not-Applicable | NORMAL      |

The "Not-Applicable" level is not defined. Could add that to section 5 
or explain here and in 5.58.

7.1.  RFC4585 - RTP/AVPF

Are the "Attr Name" here really attribute names or should one use some 
other name in this case?

7.3.  RFC6285 - Unicast-Based RAMS

This one was missing the "intro description". Could add something brief 
for consistency with rest of the doc. Same for 7.6, 9.1, 9.2, 10.x (I'll 
leave it up to you where you consider it worthwhile adding).

And is the "Attr Name" field here just "Name" intentionally? Same in 
following two sections.

13.1.  RFC5109 - RTP Payload Format for Generic FEC

    Draft draft-lennox-payload-ulp-ssrc-mux proposes a simple fix to make

This is a bit tricky. This should be a proper reference, but referencing 
expired drafts is not a good practice either. I would lean towards 
taking this sentence away since (if?) the fix is not going forward at 
the IETF.

14.2.1.  Recommendation - Procedures for Potential Configuration Pairing

    that are consistent across all the bundled media descriptions.

Could add "bundled media" to the terminology section.

15.  IANA Considerations

It would be great if someone double-checked that the "Mux Category" 
entries here match to the text. Probably a simple script could do that too.

---

From: Suhas Nandakumar <[email-masked]>
Date: Thu, 11 Dec 2014 13:17:17 +0530

Hello Ari

  Many thanks for the review. Please see inline

Thanks
Suhas

On Wed, Dec 10, 2014 at 8:27 PM, Ari Keränen <[email-masked]>
wrote:

> Hi Suhas,
>
> Here are my review comments on the SDP mux draft. Technically I think the
> doc is in pretty good shape, so these are mostly editorial issues.
>
>
> Cheers,
> Ari
>
>
> Abstract
>
> I think we are beyond "raised concerns" and have actually found out it was
> a real issue, so I would update the wording here.
>
>
[Suhas] - I will try to make it more general in the next version.

> [Suhas] - Accepted

> perhaps you can use RTCWEB as an example of where such feature is required,
> but adding an RTCWEB reference (e.g. the use case doc) would be good here
> for context.
>
> [Suhas] - Accpted

> dropped, or clarify which attributes you mean.
[Suhas] - removed the word "these" for clarity.

> (now there's only the "before" part)?
[Suhas] - I am not sure giving before and after examples be less confusing.
Since we are not defining new way of doing things to contrast the
approaches.

> add such attributes? Either way?
[Suhas] - the definition for the category already encompasses this as
highlighted below.
"Attributes that are *recommended against multiplexing* since their usage
under multiplexing might lead to incorrect behavior."

> The draft uses now 3 different ways of presenting too long SDP lines (see
> e.g., 4.5, 4.7 and 14.3.1.1). I guess they are from the defining RFCs, but
> I would rather see uniform style across this document. Personally I prefer
> the style of section 4.5 (2 char indent).
>
>
[Suhas] - Will move all the examples to reflect the format in Section 4.5

> Should clarify where do you put the SUM'ed value.

[Suhas] .. Since the calculated SUM value is expected to be consumed in the
software processing SDP, the definition of this category already captures
this fact.

> [Suhas] - The expectation is to repeat as per the BUNDLE spec. Hence we
have just added the way such a value is chosen but not the way the chose
value is represented in the SDP as it is defined in the BUNDLE spec.

> s/source draft/specification defining the attribute/

[Suhas] - Accepted

> [Suhas] -Removed since since it clarifies the text better.

> Also s/ports choses/port choices/?

[Suhas] - Done

> Since the ICE candidate behavior is already specified in the bundle draft
> at least reference there would be good? Also please make sure that these
> stay in sync with what bundle has until it's an RFC.
>
> [Suhas] - BUNDLE spec provides O/A procedures for ICE but does not provide
the category assignments for the ICE attributes.

> Should that be "Not Impacted" as in other NORMAL cases?

[Suhas] - Accepted

> [Suhas] - The example SDP was removed from this section to not duplicate
but I had forgotten the delete the reference to the example in here. Hence,
will remove the example refernce

> Could mention here briefly why RTSP is not supported (similar to the next
> section).
>
> [Suhas] - Done

> The "Not-Applicable" level is not defined. Could add that to section 5 or
> explain here and in 5.58.
>
> [Suhas] - Will add appropriate SDP attribute levels to these sections

> [Suhas] - Will clean up the column name for consistency

> [Suhas] - Agreed, I will add the intro text for all these sections. It
helps readability as well.

> expired drafts is not a good practice either. I would lean towards taking
> this sentence away since (if?) the fix is not going forward at the IETF.
>
> [Suhas] - Removed this reference.

> [Suhas] removed bundled with multiplexed to keep it uniform

> [Suhas]  - I can work on this to be sure

> _______________________________________________
> mmusic mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/mmusic
>

---

From: Ari Keränen <[email-masked]>
Date: Fri, 12 Dec 2014 11:24:38 +0200

Hi Suhas,

The updates you propose below look good to me and thanks for the 
clarifications too.

Cheers,
Ari

On 11/12/14 09:47, Suhas Nandakumar wrote:
> On Wed, Dec 10, 2014 at 8:27 PM, Ari Keränen <[email-masked]
> <mailto:[email-masked]>> wrote:
>
>     Hi Suhas,
>
>     Here are my review comments on the SDP mux draft. Technically I
>     think the doc is in pretty good shape, so these are mostly editorial
>     issues.
>
>
>     Cheers,
>     Ari
>
>
>     Abstract
>
>     Bundling is not RTCWEB-only feature, so I would rather drop the
>     RTCWEB reference at least from the abstract, and write it in more
>     general way (e.g., "in order to save resources and decrease
>     connection setup times, these is a need..." like in section 3)
>
>
>         Such a requirement has
>     I think we are beyond "raised concerns" and have actually found out
>     it was a real issue, so I would update the wording here.
>
>
>     required, but adding an RTCWEB reference (e.g. the use case doc)
>     would be good here for context.
>
>     I think it would make sense also to reference the bundle draft
>     already here and writing a sentence or two about the relationship
>     with this draft.
>
>     also:
>       s/requires/uses/
>     Not sure where "these" here refers to. I guess that word can either
>     be dropped, or clarify which attributes you mean.
>
>
>     Would it make sense to have "before" and "after" in all the SDP
>     examples (now there's only the "before" part)?
>
>
> [Suhas] - I am not sure giving before and after examples be less
> confusing. Since we are not defining new way of doing things to contrast
> the approaches.
>
>
>     Can/should we recommend here what to do in this case? Not multiplex?
>     Not add such attributes? Either way?
>
>
> "Attributes that are *recommended against multiplexing* since their
> usage under multiplexing might lead to incorrect behavior."
>
>
>     4.3.  Category: IDENTICAL
>
>              a=zrtp-hash:1.10 fe30efd02423cb054e50efd0248742__ac7a52c8f91bc2
>              df881ae642c371ba46df
>              </allOneLine>
>
>     (see e.g., 4.5, 4.7 and 14.3.1.1). I guess they are from the
>     defining RFCs, but I would rather see uniform style across this
>     document. Personally I prefer the style of section 4.5 (2 char indent).
>
> [Suhas] .. Since the calculated SUM value is expected to be consumed in
> the software processing SDP, the definition of this category already
> captures this fact.
>
>
>
>     4.5.  Category: TRANSPORT
>
>     How do you "pick" the one; by repeating the same value for each
>     bundled "m="-line?
>
>     Lower-case (non-RFC2119) "may" and "may not" would be appropriate
>     here. Also s/ports choses/port choices/?
>
>
> [Suhas] - Done
>
>
>
>     draft at least reference there would be good? Also please make sure
>     that these stay in sync with what bundle has until it's an RFC.
>
> [Suhas] - BUNDLE spec provides O/A procedures for ICE but does not
> provide the category assignments for the ICE attributes.
>
>
> but I had forgotten the delete the reference to the example in here.
> Hence, will remove the example refernce
>
>
>     The "Not-Applicable" level is not defined. Could add that to section
>     5 or explain here and in 5.58.
>
>     Are the "Attr Name" here really attribute names or should one use
>     some other name in this case?
>
>     This one was missing the "intro description". Could add something
>     brief for consistency with rest of the doc. Same for 7.6, 9.1, 9.2,
>     10.x (I'll leave it up to you where you consider it worthwhile adding).
>
>         Draft draft-lennox-payload-ulp-ssrc-__mux proposes a simple fix
>     to make
>
>     This is a bit tricky. This should be a proper reference, but
>     referencing expired drafts is not a good practice either. I would
>     lean towards taking this sentence away since (if?) the fix is not
>     going forward at the IETF.
>
>     _________________________________________________
>     mmusic mailing list
>     [email-masked] <mailto:[email-masked]>
>     https://www.ietf.org/mailman/__listinfo/mmusic
>     <https://www.ietf.org/mailman/listinfo/mmusic>
>
>