-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: add support for a:ssrc and a:ssrc-group #3
Conversation
src/lines/ssrc-line.ts
Outdated
* Model an SSRC line in SDP as defined by https://datatracker.ietf.org/doc/html/rfc5576#section-4.1. | ||
* | ||
* @example | ||
* a=ssrc:1234567 cname=foo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit, but shouldn't it be cname:foo
instead of cname=foo
?
src/parser.ts
Outdated
@@ -127,6 +133,7 @@ class SdpGrammar extends Grammar { | |||
this.addParser('a', RidLine.fromSdpLine); | |||
this.addParser('a', CandidateLine.fromSdpLine); | |||
this.addParser('a', SimulcastLine.fromSdpLine); | |||
this.addParser('a', RidLine.fromSdpLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to put something else here instead of RidLine
again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, yep. Thanks.
|
||
ssrcs: number[]; | ||
|
||
private static regex = new RegExp(`^ssrc-group:(SIM|FID|FEC) ((?:${NUM}${SP}*)+)`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the spec allows for a SSRC group line with zero SSRC IDs which would fail this regex. Not sure if this is something we should consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch...no idea what use that is :) I played with this a bit and found it quite annoying to build into the regex. Considering we support an explicit set of semantics at this point, and none of those support an empty list, I think I'll leave it as-is for now.
This PR adds classes and support for parsing a:ssrc and a:ssrc-group lines.
While implementing this I ran into the issue that the
TOKEN
helper was too permissive, and noticed that there was a strict definition for a token referred to in the RFC. Unfortunately, the existing uses ofTOKEN
were a mixture of 'I only want "SDP tokens"' and "I want any non-whitespace character". I think what we need is to probably split this into:TOKEN
: 'token' as defined by the RFCANY_NON_WS
: any non-whitespac characterand then go through and fix all the uses. I may try and do this soon, and put up another PR before merging this one.