Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple SDP parser from SIP parser #11

Open
negbie opened this issue Jun 30, 2020 · 13 comments
Open

Decouple SDP parser from SIP parser #11

negbie opened this issue Jun 30, 2020 · 13 comments

Comments

@negbie
Copy link
Collaborator

negbie commented Jun 30, 2020

I think it would make sense to decouple the SDP parser from the SIP parser. @moggle-mog already saw it here
#9 (comment)

@jart , @moggle-mog what do you think?

@moggle-mog
Copy link

Just Do IT.

@jart
Copy link
Owner

jart commented Jun 30, 2020

Reviewing the most recent code, the SIP->SDP relationship looks clean. We can absolutely improve it though. One possible idea is using the Registration Pattern. The SIP package could have a list of payload parsers. We'd need to define a new API for that. Then we could have the SDP package call sip.RegisterPayloadParser() in its init() function. I like having registration be a side-effect of the linkage choices each user makes. I also like the idea of enabling third party projects to register custom parsers.

Apropos #9 please understand that IETF documents are really well-written recommendations. Open source projects are usually rewarded for following them. However there's no punishment for not following them. I like to think of it as a challenge, to figure out a way to do what I want to do, within the arbitrary constraints defined by the Internet Engineering Task Force. If you look at it the way I do, then you'll find that implementing protocols becomes a whole lot more fun!

@negbie
Copy link
Collaborator Author

negbie commented Jun 30, 2020

I like to think of it as a challenge, to figure out a way to do what I want to do, within the arbitrary constraints defined by the Internet Engineering Task Force. If you look at it the way I do, then you'll find that implementing protocols becomes a whole lot more fun!

That's a great mindset!

Reviewing the most recent code, the SIP->SDP relationship looks clean. We can absolutely improve it though. One possible idea is using the Registration Pattern. The SIP package could have a list of payload parsers.

That sound's good and Payload is already an Interface!

@moggle-mog
Copy link

moggle-mog commented Jul 1, 2020

Reviewing the most recent code, the SIP->SDP relationship looks clean. We can absolutely improve it though. One possible idea is using the Registration Pattern. The SIP package could have a list of payload parsers. We'd need to define a new API for that. Then we could have the SDP package call sip.RegisterPayloadParser() in its init() function. I like having registration be a side-effect of the linkage choices each user makes. I also like the idea of enabling third party projects to register custom parsers.

Apropos #9 please understand that IETF documents are really well-written recommendations. Open source projects are usually rewarded for following them. However there's no punishment for not following them. I like to think of it as a challenge, to figure out a way to do what I want to do, within the arbitrary constraints defined by the Internet Engineering Task Force. If you look at it the way I do, then you'll find that implementing protocols becomes a whole lot more fun!

It sound's good, when we init SIP, we can also init the payload parser.However, can we put the payload into generic struct?So user can decide how to use it.
For example in msg_parser.rl,

if ctype == sdp.ContentType {
	ms, err := sdp.Parse(string(data[p:len(data)]))
	if err != nil {
		return nil, err
	}
	msg.Payload = ms
} else {
	msg.Payload = &MiscPayload{T: ctype, D: data[p:len(data)]}
}

Another way is,

msg.Payload = &MiscPayload{T: ctype, D: data[p:len(data)]}

User receive this message and use Content-Type to choose the better parser like this,

tp, err := sip.NewTransport(....)
if err != nil {
	panic(err)
}

....

msg := <-tp.C
switch  msg.Payload.ContentType() {
case sdp.ContentType:
        // It is a good idea?
	sp := sdp.Parse(string(msg.Payload.Data()))
	sp...
	....
}
....

Of course, this will increase the complexity of the user code, I don't know whether it will make us crazy or not.

@jart
Copy link
Owner

jart commented Jul 2, 2020

One of the things that makes gosip appealing is it empowers the user to implement a command line telephone in a few hundred lines with complete control over the main event loop. We can introduce complexity. Just want to understand benefits.

SIP+SDP are independent designs in the sense that HTTP+HTML are independent. I've never seen a SIP message carry a payload type that isn't SDP. That's why the assumption is baked into the design. Could you cite any specific publicly documented examples of existence of non SDP payloads?

@jart
Copy link
Owner

jart commented Jul 2, 2020

By the way! I just realized that the G.729 patents finally expired a couple years ago! It'd be great if we could implement that in the fastest way possible. It's an alternative RTP payload that reduces the bandwidth consumption of each telephone call from 64kb to 8kb, with quality that's somehow better than than the GSM audio codec that needs 32kb! Now that G.729 no longer costs $10 per channel, maybe that's a good area of focus for offering alternative choices.

@negbie
Copy link
Collaborator Author

negbie commented Jul 2, 2020

Mby we could use this https://github.com/BelledonneCommunications/bcg729 but it wouldn't be the fastest way possible.

@negbie
Copy link
Collaborator Author

negbie commented Jul 2, 2020

If you want reduced bandwidth usage and good audio quality opus is quite nice! https://github.com/hraban/opus

@negbie
Copy link
Collaborator Author

negbie commented Jul 2, 2020

Downside of opus is that not many carrier support it.

@jart
Copy link
Owner

jart commented Jul 2, 2020

It's unfortunate but all the commercial systems I've seen, tend to usually support a choice of either G.711 (which has been standard since T1 in 1950's) and G.729. The BCG code looks pretty clean. It looks like the kind of thing that would be fun to profile and make go even faster. For example, I'm noticing it's using Q15 arithmetic. Little known optimization secret: the PMULHRSW instruction can make that go wicked fast, on most CPUs.

@negbie
Copy link
Collaborator Author

negbie commented Jul 2, 2020

the PMULHRSW instruction can make that go wicked fast, on most CPUs.

Yip, part of SSSE3 and could be even used by Atom's.

@moggle-mog
Copy link

moggle-mog commented Jul 2, 2020

One of the things that makes gosip appealing is it empowers the user to implement a command line telephone in a few hundred lines with complete control over the main event loop. We can introduce complexity. Just want to understand benefits.

SIP+SDP are independent designs in the sense that HTTP+HTML are independent. I've never seen a SIP message carry a payload type that isn't SDP. That's why the assumption is baked into the design. Could you cite any specific publicly documented examples of existence of non SDP payloads?

Such as Content-Type: multipart/mixed;boundary="boundary1" from https://tools.ietf.org/html/rfc5621 page 4 and
message/cpim of IM.

image

@razor-1
Copy link

razor-1 commented Feb 11, 2021

SIPREC is a surprisingly common use case for a different content-type as well. RFC7866 is the spec and RFC8068 has a number of sample call flows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants