Re: [rtcweb] HTA's review of JSEP-17

[Harald Alvestrand <[e-mail]>]

Finally found the peace of mind to get these into the github buganizer.

On 11/13/2016 11:43 PM, Harald Alvestrand wrote:
> Long plane rides have advantages.
>
> These are my notes; I will file github issues in a few hours on the
> points where I think we need discussion.
>
> Overall, this draft looks almost ready to go. It seems comprehensive
> (sometimes mind-numbingly so), and I think we will learn little more by
> going over it in theory; now it's time to take the software we've
> written, match it up against the spec, and see if we can make them match.
>
> That said, there are issues. Some of which I have noted below.
>
> - Agree with Magnus on the need for a terminology section. It would be
> nice to choose just one out of "media section" vs "m= section", but if
> you really want both, please define the two terms as being equal.
Added to #387
>
> - Section 1.1: Leftover mention of modifying the offer before doing
> set-local-description. The text should instead say "inspect, and if you
> don't like it, change settings and iterate".
Filed as #412
>
> - Section 3.6: "all dimensions assume square pixels". Given the
> affordance for non-square pixels elsewhere, I think this should say "all
> dimensions are measured in pixels, whether square or not".
Filed as #413
>
> - Section 3.6.1 describes mandatory constraints applied to an incoming
> track resulting in a new offer-answer exchange. Since this is "action at
> a distance" (modify the track and the PeerConnection emits a
> negotiation-needed), I think this needs to be synced with the webrtc-1.0
> API spec, and possibly even media-capture.
>
> Also, what's the state of the track between the changing of constraints
> at the receiver and the renegotiation? It seems to me that the track
> should be scaled at the receiver to fit the agreed-upon constraints.
Filed as #414.
>
> - Section 3.6.1 "no resolutions .. SHOULD be represented by x=0 and y=0
> values". Is there any reason not t o make this a MUST?
Filed as #415
>
> - Section 3.6.1 gives an example with a minimum resolution of 16x16 -
> where did that come from? If we want to admit to the existence of
> macroblocks, we may want to say that the implementation can specify step
> sizes (typically 16) in its imageattr values.
Filed as #416
>
> - Section 3.7 Simulcast - I'm not sure it's true that the information
> about number of layers is not surfaced through any API - the number of
> elements in the encoding list, and their active/passive state, which is
> accessible through getSettings(), should surely give a hint about this.
Filed as #417, with suggestion to remove text.
>
> - Section 3.8.2 - "the media flow from these sessions can be managed by
> specifying SDP direction attributes in the descriptions". This seems out
> of place. Surely flipping track.muted is a much simpler way of managing
> them if the problem is what to play to the user?

Filed as #418
>
> - Section 4.1.1 - the description of "relay" - is it worth noting that
> the degre of obfuscation is under the control of the application based
> on its choice of turn servers? And that browser-configured turn servers
> also matter here?
Filed as #419
>
> - Section 4.1.2 addTrack: Worth noting that using addTransceiver in
> have-remote-offer will cause negotiaiton-needed to be fired when the
> track returns to stable?
Filed as #420
>
> - Section 4.1.5 createOffer: The statement about this being async is
> either a requirement or out of scope. I'd suggest out of scope (it is
> purely an API property); the alternative is saying "it is an async
> operation".
Filed as #421
>
> - Section 4.1.7.1 - the term "inactive" final answer is introduced
> without a defintion. Need to either define "inactive" or use another term.
>
> - Section 4.1.7.2 Rollback - "and possibly the answerer as well" is
> wishy-washy. Suggest "and the answerer if it has done
> set-remote-description".
>
> - Section 4.1.7.2 This is the first mention of "pending local / remote
> description". Suggest link to definition.
>
> - Section 4.1.7.2 "RTPTransceivers that were created by applying a
> remote offer that was subsequently rolled back MUST be removed". The
> term "removed" is undefined. Deleted, discarded, destroyed or detached
> from the PeerConnection? Suggestion: "stopped and removed from the
> PeerConnection". (if the JS app still has a reference to them, it may
> still inspect the object).
Filed as #422 (merged)
>
> - Section 4.1.8 setLocalDescription - "the PeerConnection must be able
> to simultaneously support... until a final answer is received". This is
> unclear for several reasons - suggest "while the connection is in the
> have-local-offer or have-remote-pranswer state". (There's a race
> condition here between media and signalling, of course.)
>
> - Section 4.1.10 currentLocalDescription - "a copy of the current
> negotiated local description" is ambiguous (see issue raised in
> webrtc-1.0 - https://github.com/w3c/webrtc-pc/issues/921). Suggest
> making language here less normative - "shows the current negotiated
> local description" would probably be weak enough.
>
> - Section 4.1.15 setConfiguration - "and may result in a change to media
> state". Not sure this belongs here - it can lead to a cascade of events
> that may end up changing media state, but it is not the call that
> directly causes the change. Suggest deleting.
>
> - Section 4.1.16 addIceCandidate - suggest deleting the
> "end-of-candidates" text here, and saying separately that there is a
> means of indicating end of candidates. There seems to be consensus that
> mixing the two was an API design mistake, discussions are about how to
> get out of it.
Filed as #423
>
> - Section 4.2.4 setCodecPreferences - "However, the codec preferences
> cannot change the order of the media formats after an answer containing
> the transceiver has been applied". I don't understand why we can't
> change the order in order to have the new order reflected on the next
> offer/answer exchange - there would be no immediate effect, but the
> current language locks in the order permanently, without a clear
> justification for why it has to be that way.
Filed as #424
>
> - Sction 5.1.1 R-16: TODOs should be deleted before Last Call :-)  Also
> sugggest renumbering the trailing sentence as R-16.
>
> - Section 5.1.2: Suggest using a different letter on the numbering. U-1
> and U-2?
Filed as #425
>
> - Section 5.2.1 "an m= section being recycled" - first use of term
> "recycled". Link?
>
> - Section 5.2.1 CreateOffer "given that no candidates have yet been
> gathered" - when people remember the pre-gathered candidate pool, this
> can be confusing. Suggest adding a note at the top of this section
> saying that CreateOffer doesn't cause candidate gathering and does not
> take candidates from the pre-gathered pool, and thus, no candidates are
> available at the initial call to CreateOffer. All subsequent references
> to "no candidates have yet been gathered" in this section should be
> replaced with "no candidates are available".
Broken out as #428 (above and below are #426)
>
> - Section 5.2.1 bullet 2 (first list) references use of the default
> candidate (which doesn't exist here). Suggest saying "UDP/TLS/RTP/SAVPF"
> and moving the default candidate language to subsequent offers.
>
> - Section 5.2.1 CreateOffer - bullet on imageattr - limits on images
> that can be decoded - suggest adding text that this is also included on
> recvonly lines because direction of lines can change.
>
> - Section 5.2.1 "The following attributes ... MUST appear only in "m="
> sections which either have a unique address or which are associated with
> the bundle-tag". This is different from BUNDLE 8.1 - they should either
> say the same thing or have a clear explanation of the difference.
>
> - Section 5.2.1 "a=fingerprint" - needs rewriting for the
> multiple-fingerprint-algorithm case (4572bis). Issue already filed?
>
> - Section 5.2.2 Subsequent offers - is there any reason to allow NOT
> incrementing the session-version?
>
> - Section 5.2.2 The example of rollback doesn't make sense - it works if
> "an offer is created with version N+1" is replaced with "an offer is
> created with version N+1 and installed with SetLocalDescription"
>
> - Section 5.2.2 recycling of zero-port m-lines - should it specify that
> the new m= section MUST have a different mid than the replaced m= section?
>
> - Section 5.2.2 "the following adjustments are made based on the content
> of the corresponding m= section in the current remote description" -
> needs to be contained by "if there is a current remote description".
Filed as #426. A couple of things in the list may be substantive enough
to be worth their own bugs.
>
> - Section 5.3.1 I'm unsure about the language on lipsync groups. I can't
> figure out without more careful analysis if it works for the case of
> 1A+1V replying to 1A+1V - in almost any other case, I think the result
> is that answer doesn't have LS groups.
Filed as #427
>
> - Section 5.3.1 needs the same kind of intro as the previous one about
> no candidates being available even if gathered.
Added to #428
>
> - Section 5.3.2 Subsequent answers - like SetCodecPreference, this also
> cements initial priority. Why?
Added to #424
>
> - Section 5.7.1 Session-level parsing, b= lines: This is the first time
> they're mentioned. Is there an affordance for adding them when
> generating offers/answers? I didn't catch any language like "add any
> other field you feel like", but I may have overlooked it.
Rethinking this subject, adding fields applies only to SDP sent to the
remote end, where rewriting is obviously allowed. Not filing bug.
>
> - Section 5.8 Applying a local description - on errrors, "processing
> MUST stop and an error MUST be returned" - should it also say that the
> state of the PC needs to be returned to the pre-apply-LD state?
Filed as #429
>
> - Section 5.8 - "begin gathering candidates for it" - or take them from
> the pool.
>
> - Section 5.8 - style: a bullet starting with "if" and a next bullet
> starting with "or" is awkward.
>
> - Section 5.9 - "AS must be ignored" - should also apply to TIAS?
>
> - Section 5.9 "If more accurate control of bandwidth is needed, TIAS
> should be used" - this is the wrong place for that statement, we're not
> choosing here. If this is wanted, just write "TIAS gives more accurate
> control of bandwidth than AS".
>
> - Section 5.10 "If the media section has been rejected ... discard any
> associated ICE components". Only if it's not a bundled section.
>
> - Section 5.10 in general - there are a lot of bullets that apply only
> to M-sections that designate a transport. Should those bullets be broken
> out in a separate list?
>
> - Section 5.10 - "If no outgoing Source RTP stream exists, choose a
> random one" - "one" should be "SSRC" for clarity.
Filed as #430
>
> - Section 6: "this is only a strawman" language needs deleting. We're in
> WG Last Call.
>
> - Section 6: For tables where many-to-one mappings are expected (such as
> SSRC to receiver when simulcast, RTX or FEC is active), it might be
> worth noting this. For tables where many-to-one is an error (such as MID
> to RTPReceiver), it might be worth noting that too.
>
> - Section 6: "If the packet's payload type is in the payload type table,
> update.." - this will have interesting consequences ("last one wins") if
> the sender sends two streams with the same PT and different SSRCs.
> Suggest discarding packets where a SSRC->Receiver mapping already exists
> for that PT. This gives issues when changing SSRC for a stream; tradeoff
> needs evaluating.
Filed as #431
>
> That's all - no comments on examples :-)
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> -
>
>
>


-- 
Surveillance is pervasive. Go Dark.