Repository navigation
Report locally measured loss and jitter per ingress SSRC without RTCP - #2201
darwvin-dev wants to merge 1 commit into
Conversation
| ent->stats->lost_bits = -1; // kernel tracker, so it doesn't count its first 32 packets as lost | ||
| ent->lost_bits = -1; |
There was a problem hiding this comment.
Should probably eliminate ent->lost_bits altogether and just use the shared data from both kernel and user space
| // sequence-gap based loss, counted in userspace and in the kernel for packets each one handled | ||
| uint32_t ssrc_packets_lost(struct ssrc_entry_call *se) { | ||
| return se->packets_lost + atomic_get_na(&se->stats->total_lost); | ||
| } | ||
|
|
||
| // RFC 3550 interarrival jitter (scaled by 16, in RTP timestamp units). The kernel keeps | ||
| // its own estimate if it measures RTP stats, which is the current one once it has taken | ||
| // over forwarding this SSRC. | ||
| uint32_t ssrc_jitter(struct ssrc_entry_call *se) { | ||
| uint32_t kernel_jitter = atomic_get_na(&se->stats->jitter); | ||
| return kernel_jitter ? kernel_jitter : se->jitter; | ||
| } | ||
|
|
There was a problem hiding this comment.
Similar comment here, would make more sense to just have a single value used by both code paths, no?
| // Mirrors kernel-module/nft_rtpengine.c:rtp_stats(). During offload handover, | ||
| // the kernel and userspace must not update the same SSRC simultaneously. | ||
| void ssrc_track_rtp(struct ssrc_entry_call *se, uint16_t seq, uint32_t timestamp) { | ||
| LOCK(&se->h.lock); |
There was a problem hiding this comment.
I'm not sure that's gonna fly, because the loss calculation is non-atomic and relies on the mutex/lock, which is handled differently between kernel and user space. So kernel and user space would race each other. It would require a lock that can be used from both contexts.
I'm ok with leaving it as it is for now, because that would probably be a larger rework.
Please don't keep adding new commits and please avoid merge commits. Instead rebase everything on the latest master if needed.
Expose locally measured RTP loss and jitter in NG query/delete output even without RTCP, merge kernel-measured per-SSRC loss/jitter from shm into userspace consumers (NG, MQTT, RTCP RRs, call summary), start the kernel loss tracker at -1 like userspace, and skip jitter metrics when no RTP clock rate is known. Closes sipwise#934
42273df to
15e630d
Compare
|
Squashed into a single commit rebased on master, and dropped the shared kernel/userspace tracker given the locking race you pointed out. |
2 participants
Closes #934
Problem
#934 asks for per-leg QoS stats (loss, interarrival jitter) for the RTP that
rtpengine receives, even when the endpoints send no RTCP.
When the issue was filed, rtpengine did not track sequence numbers or timing in
passthrough. That has changed since:
the passthrough handlers (
codec_calc_lost()/codec_calc_jitter()) and inthe transcoding sequencer, and
struct ssrc_stats(total_lost,jitter) when it measures RTP stats.Three gaps remained:
query/deleteoutput hid the loss value without RTCP. It onlyprinted
cumulative lossfor an SSRC that also had MOS stats blocks, whichcome from RTCP RRs. Jitter measured from RTP was never printed.
af96fc7 ("track SSRC stats in kernel via shm") the kernel's per-SSRC
total_lostandjitterare written to shm, but nothing read them. Beforethat commit they were merged into
ssrc_entry_call. So for kernel-forwardedstreams, MQTT
lost, NG and self-generated RTCP RRs only covered packetsseen in userspace.
ssrc_stats.lost_bitsstarted at 0, while the userspace trackerstarts at
-1. Userspace already advancesext_seq, so on kernel takeoverthe kernel counted up to 32 received packets as lost (in the per-SSRC
counter and in the interface/global
packets_loststats).measure-rtp,ssrc_collect_metrics()converted the running jitterestimator to ms in place on every call timer run. That corrupted the
estimator, which is kept in RTP timestamp units and also used for generated
RTCP RRs.
Changes
ssrc_packets_lost()/ssrc_jitter()helpers combine the userspace andkernel measurements. Loss is the sum: each side counts the packets it
handled. Jitter is the kernel's estimate if it has one, otherwise the
userspace one. They are used for NG output, MQTT
lost, and RTCP RRgeneration/substitution.
SSRCdict)now always contains
cumulative lossandjitter(ms, omitted if the clockrate is unknown), whether or not RTCP was received. Egress entries are
unchanged. Their only data source would be RTCP from the far end, which the
existing MOS fields already cover.
lost_bitsto-1(userspace only, no kernel codechange).
ssrc_collect_metrics()converts a copy of the jitter value instead ofoverwriting the estimator.
get_rtp_payload_type()takes aconst struct codec_store *, so that theconst
call_mediainng_stats_media()can be used.ingress SSRCs/egress SSRCsare now described indocs/ng_control_protocol.md.The existing
cumulative losskey keeps its meaning (local sequence-gap loss,since 90f4f8e) and its position, so consumers that read it alongside MOS see no
change.
Testing
debian/control.makebuilds without warnings.t/auto-daemon-tests-ssrc.pl: one RTP packet is dropped out of 41with no RTCP. It checks
cumulative loss == 1,jitter > 0, that there areno MOS keys, the top-level
SSRCdict, and that egress entries have neitherkey. It fails on unmodified master (3 failures) and passes with this change.
make -C t unit-tests: all pass.daemon-testspasses (0not ok), includingdaemon-tests-ssrc(117),-rtcp(35),-measure-rtp,-mos-legacy,-mos-fullband,-main(3035),-redis*and-bundle. I also randaemon-tests-kernel-api(343) anddaemon-tests-stats(479) withRTPENGINE_EXTENDED_TESTS=1, and both pass. I did not run the otherextended-only suites (DTX, EVS, audio player and so on); they don't touch
this code.
t/auto-daemon-tests-rtcp.plexpected a jitter of exactly 0 in rtpengine-generated RRs. That 0 came from
the
ssrc_collect_metrics()bug, which divided the estimator by 8 everysecond under
--measure-rtp. The real value for its 3 back-to-back packets isabout 0x10–0x11 timestamp units, so the regex now accepts 1..127.
Limitations / not covered
in the test environment. The kernel code is unchanged: this PR only reads
fields the kernel already maintains, and initialises one shm field.
reti->rtp_statsis set, whichmeans
measure-rtp, RTCP generation, or MQTT. Without one of those, packetsforwarded by the kernel are not included in the values. This is documented.
of 32 or more packets are approximated, and jumps over 256 are treated as a
sequence reset. Small inaccuracies are possible around userspace/kernel
handovers, because each side keeps its own reordering window. This PR does
not change the estimator.
🤖 Generated with Claude Code