search

opentelecoms-org / lumicall

SIP and ENUM dialer for Android with ZRTP/SRTP encryption, SIP over TLS, ICE/TURN for NAT, G.729 and many other features
http://www.lumicall.org
Other
144 stars 88 forks source link

Adding PUBLISH #40

Closed pranavjain closed 8 years ago

pranavjain commented 8 years ago

Mentored By : @saghul @dpocock

saghul commented 8 years ago

Left some comments. Not ready to land yet, but we're close!

dpocock commented 8 years ago

@saghul I've been chatting with @pranavjain on IRC and testing this with him. I observed problems with it not sending the authentication headers after a challenge and not incrementing the CSeq when authenticating. When we tested today it appeared to be working. pranav2 would appear online in my Jitsi (for 60 seconds).

Pranav has hard-coded expiry to 60 seconds while testing, it should be changed to follow the same timeout as REGISTER before we merge this.

saghul commented 8 years ago

@dpocock I understand the cseq and auth issues are handled now?

dpocock commented 8 years ago

it looked like it was working, it was authenticating and the CSeq had increased on the second PUBLISH. I didn't check whether it is being set correctly when the PUBLISH has expired and a new transaction is made, it would be good to compare that with the PUBLISH messages from another client like Jitsi.

saghul commented 8 years ago

It's a new transaction, so it can start at 1. I'm on mobile, but IIRC that's how we do it in Blink.

On Aug 20, 2016 09:32, "Daniel Pocock" notifications@github.com wrote:

it looked like it was working, it was authenticating and the CSeq had increased on the second PUBLISH. I didn't check whether it is being set correctly when the PUBLISH has expired and a new transaction is made, it would be good to compare that with the PUBLISH messages from another client like Jitsi.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/opentelecoms-org/lumicall/pull/40#issuecomment-241185084, or mute the thread https://github.com/notifications/unsubscribe-auth/AATYGBr0hQHUniG-cDu_wO_9hFpIN33Zks5qhq2SgaJpZM4Jnfzo .

pranavjain commented 8 years ago

@saghul I have done the changes. Please check! I have put '+1' on the comments which I have processed.

saghul commented 8 years ago

left another round of comments.

dpocock commented 8 years ago

@saghul Pranav has now committed a more concise version of the changes on another branch, without all the formatting changes, https://github.com/pranavjain/lumicall/commit/3153d60c1b43a246e5fab2abfb52f52cdd86f503

It will become another pull request