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.

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 :/

The data written to the ringbuffer is continous.

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;

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;
 

that segtotal +4 makes it even work for

gst-launch audiotestsrc ! alsasink buffer-time=50000

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).

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

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));

Could this bug and the changes it incurred be related to
http://bugzilla.gnome.org/show_bug.cgi?id=580007 

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.

Tristan, I am not sure if its that same what you are seeing. Could you try the patch?

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.

(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?

Stefan, I tried the patch and it didn't solve #580007

(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)

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.

(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?

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?

(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 */
<------------------------------->

(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)

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;
<---------------------------------------------------------------->

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?

I am using it localy, but I belive wim is not fond of the solution. 

Created attachment 138972 [details] [review]
don't wait if we already have the ringbuffer filled

Updated the patch against git HEAD.

*** Bug 591043 has been marked as a duplicate of this bug. ***

ok!

I'll go over it an commit it.

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