lukeweber / webrtc-jingle-client

Webrtc audio + jingle protocol brought to IOS and Android.
https://groups.google.com/forum/?fromgroups#!forum/webrtc-jingle
BSD 3-Clause "New" or "Revised" License
334 stars 137 forks source link

iOS: Integrate with xmppframework #69

Open hailg opened 11 years ago

hailg commented 11 years ago

Recently, I used xmppframework (https://github.com/robbiehanson/XMPPFramework) to connect to my ejabberd server and it works very well. Now, I think I could use xmppframework and webrtc-jingle-client together in my app. After reading the voiceclient example, I think I can modify TXmppPump class to use xmppframework connection (replace buzz::XmppClient with XMPPStream). The reason I wan to do that is because xmppframework is quite mature and it has a lot of xep extensions. Do you think that it's a good idea?

lukeweber commented 11 years ago

I'm definitely trying to keep things open so we could have a solution that once working could be quickly ported to IOS, Android and maybe in the future windows phones, or even Blackberry depending on NDKs, and if there's any interest. This solution would decidedly take us further from that goal, but get us quickly a lot closer to a solid xmpp for an IOS client.

Currently as a baseline, we still need a rostertask(use an incomplete one from examples or else it's xmpp/rostermoduleimpl.cc, but I haven't tested to see which one is better), and I need to fix presence to correctly return presence for all endpoints to the client,(currently it's voip only and not encapsulated in a task).

Nice to have features to bring it up to a minimum would be MUC chat, which to send and receive messages is probably only a few lines of code. Joining/leaving rooms, is basically all there in the call example, we would just need to add the missing code in presencepushtask and presenceouttask that's related to muc.

I would prefer to hustle on getting these pieces of the core working, because they would then work on other platforms as well, pushing android ahead at the same time. Do you think I could get the things working for you, which modules are you missing? If you want I'm fine with you going this route, but I have concerns about maintainability and think this would take it down the route of having a fork which is more customized to IOS, as oppose to one codebase that builds onto IOS with a thin wrapper for IOS specifics.

lukeweber commented 11 years ago

As a note, you can connect to ejabberd, it's just controlled by the define XMPP_COMPATIBILITY. Ideally this would be a param, but it was pretty deep in the code so I'll have to add it as a task for an improvement.

hailg commented 11 years ago

Hi lukeweber, I don't understand your comment much. Actually, what I mean is using xmppframework to do Jingle negotiation stuff. Because, I've already use xmppframework, connect to the ejabberd server and do xmpp tasks (login, send message, roster, privacy,...). As I understand so far, webrtc-jingle currrently use its own xmppclient to login and negotiate jingle session. So if I use them (xmppframework and webrtc-jingle) together without modify, then I will have 2 connections from my phone to server. That's is what I don't want. So I think I should modify webrtc-jingle to use xmppframework stream for jingle stuff. The reason I choose xmppframework over webrtc is because xmppframework have many xep implemented (privary, archive, muc,...). I just don't know this is a good idea or not.

lukeweber commented 11 years ago

Yeah, understand. It seems reasonable and is a valid use case.

I'd give it a shot, and don't see why it wouldn't work. Mostly everything runs on that xmppclient interface.

SessionManagerTask uses the xmppclient, and MediaSessionClient uses SessionManager to handle most of the call logic. Then you'll have a few tasks in clientsignalingthread.cc that you won't need, like the pump task and I would wrap them in a build flag for EXTERNAL_XMPP_CONNECTION to remove them being called.

After that I would be interested in breaking clientsignalingthread.cc into two pieces based on separating the tasks out a bit, etc, but would like to first see you patch working, so we don't get ahead of ourselves.

hailg commented 11 years ago

Great, I will start working with this. :)

hailg commented 11 years ago

Hi, just go around with the source code and found out that, currently you are using TXmppSocket (an buzz::AsyncSocket subclass). In the mean time, xmppframework is using GCDAsyncSocket (https://github.com/robbiehanson/CocoaAsyncSocket/tree/master/GCD). So I think I can just create a buzz::AsyncSocket subclass (like you did) and wrap xmppframework gcdasyncsocket inside. After that, I can share one gcdasyncsocket instance between xmppframework and webrtc-jingle-client. Do you think it's a right track to go?

hailg commented 11 years ago

Hi, me again. After a few days of coding, now I make they use the same socket (offer by ios-xmppframework). Basically, I create a new XmppClient (called IOSXmppClient) based on your source code and make webjingle use this new one. But now, when I try to make the called (within our ejabberd system) between aaa@mysystem.com and bbb@mysystem.com, I receive the following message at aaa: < iq from='bbb@mysystem.com' to='aaa@mysystem.com/13142259651362376308992118' type='error' id='2'> < jingle xmlns='urn:xmpp:jingle:1' action='session-initiate' sid='2504081629' initiator=''> < content name='audio' creator='initiator'> < description xmlns='urn:xmpp:jingle:apps:rtp:1' media='audio' ssrc='2505610900'> < payload-type id='103' name='ISAC' clockrate='16000'/> < payload-type id='106' name='CN' clockrate='32000'/> < payload-type id='105' name='CN' clockrate='16000'/> < payload-type id='13' name='CN' clockrate='8000'/> < payload-type id='127' name='red' clockrate='8000'/> < payload-type id='126' name='telephone-event' clockrate='8000'/> < rtcp-mux/> < /description> < transport xmlns='http://www.google.com/transport/p2p'/> < /content> < /jingle> < session xmlns='http://www.google.com/session' type='initiate' id='2504081629' initiator=''> < description xmlns='http://www.google.com/session/phone'> < payload-type xmlns='http://www.google.com/session/phone' id='103' name='ISAC' clockrate='16000'/> < payload-type xmlns='http://www.google.com/session/phone' id='106' name='CN' clockrate='32000'/> < payload-type xmlns='http://www.google.com/session/phone' id='105' name='CN' clockrate='16000'/> < payload-type xmlns='http://www.google.com/session/phone' id='13' name='CN' clockrate='8000'/> < payload-type xmlns='http://www.google.com/session/phone' id='127' name='red' clockrate='8000'/> < payload-type xmlns='http://www.google.com/session/phone' id='126' name='telephone-event' clockrate='8000'/> < src-id xmlns='http://www.google.com/session/phone'> 2505610900 < /src-id> < /description> < /session> < error code='400' type='modify'> < bad-request xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'/> < /error> < /iq>

But I don't see any message send to or receive from bbb@mysystem.com. So, I think this error message is sent from the ejabberd server, right? If that is the case, what do I need to do (install, modify,...) my ejabberd server? Thank you.

lukeweber commented 11 years ago

Does look like ejabberd. Since its iq they shouldn't be filtered or rejected by the server. Are the users friends/have you already tested with the default client between these two users? Maybe pointing to Google servers would take one variable out of the mix until you're sure what you have is working.

lukeweber commented 11 years ago

Are you dumping all the stanzas on both sides in logging? Should help as well on debugging quite a lot to see what each client is sending and receiving.

hailg commented 11 years ago

Yes, I'm dumping log on both client. I'm debugging the ejabberd server now. :)

hailg commented 11 years ago

Hi, a long day of refactoring the code to make xmppframework fit with webjingle signalthread and now it's done. But now I have a weird problem. When I call from gmail (chrome browser) to my device, the method XmppEngineImpl::IncomingStanza(const XmlElement* stanza) is called but when it run into the for loop with level = HL_SINGLE and i = 1, I have exec_bad_access (code=1) with (_stanzahandlers[level])[i]->HandleStanza(stanza). I try to debug but when I set the breakpoint at that line, (_stanzahandlers[level])[i] is not NULL. I try to enable NSZombieEnabled but it not help. This problem is repeat forever but I don't know how to debug it. Do you know how can I debug this kind of problem? I'm not a master of Xcode anyway. :)

hailg commented 11 years ago

Ah, I think the problem is something relate to some SessionSendTask handlers (because when I make a call, I saw it called AddStanzaHandler with some SessionSendTask instance). But I don't know why and how to fix it. :)

lukeweber commented 11 years ago

Okay, I think this is part of your engine implementation. Basically, your engine, that's part of your client interface, has to allow handlers of stanzas, and when you have new incoming stanzas, you loop over the handlers. If they respond true, it's handled, if false, you continue, or something like that. It's part of the whole task interface. Most tasks aren't needed, but SessionSendTask, I think is directly related to placing calls.

Want to pass a diff and I have a look?

hailg commented 11 years ago

I didn't change anything in XmppEngineImpl (as I remember), what I did is making my IOSXmppClient implement MessageHandler interface. Then, when new data come from xmppframework socket, I Post the message contains new data to my IOSXmppClient in signalthread. Then, in turn, IOSXmppClient::OnMessage call the HandleInput of XmppEngine (so XmppEngineImpl::HandleInput will run in signalthread). Currently, everything works fine except when other called to me. The problem I suspect now is maybe something related to thread problem. Would you help to clarify something:

  1. What is the thread that will call XmppEngineImpl::IncomingStanza? signalthread or main_thread? I think it should be signalthread, right?
  2. Could you tell me more about the task lifetime, when it will be destroyed, from which thread? I'm reading Task, TaskRunner,... but it would be faster with basic idea. Currently, my code contains too much debug things (just to help me understand about the framework easily), so I think it's a mess and could make you crazy. I will refactor the code when it's finished and then make a patch. :)
lukeweber commented 11 years ago

As a general rule, they create everything in the signal thread, then they dispatch back to the main thread, which then talks to the UI.

So generally speaking, all engine stuff, because pump happens on the signal thread, would all be happening on the signal thread. In reality, because you create your socket inside XmppFramework and then share it, depending on how you do that, it may be running on a thread from xmppframework, but that's kind of just a guess.

The tasks lifetime is dependent on what they're constructed with. : buzz::XmppTask(parent, buzz::XmppEngine::HL_SINGLE){ and the return of ::ProcessStart() { -> {return STATE_DONE;} or {return STATE_BLOCKED}...

Have a look at client/sendmessagetask.cc and client/presencepushtask.cc as two examples, of single task and long running task. There's a few ways to use them, but that's maybe a first glimpse. As well taskrunner deletes them when correct conditions are met, but I haven't personally looked into this.

hailg commented 11 years ago

Great, thanks. I will take a look.

hailg commented 11 years ago

Oh, I fixed it now, that's all because my fault. I forget to implement RemoveXmppTask in my new IOSXmppClient. So when the sessionsendtask is finished, it's not removed from the handlers list. That cause EXEC_BAD_ACCESS. :)

lukeweber commented 11 years ago

Awesome, I'm excited to try this out once you're all done :) On Mar 5, 2013 8:40 PM, "Hai Le Gia" notifications@github.com wrote:

Oh, I fixed it now, that's all because my fault. I forget to implement RemoveXmppTask in my new IOSXmppClient. So when the sessionsendtask is finished, it's not removed from the handlers list. That cause EXEC_BAD_ACCESS. :)

— Reply to this email directly or view it on GitHubhttps://github.com/lukeweber/webrtc-jingle-client/issues/69#issuecomment-14460595 .

hailg commented 11 years ago

:), I'm figuring out how not to change some of libjingle class (just create my own or extend them) to make it work. For example, currently I have to disable login_task (set NULL) in xmppengineimpl. I think that's a good idea to keep libjingle independence as you said. And one more thing, now every stanza will be parsed twice, 1 in xmppframework and 1 in webjingle, I think that's not good for performance. But it works anyway now. I will make some test with my own ejabberd system again. Hope it work. too.

lukeweber commented 11 years ago

Ill try to help with the cleanup in clientsignalingthread when you've got a patch that's more ready. As well, you could filter for iq stanzas that contain a jingle stanza. I think that's all libjingle technically needs for its calls. Also it needs the result response as well which would be iq where type=result.

http://xmpp.org/extensions/xep-0166.html

hailg commented 11 years ago

Ah, that's a great idea. :+1:

nickflink commented 11 years ago

Sorry this is late, but better late than never. ENABLE_ZOMBIES won't do anything in c++ code it's only for objective c code, and only marks something as a zombie after it has been released. There is a trick in c++ to find when you are overrunning memory boundaries, ie ... int aVar[2]; int bVar = 0; memset(var, 0, var+0); memset(var, 1, var+1); memset(var, 2, var+2); //This will overrite bVar to be 2 instead of 0

You could catch this by doing a watch... I think watch bVar would work, but note the program will run very slowly afterwards.

Note: If your problem had been stomping memory this would be a good way to find it, but it seems you problem was more like a dangling pointer. Anyway hopefully you find this type of information more useful down the line.

On Tue, Mar 5, 2013 at 2:24 PM, Hai Le Gia notifications@github.com wrote:

Hi, a long day of refactoring the code to make xmppframework fit with webjingle signalthread and now it's done. But now I have a weird problem. When I call from gmail (chrome browser) to my device, the method XmppEngineImpl::IncomingStanza(const XmlElement* stanza) is called but when it run into the for loop with level = HL_SINGLE and i = 1, I have exec_bad_access (code=1) with (_stanzahandlers[level])[i]->HandleStanza(stanza). I try to debug but when I set the breakpoint at that line, (_stanzahandlers[level])[i] is not NULL. I try to enable NSZombieEnabled but it not help. This problem is repeat forever but I don't know how to debug it. Do you know how can I debug this kind of problem? I'm not a master of Xcode anyway. :)

— Reply to this email directly or view it on GitHubhttps://github.com/lukeweber/webrtc-jingle-client/issues/69#issuecomment-14439620 .

Nick Flink Calle de San Joaquín, 4, 28004 Madrid, Spain +34 911394660 (land line) +34 627256841 (mobile)

hailg commented 11 years ago

I've found the reason that cause my error. But thank you, it will help me and others so much. :)

hailg commented 11 years ago

Hi, currently I see that the client sent to my ejabberd server the following iq: < iq to="mindy@speakmob.com" type="set" id="3"> < jingle xmlns="urn:xmpp:jingle:1" action="session-initiate" sid="4084016898" initiator="aloo@speakmob.com/12038967141362561106821388"> < content name="audio" creator="initiator"> < description xmlns="urn:xmpp:jingle:apps:rtp:1" media="audio" ssrc="2340781823"> < payload-type id="103" name="ISAC" clockrate="16000"/> < payload-type id="106" name="CN" clockrate="32000"/> < payload-type id="105" name="CN" clockrate="16000"/> < payload-type id="13" name="CN" clockrate="8000"/> < payload-type id="127" name="red" clockrate="8000"/> < payload-type id="126" name="telephone-event" clockrate="8000"/> < rtcp-mux/> < /description> < transport xmlns="http://www.google.com/transport/p2p"/> < /content> < /jingle> < session xmlns="http://www.google.com/session" type="initiate" id="4084016898" initiator="aloo@speakmob.com/12038967141362561106821388"> < description xmlns="http://www.google.com/session/phone"> < payload-type xmlns="http://www.google.com/session/phone" id="103" name="ISAC" clockrate="16000"/> < payload-type xmlns="http://www.google.com/session/phone" id="106" name="CN" clockrate="32000"/> < payload-type xmlns="http://www.google.com/session/phone" id="105" name="CN" clockrate="16000"/> < payload-type xmlns="http://www.google.com/session/phone" id="13" name="CN" clockrate="8000"/> < payload-type xmlns="http://www.google.com/session/phone" id="127" name="red" clockrate="8000"/> < payload-type xmlns="http://www.google.com/session/phone" id="126" name="telephone-event" clockrate="8000"/> < src-id xmlns="http://www.google.com/session/phone"> 2340781823 < /src-id> < /description> < /session> </ iq>

As you can see, the < iq > contains 2 elements: < jingle > and < session >. I don't think it's valid with a request iq (as I remeber, request iq can only contains 1 element). That's why currently ejabberd server send me back bad-request stanza. So is this because of the current webjingle implement or because of me? Please help me confirm. Thank you.

lukeweber commented 11 years ago

I actually think your issue is that iq's need to be sent to a full jid(jid/resource), and you're only sending to a bare jid.

hailg commented 11 years ago

Hi, I quoted from http://xmpp.org/rfcs/rfc3920.html, IQ Semantics:

  1. An IQ stanza of type "get" or "set" MUST contain one and only one child element that specifies the semantics of the particular request or response.

I also debug my ejabberd server, too. And when it parse the iq, it failed because of that. :)

hailg commented 11 years ago

Try to call from Gmail -> Gmail and saw that it sent the same iq. So I think I need to make a patch with my ejabberd server.

lukeweber commented 11 years ago

Okay, now I'm actually seeing it. No, it's something about the client. You're sending duplicate data.

In clientsignalingthread.cc -> session->set_current_protocol(cricket::PROTOCOL_HYBRID); -> try cricket::PROTOCOL_JINGLE

I hadn't looked at this, and I'm not sure if this needs to be changed to PROTOCOL_GINGLE to work on google, or if they do actually support PROTOCOL_JINGLE, or if this is for reverse compatibility with old clients. I'd have to do more testing.

hailg commented 11 years ago

Yay, you make the save. :+1: Now I can call within my ejabberd system. Will do more testing.

nmohankumar commented 10 years ago

@lukeweber I use xmppframework and webrtc-jingle together without modify, I have 2 connections from my phone to server. So i receive each and every iq two times. @hailg you told an idea "buzz::AsyncSocket subclass (like you did) and wrap xmppframework gcdasyncsocket", but i couldn't understand. I'm a newbie to c/cpp code. I couldn't solve this issue. How to resolve this issue.