Re: [MMUSIC] Review of draft-ietf-mmusic-sdp-bundle-negotiation-32 (Part I)

Hi Ekr

It's been more than 3 weeks and I don't believe I have seen a reply. We 
need to get this document moving forward, so if you are still interested 
in having your comments addressed, can you please follow-up.

Thanks

-- Flemming (with chair hat on)

On 8/29/16 3:54 PM, Christer Holmberg wrote:
>
> Hi Ekr,
>
> Thanks for the comments! Please see inline.
>
> OVERALL
>
> >The overall framing of this document as being about attaching multiple
>
> >m= sections to a single "shared address" is extremely problematic,
>
> >for two reasons:
>
> >
>
> >- With ICE there isn't actually any notion of a shared address except
>
> >  in some vague sense. I suppose you could argue that because with
>
> >  ICE you are required to specify a port on the m= line then you
>
> >  still have a concept of an address, but that's not true in
>
> >  trickle ICE, where the port is 9, so all m= lines share the same
>
> >  address/port. So, actually when you use trickle a whole bunch of
>
> >  the statements in this document are wrong.
>
> >
>
> >- A lot more is being overloaded here than addresses. In fact, a whole
>
> >  pile of semantics are being overloaded, so the way to think about
>
> >  this is actually as gluing a whole bunch of m= lines together at
>
> >  the transport level.
>
> >
>
> > I think the right concept here is not "shared address" but "bundle group"
>
> > and the document should be written in that way, namely that there are
>
> > a  bunch of m= lines in a bundle group and the one with the bundle tag
>
> > is the one that sets all the shared parameters.
>
> It is true that the actual port value in the m- line, and the IP 
> address in the c- line address, represent the address that is actually 
> being used.
>
> But, even with ICE there is a shared address, isn’t there? Yes, that 
> shared address may change, but at any given time there will be one 
> address that is shared among the media streams within the BUNDLE 
> group. Or?
>
> > The document is badly in need of some overall framing that explains
>
> > what we are trying to accomplish and the rationale for why any
>
> > particular m= section property needs to be the same for all members of
>
> > a bundle group and why other ones do not. I'm not saying it's
>
> > necessarily bad to be explicit in (for instance Sections 11 and 12)
>
> > but it would be better if it were clear where these rules come
>
> > from. One way to deal with this would be to simply defer this to the
>
> > mux-attributes draft.
>
> Ok, I’ll look into that. People are also welcome to provide text 
> suggestions.
>
> > I'll just note again that I find the overloading of "associated" 
> makes this
>
> > document very hard to read. At minimum it would be best to talk about
>
> > an m= line as "belonging" to a BUNDLE group. If others agree I would
>
> > be happy to provide a PR that cleans this up.
>
> The intention is to say “m= line within a BUNDLE group”. If there are 
> instances of “associated” I am happy to fix that.
>
> MAJOR TECHNICAL
>
> > Maybe I missed this, but I don't see any requirement that m= lines
>
> > appear in the a=group:BUNDLE line in the order they appear in the
>
> > SDP. That would make life a lot easier.
>
> There is no such requirement.
>
> AFAIR, it was discussed at some point, but it was agreed to not 
> mandate m- lines to be listed in the order of preference (as far as 
> selecting the BUNDLE address is concerned).
>
> Also, if you mid-session adds a new m- line, and wants that to 
> represent the new BUNDLE address you have to place the new m- line 
> after the existing m- lines, but you have to place it first in the 
> a=group:BUNDLE attribute list.
>
> DETAILED COMMENTS (MIXED EDITORIAL/TECHNICAL)
>
> >S 1.
>
> >This really needs some explanation of why you would want this feature.
>
> You represent the community that wants the feature, so feel free to 
> suggest some text explaining why you want it :)
>
> >S 2; Initial offer.
>
> >"The first offer, within" . This comma is superfluous.
>
> Will be fixed.
>
> >S 5.
>
> >The first three grafs of this section seem to duplicate the intro
>
> >and the abstract.
>
> >
>
> >Also, this claim about sharing a 5-tuple is not quite right with ICE
>
> >because ICE can switch 5-tuples during aggressive nomination.
>
> Yes. But, at any given time there will only be one 5-tuple, won’t it?
>
> >S 6.
>
> >   The usage of the 'bundle-only' attribute is only defined for a
>
> >   bundled "m=" line with a zero port value, within an offer.  Other
>
> >   usage is unspecified.
>
> >
>
> > It would probably be better to actually forbid this, because it seems
>
> > like an interop problem if you somehow were to introduce it later.
>
> Ok, I’ll look into that.
>
> >S 8.
>
> >Nit:
>
> >List the forward references in section order.
>
> Will be fixed.
>
> >S 8.1.
>
> >This seems like a key concept: there are a number of different categories
>
> >of attributes and some are shared with all members of a bundle group
>
> >and some are not. To what extent is this draft just duplicative of those
>
> >listed in the mux-attributes draft?
>
> Not sure I understand the question.
>
> >S 8.2 and following
>
> >Really would be useful to have some examples here to make it less
>
> >abstract.
>
> You don’t think the examples in section 17 are enough? You want 
> examples in the normative sections describing how to create offers, 
> answers etc?
>
> >S 8.3.3.
>
> >It seems like the implication here is that an answerer cannot debundle
>
> >something that was bundled in a previous o/a transaction. Can you
>
> >make that point clearly?
>
> Ok, I’ll look into that.
>
> >S 8.5.
>
> >How does this interact with ICE Restart?
>
> From a pure ICE Restart perspective, I don’t see any conflict. ICE 
> Restart is triggered by changing the user name and password, and the 
> text does not forbid that.
>
> If you also want to change the BUNDLE address, you follow the rules in 
> section 8.5.1.
>
> >S 8.5.2
>
> >When you say "extend" do you mean "add to the end"?
>
> Yes. I can fix that.
>
> >S 10.1
>
> >In this first bullet is "if" "iff"?
>
> “iff”?
>
> >S 10.3.1.1.
>
> >   When an offerer generates an initial offer, the offerer MUST
>
> >   associate either an SDP 'rtcp-mux' attribute [RFC5761] or an SDP
>
> >   'rtcp-mux-only' attribute [I-D.ietf-mmusic-mux-exclusive] with each
>
> >   bundled RTP-based "m=" line in the offer.  The offerer MUST associate
>
> >   an SDP 'rtcp-mux-only' attribute with each bundle-only "m=" line.  If
>
> >   the offerer associates a 'rtcp-mux-only' attribute with an "m=" line,
>
> >   the offerer may also associate a 'rtcp-mux' attribute with the same
>
> >   "m=" line, as described in [I-D.ietf-mmusic-mux-exclusive].
>
> >
>
> > This is a particularly egregious case of associate disease that could
>
> > be easily solved by using "add" when referring to the attributes
>
> > you put in the m= section.
>
> People have previously said that you don’t “add” attributes to m- 
> lines: you add port values etc, but not attributes.
>
> This is an example of where I’ve had to re-write big parts of the 
> documents a number of times already, because every time someone else 
> reads the document he/she is not happy with the terminology. I just 
> don’t want to do it again.
>
> >S 11.1
>
> >   When an offerer associates a unique address with a bundled "m=" line
>
> >   (excluding any bundle-only "m=" line), the offerer MUST associate SDP
>
> >   'candidate' attributes (and other applicable ICE-related media-level
>
> >   SDP attributes), containing unique ICE properties (candidates etc),
>
> >   with the "m=" line, according to the procedures in
>
> >   [I-D.ietf-mmusic-ice-sip-sdp].
>
> >
>
> > Here is an example of where the "address" framing doesn't work, if you
>
> > are using trickle.
>
> See top of e-mail.
>
> Regards,
>
> Christer
>
>
>
> _______________________________________________
> mmusic mailing list
> mmusic@ietf.org
> https://www.ietf.org/mailman/listinfo/mmusic