Re: [rtcweb] Working Group Last Call: JSEP

[Ted Hardie <[e-mail]>]

Hi Magnus,

I've added github issues for most of these in the range 387 to 408.  The
two final issues here (numbers 31 and 32), both related to section 6,
weren't really clear enough for me to provide a useful summary.  Can you
try to rephrase those?  It would be great if you can do that by adding them
to the issues list, but I'll try to add them if you update them here.

thanks,

Ted

On Sun, Nov 13, 2016 at 4:13 PM, Magnus Westerlund <
[e-mail]> wrote:

> Hi,
>
> As I am late, and we are having the discussion tomorrow I am taking the
> easiest route for me to get my feedback out to the WG and authors, thus an
> email rather than Github issues.
>
> So here are my comments on the -17 version of JSEP.
>
> 1. Terminology: m= section. I would propose that the document creates a
> terminology section and are explicit that m= section and SDP Media
> Description are the same thing.
>
> 2. There are a couple of terms that would really need a definition and
> also possibly some reflection of how they relate to the RTCWeb terms, like
> WebRTC Endpoint. So the terms I really like defined are:
>
> JSEP endpoint
> JSEP implementation
>
> 3. Section 3.6:
>
> JSEP
>    implementations will not transmit non-square pixels, but SHOULD
>    receive and render such video with the correct aspect ratio.
>
> I have two issues with this. First of all, shouldn't JSEP implementations
> be WebRTC endpoints? The second is these statement that says that a
> JSEP/WebRTC foo will not do a thing. In most cases that is not necessary,
> as there would be no issues if a particular implementation supports this
> functionality. So, I think this should be written as:
>
> WebRTC endpoints does not need to support transmission of non-square
> pixels, but SHOULD
>    receive and render such video with the correct aspect ratio.
>
> 4. Section 3.6.2:
>
>    When communicating with a non-JSEP endpoint, multiple relevant
>    "a=imageattr recv" attributes may be received.  If this occurs,
>    attributes other than the one with the highest "q=" value MUST be
>    ignored.
>
> There is to my knowledge nothing preventing multiple parameters sets to
> have the same preference value (q). Thus, the formulation of this sentence
> needs to be addressed.
>
> 5. Section 3.6.2:
>
>    If an "a=imageattr recv" attribute references a different video codec
>    than what has been selected for the MediaStreamTrack, it MUST be
>    ignored.
>
> So, to my understanding there can be multiple video codecs and multiple
> RTP PTs that have been negotiated as possible to use. Is selected just the
> single currently used one, one or all the possible ones here?
>
> 6. Section 3.6.2:
>
>    If the attribute includes a "sar=" (sample aspect ratio) value set to
>    something other than "1.0", indicating the receiver wants to receive
>    non-square pixels, this cannot be satisfied and the sender MUST NOT
>    transmit the track.
>
> Another instances where the language should allow for flexibility if an
> implementation goes beyond the defined mandatory support. Changing "this
> cannot be" to "if this cannot be" may resolve it. Also, isn't the relevant
> "MUST NOT" is to not accept this in the negotiation?
>
> 7.  Section 3.7:
>
>    JSEP supports simulcast of a MediaStreamTrack, where multiple
>    encodings of the source media can be transmitted within the context
>    of a single m= section.
>
> I think this should be explcit that it is "simulcast transmission of a
> MediaStreamTrack".
>
> 8. Section 5.1.1:
>
>    This list of mandatory-to-implement specifications is derived from
>    the requirements outlined in [I-D.ietf-rtcweb-rtp-usage].
>
>    R-5   [RFC4568] MUST NOT be implemented to signal SDES SRTP keying
>          information.
>
> So, the list of mandatory-to-implement includes also some to not
> implement. That is fine, but probably should be moved to its own list, or
> rename the list so that it can contain both types.
>
> Secondly, this specific requirement I think comes from the security docs,
> rather than rtp-usage.
>
> 9. Section 5.1.1:
>    R-2   [RFC5764] MUST be supported for signaling the UDP/TLS/RTP/SAVPF
>          [RFC5764], TCP/DTLS/RTP/SAVPF
>          [I-D.nandakumar-mmusic-proto-iana-registration], "UDP/DTLS/
>          SCTP" [I-D.ietf-mmusic-sctp-sdp], and "TCP/DTLS/SCTP"
>          [I-D.ietf-mmusic-sctp-sdp] RTP profiles.
>
> I would note that [I-D.nandakumar-mmusic-proto-iana-registration] is
> actually published as RFC 7850.
>
> 10. Section 5.1.1:
>    R-12  [RFC5761] MUST be implemented to signal multiplexing of RTP and
>          RTCP.
>
> This should include the updated as ref also.
>
>
> 11. Section 5.1.1:
>
>   R-15  [RFC3556] with bandwidth modifiers MAY be supported for
>          specifying RTCP bandwidth as a fraction of the media bandwidth,
>          RTCP fraction allocated to the senders and setting maximum
>          media bit-rate boundaries.
>
> So, why wasn't support of RR and RS bandwidth modifiers made a MUST?
>
> Secondly, the RR and RS attributes do not specify the bandwidth as
> fraction of the media bandwidth. The specify RTP session level limits for
> RTCP bandwidth.
>
> 12. Section 5.1.1:
>
>    R-16  TODO: any others?
>
> Please remove this.
>
> 13. Section 5.1.3:
>
>    For media m= sections, JSEP endpoints MUST support both the "UDP/TLS/
>    RTP/SAVPF" and "TCP/DTLS/RTP/SAVPF" profiles and MUST indicate one of
>    these two profiles for each media m= line they produce in an offer.
>
> Do I understand this correct, we are requiring support for the
> "TCP/DTLS/RTP/SAVPF" proto so that in cases an endpoint supports the
> optional to support ICE TCP, they can indicate it, and any WebRTC endpoint
> will accept it, even if that is just one option? I do know that different
> profiles are a negotiation issue. But, wouldn't it be more reasonable in
> this case to use UDP/TLS/RTP/SAVPF in all cases where one offer any
> candidates that also use UDP? And only use TCP/DTLS/RTP/SAVPF in cases only
> TCP candidates are signalled, thus not forcing TCP/DTLS/RTP/SAVPF onto
> clients that doesn't support it?
>
> 14. Section 5.1.3:
> Otherwise, assume that AVPF is being used in an AVP
>       compatible mode and use AVP timing, i.e., "trr-int=4".
>
> I find this sentence confusing. I think this should be clarified to say:
>
> Otherwise, assume that AVPF is being used in an AVP
>       compatible mode, i.e. use AVPF timing configured with a "trr-int=4".
>
> 15. Section 5.1.3:
>
> Spurious space in:
>
> For data m= sections, JSEP endpoints MUST support receiving the
>       "UDP/ DTLS/SCTP", "TCP/DTLS/SCTP", or "DTLS/SCTP" (for backwards
>       compatibility) profiles.
>
> 16. Section 5.2.1:
>    o  Session Information ("i="), URI ("u="), Email Address ("e="),
>       Phone Number ("p="), Bandwidth ("b="), Repeat Times ("r="), and
>       Time Zones ("z=") lines are not useful in this context and SHOULD
>       NOT be included.
>
> Considering that b=CT is not useless, I am a bit surprised to see b= on
> this list. Yes, b=CT: is only useful is one like to provide a upper total
> bandwidth limitation. So, maybe some discussion of the possible choice?
>
> 17. Section 5.2.1:
>
> This is done in the order
>    that their associated RtpTransceivers were added to the
>    PeerConnection and excludes RtpTransceivers that are stopped and not
>    associated with an m= section (either due to an m= section being
>    recycled or an RtpTransceiver having been stopped before being
>    associated with an m= section) .
>
> I am bit surprised to see "recycled" on a list of things that may occur in
> generating an initial offer? Can that really occur?
>
> 18. Section 5.2.1:
>
>    o  An "a=mid" line, as specified in [RFC5888], Section 4.  When
>       generating mid values, it is RECOMMENDED that the values be 3
>       bytes or less, to allow them to efficiently fit into the RTP
>       header extension defined in
>       [I-D.ietf-mmusic-sdp-bundle-negotiation], Section 11.
>
>
> Just to note that to my understanding we are having discussion if this
> should be referencing a more specific generation algorithm.
>
> Such a change is likely to affect text on RID also:
>
> 19. Section 5.2.1:
> The list of
>       RTCP feedback mechanisms that SHOULD/MUST be supported is
>       specified in [I-D.ietf-rtcweb-rtp-usage], Section 5.1.
>
> I would request that we don't use the quite confusing "SHOULD/MUST" and
> instead write something like:
>
> mechanisms that are recommended or mandated to be supported
>
> 20. Section 5.3.1:
>
>    o  If this m= section is for media with configurable frame sizes,
>       e.g. audio, an "a=maxptime" line, indicating the smallest of the
>       maximum supported frame sizes out of all codecs included above, as
>       specified in [RFC4566], Section 6.
>
> In most cases the maxptime attribute doesn't affect the frame size of the
> encoder, instead it affects the maximum amount of media that are packetized
> into a RTP payload, i.e. how many frames that are included.
>
> 21. Section 5.3.1:
>
>    Each m= section which is not bundled into another m= section, MUST
>    contain the following attributes (which are of category IDENTICAL or
>    TRANSPORT):
>
> I react to "bundle into another m= section". I though they where "bundled
> with"?
>
> 22. Section 5.7.2:
>    o  If present, a single "a=rtcp" attribute MUST be parsed as
>       specified in [RFC3605], Section 2.1, but its value is ignored, as
>       this information is superfluous when using ICE.
>
> This is another case, where there is an exception to what is written if
> one has supports the non RTP/RTCP mux usage. Thus, this should be ammended
> to included such an exception.
>
> 23. Section 5.7.2:
>
> What about additional a= attributes? Any attribute supported by the
> implementation MAY be parsed?
>
> 24. Section 5.8:
>
>          +  Find the RtpTransceiver that corresponds to the m= section
>             with the same MID in the created offer.
>
>          +  Set the value of the RtpTransceiver's mid attribute to the
>             MID of the m= section.
>
> I find this text strange. If you already know the MID, why is it being set
> in the second sub-bullet?
>
> 25. Section 5.8:
>
>      *  If RTCP mux is indicated, prepare to demux RTP and RTCP from
>          the RTP ICE component, as specified in [RFC5761],
>          Section 5.1.1.  If RTCP mux is not indicated, but was indicated
>          in a previous description, this MUST result in an error.
>
> I note that there can be difference here between an offer and an answer if
> the RTCP mux indication may have changed or not between these two
> description.
>
> 26. Section 5.9:
>   o  For any specified "CT" bandwidth value, set this as the limit for
>       the maximum total bitrate for all m= sections, as specified in
>       Section 5.8 of [RFC4566].  The implementation can decide how to
>       allocate the available bandwidth between m= sections to
>       simultaneously meet any limits on individual m= sections, as well
>       as this overall session limit.
>
> I think this text can be improved to make it clearer that CT doesn't imply
> a fixed over time divide of the bandwidth between RTP streams. As long as
> individual media sources streams stay within other requirements such as
> b=AS/TIAS they can be changed dynamically.
>
> 27. Section 5.10:
>
>       *  If the media section has RTCP mux enabled, discard any RTCP
>          component, and begin or continue muxing RTCP over the RTP
>          component, as specified in [RFC5761], Section 5.1.3.
>          Otherwise, prepare to transmit RTCP over the RTCP component; if
>          no RTCP component exists, because RTCP mux was previously
>          enabled, this MUST result in an error.
>
> I think this should say "any RTCP ICE component"..
>
> 28. Section 5.10:
> When sending RTCP feedback, use the
>          SSRC of an outgoing Source RTP Stream as the RTCP sender SSRC;
>          if no outgoing Source RTP Stream exists, choose a random one.
>
> This is okay, but could be slightly more open in regards to sending
> feedback packets efficiently. Section 5.4.1 in
> https://datatracker.ietf.org/doc/draft-ietf-avtcore-rtp-mult
> i-stream/?include_text=1 is providing more detailed rules for things to
> consider.
>
> 29. Section 6:
>
> If
>       any payload type could map to more than one RtpReceiver, map to
>       the RtpReceiver whose m= section appears earliest in the local
>       description.
>
> I think this is a bad advice. I think it should say that if this is the
> case, PT should not be used to try to determine which m= section it belongs
> to. I think wrongly attributing a packet is worse than dropping it in cases
> where the information is incomplete. This rule can result in that one
> associate an SSRC with the wrong m= section.
>
> 30. Section 6:
>       If the packet has a MID and that MID is in the table mapping MID
>       to RtpReceiver, update the incoming SSRC mapping table to include
>       an entry that maps the packet's SSRC to the RtpReceiver for that
>       MID.
>
> This is all fine, but requires an addition. If there was already a MID
> associated with the SSRC, i.e. a new MID has been assigned to this SSRC,
> then there is deleting in the old MIDs association table that needs to
> happen.
>
> 31. Section 6:
>
> I think this section should be updated to work with the BUNDLE text as
> that matures. I understand that there are a few additional cases one likes
> to cover for non WebRTC endpoints, but still, lets not introduce
> unnecessary failure cases over that.
>
> 32. Section 6:
>
> The RTCP packet routing. This doesn't match my view of how RTCP processing
> happens. I think the "deliver a copy of the packet to
>       the RtpReceiver associated with that SSRC."
>
> Is the part that I have most issues with. What I find more relevant and
> correct and likely matching more implementations are"
>
> deliver the information to
>       the RtpReceiver associated with that SSRC.
>
>
> Cheers
>
>
> Magnus Westerlund
>
> ----------------------------------------------------------------------
> Services, Media and Network features, Ericsson Research EAB/TXM
> ----------------------------------------------------------------------
> Ericsson AB                 | Phone  [phone]
> Färögatan 6                 | Mobile [phone]
> SE-164 80 Stockholm, Sweden | mailto: [e-mail]
> ----------------------------------------------------------------------
>
>