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 545807 - [baseaudiosink] audible crack when starting the pipeline
[baseaudiosink] audible crack when starting the pipeline
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
0.10.24
Other Linux
: Normal normal
: 0.10.25
Assigned To: Wim Taymans
GStreamer Maintainers
: 591043 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-08-01 07:54 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2009-08-24 11:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
avoid taking more segments out of the ringbuffer that actually have been filled (2.08 KB, patch)
2009-04-30 07:00 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
don't wait if we already have the ringbuffer filled (1.44 KB, patch)
2009-07-22 08:56 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
accepted-commit_now Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-01 07:54:42 UTC
gst-launch audiotestsrc ! alsasink
plays with a klick one in the beginning.

Its after the initial preroll, as can be seen by modifying the buffer-time.
gst-launch audiotestsrc ! alsasink buffer-time=100000

halfing it it comes quicker.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-01 08:02:21 UTC
gst-launch audiotestsrc is-live=true ! alsasink
or when using pulseaudio
gst-launch audiotestsrc is-live=true ! alsasink slave-method=2

will have no crack.
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-07 11:32:37 UTC
GST_DEBUG="*:2,*audio*:4" gst-launch-0.10 2>&1 audiotestsrc num-buffers=40 ! alsasink | grep -o "running: .*$"

end/bin have discontinuities, but that probably rounding for output


GST_DEBUG="*:2,*audio*:4" gst-launch-0.10 2>&1 audiotestsrc num-buffers=40 ! alsasink | grep -o "rendering .*$"

GST_DEBUG="*:2,*audio*:4" gst-launch-0.10 2>&1 audiotestsrc num-buffers=30 ! alsasink buffer-time=100000 | grep -o " time .*$"

looks really continous


GST_DEBUG="*:2,*audio*:4" gst-launch-0.10 2>&1 audiotestsrc num-buffers=30 ! alsasink buffer-time=100000 | grep -o " align .*$"
 align with prev sample, ABS (1) < 22050
 align with prev sample, ABS (1) < 22050
 ...


GST_DEBUG="*:2,*audio*:4" gst-launch-0.10 2>&1 audiotestsrc num-buffers=30 ! alsasink buffer-time=100000 | grep -o "sync.*$"
sync_play:<alsasink0> ringbuffer may start now
sync_latency:<alsasink0> we are not slaved
sync-offset 0, render-delay 0:00:00.000000000, ts-offset 0
sync-offset 0:00:00.000000000
sync after discont
sync-offset 0, render-delay 0:00:00.000000000, ts-offset 0
sync-offset 0:00:00.000000000
sync-offset 0, render-delay 0:00:00.000000000, ts-offset 0
sync-offset 0:00:00.000000000
...

I am running out for what to lock for :/

Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-07 12:25:45 UTC
The data written to the ringbuffer is continous.
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-07 15:27:56 UTC
There is a dropout reading out the ringbuffer. wtay suggested something along the lines of and that helped (+5 in my case)

#
Index: gst-libs/gst/audio/gstaudiosink.c
#
===================================================================
#
RCS file: /cvs/gstreamer/gst-plugins-base/gst-libs/gst/audio/gstaudiosink.c,v
#
retrieving revision 1.29
#
diff -u -B -b -p -r1.29 gstaudiosink.c
#
--- gst-libs/gst/audio/gstaudiosink.c   9 May 2008 16:38:10 -0000  1.29
#
+++ gst-libs/gst/audio/gstaudiosink.c   7 Aug 2008 15:20:50 -0000
#
@@ -366,6 +366,8 @@ gst_audioringbuffer_acquire (GstRingBuff
#
   if (!result)
#
     goto could_not_prepare;
#
 
#
+  spec->segtotal += 4;
#
+
#
   /* set latency to one more segment as we need some headroom */
#
   spec->seglatency = spec->segtotal + 1;
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-07 15:29:37 UTC
Index: gst-libs/gst/audio/gstaudiosink.c
===================================================================
RCS file: /cvs/gstreamer/gst-plugins-base/gst-libs/gst/audio/gstaudiosink.c,v
retrieving revision 1.29
diff -u -B -b -p -r1.29 gstaudiosink.c
--- gst-libs/gst/audio/gstaudiosink.c	9 May 2008 16:38:10 -0000	1.29
+++ gst-libs/gst/audio/gstaudiosink.c	7 Aug 2008 15:20:50 -0000
@@ -366,6 +366,8 @@ gst_audioringbuffer_acquire (GstRingBuff
   if (!result)
     goto could_not_prepare;
 
+  spec->segtotal += 4;
+
   /* set latency to one more segment as we need some headroom */
   spec->seglatency = spec->segtotal + 1;
 
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-07 18:22:06 UTC
that segtotal +4 makes it even work for

gst-launch audiotestsrc ! alsasink buffer-time=50000
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-11 13:41:12 UTC
We could also have the spec->segtotal += 4; in
gstbaseaudiosink.c:gst_base_audio_sink_setcaps():573

The values are set up in gstringbuffer.c::gst_ring_buffer_parse_caps()
and the extra segments have to be added before we allocate in gstaudiosink.c::gst_ring_buffer_acquire()

Not sure if it can be integrated with gstringbuffer.c::gst_ring_buffer_parse_caps() as then it would apply to all kind of ringbuffer uses (e.g. jacksrc).
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2009-04-22 07:52:19 UTC
09:35 < slomo> ensonic: i have this with alsasink
09:35 < wim__> sometimes there is a short crack for me
09:36 < wim__> not with pulsesink because it buffers differently
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2009-04-22 10:12:47 UTC
It also work if this is done after setting up spec->seglatency. Then it won't change the latency.

--- a/gst-libs/gst/audio/gstaudiosink.c
+++ b/gst-libs/gst/audio/gstaudiosink.c
@@ -379,6 +379,9 @@ gst_audioringbuffer_acquire (GstRingBuffer * buf, GstRingBufferSpec * spec)
   /* set latency to one more segment as we need some headroom */
   spec->seglatency = spec->segtotal + 1;
 
+  /* http://bugzilla.gnome.org/show_bug.cgi?id=545807 */
+  spec->segtotal += 4;
+
   buf->data = gst_buffer_new_and_alloc (spec->segtotal * spec->segsize);
   memset (GST_BUFFER_DATA (buf->data), 0, GST_BUFFER_SIZE (buf->data));
Comment 10 Tristan Matthews 2009-04-29 20:38:06 UTC
Could this bug and the changes it incurred be related to
http://bugzilla.gnome.org/show_bug.cgi?id=580007 
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2009-04-30 07:00:36 UTC
Created attachment 133631 [details] [review]
avoid taking more segments out of the ringbuffer that actually have been filled

Without the fix, the writer writes more data (3 extra buffers) than the producer has produced initially. So 3 extra zero buffers are written, when all ALSA buffers have been filled out.
This is heard as an audible glitch when MP3 etc GST is started initially.

You can see from below, (not patched), from the line:
0:00:01.227691675    0xcfe80 DEBUG          ringbuffer gstringbuffer.c:1626:gst_ring_buffer_commit_full: pointer at 22, write to 20-0, diff -2, 
that the diff goes to -2, which MUST NOT be the case.

The patch basically generates an initial write barrier to the writer thread, so that it doesn't initially write more buffers than it should.
So when the producer has (segment / 2) amount of new input in the buffers, it wakes up the writer thread. Then everything goes as has always been the case. 

This also should fix the issues where the producer goes "slow" that it actually provides an glitch while playing back the media (very rarely, but it's heard). Because now the producer is running at the head, not at head ~ writer head.
Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2009-04-30 07:01:17 UTC
Tristan, I am not sure if its that same what you are seeing. Could you try the patch?
Comment 13 Wim Taymans 2009-04-30 10:17:02 UTC
The writer thread should block exactly when all the bytes are consumed from the ringbuffer because the hardware buffer and the ringbuffer are configured to have the same size.

We don't want to ever block the audio writer thread because we want to write silence when the ringbuffer is empty to keep the clock running and to deal with sparse streams.

Comment 14 Eero Nurkkala 2009-04-30 12:59:32 UTC
(In reply to comment #13)
> The writer thread should block exactly when all the bytes are consumed from the
> ringbuffer because the hardware buffer and the ringbuffer are configured to
> have the same size.

What about the situation, in which the audioringbuffer_thread_func writes more data than the producer has provided initially? This really happens, and the result is an audible break of the audio. How to resolve it properly?
Comment 15 Tristan Matthews 2009-04-30 15:11:30 UTC
Stefan, I tried the patch and it didn't solve #580007
Comment 16 Eero Nurkkala 2009-04-30 18:27:37 UTC
(In reply to comment #13)
> The writer thread should block exactly when all the bytes are consumed from the
> ringbuffer because the hardware buffer and the ringbuffer are configured to
> have the same size.
> 
> We don't want to ever block the audio writer thread because we want to write
> silence when the ringbuffer is empty to keep the clock running and to deal with
> sparse streams.
> 

If the medium is from local source, the clock will keep running. Right, with discontinuous media, this is may be a problem (such as Voip).

Any such chance as to define a property like "local-source" or something alike, so the user could decide whether to play sound clear or have an annoying break of audio right after the segments have been filled up? 

So having something like the patch attached, but only when the relevant property is enabled; "local-source" or "continuous-playback" where the user acknowledges his intents (and thus enables the patch, and is then fully responsible of the conseqences).

(BTW, I give you an example where this bug counts the most; imagine a HW buffer of 1 megabyte; and alsa buffer of 1920 words / 20 periods. It takes only a blink of an eye for the writer thread to fill the buffers and go with huge amount of zero data inserted right after it. And the blame is not the HW, it's the SW above it). (By HW buffers I do not mean the kernel ALSA buffers, but real HW buffers)
Comment 17 Wim Taymans 2009-05-01 07:58:08 UTC
to Comment #16: the software ringbuffer must at least be as big as the hardware buffer, this is a hard requirement. We currently query the hardware buffer and set the ringbuffer to that size + 1 period.

A better solution would be to keep the hardware read pointer and our write pointer within a reasonable distance regardless of hw write() behaves and blocks. The sunaudio driver has something like that. The thing is that all alternatives to write() have historically been very flaky on many alsa implementations. What we really want to use is plain mmap (like pulseaudio) and fix all the alsa driver out there that would fail.
Comment 18 Eero Nurkkala 2009-05-01 18:23:32 UTC
(In reply to comment #17)
> 
> A better solution would be to keep the hardware read pointer and our write
> pointer within a reasonable distance regardless of hw write() behaves and
> blocks. The sunaudio driver has something like that. The thing is that all
> alternatives to write() have historically been very flaky on many alsa
> implementations. What we really want to use is plain mmap (like pulseaudio) and
> fix all the alsa driver out there that would fail.
> 

(Just to make sure we're talking about the same thing; something like:
gst-launch filesrc ! mp3dec ! alsa or pulsesink)
Just a sidenote, this stuff also occur with pulsesink.

I'm not exatly sure what you're saying - but I interpret is as: audioringbuffer_thread_func should not get too far (from the data provided by mp3dec or wavdec etc)? That is something like the patch attached to this bug does; but probably not in the most optimal way.

Like it's now, I see no single problem with alsasink or pulsesink, they're fine. It's the very best that Gstreamer tries (and does so) to keep these buffers all filled up.

So by fixing all alsa drivers, you mean the kernel drivers? Or gstreamer alsa/pulseaudio drivers?


I've tried modifying the audioringbuffer_thread_func (thread) to be more scheduler friendly - but with no success so far. Possibly that could be another way around the problem?
Comment 19 Eero Nurkkala 2009-05-04 05:27:38 UTC
Could we talk about this a bit:

<------------------------------------------>
--- a/gst-libs/gst/audio/gstringbuffer.c
+++ b/gst-libs/gst/audio/gstringbuffer.c
@@ -1398,6 +1398,10 @@ wait_segment (GstRingBuffer * buf)
     gst_ring_buffer_start (buf);
   }
 
+  /* After starting, the writer may have wrote segments already */
+  if (g_atomic_int_get (&buf->segdone) > 0)
+    goto no_need_to_wait;
+
   /* take lock first, then update our waiting flag */
   GST_OBJECT_LOCK (buf);
   if (G_UNLIKELY (buf->abidata.ABI.flushing))
@@ -1420,6 +1424,7 @@ wait_segment (GstRingBuffer * buf)
   }
   GST_OBJECT_UNLOCK (buf);
 
+no_need_to_wait:
   return TRUE;
<------------------------------------------> 

What I see, is that when gst_ring_buffer_start() is called,
this thread won't be scheduled back until all ALSA buffers are full.

Thus, there's absolutely no need to wait, if the producer has already brought stuff for us (g_atomic_int_get (&buf->segdone) > 0) ? Is there?

With this partial "patch" only, I see much better; I get only 1 zero-inserted buffer, whereas it used to be 3.

Please comment on some?
Comment 20 Eero Nurkkala 2009-05-04 05:40:34 UTC
(In reply to comment #19)
> Could we talk about this a bit:
> 
> <------------------------------------------>
> --- a/gst-libs/gst/audio/gstringbuffer.c
> +++ b/gst-libs/gst/audio/gstringbuffer.c
> @@ -1398,6 +1398,10 @@ wait_segment (GstRingBuffer * buf)
>      gst_ring_buffer_start (buf);
>    }
> 
> +  /* After starting, the writer may have wrote segments already */
> +  if (g_atomic_int_get (&buf->segdone) > 0)
> +    goto no_need_to_wait;
> +
>    /* take lock first, then update our waiting flag */
>    GST_OBJECT_LOCK (buf);
>    if (G_UNLIKELY (buf->abidata.ABI.flushing))
> @@ -1420,6 +1424,7 @@ wait_segment (GstRingBuffer * buf)
>    }
>    GST_OBJECT_UNLOCK (buf);
> 
> +no_need_to_wait:
>    return TRUE;
> <------------------------------------------> 
> 


Should look like:

<--------------------------------------->
--- a/gst-libs/gst/audio/gstringbuffer.c
+++ b/gst-libs/gst/audio/gstringbuffer.c
@@ -1390,6 +1390,10 @@
 
     GST_DEBUG_OBJECT (buf, "start!");
     gst_ring_buffer_start (buf);
+
+   /* After starting, the writer may have wrote segments already */
+   if (g_atomic_int_get (&buf->segdone) > 0)
+     goto no_need_to_wait;
   }
 
   /* take lock first, then update our waiting flag */
@@ -1414,6 +1418,7 @@
   }
   GST_OBJECT_UNLOCK (buf);
 
+no_need_to_wait:
   return TRUE;
 
   /* ERROR */
<------------------------------->
Comment 21 Eero Nurkkala 2009-05-04 05:46:43 UTC
(In reply to comment #20)
> 
> Should look like:
> 
> <--------------------------------------->
> --- a/gst-libs/gst/audio/gstringbuffer.c
> +++ b/gst-libs/gst/audio/gstringbuffer.c
> @@ -1390,6 +1390,10 @@
> 
>      GST_DEBUG_OBJECT (buf, "start!");
>      gst_ring_buffer_start (buf);
> +
> +   /* After starting, the writer may have wrote segments already */
> +   if (g_atomic_int_get (&buf->segdone) > 0)
> +     goto no_need_to_wait;
>    }
> 
>    /* take lock first, then update our waiting flag */
> @@ -1414,6 +1418,7 @@
>    }
>    GST_OBJECT_UNLOCK (buf);
> 
> +no_need_to_wait:
>    return TRUE;
> 
>    /* ERROR */
> <------------------------------->
> 

And I confirm that this is sufficient to fix pulsesink alltogether.
(with alsasink, one extra buffer gets inserted; but as with this, this should be a lot better)
Comment 22 Eero Nurkkala 2009-05-07 08:39:17 UTC
This is rewritten, so that we're more careful about corner-cases:

<---------------------------------------------------------------->
--- a/gst-libs/gst/audio/gstringbuffer.c
+++ b/gst-libs/gst/audio/gstringbuffer.c
@@ -1381,6 +1381,9 @@ gst_ring_buffer_clear_all (GstRingBuffer * buf)
 static gboolean
 wait_segment (GstRingBuffer * buf)
 {
+  gint segments;
+  gboolean no_wait = FALSE;
+
   /* buffer must be started now or we deadlock since nobody is reading */
   if (G_UNLIKELY (g_atomic_int_get (&buf->state) !=
           GST_RING_BUFFER_STATE_STARTED)) {
@@ -1389,7 +1392,12 @@ wait_segment (GstRingBuffer * buf)
       goto no_start;
 
     GST_DEBUG_OBJECT (buf, "start!");
+    segments = g_atomic_int_get (&buf->segdone);
     gst_ring_buffer_start (buf);
+
+   /* After starting, the writer may have wrote segments already */
+   if (G_LIKELY (g_atomic_int_get (&buf->segdone) != segments))
+     no_wait = TRUE;
   }
 
   /* take lock first, then update our waiting flag */
@@ -1401,6 +1409,9 @@ wait_segment (GstRingBuffer * buf)
           GST_RING_BUFFER_STATE_STARTED))
     goto not_started;
 
+  if (no_wait)
+    goto no_need_to_wait;
+
   if (g_atomic_int_compare_and_exchange (&buf->waiting, 0, 1)) {
     GST_DEBUG_OBJECT (buf, "waiting..");
     GST_RING_BUFFER_WAIT (buf);
@@ -1412,6 +1423,7 @@ wait_segment (GstRingBuffer * buf)
             GST_RING_BUFFER_STATE_STARTED))
       goto not_started;
   }
+no_need_to_wait:
   GST_OBJECT_UNLOCK (buf);
 
   return TRUE;
<---------------------------------------------------------------->
Comment 23 Sebastian Dröge (slomo) 2009-07-16 19:24:47 UTC
Could you please attach patches as files to bugs in the future? That makes it much easier to review and apply them :)

Also, what's the state of this bug? Is the last patch ready to be committed?
Comment 24 Stefan Sauer (gstreamer, gtkdoc dev) 2009-07-17 06:11:56 UTC
I am using it localy, but I belive wim is not fond of the solution. 
Comment 25 Stefan Sauer (gstreamer, gtkdoc dev) 2009-07-22 08:56:32 UTC
Created attachment 138972 [details] [review]
don't wait if we already have the ringbuffer filled

Updated the patch against git HEAD.
Comment 26 Wim Taymans 2009-08-07 11:47:05 UTC
*** Bug 591043 has been marked as a duplicate of this bug. ***
Comment 27 Wim Taymans 2009-08-07 11:54:17 UTC
ok!
Comment 28 Wim Taymans 2009-08-07 11:55:05 UTC
I'll go over it an commit it.
Comment 29 Wim Taymans 2009-08-24 11:31:01 UTC
commit 8ad8591e41cbff2eb3fb8c1008a84c1b7241912e
Author: Eero Nurkkala <ext-eero.nurkkala at nokia.com>
Date:   Mon Aug 24 13:27:55 2009 +0200

    ringbuffer: Improve audiosink startup performance
    
    When we start the ringbuffer, immediatly continue processing samples if the
    writer prepared some for us.
    
    Fixes #545807