rzo1 commented on PR #8586:
URL: https://github.com/apache/storm/pull/8586#issuecomment-4385841547
**Question on the `d <= 0` short-circuit in `EwmaGauge.addValue`**
```java
if (d <= 0) {
return;
}
```
Is this skip intentional? Reading [RFC 3550
§A.8](https://datatracker.ietf.org/doc/html/rfc3550#appendix-A.8) (which
supersedes RFC 1889 with the same text):
```c
d = transit - s->transit;
s->transit = transit;
if (d < 0) d = -d;
s->jitter += (1./16.) * ((double)d - s->jitter);
```
The update is unconditional. When `d == 0` it collapses to `J ← J · (1 − α)`
≈ `J · 15/16`, i.e. the EWMA decays toward zero. The RFC explicitly chooses
1/16 for its *"noise reduction ratio while maintaining a reasonable rate of
convergence"* — convergence back toward zero during quiet periods is part of
the spec's intent.
For comparison, the major RTP stacks all apply the update unconditionally:
- **GStreamer** —
[`rtpsource.c:997`](https://github.com/GStreamer/gst-plugins-good/blob/master/gst/rtpmanager/rtpsource.c#L997)
```c
src->stats.jitter += diff - ((src->stats.jitter + 8) >> 4);
```
- **PJSIP** —
[`rtcp.c:434`](https://github.com/pjsip/pjproject/blob/master/pjmedia/src/pjmedia/rtcp.c#L434)
```c
sess->jitter += d - ((sess->jitter + 8) >> 4);
```
- **WebRTC** —
[`receive_statistics_impl.cc:165`](https://chromium.googlesource.com/external/webrtc/+/refs/heads/main/modules/rtp_rtcp/source/receive_statistics_impl.cc#165)
```cpp
int32_t jitter_diff_q4 = (time_diff_samples << 4) - jitter_q4_;
jitter_q4_ += ((jitter_diff_q4 + 8) >> 4);
```
(the only guard here is a `< 450000` anomaly cap, not a `d == 0`
short-circuit)
The test `EwmaGaugeTest.zeroDeviationDecays` appears to lock in the current
behavior, but its display name (*"Zero deviation decays jitter toward zero"*)
suggests the original intent matched the spec while the assertion encodes the
opposite. Worth double-checking which one was meant.
**Suggested fix:** drop the `if (d <= 0) return;` block — the CAS loop below
is already correct for `d == 0`. (Optionally short-circuit the math with
`updatedJitter = currentJitter * (1.0 - alpha)`, but you must still write it
back.)
Was the skip intentional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
