Re: CHANGES: Implementation feedback

On 09/28/11 12:06, Adam Bergkvist wrote:
> Hi
>
> We have been implementing the WebRTC spec for a while and would like to
> bring forward some proposals for changes. We have based our comments on
> the 14 September 2011 editor's draft and therefore some of the proposed
> changes may have to be adopted to ongoing changes in the spec.
Checking the aim here: Do you think that all these changes are changes 
to the description, and that the behaviour is likely to be unchanged? (I 
think so).
> 4. Peer-to-peer connections
>
> The "ICE started flag" is currently used to determine if the initial
> offer has been processed (produced or handled). I suggest we rename it
> to "initial offer processed flag" to make this clearer.
This name is due to be changed due to the discussions entitled "API 
changes".
Always a problem with overlapping change cycles....
> * PeerConnection (constructor section)
>
> I propose to add a new step 16 to the algorithm to make it simpler:
>
> - 16. If the PeerConnection's "initial offer processed flag" is false,
>        then abort these steps.
>
> This will not change the behavior since the operations below this point
> do nothing if the flag is false. As a result, we can remove the "ICE
> started flag" condition in current step 16, entire step 17 and make 18
> unconditional.
Sounds like textual improvement to me.
> * "When a user agent starts receiving media..."
>
> In step 2, "then associate the component with that media stream and
> abort these steps", could perhaps be changed to "then associate the
> component with the corresponding MediaStreamTrack object in that media
> stream and abort these steps" since it then made clearer that all
> tracks are created when the MediaStream object is created in step 3.
>
> 4.1.2 Methods
>
> * addStream and removeStream
>
> addStream and removeStream needs to know if the initial offer hasn't
> been processed yet, in which case they can back off and have their
> pending streams included in the initial offer, or if the operations
> should result in a separate signaling message. The new step 6 below
> proposes how this could be done.
>
>   - 6. If the "initial offer processed flag" is true, then the next
>        time the user agent provides a stable state, it must have the
>        PeerConnection's PeerConnection ICE Agent add a media stream for
>        stream. Any other pending stream additions and removals must be
>        processed at the same time. [ICE]
I think it may be better to separate these things, at least 
conceptually; something like
6. Set the "Streams changed" flag to TRUE.

and elsewhere (not sure where):

When the user agent provides a stable state, the following steps are 
executed:

<move steps 16-18 of the constructor definition here>

If the "Streams Changed" flag is TRUE and the SDP state is IDLE:

Compare the current state of localStreams to the state described by the 
last SDP description sent in a successful offer/answer exchange. If they 
differ, emit a new SDP OFFER.

> I would also like to add the following text as a note or to the last
> step in the algorithms that describe both addStream and removeStream:
>
> "A pending stream addition and a pending stream removal that concerns
> the same stream should cancel each other and not cause a signaling
> message."
>
> The reason is that the behavior will be different if the pending streams
> cancel each other out or are processed as an add as well as a remove.
> The first case will not cause a signaling message to be dispatched if
> these two streams are the only ones to be processed, while the other
> case will.
It seems pointless to generate a signaling message for this case; I agree.
> * processSignalingMessage
>
> The string comparison in step 4 should explicitly state if it's
> case-sensitive or not to be consistent with the rest of the spec.
> My suggestion is to make it case-sensitive. That's also how we
> implemented it. Section 4, where the SDP-prefix is added, should also be
> updated to match.
Agreed (with the note that this is also area under change).
> /Adam
>

Received on Wednesday, 28 September 2011 12:59:17 UTC