Comments on draft

I went over the draft on the plane. Comments below.

OVERALL
Is there any way to have the descriptions of the PC methods
in logical order rather than in alphabet order? This is
really confusing. It would also be really nice if they
all had their own section numbers rather than a gigantic
4.3.2.3.

MAJOR
S 4.3.1.
I still believe that it's a mistake to be executing some
operations synchronously and some asynchronously. It makes
it very hard to determine what happens in event of a race
and in case of reentrancy. I'd like to discuss this at the
meeting.

stream sets need to be ordered because JSEP depends on that
for assignment to m-lines. This is also an issue in the
getLocalSteams, getRemoteStreams description.


I'm not clear on what happens when you change iceTransports
from "relayed" to "all".



4.3.2.3.
Yes, addIceCandidate needs to be queued. Otherwise it might
be applied to m-lines which don't yet exist because setRemoteDescription
has never been called and the candidates will be lost.

  If connection's RTCPeerConnection signalingState is stable, then
  fire a negotiationneeded event at connection.


This is already lame prior to CreateOffer() being called but it's
going to be even lamer when we add addTrack() and people call
it twice and then the onnegotiationneeded handler repeatedly
tries to CreateOffer and send to to the other side. We should
suppress this prior to a call being created.


The description of close doesn't tell us anything about how
to handle queued operations. Neither does the description of
queue processing in S 4.3.1. I'm assuming that if we have
outstanding operations and then call .close(), this causes
all the error callbacks to fire? But where does it say this?
OK... I just found this *in the description* of createAnswer().
Seriously? Also, if you look at the description of setLocal(),
it contemplates doing everything and then ignoring the
results, but this has side effects.

addStream and removeStream only fire negotiationneeded if
the signalingState is stable. But this means that you are
not going to get told that negotiation is needed even in
cases where you would need to initiate. For instance,
consider what happens if you call createOffer()/setLocalDescription()
and then removeStream() while waiting for the other side's
description and setRemoteDescription(). Why aren't you
told that negotiation is needed. This is particularly bad
because setRemoteDescription is run asynchronously whereas
removeStream is not. Consider the following code:

  pc.addStream(s);
  pc.createOffer(function(x) {
    pc.setLocalDescription(x);
  }

  ...

  pc.setRemoteDescription(remote);
  pc.removeStream();

Note that this does not call onnegotiationneeded even though
logically the pc is in its way to stable state because
setRemoteDescription is processed asynchronously as defined
in 4.3.1.


setLocalDescription:

  If a local description contains a different set of ICE credentials,
  then the ICE Agent must trigger an ICE restart. When ICE restarts,
  the gathering state will be changed back to "gathering", if it was
  not already gathering. If the IceConnectionState was "completed", it
  will be changed back to "connected".

You actually need to check the ICE credentials to see if they
match those in a createOffer/Answer. Otherwise, this is an error.

updateIce:
The way updateIce() is written it seems like you must supply all
your STUN servers even if iceTransports is "relayed" so that
the candidates are available when you change it. Also, should
updateIce.iceTransports = all cause onicecandidate? What if
you are already apparently done gathering. This seems like
it needs some work.

There appear to be a contradiction between this section, which
says:

  If a new list of servers replaces the ICE Agent's existing ICE
  servers list, no action will taken until the RTCPeerConnection's ice
  gathering state transitions to gathering. If a script wants this to
  happen immediately, it should do an ICE restart.

and S 4.4.2 which says:

  The ICE engine has completed gathering. Events such as adding a new
  interface or a new TURN server will cause the state to go back to
  gathering.


4.6.1.
  Errors are indicated in two ways: exceptions and objects passed to
  error callbacks. Exceptions are thrown to indicate invalid state and
  other programming errors. For example when a method is called when
  the RTCPeerConnection is in an invalid state, or a state in which
  that particular method is not allowed to be executed. In all other
  cases, an error object must be provided to the error callback.

This is violated in, for instance S 4.3.2.3:

If the candidate parameter is malformed, throw a SyntaxError exception
and abort these steps.


5.1.2.
createDataChannel should be queued.


MINOR
4.2.4.2.
  When the value of this dictionary member is false, and the
  localDescription attribute has valid ICE credentials, the generated
  description will have the same ICE credentials as the current value
  from the localDescription attribute.

How could it not have valid ice credentials and be non-nukk?

  In some cases, an RTCPeerConnection may wish to receive audio but
  not send any audio. The RTCPeerConnection needs to know if it should
  signal to the remote side whether it wishes to receive audio. This
  option allows an application to indicate its preferences for the
  number of audio streams to receive when creating an offer.

This needs text about how bool is processed. Or did we decide not
to allow that?


  voiceActivityDetection of type boolean, defaulting to true

We should move this to a doohickey


4.3.1.
  1. Validate the RTCConfiguration argument by running the steps
  defined by the updateIce() method.

It needs to be clear that if those steps abort, so do these.


  execute the first object queued asynchronously and repeat this step
  on completion.

rewrite as "asynchronously execute"


This whole description of notifications is baffling and repetitive.
Please rewrite so that you have one section describing how to do
notifications rather than repeating all this stuff about
having connection be the RTCPeerConnection and checking for closed.

In addition, this section doesn't seem to describe how to
notify for other ICE state changes.


  User agents negotiate the codec resolution, bitrate, and other media
  parameters. It is recommended that user agents initially negotiate
  for the maximum resolution of a video stream. For streams that are
  then rendered (using a video element), it is recommended that user
  agents renegotiate for a resolution that matches the rendered
  display size.

This doesn't actually seem like that useful advice, given that
both sides are quite likely to know what sort of element they are
going to stuff the object in (and indeed it's quite likely that
the answerer stuffs it in a <video> in onaddstream). Do any
existing UAs renegotiate in this case.


  The creation of new incoming MediaStreams may be triggered either by
  SDP negotiation or by the receipt of media on a given flow.

Does unified plan presently allow this? It appears to contradict
the description of "onaddstream" in 4.3.2.2.
"This will be fired only as a result of setRemoteDescription."

  When a user agent has negotiated media for a component that belongs
  to a media stream that is already represented by an existing
  MediaStream object, the user agent must associate the component with
  that MediaStream object.

What does this mean?


  To prevent network sniffing from allowing a fourth party to
  establish a connection to a peer using the information sent
  out-of-band to the other peer and thus spoofing the client, the
  configuration information should always be transmitted using an
  encrypted connection.

I have no idea what this means, but git blame says it was in
the initial commit, so it's probably OBE. Given the current
COMSEC mechanisms in place, there are two categories of relevant
information here:

  - The ICE credentials, needed to establish tansport-level
    connectivity
  - The DTLfingerprints, needed to establish a DTLassociation.

The former need confidentiality. The latter only integrity.
It is of course good advice that a secure connection be
used, but I suggest merely describing the implications of
not using a secure connection in the security considerations.


4.3.2.2.
Several of these event handlers look like they might usefully
have a description of when they are called. E.g., onicecandidate
and onnegotatiationneeded.

Why isn't onremovestream called when I setLocal removing a
stream?

onsignalingstatechange says " It is called any time the readyState
changes". This should be "signalingState"

Currently addStream silently aborts when you call it with a
stream that is already in the local streams set. This is bad.
An exception should be thrown [TODOEX]


  Add stream to connection's local streams set.

This needs to say "append"


Both CreateOffer and CreateAnswer say:

  Calling this method is needed to get the ICE user name fragment and
  password.

Clearly that cannot be true.


setLocalDescription:
The description here doesn't seem to conform well with the queueing
logic. I.e., when do we throw InvalidStateError wrt being closed?
Presumably pre-queue, but then the next bullet is about IceRestart
and that must clearly be async.

  4. If the local description was set, connection's ice gathering state
  is new, and the local description contains media, then set
  connection's ice gathering state to gathering.

  5. If the local description was set with content that caused an ICE
  restart, then set connection's ice gathering state to gathering.

What happens if you do a setLocal() that causes the need for more
candidates after gathering has completed
e.g., addstream(), createOffer()/setLocal(), addStream(),
createOffer()/setLocal()


updateIce:
updateIce isn't queued? Even though you can do ICE restart in
setLocal/setRemote


5.2.3.
Something is wrong here since send() is described in this section.

So wait, if someone has called .close(), they can still call .send()
wwhich stuffs data in the buffer but doesn't generate an error
per step 4? Huh?


6.1.1.
What happens if I try to create a DTMFsender on a video track?

6.2.1.
Why would canInsertDTMF be false?

I'm confused about what happens if I call
insertDTMF twice with different gaps. It just overwrites
the old gap even for queued tones? Gah.


7.2.1.
This text doesn't work:

   When the getStats() method is invoked, the user agent must queue a
   task to run the following steps:

   1. If the RTCPeerConnection object's RTCPeerConnection
   signalingState is closed, throw an InvalidStateError exception.

   2. Return, but continue the following steps in the background.

If you've queued a task, that task shouldn't be throwing exceptons.

   4. If selectorArg is an invalid selector, the user agent must queue
   a task to invoke the failure callback (the method's third
   argument).

   ...

   6. When the relevant stats have been gathered, queue a task to
   invoke the success callback (the method's second argument) with a
   new RTCStatsReport object, representing the gathered stats, as its
   argument.

We're already in a bg task here....


8.4.1.
"idvalidationperror" -> "idpvalidationerror"