[rtcweb] HTA's review of JSEP-17

[Harald Alvestrand <[e-mail]>]

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.

- 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".

- 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".

- 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.

- 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?

- 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.

- 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.

- 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?

- 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?

- 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?

- 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".

- 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).

- 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.

- 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.

- 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?

- 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".

- 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".

- 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.

- Section 5.3.1 needs the same kind of intro as the previous one about
no candidates being available even if gathered.

- Section 5.3.2 Subsequent answers - like SetCodecPreference, this also
cements initial priority. Why?

- 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.

- 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?

- 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.

- 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.

That's all - no comments on examples :-)














-



-- 
Surveillance is pervasive. Go Dark.