[kamailio/kamailio] rtpengine: improve compatibility of rtpengine per call leg stats parsing (PR #4233)
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [*] Commit message has the format required by CONTRIBUTING guide - [*] Commits are split per component (core, individual modules, libs, utils, ...) - [*] Each component has a single commit (if not, squash them into one commit) - [*] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [*] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [*] PR should be backported to stable branches - [*] Tested changes locally - [*] Related to issue #4232
#### Description not all versions of rtpengine sends a key SSRC per stream. For those that do not the same information can be found in ingress SSRCs. Add logic to check for the SSRC value there if the SSRC key is not present. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/4233
-- Commit Summary --
* rtpengine: improve compatibility of rtpengine per call leg stats parsing
-- File Changes --
M src/modules/rtpengine/rtpengine.c (19)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/4233.patch https://github.com/kamailio/kamailio/pull/4233.diff
@tsearle pushed 1 commit.
c3139ee38367010bf7d93d59218ae08493391156 rtpengine: improve compatibility of rtpengine per call leg stats parsing
@rfuchs commented on this pull request.
@@ -4545,9 +4546,21 @@ static void parse_call_stats_1(struct minmax_mos_label_stats *mmls,
(char *)stream->child->iov[1].iov_base); LM_DBG("rtpengine: XXX stream child val type %i\n", stream->child->sibling->type); - if((ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1)) - == -1) - continue; + ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1); + if(ssrc == -1) { + ingress_ssrcs = bencode_dictionary_get_expect( + stream, "ingress SSRCs", BENCODE_LIST); + if(!ingress_ssrcs || !ingress_ssrcs->child) + continue; + LM_DBG("rtpengine: XXX got ingress SSRCs\n");
Is this here intentionally or as a leftover?
@tsearle commented on this pull request.
@@ -4545,9 +4546,21 @@ static void parse_call_stats_1(struct minmax_mos_label_stats *mmls,
(char *)stream->child->iov[1].iov_base); LM_DBG("rtpengine: XXX stream child val type %i\n", stream->child->sibling->type); - if((ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1)) - == -1) - continue; + ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1); + if(ssrc == -1) { + ingress_ssrcs = bencode_dictionary_get_expect( + stream, "ingress SSRCs", BENCODE_LIST); + if(!ingress_ssrcs || !ingress_ssrcs->child) + continue; + LM_DBG("rtpengine: XXX got ingress SSRCs\n");
I'm not sure I fully understand the question... but this is here intentionally to make sure that ingress_ssrcs exists and has at least 1 element.
and the log message to indicate the affirmative
@tsearle commented on this pull request.
@@ -4545,9 +4546,21 @@ static void parse_call_stats_1(struct minmax_mos_label_stats *mmls,
(char *)stream->child->iov[1].iov_base); LM_DBG("rtpengine: XXX stream child val type %i\n", stream->child->sibling->type); - if((ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1)) - == -1) - continue; + ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1); + if(ssrc == -1) { + ingress_ssrcs = bencode_dictionary_get_expect( + stream, "ingress SSRCs", BENCODE_LIST); + if(!ingress_ssrcs || !ingress_ssrcs->child) + continue; + LM_DBG("rtpengine: XXX got ingress SSRCs\n");
so in the case there is no SSRC but an "ingress SSRC" the log looks as follows ``` rtpengine: XXX got ingress SSRCs rtpengine: found SSRC '1778154774' for label 'Bleg_label' ```
@rfuchs commented on this pull request.
@@ -4545,9 +4546,21 @@ static void parse_call_stats_1(struct minmax_mos_label_stats *mmls,
(char *)stream->child->iov[1].iov_base); LM_DBG("rtpengine: XXX stream child val type %i\n", stream->child->sibling->type); - if((ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1)) - == -1) - continue; + ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1); + if(ssrc == -1) { + ingress_ssrcs = bencode_dictionary_get_expect( + stream, "ingress SSRCs", BENCODE_LIST); + if(!ingress_ssrcs || !ingress_ssrcs->child) + continue; + LM_DBG("rtpengine: XXX got ingress SSRCs\n");
Ok, just thought the "XXX" looks like a leftover from some debugging
Merged #4233 into master.
@henningw commented on this pull request.
@@ -4545,9 +4546,21 @@ static void parse_call_stats_1(struct minmax_mos_label_stats *mmls,
(char *)stream->child->iov[1].iov_base); LM_DBG("rtpengine: XXX stream child val type %i\n", stream->child->sibling->type); - if((ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1)) - == -1) - continue; + ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1); + if(ssrc == -1) { + ingress_ssrcs = bencode_dictionary_get_expect( + stream, "ingress SSRCs", BENCODE_LIST); + if(!ingress_ssrcs || !ingress_ssrcs->child) + continue; + LM_DBG("rtpengine: XXX got ingress SSRCs\n");
I also noticed that. Generally we should try to have log messages that are understandable and have some meaning. I don't get the meaning of this XXX in this case. ;-)
@tsearle commented on this pull request.
@@ -4545,9 +4546,21 @@ static void parse_call_stats_1(struct minmax_mos_label_stats *mmls,
(char *)stream->child->iov[1].iov_base); LM_DBG("rtpengine: XXX stream child val type %i\n", stream->child->sibling->type); - if((ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1)) - == -1) - continue; + ssrc = bencode_dictionary_get_integer(stream, "SSRC", -1); + if(ssrc == -1) { + ingress_ssrcs = bencode_dictionary_get_expect( + stream, "ingress SSRCs", BENCODE_LIST); + if(!ingress_ssrcs || !ingress_ssrcs->child) + continue; + LM_DBG("rtpengine: XXX got ingress SSRCs\n");
all the log messages in the method have XXX in them... I was simply matching the existing code
-
Henning Westerholt -
Richard Fuchs
-
tsearle