[Bug 772646] rtpjitterbuffer: fix lost-event using dts instead of pts

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=772646

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #337275|none                        |reviewed
             status|                            |

--- Comment #2 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 337275:
 --> (https://bugzilla.gnome.org/review?bug=772646&attachment=337275)

Just some comments for now

::: gst/rtpmanager/gstrtpjitterbuffer.c
@@ +2342,2 @@
     /* calculate expected arrival time of the next seqnum */
+    expected = pts + priv->packet_spacing;

Isn't the arrival time dependant on the dts (aka arrival time at this point)?
While the packet spacing would depend on the pts I guess

@@ +2408,3 @@
 static void
 calculate_expected (GstRtpJitterBuffer * jitterbuffer, guint32 expected,
+    guint16 seqnum, GstClockTime pts, gint gap)

Same here, shouldn't this consider the actual arrival times of previous packets
(dts) too?

@@ +2832,3 @@

+  /* calculate a pts based on rtptime and arrival time (dts) */
+  pts = rtp_jitter_buffer_calculate_pts (priv->jbuf, dts, rtptime,

Calculating the PTS changes jbuf's state... is this going to cause problems if
we later drop this specific packet? Previously we would only change the state
once the packet is definitely inserted.

@@ +2963,3 @@
      * possible moment to push this buffer, maybe we get an earlier seqnum
      * while we wait */
+    set_timer (jitterbuffer, TIMER_TYPE_DEADLINE, seqnum, pts);

And timers also seem like something that is related to dts (arrival times)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.