GNOME Bugzilla – Bug 323017
While(1) loop with sleep(0) in basertpdepayload.c
Last modified: 2005-12-12 12:40:53 UTC
Please describe the problem: Please see: http://article.gmane.org/gmane.comp.video.gstreamer.devel/14298 Steps to reproduce: 1. 2. 3. Actual results: Expected results: Does this happen every time? Other information:
Marking as blocker, as it's effectively a 100% CPU loop and both the problem and the fix seem rather straight-forward.
Looks fine to me. Go ahead and commit.
The bug here is that GST_NSECOND is '1', because nanoseconds are GStreamer's clock unit. I'm not sure what the units of clock_rate are so it's hard to know what the statement is TRYING to calculate. As you say (GST_NSECOND / filter->clock_rate) is the wrong thing to do. I'd guess you probably wanted (GST_SECOND / filter->clock_rate) instead, which would sleep for 1^9/filter->clock_rate nanoseconds. Making the filter getrange based doesn't sound like the right approach, since it's not a random access stream.
Jan's change looks more correct -- clock_rate appears to be in per-second units.
2005-12-05 Andy Wingo <wingo@pobox.com> patch by: Kai Vehmanen <kv2004 eca cx> * gst-libs/gst/rtp/gstbasertpdepayload.c (gst_base_rtp_depayload_thread): Fix busy loop (#323017).
The original intent was probably 'GST_SECOND/filter->clock_rate', but this also is far from optimal. For example most current depayloaders in gstreamer use a clock-rate of 8000, and packetization interval of 20 or 30ms (for audio, much longer intervals for video). Sleeping GST_SECOND/8000 gets us 0.12msec sleep interval, although packets are received at most every ~20msec. Thus the depayloader thread is woken up unnecessarily roughly 160 times (with no work to be done). Ideally the data should be read from the depayloader/jitter buffer when the pipeline sink (which is consuming the data) is ready to receive more data. The next option is to match the wakeup frequency to the packetization (or packet arrival) frequency. Unfortunately this might not match the consumption rate in all cases. When data is read from a reliable source (a file, TCP connection, etc), the application can set 'queue-delay' to zero, and disable the jitter-buffering altogether. I don't know enough about gstreamer architecture to comment on the usability of getrange in this case. Still, especially for audio, using sleep() to schedule buffers for playback, does not seem like the right way to go. The depayloader/jitter buffer should be waken up (=> polled) when more data is needed for playback.
As I see it, the post 0.10 fix will be to remove this thread and jitter buffer completely, add a small dynamic buffer to basertpdepayloader solely for reordering packets when a depayloader requires multiple packets per frame. Then we will generate a clock from the rtp stream and clock the pipeline to this, then the rest of the reordering and jitter correction can be left to the audio sink.
We have to consider the case when an audio sink isnt used. It can be video, the the stream can be saved to a file or taken somewhere else. We can't rely on the audio sink. The jitter is very important for live audio, but its not so important when streaming to a file or for video. The ordering on the other hand is important for both, and it probably needs to be done before any sinks. Philippe
After some talk, it has been decided to move the whole reordering/dropping algorithms into the rtprecv element instead of the depayloader. The depayloader base class simply has to put together the packets into frames and probably also still do the RTP TS->GST TS conversion, or even that might get moved out. As for releasing the queue that will be in rtprecv, a simple algorithm that calculates the average time difference between the rtp packets is enough to determine the interval of release.
A new proposal (based on comments to this bug) for playout buffering, has been now documented at: http://article.gmane.org/gmane.comp.telephony.farsight.devel/1
About reordering at sink: Reordering must happen before codecs (i.e. not in the sinks), because some decoders are stateful and need to be fed with frames in correct order.