[Home]

Summary:ASTERISK-14758: [patch] buggy output in "sip show channelstats"
Reporter:klaus3000 (klaus3000)Labels:
Date Opened:2009-09-02 12:09:17Date Closed:2010-01-13 05:37:58.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) asterisk-1.6.2-beta4-sipshowchannelstats-patch.txt
( 1) asterisk-1.6.2-beta4-sipshowchannelstats-patch-0.2.txt
( 2) asterisk-initialize-stats-1.4.txt
( 3) asterisk-initialize-stats-1.6.0.txt
( 4) asterisk-initialize-stats-sip-show-channelstats-1.6.1.txt
( 5) asterisk-initialize-stats-sip-show-channelstats-1.6.2.txt
( 6) asterisk-initialize-stats-sip-show-channelstats-trunk.txt
( 7) asterisk-sip-show-channelstats-1.6.1.txt
( 8) asterisk-sip-show-channelstats-1.6.2.txt
( 9) asterisk-sip-show-channelstats-trunk.txt
Description:Hi!

"sip show channelstats" output is wrong
- the packetloss in % is always 0 due to integer division instead of float divison
- the local measured jitter is reported in as rxjitter (which in in seconds) * 100, converted to int. Multiply with 100 makes no sense - I supsect a type and it should be 1000 as all other jittervalues (max/min...) are also multiplyed with 1000.

****** ADDITIONAL INFORMATION ******

Maybe it would be even better to display the jitter not as integer, but as float, as values are often below 1ms and thus reported as 0.
Comments:By: klaus3000 (klaus3000) 2009-09-02 12:25:03

Hi! I have updated the patch as the received loss in % were calculated wrong.

btw: the received loss will always be a bit incorrect as the "loss" value will only be updated during sending RTCP packets.
By: Olle Johansson (oej) 2009-09-03 07:14:21

Cool. Thanks!
By: Olle Johansson (oej) 2009-09-03 07:19:01

Can you test trunk as well? The stats part has changed quiet a lot there.
By: Digium Subversion (svnbot) 2009-09-03 07:25:16

Repository: asterisk
Revision: 215887

U   branches/1.6.2/channels/chan_sip.c
U   branches/1.6.2/main/rtp.c

------------------------------------------------------------------------
r215887 | oej | 2009-09-03 07:25:16 -0500 (Thu, 03 Sep 2009) | 10 lines

Fix bad reports in "sip show channelstats".

Not directly mergeable in svn trunk, needs more tests, therefore committed directly to 1.6.2.
(closes issue ASTERISK-14758)
Reported by: klaus3000
Patches:
     asterisk-1.6.2-beta4-sipshowchannelstats-patch-0.2.txt uploaded by klaus3000 (license 65)
Tested by: klaus3000, oej


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=215887
By: Olle Johansson (oej) 2009-09-03 07:27:02

Re-opened by request of myself, since trunk is still an open issue.
By: Olle Johansson (oej) 2009-09-03 07:29:34

Updated 1.4 branch too.
By: Olle Johansson (oej) 2009-09-03 09:47:56

I guess trunk depends on the outcome of issue 15807
By: klaus3000 (klaus3000) 2009-09-03 10:20:07

probably yes. There are some more strange things in the channel-stats ...
By: klaus3000 (klaus3000) 2009-09-04 11:29:42

I just found out that this whole stuff should also check for "division by zero"!!!

And further the RTCP structure is not intialized - thus before the first RTCP is received, the statistics are wrong.
By: Olle Johansson (oej) 2009-09-04 11:37:04

Also, if there's a p2p rtp bridge there's nothing there at all.

Ok - will you produce a patch for the division by zero?

The RTCP initialization could be triggered at first check if not before. I see no reason to not initialize them.
By: Olle Johansson (oej) 2009-09-04 11:38:02

Thank you for all your work in fixing RTCP.

At some point I would love to see if we can go as far as to produce some MOS for each call. That would be awsome.

Have a nice weekend, Klaus!
By: klaus3000 (klaus3000) 2009-09-04 15:53:09

yes, I am working on a patch.

Why not just memset(rtp->rtcp,0,sizeof(rtp->rtcp)) when the RTCP "session" is allocated?

p2p: How is p2p-bridge implemented - is p2p actually handled by the RTP stack or is it just a "UDP forwarder"?

MoS: O yes :-) PESQ: Reference code is in ITU P.862 - but there are license issues. I thought of sending a reference stream, Record(), and then compare with the reference. I just have not figured out yet, how PESQ handles delay.
By: Olle Johansson (oej) 2009-09-05 01:27:53

The p2p bridge is an optimized bridge that works like the good ol' RTPproxy more or less. I don't know all the details, I think it terminates RTP streams, and forwards on RTP layer more than UDP layer, but never parses the content of either RTP or RTCP. It for situations where we can't use the remote RTP functionality (directmedia/reinvite stuff) because of NAT, but have no requirements to actually be involved in the content (no recordings or such).

Writing this I don't know whether that bridge can still parse DTMF. But that's a separate issue.

Yes, the memset sounds fine, I never checked code on how complicated "initializing rtcp" was, but if a memset is all it takes, then we'll move it.

It's always easier to do things in a series of small patches, so I can merge the memset a.s.a.p so we're done with that. As soon as we have a patch. I see it as a bug, so we can merge it from 1.4 and up.
By: klaus3000 (klaus3000) 2009-09-07 04:30:29

I changed the outputformat to:
gucci*CLI> sip show channelstats
Peer             Call ID      Duration Recv: Pack  Lost       (     %) Jitter Send: Pack  Lost       (     %) Jitter
88.198.53.113    1eac1e7d-dc  00:00:01 0000000072  0000000000 ( 0.00%) 000000 0000000071  0000000000 ( 0.00%) 000000

So, packetloss in % is now decimal, calculated correctly, and checked against divsion-by-zero, and rtp/rtcp structure is initialized with memset(0) to have all variables set to 0.

The jitter is still wrong, but the bug is not in chan_sip and will be a separate patch.

@oej:
> Updated 1.4 branch too.

Why/how? I could not find channelstats this in 1.4 branch.


I added patches for 1.4-trunk.
By: Olle Johansson (oej) 2009-09-07 05:06:56

I have a separate branch for channelstats in 1.4 :: team/oej/sipchanstats
By: Olle Johansson (oej) 2009-09-07 05:58:49

I wonder about those memsets()

The ast_calloc should be initialized. It's a wrapper around calloc. The man page says
"The calloc() function contiguously allocates enough space for count objects that are size
    bytes of memory each and returns a pointer to the allocated memory.  The allocated memory
    is filled with bytes of value zero.
"
So it doesn't really make sense to call memset after the ast_calloc. Wonder what's going on here, since I belive you believe it did solve an issue?
By: klaus3000 (klaus3000) 2009-09-07 07:21:15

Yes, I had an issue where rxloss was uninitialized before the first RTCP packet was received. I will review it.
By: klaus3000 (klaus3000) 2009-09-09 06:14:24

Indeed, you are right. memset did not solved the problem. The bug is somewhere else - in the parser of the received RTCP packets.

So, to fix the chan_sip issue: apply the new patches for 1.6.1, 1.6.2 and trunk. 1.4 and 1.6.0 do not need patches as they do not have "sip show channelstats".

Then close this bug - I opened a new one for the RTCP problem (ASTERISK-14801).


By: Olle Johansson (oej) 2009-09-10 07:10:21

Ok, will take care of this in a while. Thanks for your help!
By: klaus3000 (klaus3000) 2009-10-30 05:09:36

ping
By: Olle Johansson (oej) 2010-01-13 03:53:34.000-0600

Merging this patch in my "sipchanstats" branch for 1.4
By: Digium Subversion (svnbot) 2010-01-13 04:24:25.000-0600

Repository: asterisk
Revision: 239663

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r239663 | oej | 2010-01-13 04:24:24 -0600 (Wed, 13 Jan 2010) | 11 lines

SIP Show channelstats fix - use float division to show proper stats

(closes issue ASTERISK-14758)
Reported by: klaus3000
Patches:
     asterisk-sip-show-channelstats-trunk.txt uploaded by klaus3000 (license 65)
Tested by: klaus3000, oej

This patch is for trunk only and will be blocked in 1.6.2


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=239663
By: Digium Subversion (svnbot) 2010-01-13 04:36:08.000-0600

Repository: asterisk
Revision: 239664

_U  branches/1.6.2/

------------------------------------------------------------------------
r239664 | oej | 2010-01-13 04:36:08 -0600 (Wed, 13 Jan 2010) | 18 lines

Blocked revisions 239663 via svnmerge

........
r239663 | oej | 2010-01-13 11:24:23 +0100 (Ons, 13 Jan 2010) | 11 lines

SIP Show channelstats fix - use float division to show proper stats

(closes issue ASTERISK-14758)
Reported by: klaus3000
Patches:
     asterisk-sip-show-channelstats-trunk.txt uploaded by klaus3000 (license 65)
Tested by: klaus3000, oej

This patch is for trunk only and will be blocked in 1.6.2


........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=239664
By: Digium Subversion (svnbot) 2010-01-13 05:25:56.000-0600

Repository: asterisk
Revision: 239703

U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r239703 | oej | 2010-01-13 05:25:56 -0600 (Wed, 13 Jan 2010) | 14 lines

Show proper stats in "sip show channelstats"

SIP show channelstats show current RTCP statistics for calls - if we have it. Calls bridged
in RTP p2p bridge doesn't have any statistics.  In calls where the remote end doesn't send
RTCP or we can't receive it due to NAT, there's no reliable data as well.

Thanks, Klaus, for the patch. Sorry for the delay.
(closes issue ASTERISK-14758)
Reported by: klaus3000
Patches:
     asterisk-sip-show-channelstats-1.6.2.txt uploaded by klaus3000 (license 65)
Tested by: klaus3000, oej


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=239703
By: Digium Subversion (svnbot) 2010-01-13 05:37:57.000-0600

Repository: asterisk
Revision: 239706

U   branches/1.6.1/channels/chan_sip.c

------------------------------------------------------------------------
r239706 | oej | 2010-01-13 05:37:57 -0600 (Wed, 13 Jan 2010) | 8 lines

Show proper stats in "sip show channelstats"

(closes issue ASTERISK-14758)
Reported by: klaus3000
Patches:
     asterisk-sip-show-channelstats-1.6.1.txt uploaded by klaus3000 (license 65)
Tested by: klaus3000, oej

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=239706