Branch that implements call1, needs review

http://cgit.collabora.com/git/user/tester/telepathy-rakia.git/log/?h=call1

Also, needs this branch of tp-glib, also needs review

http://cgit.collabora.com/git/user/tester/telepathy-glib.git/log/?h=call1-misc

(In reply to comment #1)
> Also, needs this branch of tp-glib, also needs review
> 
> http://cgit.collabora.com/git/user/tester/telepathy-glib.git/log/?h=call1-misc

"BaseMediaCallStream: Make it possible to set the Content at construct time" contains this change, which doesn't seem to fit:

> + if (!tp_base_call_channel_is_accepted (TP_BASE_CALL_CHANNEL (channel)))
> + goto done;

Shouldn't that be under a separate commit title?

The branch looks OK in principle, and the first two commits are clearly right. I don't think I understand Call well enough to review the rest; perhaps Sjoerd could have a look?

Good catch, made it into a separate commit.

Some rationale for cf0bc929736569003deedc447133ea2b2c4c5781 would be great. It seems odd to me.

I'm not sure about 0d7ed54aa1e20dd5d69d0e243d16d1794885aae2, the logic is a bit tricky to understand. But also if unholding fails, why does the stream that causes it to fail not also go to an error state ? Especially since the unholding failure doesn't seem to have detailed errors. I would expect that i would first get signalled about a sending/recieving failure on a stream followed by the general unhold failing. (Such that it's guaranteed that at the point unhold reports an error i can check my individual streams).

I believe the use-case to force calling _update_sending_state() is because the contetn/channel can be set after the object has been created, so we need to take it into account..

The idea is that unhold failing is probably due to something else still holding the resource.

(In reply to comment #5)
> I believe the use-case to force calling _update_sending_state() is because the
> contetn/channel can be set after the object has been created, so we need to
> take it into account..

Does this not simply mean that we should also update the sending state if those are initially set instead of hacking it to happen as a side-effect of another call?

> The idea is that unhold failing is probably due to something else still holding
> the resource.

Sure but from the looks of it you don't report errors on the stream if it's during unholding which seems odd? I'd still like to get the actual error from the stream i would expect.

Actually, the solution I ended up using in Rakia is to just set the Content and Channel on the stream at creation time instead of adding it later...

Report that error to whom? Maybe we should add a separate call "unhold_failed_because" or something.

The same problem happened in gabble, so I removed that patch and instead I added a g_object_notify() when the channel is set and have the media subclass call the update functions when that happens.. My call1-misc branch was updated with that.

There is now a gabble branch to fix a unit test to match the tp-glib change..

http://cgit.collabora.com/git/user/tester/telepathy-gabble.git/log/?h=call1-misc

(In reply to comment #7)
> Actually, the solution I ended up using in Rakia is to just set the Content and
> Channel on the stream at creation time instead of adding it later...
 
> Report that error to whom? Maybe we should add a separate call
> "unhold_failed_because" or something.

To whoever wants to know why unholding failed :).. Could you split up your branch so we can merge all the bits except for this hold madness (so we can move on with rakia at least)..

For hold, i'd like to understand what should happen on a dbus level, otherwise i don't think we can really decide on that patch :)

(In reply to comment #8)
> The same problem happened in gabble, so I removed that patch and instead I
> added a g_object_notify() when the channel is set and have the media subclass
> call the update functions when that happens.. My call1-misc branch was updated
> with that.

That solutions looks good to me.

When you try to unhold, you call RequestHold(), then the CM tells the other side that we're going to unhold and in parallel tp-glib emits SendingStateChanged and ReceivingStateChange and waits for those to be Completed. Then tp-glib emits HoldStateChanged.

If the Sending or Receiving state can't be completed to STARTED, then ReportSending/ReceivingFailure are called. But we don't want those to change the direction of the stream or end the call (which is what you'd normally do). Instead, you just want to abort the Unhold and change the hold reason to Resource_Not_Available. The reason for the  Report*Failure call is useless and is always the same (TP_CALL_STATE_CHANGE_REASON_INTERNAL_ERROR, TP_ERROR_STR_MEDIA_STREAMING_ERROR) because nothing else makes sense.

Also, the unhold failure thing isn't rakia specific, gabble needs the same thing I believe.

(In reply to comment #11)
> When you try to unhold, you call RequestHold(), then the CM tells the other
> side that we're going to unhold and in parallel tp-glib emits
> SendingStateChanged and ReceivingStateChange and waits for those to be
> Completed. Then tp-glib emits HoldStateChanged.
> 
> If the Sending or Receiving state can't be completed to STARTED, then
> ReportSending/ReceivingFailure are called. But we don't want those to change
> the direction of the stream or end the call (which is what you'd normally do).
> Instead, you just want to abort the Unhold and change the hold reason to
> Resource_Not_Available. The reason for the  Report*Failure call is useless and
> is always the same (TP_CALL_STATE_CHANGE_REASON_INTERNAL_ERROR,
> TP_ERROR_STR_MEDIA_STREAMING_ERROR) because nothing else makes sense.

Well ideally we'd report to the user. Can't unhold, couldn't access your camera. But i see that the Hold specification doesn't allow for this atm. So with a light sound of annoyance i agree with your patch for now. For the future i think we need to update the specific such that Hold can give a detailed error reason (e.g. failed because of this stream here which had this detailed error).

Alright, the tp-glib branch is merged.. Let's figure out what to do with the rakia branch.. Do we get a volunteer to review it ?

For the record, telepathy-glib 0.17.6 has been released and should have the required bits.

tbh, I vote to just merge this, it can't be worse than current situation since rakia does not work at all anymore (streamed media is dead and removed from empathy/gnome-shell).

That branch is huge to review, I doubt anyone will be to it... :(

Olivier and I did quite a lot of testing with this. We made it fail a lot but weren't sure whether it was the SIP server or not. Olivier was going to install a SIP server on his laptop to get more logs but I'm not sure if that happened.

No-one is going to review this, no, but let's make sure it kind of works first.

I installed Freeswitch on my laptop, but I never managed to make it fail. I can make it fail with the Collabora freeswitch SIP server, when the server just stops forwarding messages, but I don't have access to the log, I can't debug it.

Does it fail as well with StreamedMedia?

If no one is currently working on tracking this specific issue I'd be in favor of merging and releasing the branch as well. Letting him rot on the bugzilla won't help fixing it or finding regressions.

This was merged and released into 0.7.4 ..