SIP plugin: Save codec name on update request

[ihusejnovic]

Hi,

I noticed that recording is not working for stream added with "update" request because codec name was missing.
Here is the fix.

[lminiero]

I'm not sure that's a proper fix. The audio and video payload types are updated when we get the remote SDP (see lines 5798 and 5800, which seem not to be constrained by update). As such, it looks like you're simply starting the recording too soon, before the other party accepts the new stream? (which might be rejected).

[lminiero]

Unless of course the browser is the one answering here, and so the payload types we took note of when processing the remote offer are not the same as the ones the browser answering eventually chose. Is this your case?

[ihusejnovic]

Unless of course the browser is the one answering here, and so the payload types we took note of when processing the remote offer are not the same as the ones the browser answering eventually chose. Is this your case?

Yes, this is the case when the browser is sending an answer on reinvite. But, the payload types (and payload/codec names) are not changed, they are missing for streams added in reinvite. In this PR I have added it in "update" request as it's done for "accept" request (see lines 3648 and 3652). Also, I have added additional check if they are already set to not override it.

[lminiero]

But, the payload types (and payload/codec names) are not changed, they are missing for streams added in reinvite.

But that's what puzzles me: if the browser that is answering is confirming the new choices made by the offerer, then we should have those payload types already, as they're updated here:
https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_sip.c#L5798
https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_sip.c#L5800

Since you have a testbed to replicate this scenario, could you check what those lines are doing, if anything, and why they're not updating payload types as needed? I'd say that, if there's an issue there, this should be fixed too.

[ihusejnovic]

But, the payload types (and payload/codec names) are not changed, they are missing for streams added in reinvite.

But that's what puzzles me: if the browser that is answering is confirming the new choices made by the offerer, then we should have those payload types already, as they're updated here:
https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_sip.c#L5798
https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_sip.c#L5800

Since you have a testbed to replicate this scenario, could you check what those lines are doing, if anything, and why they're not updating payload types as needed? I'd say that, if there's an issue there, this should be fixed too.

That's not where they are updated since the offerer is sending an offer, not an answer. (see line https://github.com/meetecho/janus-gateway/blob/master/plugins/janus_sip.c#L5791).

Payload types are updated only when you are sending or receiving an answer.
In this case, payload types are updated in the function janus_sip_sdp_manipulate when we get an answer from the browser, but payload names are not.

[lminiero]

That's not where they are updated since the offerer is sending an offer, not an answer.

Oh you're right! Maybe I should start drinking coffee in the morning again... 🤭
Then your fix is indeed needed and I'll merge it right away, thanks 👍 I'm pretty sure the same fix will be needed in the NoSIP plugin too, I'll take care of that myself after this is merged.

[lminiero]

Looks like the NoSIP plugin isn't affected, so no additional commit is needed.

[ihusejnovic]

Thank you :)