Bug 28705

More from 24936

> * Does it ever make sense to remove a local candidate? If it does, we'll need a
> LocalCandidatesRemoved signal
> 
> * What is LocalCredentials and what is its rationale?
> 
> * LocalCredentials will need to be a named <tp:struct> (or a pair of string
> properties), otherwise telepathy-qt4 will be unable to bind it
> 
> * How many times can LocalCredentialsSet happen? 0-1? 0-infinity?
> 
> * Does SetCredentials() change LocalCredentials? How many times can it be
> called?
> 
> * What is a candidate anyway? What is a component anyway? (Perhaps this
> interface is only meant for use by people who speak fluent RTP, but I'm only
> dimly aware of what a candidate is...)
> 
> * Am I right in thinking that Stream.I.Media deals with local candidates (ways
> in which the local user tells remote users that we can perhaps be contacted)
> while remote candidates (ways in which remote users tell us they can perhaps be
> contacted) are all dealt with by Endpoint? The (missing) introductory docstring
> should say this sort of thing.
> 
> * If I infer correctly that LocalCredentials, LocalCandidates come from the
> streaming implementation and nowhere else, do they actually need to be readable
> at all, or can they be "write-only" (i.e. not exist as properties at all, only
> as setter methods)?
> 
> * In Candidate_Info: we conventionally use "g-object-case" for un-namespaced
> bags of strings, and reserve CamelCase for D-Bus properties. Or is there some
> external thing we're being consistent with?
> 
> * The descriptions in Candidate_Info aren't sufficient for me to understand
> what they're for.

(In reply to comment #1)
> More from 24936
> 
> > * Does it ever make sense to remove a local candidate? If it does, we'll need a
> > LocalCandidatesRemoved signal

No, never.

> > * What is LocalCredentials and what is its rationale?

It is the crds that you put in the SDP you send.

> > * How many times can LocalCredentialsSet happen? 0-1? 0-infinity?

If you call it again, it mans you're doing a ICE restart.. Which mean syou forget about remote canifdates and expect new ones.

> > * Am I right in thinking that Stream.I.Media deals with local candidates (ways
> > in which the local user tells remote users that we can perhaps be contacted)
> > while remote candidates (ways in which remote users tell us they can perhaps be
> > contacted) are all dealt with by Endpoint? The (missing) introductory docstring
> > should say this sort of thing.

Yes are right, it should be clearer though. You can have multiple endpoints because with SIP you can get multiple replies from the same invite request.

> > * If I infer correctly that LocalCredentials, LocalCandidates come from the
> > streaming implementation and nowhere else, do they actually need to be readable
> > at all, or can they be "write-only" (i.e. not exist as properties at all, only
> > as setter methods)?

Being able to read them could be useful for debugging I guess ?


> > * The descriptions in Candidate_Info aren't sufficient for me to understand
> > what they're for.

About Candidate_Info:
- Type is a uint, but we have no idea what the values mean
- BaseIP should be a string, not uint
- BasePort is missing
- What is the scale of the priority? (it should be a number between 0 and 65535)
- Add multicast type and a TTL field for it
- Protocol is also a uint, but we have no idea what it means
- RawUDPFallback should go away, the lowest priority candidate is normally the fallback (its the relay).

Maybe PleaseRestartICE() should have a matching property for recoverability?


Started writing stuff at (until I ran out of battery):
http://git.collabora.co.uk/?p=user/tester/telepathy-spec.git;a=shortlog;h=refs/heads/stream-etc

Also includes fixes for bug #31160

(sorry if this comment happens twice: I thought I submitted it last night, but it wasn't there when I refreshed)

> About Candidate_Info:
> - Type is a uint, but we have no idea what the values mean
Made a new enum Call_Stream_Candidate_Type, with docs. Quite tempted to rename this to "topology", because Type is a bit generic. Thoughts?

I had to do a bit of s/\t/        /. Remember to set up your editor correctly next time you do spec work.
> - BaseIP should be a string, not uint
Changed this, but I don't know what this is used for, so I was unable to add a useful docstring.
> - BasePort is missing
Added this, but again without a very useful docstring.

> - What is the scale of the priority? (it should be a number between 0 and
> 65535)
Done.

> - Add multicast type and a TTL field for it
There is already a multicast type for the Stream. I have added one for the candidate too, and put it as a separate patch.

> - Protocol is also a uint, but we have no idea what it means
Referenced to base proto from StreamedMedia.

> - RawUDPFallback should go away, the lowest priority candidate is normally the
> fallback (its the relay).
Done. If I remember correctly, the rationale for this is that you can know whether you're using ICE or rawudp from the type of the endpoint when it pops up, and the CM can pick the lowest priority candidate as its rawudp candidate. I also expanded the documentation of Priority (which addresses #34038) while I was at it.

> Maybe PleaseRestartICE() should have a matching property for recoverability?
Haven't addressed this. Could this be done by exposing the CM's view of the state machine for the stream, or would you say that it is orthogonal? Also, you say that LocalCredentials is mostly useful for debugging. Could we just set it to "", "" but not clear LocalCandidates when we emit PleaseRestartIce?

I also noticed that the keys for CandidateInfo are Capitalised, and the keys for RelayInfo are lowercase. Should I leave this as-is or should we standardise on one method of capitalisation?

pushed to alsuren/stream-etc (http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/stream-etc)

Definition of base (taken From RFC 5245 section 2.1:

      Note: "Base" refers to the address an agent sends from for a
      particular candidate.  Thus, as a degenerate case host candidates
      also have a base, but it's the same as the host candidate.


I'm really not sure what to do about the PleaseRestartICE property. I like the explicitness of having a "RestartICE" property, but I also like the simplicity of clearing the LocalCredentials property.

I'm not sure topology is really what you mean by candidate type.. and RFC 5245 calls them "type", so I would stay with their naming.

Also, I noticed that RFC 5245 calls it "preference" not "priority", not sure if it matters.

Otherwise it looks good

Okay: I have pushed two commits to stream-etc, to address your comments (#35012) so that I can think about ICE restarts later.

If you glance at http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/stream-etc and give it a quick ++, then I can merge it into alsuren/call and tick a few bugs off my list.

Could be associated with a Relay too or a peer reflexive.

(In reply to comment #7)
> Could be associated with a Relay too or a peer reflexive.
Re-worded and pushed a new head to http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=commitdiff;h=0dd2a48dc31947a6927d452cd3f5bff70395721f if you could review that.

++

Abusing Assigned to mean "merged in alsuren/call and available at
http://people.freedesktop.org/~alsuren/telepathy-spec-call/spec/".

Merged to master.