After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 323017 - While(1) loop with sleep(0) in basertpdepayload.c
While(1) loop with sleep(0) in basertpdepayload.c
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: Normal blocker
: 0.10.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2005-12-02 13:44 UTC by Kai Vehmanen
Modified: 2005-12-12 12:40 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Kai Vehmanen 2005-12-02 13:44:32 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:
Comment 1 Tim-Philipp Müller 2005-12-02 16:13:07 UTC
Marking as blocker, as it's effectively a 100% CPU loop and both the problem and
the fix seem rather straight-forward.
Comment 2 Andy Wingo 2005-12-02 17:12:16 UTC
Looks fine to me. Go ahead and commit.
Comment 3 Jan Schmidt 2005-12-03 11:32:07 UTC
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. 
Comment 4 Andy Wingo 2005-12-05 10:18:45 UTC
Jan's change looks more correct -- clock_rate appears to be in per-second units.
Comment 5 Andy Wingo 2005-12-05 10:48:25 UTC
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).
Comment 6 Kai Vehmanen 2005-12-05 11:30:57 UTC
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.
Comment 7 Rob Taylor 2005-12-05 15:27:55 UTC
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.
Comment 8 Philippe Khalaf 2005-12-05 15:59:26 UTC
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
Comment 9 Philippe Khalaf 2005-12-05 17:43:59 UTC
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.
Comment 10 Kai Vehmanen 2005-12-12 12:39:27 UTC
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
Comment 11 Kai Vehmanen 2005-12-12 12:40:53 UTC
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.