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

sdp-testcase: reproducer for pt + profile-level-id parsing bug #2543

Closed
wants to merge 1 commit into from

Conversation

tmatth
Copy link
Contributor

@tmatth tmatth commented Feb 3, 2021

To run, simply build janus and:
./sdp-testcase

DO NOT MERGE

To run, simply build janus and:
./sdp-testcase
@lminiero lminiero marked this pull request as ready for review February 3, 2021 11:19
@lminiero
Copy link
Member

lminiero commented Feb 3, 2021

Whoops, sorry, I pressed the wrong button and marked it ready to review. How to make it a draft again?

@lminiero lminiero marked this pull request as draft February 3, 2021 11:20
@lminiero
Copy link
Member

lminiero commented Feb 3, 2021

Done, sorry for the noise 😄

@lminiero
Copy link
Member

lminiero commented Feb 3, 2021

The issue is that there are multiple rtpmap attributes for the same codec, e.g.:

a=rtpmap:102 H264/90000
a=rtpmap:127 H264/90000
a=rtpmap:125 H264/90000
a=rtpmap:108 H264/90000

Since we parse the attributes in order, any time we find a matching rtpmap attribute we update the pt property: when looking for a H.264 profile, the last value of pt is 108, so that's the payload type we check when looking for fmtp attributes. That's also why you sometimes get the wrong pt back (there's indeed such a profile for that payload type) or -1 (no such profile for that specific payload type).

I'll have to think about the best way to address this. Rather than only keeping track of a single pt integer as we do now, we may have to start using a list of some sort (e.g., a GList of GINT_TO_POINTER elements) in janus_sdp_get_codec_pt_full, so that if, for instance, H.264 has 4 different payload types, we keep track of them all, and when we have to check fmtp, we accept any of those for our validation process. An alternative may be a GHashTable but that sounds heavier. I'll play with this a bit and keep you posted: in case I manage to get something that "fixes" your testcase, I'll prepare a PR that can be used to ensure it doesn't mess with anything else that was still working instead.

@lminiero
Copy link
Member

lminiero commented Feb 3, 2021

Closing as I have your tester locally, and this doesn't really need to be merged. Thanks!

@lminiero lminiero closed this Feb 3, 2021
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

Successfully merging this pull request may close these issues.

2 participants