RTP processing: Modified RTP sequence verification

The patch changes:
- Removed first_packet_mac_addr staff. It was commented out many years ago...
- Removed delta_timestamp item. Not used.
- Sequence verification takes into account timestamp therefore it is able to detect delayed packets more clearly. As consequence of it, #16330 is solved.
- If packet is delayed, it is not used in calculation of diff/jitter/skew. It just mess output. As consequence of it, #16330 is solved.

I checked output with many RTP streams and looks well. But I have no
sample with wrapped timestamp and I have just a few samples with
missing/reordered packets. Nevertheless all are calculated same way as
before and #16330 is solved too.

3 files changed, 103 insertions, 85 deletions

diff --git a/ui/tap-rtp-analysis.c b/ui/tap-rtp-analysis.c
index ee7158eb9e..f1e2c3a4f9 100644
--- a/ui/tap-rtp-analysis.c
+++ b/ui/tap-rtp-analysis.c
@@ -156,6 +156,8 @@ get_dyn_pt_clock_rate(const gchar *payload_type_str)
     return 0;
 }
 
+#define TIMESTAMP_DIFFERENCE(v1,v2) ((gint64)v2-(gint64)v1)
+
 /****************************************************************************/
 void
 rtppacket_analyse(tap_rtp_stat_t *statinfo,
@@ -170,6 +172,7 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
     double expected_time;
     double absskew;
     guint32 clock_rate;
+    gboolean in_time_sequence;
 
     /* Store the current time */
     current_time = nstime_to_msec(&pinfo->rel_ts);
@@ -177,9 +180,6 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
     /*  Is this the first packet we got in this direction? */
     if (statinfo->first_packet) {
         /* Save the MAC address of the first RTP frame */
-        if( pinfo->dl_src.type == AT_ETHER){
-            copy_address(&(statinfo->first_packet_mac_addr), &(pinfo->dl_src));
-        }
         statinfo->start_seq_nr = rtpinfo->info_seq_num;
         statinfo->stop_seq_nr = rtpinfo->info_seq_num;
         statinfo->seq_num = rtpinfo->info_seq_num;
@@ -220,23 +220,10 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
 
     /* Reset flags */
     statinfo->flags = 0;
-#if 0
-    /*According to bug https://gitlab.com/wireshark/wireshark/-/issues/11478
-     * this code causes problems. A better solution is needed if there is need for the functionality */
-    /* Try to detect duplicated packets due to mirroring/span ports by comparing src MAC addresses.
-     * Check for duplicates (src mac differs from first_packet_mac_addr) */
-    */
-        if( pinfo->dl_src.type == AT_ETHER){
-            if(!addresses_equal(&(statinfo->first_packet_mac_addr), &(pinfo->dl_src))){
-                statinfo->flags |= STAT_FLAG_DUP_PKT;
-                statinfo->delta = current_time-(statinfo->time);
-                return;
-            }
-        }
-#endif
+
     /* When calculating expected rtp packets the seq number can wrap around
      * so we have to count the number of cycles
-     * Variable cycles counts the wraps around in forwarding connection and
+     * Variable seq_cycles counts the wraps around in forwarding connection and
      * under is flag that indicates where we are
      *
      * XXX How to determine number of cycles with all possible lost, late
@@ -248,11 +235,32 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
      * where below code won't work correctly - statistic may be wrong then.
      */
 
+    /* Check if time sequence of packets is in order. We check whether
+     * timestamp difference is below 1/2 of timestamp range (hours or days).
+     * Packets can be in pure sequence or sequence can be wrapped arround
+     * 0xFFFFFFFF.
+     */
+    if ((statinfo->first_timestamp <= rtpinfo->info_timestamp) &&
+        TIMESTAMP_DIFFERENCE(statinfo->first_timestamp, rtpinfo->info_timestamp) < 0x80000000) {
+        // Normal timestamp sequence
+        in_time_sequence = TRUE;
+    } else if ((statinfo->first_timestamp > rtpinfo->info_timestamp) &&
+        (TIMESTAMP_DIFFERENCE(0x00000000,statinfo->first_timestamp) + TIMESTAMP_DIFFERENCE(rtpinfo->info_timestamp, 0xFFFFFFFF)) < 0x80000000) {
+        // Normal timestamp sequence with wraparound
+        in_time_sequence = TRUE;
+    } else {
+        // New packet is not in sequence (is in past)
+        in_time_sequence = FALSE;
+        statinfo->flags |= STAT_FLAG_WRONG_TIMESTAMP;
+    }
+
     /* So if the current sequence number is less than the start one
      * we assume, that there is another cycle running
      */
-    if ((rtpinfo->info_seq_num < statinfo->start_seq_nr) && (statinfo->under == FALSE)){
-        statinfo->cycles++;
+    if ((rtpinfo->info_seq_num < statinfo->start_seq_nr) &&
+        in_time_sequence &&
+        (statinfo->under == FALSE)) {
+        statinfo->seq_cycles++;
         statinfo->under = TRUE;
     }
     /* what if the start seq nr was 0? Then the above condition will never
@@ -260,12 +268,15 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
      * if one of the packets with seq nr 0 or 65535 would be lost or late
      */
     else if ((rtpinfo->info_seq_num == 0) && (statinfo->stop_seq_nr == 65535) &&
-            (statinfo->under == FALSE)) {
-        statinfo->cycles++;
+             in_time_sequence &&
+             (statinfo->under == FALSE)) {
+        statinfo->seq_cycles++;
         statinfo->under = TRUE;
     }
     /* the whole round is over, so reset the flag */
-    else if ((rtpinfo->info_seq_num > statinfo->start_seq_nr) && (statinfo->under != FALSE)) {
+    else if ((rtpinfo->info_seq_num > statinfo->start_seq_nr) &&
+             in_time_sequence &&
+             (statinfo->under != FALSE)) {
         statinfo->under = FALSE;
     }
 
@@ -276,17 +287,25 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
     /* If the current seq number equals the last one or if we are here for
      * the first time, then it is ok, we just store the current one as the last one
      */
-    if ( (statinfo->seq_num+1 == rtpinfo->info_seq_num) || (statinfo->flags & STAT_FLAG_FIRST) )
+    if ( in_time_sequence &&
+         ( (statinfo->seq_num+1 == rtpinfo->info_seq_num) || (statinfo->flags & STAT_FLAG_FIRST) )
+       ) {
         statinfo->seq_num = rtpinfo->info_seq_num;
+    }
     /* If the first one is 65535 we wrap */
-    else if ( (statinfo->seq_num == 65535) && (rtpinfo->info_seq_num == 0) )
+    else if ( in_time_sequence &&
+              ( (statinfo->seq_num == 65535) && (rtpinfo->info_seq_num == 0) )
+            ) {
         statinfo->seq_num = rtpinfo->info_seq_num;
+    }
     /* Lost packets. If the prev seq is enormously larger than the cur seq
      * we assume that instead of being massively late we lost the packet(s)
      * that would have indicated the sequence number wrapping. An imprecise
      * heuristic at best, but it seems to work well enough.
      * https://gitlab.com/wireshark/wireshark/-/issues/5958 */
-    else if (statinfo->seq_num+1 < rtpinfo->info_seq_num || statinfo->seq_num - rtpinfo->info_seq_num > 0xFF00) {
+    else if ( in_time_sequence &&
+              (statinfo->seq_num+1 < rtpinfo->info_seq_num || statinfo->seq_num - rtpinfo->info_seq_num > 0xFF00)
+            ) {
         statinfo->seq_num = rtpinfo->info_seq_num;
         statinfo->sequence++;
         statinfo->flags |= STAT_FLAG_WRONG_SEQ;
@@ -336,56 +355,60 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
         }
     }
 
-    /* Handle wraparound ? */
-    arrivaltime = current_time - statinfo->start_time;
+    /* diff/jitter/skew calculations are done just for in sequence packets */
+    if ( in_time_sequence ) {
 
-    nominaltime = (double)(guint32_wraparound_diff(rtpinfo->info_timestamp, statinfo->first_timestamp));
+        /* Handle wraparound ? */
+        arrivaltime = current_time - statinfo->start_time;
 
-    /* Can only analyze defined sampling rates */
-    if (clock_rate != 0) {
-        statinfo->clock_rate = clock_rate;
-        /* Convert from sampling clock to ms */
-        nominaltime = nominaltime /(clock_rate/1000);
+        nominaltime = (double)(guint32_wraparound_diff(rtpinfo->info_timestamp, statinfo->first_timestamp));
 
-        /* Calculate the current jitter(in ms) */
-        if (!statinfo->first_packet) {
-            expected_time = statinfo->time + (nominaltime - statinfo->lastnominaltime);
-            current_diff = fabs(current_time - expected_time);
-            current_jitter = (15 * statinfo->jitter + current_diff) / 16;
+        /* Can only analyze defined sampling rates */
+        if (clock_rate != 0) {
+            statinfo->clock_rate = clock_rate;
+            /* Convert from sampling clock to ms */
+            nominaltime = nominaltime /(clock_rate/1000);
 
-            statinfo->delta = current_time-(statinfo->time);
-            statinfo->jitter = current_jitter;
-            statinfo->diff = current_diff;
-        }
-        statinfo->lastnominaltime = nominaltime;
-        /* Calculate skew, i.e. absolute jitter that also catches clock drift
-         * Skew is positive if TS (nominal) is too fast
-         */
-        statinfo->skew    = nominaltime - arrivaltime;
-        absskew = fabs(statinfo->skew);
-        if(absskew > fabs(statinfo->max_skew)) {
-            statinfo->max_skew = statinfo->skew;
-        }
-        /* Gather data for calculation of average, minimum and maximum framerate based on timestamp */
-#if 0
-        if (numPackets > 0 && (!hardPayloadType || !alternatePayloadType)) {
-            /* Skip first packet and possibly alternate payload type packets */
-            double dt;
-            dt     = nominaltime - statinfo->lastnominaltime;
-            sumdt += 1.0 * dt;
-            numdt += (dt != 0 ? 1 : 0);
-            mindt  = (dt < mindt ? dt : mindt);
-            maxdt  = (dt > maxdt ? dt : maxdt);
-        }
-#endif
-        /* Gather data for calculation of skew least square */
-        statinfo->sumt   += 1.0 * arrivaltime;
-        statinfo->sumTS  += 1.0 * nominaltime;
-        statinfo->sumt2  += 1.0 * arrivaltime * arrivaltime;
-        statinfo->sumtTS += 1.0 * arrivaltime * nominaltime;
-    } else {
-        if (!statinfo->first_packet) {
-            statinfo->delta = current_time-(statinfo->time);
+            /* Calculate the current jitter(in ms) */
+            if (!statinfo->first_packet) {
+                expected_time = statinfo->time + (nominaltime - statinfo->lastnominaltime);
+                current_diff = fabs(current_time - expected_time);
+                current_jitter = (15 * statinfo->jitter + current_diff) / 16;
+
+                statinfo->delta = current_time-(statinfo->time);
+                statinfo->jitter = current_jitter;
+                statinfo->diff = current_diff;
+            }
+            statinfo->lastnominaltime = nominaltime;
+            /* Calculate skew, i.e. absolute jitter that also catches clock drift
+             * Skew is positive if TS (nominal) is too fast
+             */
+            statinfo->skew = nominaltime - arrivaltime;
+            absskew = fabs(statinfo->skew);
+            if (absskew > fabs(statinfo->max_skew)) {
+                statinfo->max_skew = statinfo->skew;
+            }
+            /* Gather data for calculation of average, minimum and maximum framerate based on timestamp */
+    #if 0
+            if (numPackets > 0 && (!hardPayloadType || !alternatePayloadType)) {
+                /* Skip first packet and possibly alternate payload type packets */
+                double dt;
+                dt     = nominaltime - statinfo->lastnominaltime;
+                sumdt += 1.0 * dt;
+                numdt += (dt != 0 ? 1 : 0);
+                mindt  = (dt < mindt ? dt : mindt);
+                maxdt  = (dt > maxdt ? dt : maxdt);
+            }
+    #endif
+            /* Gather data for calculation of skew least square */
+            statinfo->sumt   += 1.0 * arrivaltime;
+            statinfo->sumTS  += 1.0 * nominaltime;
+            statinfo->sumt2  += 1.0 * arrivaltime * arrivaltime;
+            statinfo->sumtTS += 1.0 * arrivaltime * nominaltime;
+        } else {
+            if (!statinfo->first_packet) {
+                statinfo->delta = current_time-(statinfo->time);
+            }
         }
     }
 
@@ -414,19 +437,11 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
     if (statinfo->bw_index == BUFF_BW) statinfo->bw_index = 0;
 
 
-    /* Used by GTK code only */
-    statinfo->delta_timestamp = guint32_wraparound_diff(rtpinfo->info_timestamp, statinfo->timestamp);
-
     /* Is it a packet with the mark bit set? */
     if (rtpinfo->info_marker_set) {
         statinfo->flags |= STAT_FLAG_MARKER;
     }
 
-    /* Difference can be negative. We don't expect difference bigger than 31 bits. Difference don't care about wrap around. */
-    gint32 tsdelta=rtpinfo->info_timestamp - statinfo->timestamp;
-    if (tsdelta < 0) {
-        statinfo->flags |= STAT_FLAG_WRONG_TIMESTAMP;
-    }
     /* Is it a regular packet? */
     if (!(statinfo->flags & STAT_FLAG_FIRST)
             && !(statinfo->flags & STAT_FLAG_MARKER)
@@ -460,7 +475,12 @@ rtppacket_analyse(tap_rtp_stat_t *statinfo,
         statinfo->reg_pt = statinfo->pt;
     }
 
-    statinfo->time = current_time;
+    if (in_time_sequence) {
+        /* We remember last time just for in_time sequence packets
+         * therefore diff calculations are correct for it
+         */
+        statinfo->time = current_time;
+    }
     statinfo->timestamp = rtpinfo->info_timestamp;
     statinfo->stop_seq_nr = rtpinfo->info_seq_num;
     statinfo->total_nr++;
diff --git a/ui/tap-rtp-analysis.h b/ui/tap-rtp-analysis.h
index 7ed83b980f..99ebb72d15 100644
--- a/ui/tap-rtp-analysis.h
+++ b/ui/tap-rtp-analysis.h
@@ -45,12 +45,10 @@ typedef struct _tap_rtp_stat_t {
     /* all of the following fields will be initialized after
      * rtppacket_analyse has been called
      */
-    address         first_packet_mac_addr; /**< MAC address of first packet, used to determine duplicates due to mirroring */
     guint32         flags;      /* see STAT_FLAG-defines below */
     guint16         seq_num;
     guint32         timestamp;
     guint32         first_timestamp;
-    guint32         delta_timestamp;
     double          bandwidth;
     bw_history_item bw_history[BUFF_BW];
     guint16         bw_start_index;
@@ -78,7 +76,7 @@ typedef struct _tap_rtp_stat_t {
     guint32         total_nr;
     guint32         sequence;
     gboolean        under;
-    gint            cycles;
+    gint            seq_cycles;
     guint16         pt;
     int             reg_pt;
     guint32         first_packet_num;
diff --git a/ui/tap-rtp-common.c b/ui/tap-rtp-common.c
index 2efa2dcaa2..baa3a6c9cc 100644
--- a/ui/tap-rtp-common.c
+++ b/ui/tap-rtp-common.c
@@ -487,7 +487,7 @@ void rtpstream_info_calculate(const rtpstream_info_t *strinfo, rtpstream_info_ca
 
         calc->packet_count = strinfo->packet_count;
         /* packet count, lost packets */
-        calc->packet_expected = (strinfo->rtp_stats.stop_seq_nr + strinfo->rtp_stats.cycles*65536)
+        calc->packet_expected = (strinfo->rtp_stats.stop_seq_nr + strinfo->rtp_stats.seq_cycles*0x10000)
             - strinfo->rtp_stats.start_seq_nr + 1;
         calc->total_nr = strinfo->rtp_stats.total_nr;
         calc->lost_num = calc->packet_expected - strinfo->rtp_stats.total_nr;