Bug 28707

The Media Interface should have a way to tell the Streaming Implementation to stop receiving as well as sending.

The CM has to be able to ask tp-fs to Hold
The CM has to be able to ask tp-fs to UnHold.. and a confirmation that it has unheld

Hold has to be on the Content and preferably even the stream.. because when using 3pcc the controller can change the direction on the Call (SIP session) and the Content (SIP Media) arbitrarily...

*** Bug 31278 has been marked as a duplicate of this bug. ***

Hold can mean two things:

1. Local hold.. the local user wants to hold a call:
 1.1 Stop sending
 1.2 Stop receiving

2. Remote hold, a remote contact has placed a call on hold:
 2.1 Stop sending
 2.2 Continue receiving

There is also a 3rd reason to stop sending, if the local user has muted the Content.

The current API has a 4th way to stop sending, it is the Stream.SetSending(), I'm not sure what its use-case is and how it differs from Content.Mute, apart being per-stream. I suggest renaming it to SetMuted() and the "LocalSendingState" property to "Muted".

The use-case for per-stream-muting is to way to tell something to only a sub-group of a conf-call, so one would mute everyone else.

The Mute interface should have a success/failure return and not be immediate (since unmuting is not immediate and can fail). 

Also, Stream.RequestReceiving() .. isn't that the same as requesting a contact to Unmute ? Maybe it should be Stream.RequestUnmute() ?

As in bug #28693, I suggest moving LocalSendingState/SetSending to the Stream.Media interface, the CM should only request us to start sending if we are neither in local hold, remote hold, Content.Muted or Stream.Muted. 

Local hold is the only state when we want to stop receiving and local hold is only available per-channel in the current API, it makes sense to have a per-channel Media interface for that since re-starting to receive may fail and tp-fs needs to tell that to the CM before it can proceed.

The only thing we're missing is a way to inform the UI when the remote side has placed a per-Content remote hold (we currently only have CallState for that which is per channel).

If we want to allow the local user to do a per-content Hold, we can just allow the Channel.I.Hold interface to be applied to a Call.Content. And then also we need to move the Hold.Media interface to the Content.

*** Bug 28693 has been marked as a duplicate of this bug. ***

Meeting notes, needs drafting:

1. Merge mute to Content and Call
2. Add Hold Interface to Content and Call
3. Keep directions on Stream

Write guidelines
1. Mute is temporary
2. Stream direction is for long term changes

Stream.I.Media:
Prop: SendingState: Not-Sending, PendingSend, PendingNotSend + Reason (enum: muted or hold or setsendingfalse or failed or temporary-failed)
Method:
- SendingStarted()
- SendingFailed(enum, Ddbus error, Message)
- SendingStopped()
- Muted()

SendingFailed can be called at any time for errors

Same thing exists for s/Sending/Receiving/

Sending/ReceivingState properties on the stream ... with change notification with error

Tp-fs should only call/modify/manipulate/groom the Media interfaces

*** Bug 31277 has been marked as a duplicate of this bug. ***

Okay... I don't think I know what is required for this. Also, I suspect that my stream-etc branch is going to conflict all over this shit, so if someone could review the last commit on that first (bug #28705) so I can get it merged, that would be excellent.

(In reply to comment #8)
> Meeting notes, needs drafting:
> 
> 1. Merge mute to Content and Call
There is already Content.Interface.Mute. Do you want it put into the main Content interface or what? You want it duplicated in Call? Why?

> 2. Add Hold Interface to Content and Call
TODO.
> 3. Keep directions on Stream
Not sure what you mean here. Also: what do you want doing with Call.Stream.RemoteMembers (map between contacts and their Sending State).
 
> Write guidelines
> 1. Mute is temporary
> 2. Stream direction is for long term changes
> 
TODO.

> Stream.I.Media:
> Prop: SendingState: Not-Sending, PendingSend, PendingNotSend
Made this enum Stream_Flow_State, with equivalents for these states, and also "flowing".

> + Reason (enum:
> muted or hold or setsendingfalse or failed or temporary-failed)
I'm not sure about the usefulness of having a Muted() callback, so I didn't include it in the draft (calling SendingStopped() in response to SendingStateChanged(PendingStop, Muted, "") sounds just as good)
> Method:
> - SendingStarted()
> - SendingFailed(enum, Ddbus error, Message)
> - SendingStopped()
> - Muted()
> 
> SendingFailed can be called at any time for errors


> Same thing exists for s/Sending/Receiving/
I started drafting it like that, but actually the Sending and Receiving cases are not quite symmetrical, are they? If you want to take a look at http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/mute to make sure I'm on the right track then you can.


> Sending/ReceivingState properties on the stream ... with change notification
> with error
Hrrrm... Calling them the same thing on two different interfaces of the same object. That's going to get confusing. TODO.

(In reply to comment #10)
> (In reply to comment #8)
> > Meeting notes, needs drafting:
> > 
> > 1. Merge mute to Content and Call
> There is already Content.Interface.Mute. Do you want it put into the main
> Content interface or what? You want it duplicated in Call? Why?

We decided mute wasn't optional. The reason to duplicate it in Call is just to make it easier for applications.
 
> > 3. Keep directions on Stream
> Not sure what you mean here. Also: what do you want doing with
> Call.Stream.RemoteMembers (map between contacts and their Sending State).

I meant that SetSending/RequestReceiving() would stay on the Stream Interface.

> > Stream.I.Media:
> > Prop: SendingState: Not-Sending, PendingSend, PendingNotSend
> Made this enum Stream_Flow_State, with equivalents for these states, and also
> "flowing".
> 
> > + Reason (enum:
> > muted or hold or setsendingfalse or failed or temporary-failed)
> I'm not sure about the usefulness of having a Muted() callback, so I didn't
> include it in the draft (calling SendingStopped() in response to
> SendingStateChanged(PendingStop, Muted, "") sounds just as good)

Mute != SendingStopped... with mute you could continue sending, but send silence. That's actually what your N900 does.

> > Method:
> > - SendingStarted()
> > - SendingFailed(enum, Ddbus error, Message)
> > - SendingStopped()
> > - Muted()
> > 
> > SendingFailed can be called at any time for errors
> 
> 
> > Same thing exists for s/Sending/Receiving/
> I started drafting it like that, but actually the Sending and Receiving cases
> are not quite symmetrical, are they? If you want to take a look at
> http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/mute
> to make sure I'm on the right track then you can.

If you use the same enum for sending/receiving.. Rename Pending_SEnd... maybe to Pending_Flow ..

I'm not how how its not symmetrical ?

> > Sending/ReceivingState properties on the stream ... with change notification
> > with error
> Hrrrm... Calling them the same thing on two different interfaces of the same
> object. That's going to get confusing. TODO.

If you have a better idea.

> > > 1. Merge mute to Content and Call
> > There is already Content.Interface.Mute. Do you want it put into the main
> > Content interface or what? You want it duplicated in Call? Why?
> 
> We decided mute wasn't optional. The reason to duplicate it in Call is just to
> make it easier for applications.
> 
Okay. Thanks. Done. Note that I changed the wording of MuteState's docstring in the same commit. Sorry. Please re-read it to make sure you agree.

> > > 3. Keep directions on Stream
> > Not sure what you mean here. Also: what do you want doing with
> > Call.Stream.RemoteMembers (map between contacts and their Sending State).
> 
> I meant that SetSending/RequestReceiving() would stay on the Stream Interface.
> 
Okay, so nothing to do here. Good Good.

> Mute != SendingStopped... with mute you could continue sending, but send
> silence. That's actually what your N900 does.
> 
Fixed. Thanks for clarifying.

> If you use the same enum for sending/receiving.. Rename Pending_SEnd... maybe
> to Pending_Flow ..
> 
Thanks for spotting that. Good catch.

> I'm not how how its not symmetrical ?
> 
See the top of your comment #5 regarding Hold. It seems that the only asymmetry that seems to be reflected in the spec in its current form is that there is no ReceivingMuted method. I will try to make sure that there is enough rationale to explain the asymmetry before I merge, but I suppose it's not massively important.

> > > Sending/ReceivingState properties on the stream ... with change notification
> > > with error
> > Hrrrm... Calling them the same thing on two different interfaces of the same
> > object. That's going to get confusing. TODO.
> 
> If you have a better idea.
I don't think that the UI really cares about the specific state of each direction of each stream. It probably cares more whether the request for mute/hold has actually been propagated to the media layer. I will take a look at that next week. (TODO)

Note that the branch has been force-updated since you last looked. This is just to rebase it on top of the current Call branch for my own sanity. The first commit was not changed at all, and there were no conflicts.

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

For the StopSending description:
s/is sending silence/may opt to send silence instead of stopping the stream/
Stop Sending is permanent as opposed to Mute which is more temporary.

I realise this is just copy pasted from another file, but the MutedState variable on the content should refer to the local mute state. So it only changes if you call SetMuted().. If its a mute from a remote content, it should be a separate notification/variable.

Okay... I have the following on my task list for this bug:

* Hold interface also applicable to Content
** specced up at alsuren/mute. Should it also be possible to hold a stream?
** make sure this interface actually makes sense recursively.

* Mute method and state on Call and Stream
** I think I will turn Mute back into an interface, and let it apply to all 3
** Make mute mandatory on Call and Content.
** make sure this interface actually makes sense recursively.

* See also States/Flags (#35128). Will probably do these both in the same branch or something.

If anyone has any objections to this structure, speak now.

(In reply to comment #13)
> For the StopSending description:
> s/is sending silence/may opt to send silence instead of stopping the stream/
> Stop Sending is permanent as opposed to Mute which is more temporary.
> 
Can't find where you mean. Maybe I fixed it and then forgot about it?


> I realise this is just copy pasted from another file, but the MutedState
> variable on the content should refer to the local mute state. So it only
> changes if you call SetMuted().. If its a mute from a remote content, it should
> be a separate notification/variable.
> 
There are MemberFlags on the Call, and RemoteMembers on the Stream, which both represent different things about the call. I have added a Muted flag to MemberFlags (there is already Held), but it will only be set if all contents from that user are muted remotely and signalled as such. Our spec allows Contents/Streams to be muted/held independently in some cases. I'm quite happy to leave the "some contents are muted/held" as an un-signalled case (mute/hold are often informational anyway, right?).

> > > > Sending/ReceivingState properties on the stream ... with change notification
> > > > with error
> 
RemoteMembers and LocalSendingState on the stream seem to fit this purpose (RemoteMembers can represent a superset of what ReceivingState can represent). They currently use the Sending_State enum. I might make them use the equivalent Stream_Flow_State enum instead, to avoid duplication.

I'll probably add a Reason property/arg to the RM/LSS related signals, but there are quite a lot of Reason properties/args floating around in Call. Maybe I should try to consolidate them into a single "Call_State_Change_Reason" enum before I continue.

(In reply to comment #15)
> (In reply to comment #13)
> > For the StopSending description:
> > s/is sending silence/may opt to send silence instead of stopping the stream/
> > Stop Sending is permanent as opposed to Mute which is more temporary.
> > 
> Can't find where you mean. Maybe I fixed it and then forgot about it?

I'm not sure either, I may have been referencing this:
http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=commitdiff;h=da47cad093f1845c1cc4ee3111b2265eaa7ab132

> There are MemberFlags on the Call, and RemoteMembers on the Stream, which both
> represent different things about the call. I have added a Muted flag to
> MemberFlags (there is already Held), but it will only be set if all contents
> from that user are muted remotely and signalled as such. Our spec allows
> Contents/Streams to be muted/held independently in some cases. I'm quite happy
> to leave the "some contents are muted/held" as an un-signalled case (mute/hold
> are often informational anyway, right?).

Ok, that's good, just add a note to the "MutedState" property on the Content to say it means local muting.

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

Ended up shoving my changes to states in the middle of it. If you want me to rebase, I probably can. Otherwise, I will just leave this blocking #35128

I think the Mute stuff can be simplifieda but because I don't think muting can fail.

Otherwise++

This is still blocking on me talking with sjoerd about the states sub-branch... Y'know what: I think I might just rebase states into its own branch...

Done (removed 3 commits from this branch) with one commit added to the end to make it compile and an optional commit for re-ordering. (Note that "Add a Muted CallMemberFlag." has always been wrong, and I don't think it compiled even before the rebase).

It might actually be worth re-ordering, so that the flags are [ringing, held, muted, conference_host] rather than [ringing, held, conference_host, muted]. What do you think? (see the last commit on alsuren/muted. I'll happily merge with or without this change)


(In reply to comment #18)
> I think the Mute stuff can be simplifieda but because I don't think muting can
> fail.
> 
> Otherwise++
The extra states in Mute are not just in case mute can fail. It is also to provide feedback in case muting the stream takes a non-zero amount of time. I'd rather not risk having the UI say that the call is muted when it isn't (through implementation bugs or otherwise).

Looks ++
I don't really care about the ordering, so whichever you prefer

merged ages ago and forgot to update the bug.

merged to master.