GNOME Bugzilla – Bug 762988
rtpjitterbuffer: Performance improvements
Last modified: 2016-04-07 08:16:33 UTC
These are two fixes I implemented while investigating issues with a high packet-rate stream
Created attachment 322857 [details] [review] jitterbuffer: Add tracing of lock usage Helps with debugging lock usage
Created attachment 322858 [details] [review] jitterbuffer: Don't create lost events if we don't need them When "do-lost" is set to FALSE we don't use/send the lost events. In that case, don't create them to start with :)
Created attachment 322859 [details] [review] jitterbuffer: Speed up lost timeout handling When downstream blocks, "lost" timers are created to notify the outgoing thread that packets are lost. The problem is that for high packet-rate streams, we might end up with a big list of lost timeouts (had a use-case with ~1000...). The problem isn't so much the amount of lost timeouts to handle, but rather the way they were handled. All timers would first be iterated, then the one selected would be handled ... to re-iterate the list again. All of this is being done while the jbuf lock is taken, which in some use-cases would return in holding that lock for 10s... blocking any buffers from being accepted in input... which would then arrive late ... which would create plenty of lost timers ... which would cause the same issue. In order to avoid that situation, handle the lost timers immediately when iterating the list of pending timers. This modifies the complexity from a combinatorial to a linear complexity.
Comment on attachment 322858 [details] [review] jitterbuffer: Don't create lost events if we don't need them Makes sense to me but maybe add a comment somewhere (where the items are taken from the queue) that NULL ITEM_TYPE_LOST is valid and has to be handled correctly.
Review of attachment 322859 [details] [review]: It's quadratic to linear complexity, right? At least for the case of LOST timers... for other timers it can still be quadratic. ::: gst/rtpmanager/gstrtpjitterbuffer.c @@ +3477,3 @@ } } if (timer && !priv->blocked) { It might make sense to add an assertion that do_timeout() is never called with LOST anymore. because if it is, your code is wrong
Edward?
Fixed commits accordingly and pushed them commit b82da6292239ff2f8bcf620b1595d6b08ad591f4 Author: Edward Hervey <edward@centricular.com> Date: Wed Mar 2 14:25:24 2016 +0100 jitterbuffer: Speed up lost timeout handling When downstream blocks, "lost" timers are created to notify the outgoing thread that packets are lost. The problem is that for high packet-rate streams, we might end up with a big list of lost timeouts (had a use-case with ~1000...). The problem isn't so much the amount of lost timeouts to handle, but rather the way they were handled. All timers would first be iterated, then the one selected would be handled ... to re-iterate the list again. All of this is being done while the jbuf lock is taken, which in some use-cases would return in holding that lock for 10s... blocking any buffers from being accepted in input... which would then arrive late ... which would create plenty of lost timers ... which would cause the same issue. In order to avoid that situation, handle the lost timers immediately when iterating the list of pending timers. This modifies the complexity from a quadratic to a linear complexity. https://bugzilla.gnome.org/show_bug.cgi?id=762988 commit d656fe8d549ed0ac06e4baf169e06066d0b99e6f Author: Edward Hervey <edward@centricular.com> Date: Wed Mar 2 14:23:01 2016 +0100 jitterbuffer: Don't create lost events if we don't need them When "do-lost" is set to FALSE we don't use/send the lost events. In that case, don't create them to start with :) https://bugzilla.gnome.org/show_bug.cgi?id=762988 commit cf866a84694862cee766b63c80f5bc8d6e213364 Author: Edward Hervey <edward@centricular.com> Date: Wed Mar 2 13:57:07 2016 +0100 jitterbuffer: Add tracing of lock usage Helps with debugging lock usage https://bugzilla.gnome.org/show_bug.cgi?id=762988