GNOME Bugzilla – Bug 593354
rtpjitterbuffer sometimes outputs packets with timestamps in the past
Last modified: 2009-11-18 10:03:39 UTC
Created attachment 141909 [details] Test program Under the right conditions, the jitterbuffer can output packets that have timestamps in the past. I guess the solution is to reset the jb's state if that happens. I'm not sure how to properly do that without having to query the clock on every buffer. Attaching a patch that tries to do that.. but there must be a smarter solution. One case I can see is if the skew ends up being so much that its more than the length of the jb. It also happens if for some reason the rtp and gst timestamps get desynchronized (like if the other sides does a "hold" in a voip call and just stops its clock entirely while on hold).
Created attachment 141910 [details] [review] rtpjitterbuffer: Reset computation if result is in the past If the jitterbuffer's computation would result in a timestamp that is in the past, then reset it. It means that either there was too much skew or that the sender did something wrong.
Created attachment 141911 [details] [review] rtpjitterbuffer: Reset computation if result is in the past If the jitterbuffer's computation would result in a timestamp that is in the past, then reset it. It means that either there was too much skew or that the sender did something wrong.
The test did not cause any problems for me. I commited this patch that I had laying around and fixed something related. commit e254936e3417d283c72b49496c927fb5a8248e4b Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Aug 31 12:47:15 2009 +0200 jitterbuffer: make sure time never goes invalid Since the skew can be negative, we might end up with invalid timestamps. Check for negative results and clamp to 0. See #593354
Another patch I had to improve robustness: commit 4814d899c27df764e2e9ad15a0765235d59abc22 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Aug 31 12:57:32 2009 +0200 jitterbuffer: reset skew when timestamps change Refactor the jitterbuffer resync code. Reset the skew correction when we detect a big timestamp discont. See #593354
The case with this testcase is that the client thresholds are not exceeded and thus the client thinks that packets were just 0.5 seconds queued on the network somewhere. It then tries its best to still play those packets and eventually caches up when the skew correction figures it out. During the period where the skew algorithm has to adapt, we produce timestamps that are past the time of the clock (and will thus be late depending on the downstream latency) I don't know exactly what to do here. Your patch checks against the current clock time but that might be suboptimal as it is not known how much downstream latency there is.
Do we really care about downstream latency ? If the downstream elements add latency, they will probably delay the packets too. I guess the jb should assume that there is a sink with 0 latency afterwards (and hence that only upstream latency matters). That said, I have no idea what the right solution is. But it should probably cap the correction to make sure that if the packets are received in line, they should go out on time..
If the output timestamp (after skew correction and latency adjustment) is less than the incomming timestamp, we have a problem. I'm going to try to implement this.
This one pretty much fixes it. commit 8d924611e74ba8b6c36fe24db5b3648850751bf7 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Tue Sep 1 12:41:36 2009 +0200 jitterbuffer: make sure time does not go backwards When we construct a timestamp that would result in a timestamp that is earlier than when the packet was received, reset the skew calculation as this is probably a sign that the sender restarted or paused. Fixes #593354
After applying the patch by Wim from Comment 8 I have started to get dead locks. The following happens: - Thread 1 sets rtspsrc to PLAYING. As a result of this it hangs trying to take a gstrtpjitterbuffer lock in gst_rtp_jitter_buffer_clear_pt_map() called from gst_rtp_bin_clear_pt_map() where it also takes a gstrtpsession lock Thread 1 hangs because thread 2 has already taken the gstrtpjitterbuffer lock in gst_rtp_jitter_buffer_chain(). But then thread 1 hangs trying to take the gstrtpsession lock held by thread 1 when it calls gst_rtp_jitter_buffer_get_clock_rate()->get_pt_map() I'm not sure what the best solution is but perhaps it would be ok to release the lock taken in gst_rtp_jitter_buffer_chain before calling gst_rtp_jitter_buffer_get_clock_rate() and taking it again afterwards?
Created attachment 147472 [details] [review] Release/take lock before/after call to gst_rtp_jitter_buffer_get_clock_rate
Created attachment 147479 [details] [review] improved patch Improved patch. We should call get_clock_rate with the lock so we must release the lock right before the signal emission.
*** Bug 602231 has been marked as a duplicate of this bug. ***
commit f52859432f780d3540002f25a6412ae34c231ef3 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Wed Nov 11 15:50:19 2009 +0100 jitterbuffer: release lock before emiting signals Release the jbuf lock before emiting the request-pt-map signal to avoid deadlocks. We also need to catch the shutdown case when locking again. Fixes #593354