Fix race condition in gapless_switch (amarok stops playing tracks bug)

hi,

this is another "amarok stops playing tracks bug". actually that must
be the original one, but then some people reported the pulseaudio
issue on the same bugzilla entry...

yesterday i finally traced the deadlock down to a race condition in
gapless_switch. searching my mail for any recent discussion on that
subject, i've found that this problem was also reported back in 2007
by Stas Sergeev but it was never fixed. Kudos to Thibaut for
suggesting the nice fix.

the problem is serious as it seems to be affecting a lot of users. i
was able to reproduce it myself with kaffeine so it doesn't seem
specific to a particular frontend logic.

somehow some recent change in xine-lib seems to have made this race
condition possible. i suspect the demux_unstick_ao_loop() which causes
_x_demux_control_headers_done() to return with header_count_*
conditions unmet. this would explain why gapless_switch variable
changes before the decoders have processed all control buffers.

if there are no objections i will commit it tomorrow. my apologies for
writing the original unsafe code in 2005, it is really amazing how
long did it pass unnoticed.

i still have the pause/resume patch pending though.

regards,

Miguel

[xine-lib-gapless-race-fix.patch]

---

diff -r ce4b1533a0af src/xine-engine/audio_decoder.c
--- a/src/xine-engine/audio_decoder.c Sun Jan 11 22:24:42 2009 +0000
+++ b/src/xine-engine/audio_decoder.c Mon Feb 09 00:28:11 2009 -0200
@@ -89,16 +89,18 @@
       if (stream->audio_decoder_plugin) {
 
  lprintf ("close old decoder\n");
-
+      
+ stream->keep_ao_driver_open = !!(buf->decoder_flags & BUF_FLAG_GAPLESS_SW);
  _x_free_audio_decoder (stream, stream->audio_decoder_plugin);
  stream->audio_decoder_plugin = NULL;
  stream->audio_track_map_entries = 0;
  stream->audio_type = 0;
+ stream->keep_ao_driver_open = 0;
       }
       
       running_ticket->release(running_ticket, 0);
       
-      if( !stream->gapless_switch )
+      if( !(buf->decoder_flags & BUF_FLAG_GAPLESS_SW) )
         stream->metronom->handle_audio_discontinuity (stream->metronom, DISC_STREAMSTART, 0);
       
       buftype_unknown = 0;
diff -r ce4b1533a0af src/xine-engine/audio_out.c
--- a/src/xine-engine/audio_out.c Sun Jan 11 22:24:42 2009 +0000
+++ b/src/xine-engine/audio_out.c Mon Feb 09 00:28:11 2009 -0200
@@ -1609,7 +1614,7 @@
   pthread_mutex_unlock(&this->streams_lock);
 
   /* close driver if no streams left */
-  if (!ite && !this->grab_only && !stream->gapless_switch) {
+  if (!ite && !this->grab_only && !stream->keep_ao_driver_open) {
     xprintf (this->xine, XINE_VERBOSITY_DEBUG, "audio_out: no streams left, closing driver\n");
 
     if (this->audio_loop_running) {
diff -r ce4b1533a0af src/xine-engine/buffer.h
--- a/src/xine-engine/buffer.h Sun Jan 11 22:24:42 2009 +0000
+++ b/src/xine-engine/buffer.h Mon Feb 09 00:28:11 2009 -0200
@@ -378,6 +378,9 @@
  * decoder_info[2] carries denominator for display aspect ratio       */
 #define BUF_FLAG_ASPECT      0x0800
 
+/* represent the state of gapless_switch at the time buf was enqueued */
+#define BUF_FLAG_GAPLESS_SW  0x1000
+
 
 /* Special buffer types:
  * Sometimes there is a need to relay special information from a demuxer
diff -r ce4b1533a0af src/xine-engine/demux.c
--- a/src/xine-engine/demux.c Sun Jan 11 22:24:42 2009 +0000
+++ b/src/xine-engine/demux.c Mon Feb 09 00:28:11 2009 -0200
@@ -228,15 +228,18 @@
 void _x_demux_control_start( xine_stream_t *stream ) {
 
   buf_element_t *buf;
+  uint32_t flags = (stream->gapless_switch) ? BUF_FLAG_GAPLESS_SW : 0;
 
   pthread_mutex_lock(&stream->demux_mutex);  
 
   buf = stream->video_fifo->buffer_pool_alloc (stream->video_fifo);
   buf->type = BUF_CONTROL_START;
+  buf->decoder_flags = flags;
   stream->video_fifo->put (stream->video_fifo, buf);
 
   buf = stream->audio_fifo->buffer_pool_alloc (stream->audio_fifo);
   buf->type = BUF_CONTROL_START;
+  buf->decoder_flags = flags;
   stream->audio_fifo->put (stream->audio_fifo, buf);
 
   pthread_mutex_unlock(&stream->demux_mutex);  
diff -r ce4b1533a0af src/xine-engine/video_decoder.c
--- a/src/xine-engine/video_decoder.c Sun Jan 11 22:24:42 2009 +0000
+++ b/src/xine-engine/video_decoder.c Mon Feb 09 00:28:11 2009 -0200
@@ -160,10 +160,10 @@
       
       running_ticket->release(running_ticket, 0);
       
-      if( !stream->gapless_switch )
+      if( !(buf->decoder_flags & BUF_FLAG_GAPLESS_SW) )
         stream->metronom->handle_video_discontinuity (stream->metronom,
       DISC_STREAMSTART, 0);
       
       buftype_unknown = 0;
       break;
 
diff -r ce4b1533a0af src/xine-engine/xine.c
--- a/src/xine-engine/xine.c Sun Jan 11 22:24:42 2009 +0000
+++ b/src/xine-engine/xine.c Mon Feb 09 00:28:11 2009 -0200
@@ -419,6 +426,7 @@
 static void close_internal (xine_stream_t *stream) {
 
   int i ;
+  int gapless_switch = stream->gapless_switch;
 
   if( stream->slave ) {
     xine_close( stream->slave );
@@ -429,7 +437,7 @@
     }
   }
 
-  if( !stream->gapless_switch ) {
+  if( !gapless_switch ) {
     /* make sure that other threads cannot change the speed, especially pauseing the stream */
     pthread_mutex_lock(&stream->speed_change_lock);
     stream->ignore_speed_change = 1;
@@ -445,7 +453,7 @@
   
   stop_internal( stream );
   
-  if( !stream->gapless_switch ) {
+  if( !gapless_switch ) {
     if (stream->video_out)
       stream->video_out->set_property(stream->video_out, VO_PROP_DISCARD_FRAMES, 0);  
     if (stream->audio_out)
@@ -596,6 +604,7 @@
   stream->early_finish_event     = 0;
   stream->delay_finish_event     = 0;
   stream->gapless_switch         = 0;
+  stream->keep_ao_driver_open    = 0;
 
   stream->video_out              = vo;
   if (vo)
diff -r ce4b1533a0af src/xine-engine/xine_interface.c
--- a/src/xine-engine/xine_interface.c Sun Jan 11 22:24:42 2009 +0000
+++ b/src/xine-engine/xine_interface.c Mon Feb 09 00:28:11 2009 -0200
@@ -527,6 +527,9 @@
   
   case XINE_PARAM_GAPLESS_SWITCH:
     stream->gapless_switch = !!value;
+    if( stream->gapless_switch && !stream->early_finish_event ) {
+      xprintf (stream->xine, XINE_VERBOSITY_DEBUG, "frontend possibly buggy: gapless_switch without early_finish_event\n");
+    }
     break;
     
   default:
diff -r ce4b1533a0af src/xine-engine/xine_internal.h
--- a/src/xine-engine/xine_internal.h Sun Jan 11 22:24:42 2009 +0000
+++ b/src/xine-engine/xine_internal.h Mon Feb 09 00:28:11 2009 -0200
@@ -361,6 +361,7 @@
   int                        early_finish_event; /* do not wait fifos get empty before sending event */
   int                        gapless_switch;     /* next stream switch will be gapless */
   int                        delay_finish_event; /* delay event in 1/10 sec units. 0=>no delay, -1=>forever */
+  int                        keep_ao_driver_open;
 #endif
 };
 

---

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com
_______________________________________________
xine-devel mailing list
xine-devel@...
https://lists.sourceforge.net/lists/listinfo/xine-devel

[lorenzodes]

Miguel Freitas ha scritto:

> hi,
>
> this is another "amarok stops playing tracks bug". actually that must
> be the original one, but then some people reported the pulseaudio
> issue on the same bugzilla entry...
>
> yesterday i finally traced the deadlock down to a race condition in
> gapless_switch. searching my mail for any recent discussion on that
> subject, i've found that this problem was also reported back in 2007
> by Stas Sergeev but it was never fixed. Kudos to Thibaut for
> suggesting the nice fix.

[...]
> if there are no objections i will commit it tomorrow. my apologies for
> writing the original unsafe code in 2005, it is really amazing how
> long did it pass unnoticed.


Just a quick question: why is this needed?

+    if( stream->gapless_switch && !stream->early_finish_event ) {
+      xprintf (stream->xine, XINE_VERBOSITY_DEBUG, "frontend possibly buggy:
gapless_switch without early_finish_event\n");
+    }

While normally those two options are active at the same time, in my xine
front-end I disable early_finish_event and set gapless_switch to 1, just
before playing the last track in a playlist. This way I can be sure xine
plays such track till its end. Is this approach wrong?

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com
_______________________________________________
xine-devel mailing list
xine-devel@...
https://lists.sourceforge.net/lists/listinfo/xine-devel

On Mon, Feb 9, 2009 at 5:34 PM, lorenzodes <lorenzodes@...> wrote:
> Just a quick question: why is this needed?
>
> +    if( stream->gapless_switch && !stream->early_finish_event ) {
> +      xprintf (stream->xine, XINE_VERBOSITY_DEBUG, "frontend possibly buggy:
> gapless_switch without early_finish_event\n");
> +    }

of course it is not needed ;-)

it is just that i noticed some frontends might not be implementing
gapless correctly, if gapless_switch is set but the event was not
"early" you will definitely have a gap.

kaffeine, for example, seems to be doing it correctly most of the
time. but then i tried to change audio driver and early_finish_event
flag was cleared. i still have to contact them about this but i
suppose a new stream was created and they forgot to set the flag
there.

> While normally those two options are active at the same time, in my xine
> front-end I disable early_finish_event and set gapless_switch to 1, just
> before playing the last track in a playlist. This way I can be sure xine
> plays such track till its end. Is this approach wrong?

no, it is correct. that's why i wrote *possibly* there :-)

what about changing the order of your xine_set_param? set gapless and
then disable early_finish_event?

this way you frontend won't cause any warning and we may still catch
the buggy frontends around...

regards,

Miguel

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com
_______________________________________________
xine-devel mailing list
xine-devel@...
https://lists.sourceforge.net/lists/listinfo/xine-devel

[lorenzodes]

Unless Kaffeine and Amarok set gapless playback even when they play internet
streams (e.g. internet radios), maybe fixing that is not enough.

The attached patch tweaks cs 9617:2c5059a74b28 and cs 9618:ce60f8b5995a to
various extents. The most important part is that
_x_demux_control_headers_done() and demux_loop() won't break as soon as
demux_unstick_ao_loop() reports a possible hang.

It has been tested with Kaffeine and Amarok/Phonon and got no hangs, not even
with broken/problematic asf streams, which was the reason that led to the
original patches.


[xine-unstick.diff]

---

# HG changeset patch
# User Lorenzo Desole <lorenzodes@...>
# Date 1234215520 -3600
# Node ID a2fdd33877e08efa863d61b6e9d46edd148e009f
# Parent  e3fa66d1aff4d4d7f18e1e79586e662ced59502e
Don't give up immediately if demux_unstick_ao_loop() reports that xine might be stuck, because it's not necessarily so.
According to my tests, this fixes http://bugs.kde.org/show_bug.cgi?id=180339#c42 and http://bugs.debian.org/514114.

This has been tested with Amarok and kde 4.1.x (with phonon) and kaffeine.

diff -r e3fa66d1aff4 -r a2fdd33877e0 src/combined/ffmpeg/ff_audio_decoder.c
--- a/src/combined/ffmpeg/ff_audio_decoder.c Sun Feb 08 14:19:24 2009 +0000
+++ b/src/combined/ffmpeg/ff_audio_decoder.c Mon Feb 09 22:38:40 2009 +0100
@@ -333,8 +333,10 @@
         while (out < decode_buffer_size) {
           int stream_status = xine_get_status(this->stream);
 
-          if (stream_status == XINE_STATUS_QUIT || stream_status == XINE_STATUS_STOP)
+          if (stream_status == XINE_STATUS_QUIT || stream_status == XINE_STATUS_STOP){
+    this->size = 0;
             return;
+  }
 
           audio_buffer =
             this->stream->audio_out->get_buffer (this->stream->audio_out);
diff -r e3fa66d1aff4 -r a2fdd33877e0 src/xine-engine/demux.c
--- a/src/xine-engine/demux.c Sun Feb 08 14:19:24 2009 +0000
+++ b/src/xine-engine/demux.c Mon Feb 09 22:38:40 2009 +0100
@@ -120,6 +120,16 @@
 }
 
 
+struct timespec _x_compute_interval(unsigned int millisecs){
+  struct timespec ts;
+  clock_gettime(CLOCK_REALTIME, &ts);
+  uint64_t ttimer = (uint64_t)ts.tv_sec*1000 + ts.tv_nsec/1000000 + millisecs;
+  ts.tv_sec = ttimer/1000;
+  ts.tv_nsec = (ttimer%1000)*1000000;
+  return ts;
+}
+
+
 void _x_demux_control_newpts( xine_stream_t *stream, int64_t pts, uint32_t flags ) {
 
   buf_element_t *buf;
@@ -147,19 +157,20 @@
  */
 static int demux_unstick_ao_loop (xine_stream_t *stream)
 {
-  if (!stream->audio_thread_created)
-    return 0;
+//  if (!stream->audio_thread_created)
+//    return 0;
 
   int status = xine_get_status (stream);
-  if (status != XINE_STATUS_QUIT && status != XINE_STATUS_STOP)
+  if (status != XINE_STATUS_QUIT && status != XINE_STATUS_STOP && stream->demux_plugin->get_status(stream->demux_plugin) != DEMUX_FINISHED)
     return 0;
-
+#if 0
   /* right, stream is stopped... */
   audio_buffer_t *buf = stream->audio_out->get_buffer (stream->audio_out);
   buf->num_frames = 0;
   buf->stream = NULL;
   stream->audio_out->put_buffer (stream->audio_out, buf, stream);
-
+#endif
+  lprintf("stuck\n");
   return 1;
 }
 
@@ -200,24 +211,27 @@
   stream->audio_fifo->put (stream->audio_fifo, buf_audio);
 
   pthread_mutex_unlock(&stream->demux_mutex);  
+  unsigned int max_iterations = 0;
 
   while ((stream->header_count_audio < header_count_audio) ||
          (stream->header_count_video < header_count_video)) {
-    struct timeval tv;
-    struct timespec ts;
 
     lprintf ("waiting for headers. v:%d %d   a:%d %d\n",
      stream->header_count_video, header_count_video,
      stream->header_count_audio, header_count_audio);
+  
+    struct timespec ts = _x_compute_interval(1000);
+    int ret_wait;
 
-    gettimeofday(&tv, NULL);
-    ts.tv_sec  = tv.tv_sec + 1;
-    ts.tv_nsec = tv.tv_usec * 1000;
     /* use timedwait to workaround buggy pthread broadcast implementations */
-    pthread_cond_timedwait (&stream->counter_changed, &stream->counter_lock, &ts);
+    ret_wait = pthread_cond_timedwait (&stream->counter_changed, &stream->counter_lock, &ts);
 
-    if (demux_unstick_ao_loop (stream))
+    if (ret_wait == ETIMEDOUT && demux_unstick_ao_loop (stream) && ++max_iterations > 4){
+      xine_log(stream->xine,
+  XINE_LOG_MSG,_("Stuck in _x_demux_control_headers_done(). Taking the emergency exit\n"));
+      stream->emergency_brake = 1;
       break;
+    }
   }
 
   stream->demux_action_pending = 0;
@@ -371,13 +385,21 @@
   pthread_mutex_unlock( &stream->demux_lock );
 
   pthread_mutex_lock (&stream->counter_lock);
+  struct timespec ts;
+  unsigned int max_iterations = 0;
+  int ret_wait;
   while ((stream->finished_count_audio < finished_count_audio) ||
          (stream->finished_count_video < finished_count_video)) {
     lprintf ("waiting for finisheds.\n");
-    pthread_cond_wait (&stream->counter_changed, &stream->counter_lock);
+    ts = _x_compute_interval(1000);
+    ret_wait = pthread_cond_timedwait (&stream->counter_changed, &stream->counter_lock, &ts);
 
-    if (demux_unstick_ao_loop (stream))
-      /* break amarok */;
+    if (ret_wait == ETIMEDOUT && demux_unstick_ao_loop (stream) && ++max_iterations > 4){
+      xine_log(stream->xine,
+  XINE_LOG_MSG,_("Stuck in demux_loop(). Taking the emergency exit\n"));
+      stream->emergency_brake = 1;
+      break;
+    }
   }
   pthread_mutex_unlock (&stream->counter_lock);
   

---

------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com
_______________________________________________
xine-devel mailing list
xine-devel@...
https://lists.sourceforge.net/lists/listinfo/xine-devel

On Mon, 9 Feb 2009 08:18:45 -0200
Miguel Freitas <mfreitas@...> wrote:

> hi,
>
> this is another "amarok stops playing tracks bug". actually that must
> be the original one, but then some people reported the pulseaudio
> issue on the same bugzilla entry...

        I do not use pulseaudio and I always have this problem when
listening to last.fm using the latest Amarok SVN.

        First I thought it was a phonon bug, but reading this thread it
seems a xine bug, right?

        So, I'm using xine 1.1.16.3, the latest one possible to use
with phonon and the problem remains.

        What are your suggestions? Any hints? How can I help you to
debug this, because it's really annoying behaviour and I will do my
best to help debug it.

        Thanks.

--
Linux 2.6.30-rc2: Temporary Tasmanian Devil



------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations
Conference from O'Reilly Media. Velocity features a full day of
expert-led, hands-on workshops and two days of sessions from industry
leaders in dedicated Performance & Operations tracks. Use code vel09scf
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf
_______________________________________________
xine-devel mailing list
xine-devel@...
https://lists.sourceforge.net/lists/listinfo/xine-devel

On Mon, 9 Feb 2009 08:18:45 -0200
Miguel Freitas <mfreitas@...> wrote:

> hi,
>
> this is another "amarok stops playing tracks bug". actually that must
> be the original one, but then some people reported the pulseaudio
> issue on the same bugzilla entry...

        I do not use pulseaudio and I always have this problem when
listening to last.fm using the latest Amarok SVN.

        First I thought it was a phonon bug, but reading this thread it
seems a xine bug, right?

        So, I'm using xine 1.1.16.3, the latest one possible to use
with phonon and the problem remains.

        What are your suggestions? Any hints? How can I help you to
debug this, because it's really annoying behaviour and I will do my
best to help debug it.

        Thanks.


--
Linux 2.6.30-rc2: Temporary Tasmanian Devil



------------------------------------------------------------------------------
Stay on top of everything new and different, both inside and
around Java (TM) technology - register by April 22, and save
$200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
300 plus technical and hands-on sessions. Register today.
Use priority code J9JMT32. http://p.sf.net/sfu/p
_______________________________________________
xine-devel mailing list
xine-devel@...
https://lists.sourceforge.net/lists/listinfo/xine-devel