http://git.collabora.co.uk/?p=user/danni/telepathy-spec.git;a=shortlog;h=refs/heads/call-31274

I'm not sure why you want to use an a{sa{sv}}. We should be able to do what we do elsewhere in Telepathy and use an a{sv} qualified-values map, so your properties are fully namespaced and all immutable properties from all interfaces of the object can be included (see also, the NewChannels signal).

ahh fully qualified props should work too

Updated dannielles branch with a more generic approach for transferring information about the codec offers (now called descriptions). Which is more extensible for cases where we have non-codec information

Updated, html version at:
  http://people.freedesktop.org/~sjoerd/telepathy-spec-description_objects/spec/

git in url :)

General comment: rationales and hyperlinks would be useful.

+    <tp:mapping name="ContactDescriptionPropertiesMap">

C_D_P_M please, otherwise it'll come out as TP_HASH_TYPE_CONTACTDESCRIPTIONPROPERTIESMAP in codegen. Likewise for DescriptionProperties, DescriptionOffer.

We usually use Ugly_Case for <arg> and <tp:member> names, too.

In DescriptionOffer:

       <tp:member name="Codec_Offer" type="o">

you probably want to rename this.


-          The contact handle that this codec offer applies to.
+          The contact handle that this description applies.

"... applies to" (or, "the contact handle to which this description applies")

+        <p>A map from contact handles to descriptions give supported by that

s/give // I think?

+          The object path of the new description. This replaces any previous
+          description.

If '/' means no description, please say so here too.

+    <signal name="LocalDescriptionsChanged"

In practice, do several local descriptions really change atomically? If not, it'd be a lot simpler to have a LocalDescriptionChanged (which implies Added if necessary) and a ...Removed. Likewise for RemoteDescriptionsChanged.

In DescriptionOffer:

+            >RemoteContact</tp:dbus-ref> and
+            a of the Descriptions properties.

and a what? "and a mapping containing the _Description_'s properties", presumably.

The semantics of NewDescriptionOffer look as though it ought to be called DescriptionOfferChanged - it seems astonishing to have a NewFoo signal when a Foo is deleted.

> True if this offer contains no information from the remote side. Iotw, the
> Accept response solely depends on the capabilities and preferences of the
> local side. In most protocols this property will be True for the initial
> DescriptionOffer on an outgoing call. 

side: in other words, the

> This propery will not be part of the DescriptionProperties used to
> describe this object.

"property"

I think it might make more sense if the sense of EmptyDescription was reversed: "RemoteDescriptionReceived" or something?

The semantics of Description_Properties (with EmptyDescription implicit from the presence/absence of RemoteCodecs even if empty) seem rather odd. I assume that the rationale here is that it lets us distinguish between "this is an empty description" and "this is a non-empty description with an empty list of remote codecs", which we couldn't previously do?

> The contact handle that this codec offer applies to or 0 a description
> that will apply globally.

"0 for a description that"

"... that will apply to all remote contacts in the _Call_" perhaps?

Department of namespacing: some of the type names seem pretty vague. I think it's OK for the Content.Media interface to just have "Description" in its property/signal names, because in the context of a Call, the prevailing meaning for "description" is (perhaps) the SDP one.

However, types are a flat global namespace (in our codegen) and if I say "description" with only Telepathy as context, I'd expect it to be some sort of human-readable blurb about a chatroom or something. Perhaps the types could say Media_, so we get Media_Description_Properties:a{sv}, Contact_Media_Description[_Properties]_Map:a{ua{sv}} and Media_Description_Offer:(oua{sv}), or something?

I'd be very tempted to rename Description_Properties to Description_Description :-)

(In reply to comment #5)
> We usually use Ugly_Case for <arg> and <tp:member> names, too.

In fact, for <tp:member> the names are probably used in telepathy-qt4 codegen, so you should definitely use Ugly_Case. I don't think we use <arg> names anywhere that changing them would be an API/ABI break, but please use Ugly_Case there too, for consistency.

Mostly at notes to myself what i fixed in my branch thusfar :)

(In reply to comment #5)
> General comment: rationales and hyperlinks would be useful.
> 
> +    <tp:mapping name="ContactDescriptionPropertiesMap">
> 
> C_D_P_M please, otherwise it'll come out as
> TP_HASH_TYPE_CONTACTDESCRIPTIONPROPERTIESMAP in codegen. Likewise for
> DescriptionProperties, DescriptionOffer.
> 
> We usually use Ugly_Case for <arg> and <tp:member> names, too.

fixed
> 
> In DescriptionOffer:
> 
>        <tp:member name="Codec_Offer" type="o">
> 
> you probably want to rename this.

fixed

> -          The contact handle that this codec offer applies to.
> +          The contact handle that this description applies.
> 
> "... applies to" (or, "the contact handle to which this description applies")

fixed

> +        <p>A map from contact handles to descriptions give supported by that
> 
> s/give // I think?
>

fixed

> +          The object path of the new description. This replaces any previous
> +          description.
> 
> If '/' means no description, please say so here too.

done

> 
> +    <signal name="LocalDescriptionsChanged"
> 
> In practice, do several local descriptions really change atomically? If not,
> it'd be a lot simpler to have a LocalDescriptionChanged (which implies Added if
> necessary) and a ...Removed. Likewise for RemoteDescriptionsChanged.

FIXME

> In DescriptionOffer:
> 
> +            >RemoteContact</tp:dbus-ref> and
> +            a of the Descriptions properties.
> 
> and a what? "and a mapping containing the _Description_'s properties",
> presumably.

fixed

> The semantics of NewDescriptionOffer look as though it ought to be called
> DescriptionOfferChanged - it seems astonishing to have a NewFoo signal when a
> Foo is deleted.

fixed, FIXME should require to always go via DescriptionOfferDone though

> > True if this offer contains no information from the remote side. Iotw, the
> > Accept response solely depends on the capabilities and preferences of the
> > local side. In most protocols this property will be True for the initial
> > DescriptionOffer on an outgoing call. 
> 
> side: in other words, the

fixed

> > This propery will not be part of the DescriptionProperties used to
> > describe this object.
> 
> "property"
> 
> I think it might make more sense if the sense of EmptyDescription was reversed:
> "RemoteDescriptionReceived" or something?

fixed

> The semantics of Description_Properties (with EmptyDescription implicit from
> the presence/absence of RemoteCodecs even if empty) seem rather odd. I assume
> that the rationale here is that it lets us distinguish between "this is an
> empty description" and "this is a non-empty description with an empty list of
> remote codecs", which we couldn't previously do?

fixed

> > The contact handle that this codec offer applies to or 0 a description
> > that will apply globally.
> 
> "0 for a description that"
> 
> "... that will apply to all remote contacts in the _Call_" perhaps?

FIXME clarify what this mean

> Department of namespacing: some of the type names seem pretty vague. I think
> it's OK for the Content.Media interface to just have "Description" in its
> property/signal names, because in the context of a Call, the prevailing meaning
> for "description" is (perhaps) the SDP one.
>
> However, types are a flat global namespace (in our codegen) and if I say
> "description" with only Telepathy as context, I'd expect it to be some sort of
> human-readable blurb about a chatroom or something. Perhaps the types could say
> Media_, so we get Media_Description_Properties:a{sv},
> Contact_Media_Description[_Properties]_Map:a{ua{sv}} and
> Media_Description_Offer:(oua{sv}), or something?
> 
> I'd be very tempted to rename Description_Properties to Description_Description
> :-)

FIXME

> > +    <signal name="LocalDescriptionsChanged"
> > 
> > In practice, do several local descriptions really change atomically? If not,
> > it'd be a lot simpler to have a LocalDescriptionChanged (which implies Added if
> > necessary) and a ...Removed. Likewise for RemoteDescriptionsChanged.
> 
> FIXME

Removed is now one signal for both  (as it will alwyas apply to both at the same time)

> > The semantics of NewDescriptionOffer look as though it ought to be called
> > DescriptionOfferChanged - it seems astonishing to have a NewFoo signal when a
> > Foo is deleted.
> 
> fixed, FIXME should require to always go via DescriptionOfferDone though

required!

> > > The contact handle that this codec offer applies to or 0 a description
> > > that will apply globally.
> > 
> > "0 for a description that"
> > 
> > "... that will apply to all remote contacts in the _Call_" perhaps?
> 
> FIXME clarify what this mean
> 
> > Department of namespacing: some of the type names seem pretty vague. I think
> > it's OK for the Content.Media interface to just have "Description" in its
> > property/signal names, because in the context of a Call, the prevailing meaning
> > for "description" is (perhaps) the SDP one.
> >
> > However, types are a flat global namespace (in our codegen) and if I say
> > "description" with only Telepathy as context, I'd expect it to be some sort of
> > human-readable blurb about a chatroom or something. Perhaps the types could say
> > Media_, so we get Media_Description_Properties:a{sv},
> > Contact_Media_Description[_Properties]_Map:a{ua{sv}} and
> > Media_Description_Offer:(oua{sv}), or something?
> > 
> > I'd be very tempted to rename Description_Properties to Description_Description
> > :-)
> 
> FIXME

Changed the name to Media_Description, not  Description_Description because that would be silly :p


All issues are now fixed, please re-re-review :) There is one open FIXME to clarify the usage of 0 a bit more , but i'm going to punt on that one a bit more

> +      <p> If the remote codecs and other content information is available

trivia: are available

> +         >NoRemoteInformation</tp:dbus-ref> property will be false

I'm a bit hesitant to have "negative" boolean properties if there's no good reason for it (so I'd prefer to define HasRemoteInformation to be the inverse of your NoRemoteInformation, and remove NoRemoteInformation).

>        <h4>Changing codecs mid-call</h4>
> ...
> +        accepted, then 

trivia: trailing whitespace after "then"

+      <tp:member name="Remote_Contact" type="u" tp:type="Handle">
+        <tp:docstring>
+          The remote contact this description refers to
+        </tp:docstring>

Can this be 0?

+      <tp:docstring>
+        <p>A map from contact handles to the descriptions as response.</p>

This could do with expanding. Do you mean: each key is a remote contact handle (is 0 allowed here btw?), and depending how far into the negotiation we've got, the value is either the description we have offered to that remote contact, or the intersection of local/remote descriptions that we have negotiated with the remote contact?

+      <arg name="MediaDescription" type="o">

trivia: Ugly_Case please (it matters somewhat in Qt-land)

In LocalMediaDescriptionsChanged and RemoteMediaDescriptionsChanged, is the map a diff, or a new value for the property? (I assume it's a diff, so keys that are omitted from the change notification stay the same.)

+      <arg name="Removed_Media_Descriptions" type="au">

tp:type="Contact_Handle" I assume? It'd be nice to clarify that members of this list are removed from the keys of both maps (I assume that that's what happens).

+    <property name="MediaDescriptionOffer"

In a multi-user call, can we have more than one of these active? I assume the answer must be "no" - is that because it can't arise anyway, or is it enforced by the CM not announcing a new media description until the previous one has been processed?

         <p>Change notification is via the
+          <tp:member-ref>NewMediaDescriptionOffer</tp:member-ref> and
+          <tp:member-ref>MediaDescriptionOfferDone</tp:member-ref> signal.

trivia: ... signals.

+    <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
+      This object represents a remote Description Offer to which the local
+      streaming implementation should reply with its local Description
+    </tp:docstring>

If Description is well-known jargon, perhaps you could hyperlink to SDP or Jingle or something for the definition?

+        <p>Extra interfaces provided by this codec offer.  This SHOULD
+          NOT include the Description interface itself, and cannot change
+          once the content has been created.</p>

trivia: this is neither a codec offer nor a content

+    <method name="Reject" tp:name-for-bindings="Reject">
+      <tp:docstring>
+        Reject the proposed update to the remote description
+        FIXME add error codes and strings here
+      </tp:docstring>
+    </method>

I hope there's a bug for that :-)

+        <p> This property will never be part of the DescriptionProperties used
+            to describe this object. If this property is true then the
+            DescriptionProperties describing this object will be empthy (as
+            there is no remote information to put in it)

empty

DescriptionProperties seems like a dangling reference, which should be to Media_Description_Properties; but you seem to be using Qualified_Property_Value_Map directly, elsewhere.

Is the contact handle in a Media_Description_Offer and/or Contact_Media_Description_Properties_Map redundant with the MediaDescription.RemoteContact?

Please open a bug for the Remote_Contact = 0 thing, unless you plan to leave this bug open after merging to represent it.

> In LocalMediaDescriptionsChanged and RemoteMediaDescriptionsChanged, is the map
> a diff, or a new value for the property? (I assume it's a diff, so keys that
> are omitted from the change notification stay the same.)

Yeah, it's only the descriptions that were updated, as it says: ``The local content descriptions that were updated'' and not ``The local descriptions''

Would you like to see it spelled out a bit more?
 
> +      <arg name="Removed_Media_Descriptions" type="au">
> 
> tp:type="Contact_Handle" I assume? It'd be nice to clarify that members of this
> list are removed from the keys of both maps (I assume that that's what
> happens).

It talks about the local _and_ remote media descriptions that are no longer a part of... Is that not clear enough ?
 
> +    <property name="MediaDescriptionOffer"
> 
> In a multi-user call, can we have more than one of these active? I assume the
> answer must be "no" - is that because it can't arise anyway, or is it enforced
> by the CM not announcing a new media description until the previous one has
> been processed?

No indeed, see also the text on NewMediaDescriptionOffer

> +    <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
> +      This object represents a remote Description Offer to which the local
> +      streaming implementation should reply with its local Description
> +    </tp:docstring>
> 
> If Description is well-known jargon, perhaps you could hyperlink to SDP or
> Jingle or something for the definition?

It's well-known but imprecise jargon. I'd rather not refer to things that are vaguely similar, if people know about SDP and a bit about jingle Description will sound familiar otherwise it's not worth explaining its roots really..

> +    <method name="Reject" tp:name-for-bindings="Reject">
> +      <tp:docstring>
> +        Reject the proposed update to the remote description
> +        FIXME add error codes and strings here
> +      </tp:docstring>
> +    </method>
> 
> I hope there's a bug for that :-)
#32971

> Is the contact handle in a Media_Description_Offer and/or
> Contact_Media_Description_Properties_Map redundant with the
> MediaDescription.RemoteContact?

Yes, added a reference from the map definition now

> Please open a bug for the Remote_Contact = 0 thing, unless you plan to leave
> this bug open after merging to represent it.
#32972

Did some updates to sjoerd's branch:

Only one Local MediaDescription per content: The whole point of a content is that you have a single source that you broadcast to everyone. If you want to have separate contents with separate data to the different people in the call, then they have to be negotiated separately and the CM/UI should probably have different contents.

That said, we still need a way to clearly do mixing in the Muji case, maybe
have a way to mark the contents as being the same ?

Also, since the same property names are used in the answer, it does not make
sense to put the word Remote in there, since the property is also
used to describe the local codecs in this context.

See http://git.collabora.co.uk/?p=user/tester/telepathy-spec.git;a=shortlog;h=refs/heads/mediadesc-update

ocrete: I'm not a massive fan of your changes.

"The whole point of a content is that you have a single source that you broadcast to everyone. " in comment 11 confused me for a second because I thought you meant that you broadcast the same media to everyone. I guess what you mean is that you broadcast the same description to everyone?

Am I right in assuming that RemoteMediaDescriptions is what each remote side can receive/decode, and the LocalMediaDescription is what you were able to accept from remote peers.

what do we do we put in LocalMediaDescription the following muji case: 
userA says h264 is id 104, vp8 is id 105
Farstream accepts the offer, saying it can support both codecs
userB says vp8 is id 104, h264 is id 105
Farstream accepts the offer, saying it can support both codecs

I think that we either want it to remain as a map from user to properties, or not exist at all (in this case, UpdateLocalMediaDescription would need a remote member argument as well). I hope that either you or sjoerd can put me right and explain why you made this decision on Friday.

I like the rename to UpdateLocalMediaDescription. I don't think you've fully justified the reason for calling it with Renegotiate=False vs Renegotiate=True though. Also, I'm not quite sure why our streaming implementation would suddenly decide to renegotiate codecs: has your DSP suddenly realised that it can accelerate h264 or something? An example of each would be good.

Also, when I checked it out and ran make, I got:
 WARNING: Key 'RemoteCodecs' not known in namespace 'org.freedesktop.Telepathy.Call.Content.MediaDescription.DRAFT'
         (<tp:dbus-ref> in Interface(org.freedesktop.Telepathy.Call.Content.Interface.Media.DRAFT2))

The goal of the negotiation is to reduce all descriptions to a single one that is acceptable to everyone. You have to think of a multicast group where the exact same media stream is sent to everyone. Farsight2 will accept the Codec PT from the first peer as-is, but will override those of any subsequent peer with its own. If you need to encode differently for different people, you need to have multiple contents.

If in the muji-style stuff we want to have different mixers that produce different formats, then we need to split Content.Media from Content... and make it a subsidiary object and have a Call->Content->ContentMedia->Stream->Endpoint hierarchy... (because a stream cannot cross multiple ContentMedia in that case)

Your streaming framework might want to renegotiate in many cases:
1. You realize that you need more bandwidth.
2. You're a gateway and the other side is changed
3. You try to stream from a file, and you change files, you want to see if the other side can do more optimal settings?
4. You want to record a stream and would prefer the other side to have different codecs

That said, the Renegotiate param is only for XMPP, in SIP you always renegotiate in practice (since the way to update codecs is to do a re-invite with everything but the codecs not changing).

I'm just going to brainstorm the SIP call forking case here so that we can know that the API is sane.

Make a call
channel pops up
content pops up
MediaDescription pops up empty
You fill it with codecs = h264 and vp8
Stream pops up
You fill it with candidates
INVITE sent

answer comes back from A as RINGING, with candidates and codecs = h264
endpoint pops up with candidates
RemoteMediaDescription changes to codecs = h264, or does it stay empty?

answer comes back from B as RINGING, with candidates and codecs = vp8
endpoint pops up with candidates
RemoteMediaDescription changes to codecs = vp8, or does it stay empty?

endpoint A picks up
A new MediaDescriptionOffer pops up? (should we clarify this in the spec preamble?)
RemoteMediaDescription changes to codecs = h264
The stream goes to bidirectional

I think that sounds passable. We can't support early media unless RemoteMediaDescription is keyed by remote endpoint path or something, but do we care?

Can you explain why RemoteMediaDescription is keyed by remote contact (and separate from LocalMediaDescription), if the aim is to make them the same by the end of the negotiation? I don't think I've quite understood this part of the spec.

RemoteMD is keyed by contact because for some codecs (like H.264 and Theora) you need a header before you can decode the media. This header (or config-data or configuration) is often passed in the SDP (so it is realiably transmitted). And it will often be different for each remote side. So that's why there is a remoteMD for each remote peer.

So early media is like normal media, and we just have to set it before the call is accept, and then replace it with the real one when it shows up.

That said, in this case for early media, they are both the same contact (so the same handle), so in tp terms we are screwed... Or maybe the CM should use local handles for that.. and have a way to say "these local handles are all this global handle".

that said, I don't think it makes sense to get early media from multiple parties at the same time...

(In reply to comment #17)
> that said, I don't think it makes sense to get early media from multiple
> parties at the same time...

We could either make the CM arbitrarily send "mute" to one of the remote endpoints, or simply drop packets from whichever endpoint we don't like the look of. http://tools.ietf.org/html/rfc3960 doesn't seem to suggest any better solutions than this for multiple streams of early media, so that's probably good enough.

I'll try to write this up today and pop it on the end of your branch. Might also exchange 2 words with sjoerd about offer/answer.

the problem is that the CM doesn't know which oen is sending early media.. I would just play them all back at the same time..

I pushed some changes to to explain my thoughts.

http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/mediadesc-update

== "Clarify UpdateLocalMediaDescription's purpose." ==
is mostly a demonstration of how the semantics of your Renegotiate parameter for UpdateLocalMediaDescription gets messy very quickly, so please don't merge it into master. If someone proposes some good names for a pair of functions to replace this function (I'm not feeling very imaginative) that would be good. I might not actually understand the use-case for that properly though, so please read my commit and correct my understanding if necessary.

I found myself struggling to document the difference between a NewMediaDescriptionOffer signalled because of an incoming SDP-style offer and a NewMediaDescriptionOffer signalled because of an SDP-style answer. I'm pretty sure that either Farstream or Rakia should be reacting differently in each case, so I documented the latter as "the CM will almost-silently drop the arguments to Accept on the floor for the latter case" but there's no way to distinguish between the two cases in advance, so this doesn't seem like the right solution.

Incoming MediaDescription updates that don't require negotiation (see my concerns about UpdateLocalMediaDescription, above) are another case of not being able to tell the difference between "need to reply to this" and "no more negotiation required".

I was thinking of splitting the NewMDOffer signal into NewMDOffer, NewMDAnswer and NewMDUpdate or something, but sjoerd said that it complicates the Farstream side unnecessarily. Should I be digging into tpfs' mechanics to convince myself that it doesn't need to distinguish between offer, answer and update?

== The other commits ==
The other commits document some other subtle bits of the API. If you could skim read them and make sure they actually describe the api correctly, that would be good. Hopefully they don't stumble on any subtle api limitations.

After a lot of mulling things over for a few weeks, and bitching to sjoerd, I think I've got a solution to the Renegotiate parameter:

I've put a FurtherNegotiationRequired flag on the MediaDescription

If this is set to True by the CM in a MediaDescriptionOffer, it means "This is an offer under the SDP Offer/Answer model. Whatever you accept this offer with is what I will send to the other side in my answer."

If this is set to False by the CM then it means "This is an Answer under the SDP Offer/Answer model, and if everything is in order then no more negotiation is required."

If this is set to True by the streaming implementation (e.g. in an Accept or UpdateLocalMediaDescription call) then a new SDP Offer/Answer round-trip will be initiated.

A nice advantage of this approach is that Accept() and UpdateLocalMediaDescription() have the same signature, and the only time we should need to call UpdateLocalMediaDescription is if there is no MediaDescriptionOffer in flight.

Also, because I don't like the lack of elegance/extensibility that an empty initial offer provides, I have mandated that HasRemoteInformation appears in the dict and is set to False (so the dict is *never* empty).

Again, on the theme of elegance over "efficiency", I think that that All properties on the MediaDescription object should be represented in the map. This is more like the NewChannels. (Yes: this includes the Contact attribute, even though it is redundant in most places that it appears.)

There are some other clean-up ideas that I would like to try, but I will save them for after DRAFT2 when we actually have this shit implemented. (That way, I can test my ideas in code to make sure that they're sane.)

I have scrapped my mediadesc-update branch, because it was mostly a proof-by-contradiction-esque Troll.

tl;dr: I have overwritten all of ocrete's changes that I didn't like, and I think that my patches to http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/mediadesc-FurtherNegotiationRequired are ready for review.

For RemoteContact=0, can you specify that it's only for the initial offer.. The value in Accept() is always broadcast to all remote contacts.

In the "remove renegotiate param" patch.. s/decription/description/

There will always be 0 or 1 MediaDescription object per Content (not "in most case").

I think the Interfaces can change.. if we do SDPCagNeg by doing multiple MDs until one is accepted... Maybe you should say "and cannot change for a content once a description has been accepted has been created"

We ehould add a note that "RemoteContact" should not be in the properties passed to Accept() or UpdateLocalMediaDescription(). Since that will apply to everyone.

(In reply to comment #22)
> For RemoteContact=0, can you specify that it's only for the initial offer.. The
> value in Accept() is always broadcast to all remote contacts.
Said that the value should be 0 or omitted.

> In the "remove renegotiate param" patch.. s/decription/description/
Done

> There will always be 0 or 1 MediaDescription object per Content (not "in most
> case").
Done

> I think the Interfaces can change.. if we do SDPCagNeg by doing multiple MDs
> until one is accepted... Maybe you should say "and cannot change for a content
> once a description has been accepted has been created"
Just deleted this restriction. The immutable tag on the property is the only thing that we can guarantee for now.

> We should add a note that "RemoteContact" should not be in the properties
> passed to Accept() or UpdateLocalMediaDescription(). Since that will apply to
> everyone.
Said that the value should be 0 or omitted.

Pushed one commit to the end of mediadesc-FurtherNegotiationRequired.

++

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

Merged to master.