Re: WebRTC spec errata (regarding states)

On Mon, Mar 11, 2013 at 5:10 AM, Adam Bergkvist <adam.bergkvist@ericsson.com
> wrote:

> Hi
>
> I've collected the comments from github in this mail as well.
>
> Github version can be viewed online here (note that links doesn't work):
>
> http://htmlpreview.github.com/**?https://github.com/fluffy/**
> webrtc-w3c/blob/master/webrtc.**html<http://htmlpreview.github.com/?https://github.com/fluffy/webrtc-w3c/blob/master/webrtc.html>
>
> The github spec is not updated with these changes yet.
>
>  From spec:
>>> The gathering process is done.
>>>
>>
>  juberti@github:
>> I suggest that we should transition the gathering
>> state to completed here, before dispatching this callback (which
>> indicates that that state has changed).
>>
>
> Agreed.
>
>  From spec:
>>> This can happen, e.g., if there are insufficient resources to apply
>>> the SDP. The user agent MUST then roll back as necessary if the new
>>> description was partially applied when the failure occurred.
>>>
>>
>> juberti@github:
>> Rollback may not always be possible in this case. I think this should
>> be treated as a fatal error.
>>
>
> What's the result of a fatal error? Close the PeerConnection?
>
> What should errorType be in this case, INCOMPATIBLE_SESSION_**DESCRIPTION?
>

I think this would be the dreaded INTERNAL_ERROR; the PC is essentially in
an undefined state at this point, and the application should discard it.
The session description might have been fine, but we just exploded when
trying to apply it.

We could also have a INCOMPATIBLE_SESSION_DESCRIPTION error that indicates
that there was some bad value in the SDP, but rollback was possible and the
state has returned to the previous state.

>
>  From spec:
>>> When the method is invoked, the user agent must follow the
>>> processing model of setLocalDescription() .
>>>
>>
>  juberti@github:
>> I think it would be better to spell this out explicitly here. For
>> example, the ICE-related processing is different - this doesn't
>> trigger any gathering changes.
>>
>> Also, if the remotedesc ICE credentials are different, this means
>> that createAnswer should generate new ICE credentials in the next
>> call (so that a local ICE restart can be performed). LMK if you'd
>> like me to write some text for this part.
>>
>
> I wrote this as a shared processing model since most of the text would be
> the same for both methods and it's a whole lot easier to maintain one text
> at this stage.
>
> The points you mention above are taken into consideration since the
> current model sometimes talks about setting a specific type of description.
> For example:
>
> "4. If the local description was set with content that caused an ICE
> restart, then set connection's ice gathering state to gathering."
>
> We could move entire bullets, that only deals with either local or remote
> descriptions, out of the shared model, but I would, at least for now, keep
> a shared model for the error and success cases.


OK.

>
>
> On 2013-03-10 18:54, Justin Uberti wrote:
>
>> Thanks for making these changes. A few comments inline:
>>
>>
>> On Wed, Mar 6, 2013 at 10:04 AM, Adam Bergkvist
>> <adam.bergkvist@ericsson.com <mailto:adam.bergkvist@**ericsson.com<adam.bergkvist@ericsson.com>>>
>> wrote:
>>
>>               4. 4.3.2.2 <http://4.3.2.2>: setLocalDescription
>>
>>
>>                  The description for this function should indicate that it
>>                  triggers a signaling state change. It should also
>>         indicate that
>>                  an INVALID_STATE exception is triggered if the wrong
>>                  SessionDescription.type is supplied for the current
>> state.
>>               5. 4.3.2.2 <http://4.3.2.2>: setRemoteDescription
>>
>>
>>                  The description for this function should indicate that it
>>                  triggers a signaling state change. It should also
>>         indicate that
>>                  an INVALID_STATE exception is triggered if the wrong
>>                  SessionDescription.type is supplied for the current
>> state.
>>
>>
>>     Updated with the modifications we talked about in a prior reply to
>>     this mail. I've tried to address some stuff that was unspecified
>>     around these functions. We have a bit of redundancy with both a
>>     callback and an event when a description is applied successfully.
>>     I've picked an order for these which is event first, and callback
>>     second. Please review.
>>
>>
>> That ordering sounds good to me. I would expect that when I got the
>> success callback, I should be able to call other functions, so all
>> states should already be up-to-date.
>>
>> I do think we need to have separately-specified processing for local and
>> remote descriptions. For example, the local description may influence
>> the gathering process, whereas the remote description does not.
>>
>
> Addressed above.
>
>                2. 4.3.2.1 <http://4.3.2.1>: onicecandidate
>>
>>
>>                  The description for this callback should indicate that
>>         it is
>>                  invoked with a null argument when gathering completes.
>>         Prior to
>>                  dispatching this null callback, the iceGatheringState
>>         will be
>>                  set to "completed".
>>
>>
>>     I've seen that we have descriptions on event handler attributes in
>>     the form "this callback should be invoked when...". There are other
>>     ways to listen for events so the attribute handler shouldn't be
>>     called for any special reason other then the event being fired.
>>     Anyways, to address this particular problem, I've added a paragraph
>>     about how the ICE Agent notifies the script about the candidate
>>     gathering process (new candidate or gathering done). Please review.
>>     Perhaps the ICE Agent should have it's own sub-section in the "4.3.1
>>     Operation" section where we collect this kind of stuff.
>>
>>
>> I didn't quite follow what you were saying here.
>>
>
> My point was that I didn't want to extend the description of the callback
> because it's the wrong place do document this kind of behavior. That was
> basically the motivation for the new text about how the ICE Agent talks to
> the script.


OK - I'll wait for the updated version of the spec to see where you chose
to document it. I don't really mind as long as it's in there somewhere.

>
>
>  I do think we need some
>> way to specify when the gathering state transition to "completed" occurs.
>>
>
> Addressed above.
>
> /Adam
>