Stop abusing the queuing features, and start treating them with respect. This seems to solve audio skipping and track skipping issues as well as seeking inside the disk buffer for me.

git-svn-id: svn://svn.rockbox.org/rockbox/trunk@9534 a1c6a512-1295-4272-9138-f99709370657

1 files changed, 105 insertions, 71 deletions

diff --git a/apps/playback.c b/apps/playback.c
index 6baf683342..ce80ee515c 100644
--- a/apps/playback.c
+++ b/apps/playback.c
@@ -85,9 +85,9 @@ static volatile bool paused;
 #define CODEC_AIFF     "/.rockbox/codecs/aiff.codec"
 
 #define AUDIO_DEFAULT_FIRST_LIMIT    (1024*1024*10)
-#define AUDIO_FILL_CYCLE             (1024*256)
 #define AUDIO_DEFAULT_WATERMARK      (1024*512)
 #define AUDIO_DEFAULT_FILECHUNK      (1024*32)
+#define AUDIO_FILEBUF_CRITICAL       (1024*128)
 
 enum {
     Q_AUDIO_PLAY = 1,
@@ -104,7 +104,7 @@ enum {
     Q_AUDIO_DIR_NEXT,
     Q_AUDIO_DIR_PREV,
     Q_AUDIO_POSTINIT,
-    Q_AUDIO_CHECK_BUFFER,
+    Q_AUDIO_FILL_BUFFER,
 
     Q_CODEC_LOAD,
     Q_CODEC_LOAD_DISK,
@@ -448,6 +448,11 @@ void codec_set_offset_callback(size_t value)
     }
 }
 
+static bool filebuf_is_lowdata(void)
+{
+    return filebufused < AUDIO_FILEBUF_CRITICAL;
+}
+
 static void advance_buffer_counters(size_t amount) {
     buf_ridx += amount;
     if (buf_ridx >= filebuflen)
@@ -456,8 +461,10 @@ static void advance_buffer_counters(size_t amount) {
     cur_ti->available -= amount;
     filebufused -= amount;
 
-    if (!pcmbuf_is_lowdata() && !mutex_bufferfill.locked)
-        queue_post(&audio_queue, Q_AUDIO_CHECK_BUFFER, 0);
+    /* Start buffer filling as necessary. */
+    if (!pcmbuf_is_lowdata() && !filling)
+        if (conf_watermark && filebufused <= conf_watermark && playing)
+            queue_post(&audio_queue, Q_AUDIO_FILL_BUFFER, 0);
 }
 
 /* copy up-to size bytes into ptr and return the actual size copied */
@@ -480,7 +487,7 @@ size_t codec_filebuf_callback(void *ptr, size_t size)
 
     /* Let the disk buffer catch fill until enough data is available */
     while (copy_n > cur_ti->available) {
-        queue_post(&audio_queue, Q_AUDIO_CHECK_BUFFER, 0);
+        queue_post(&audio_queue, Q_AUDIO_FILL_BUFFER, 0);
         yield();
         if (ci.stop_codec || ci.reload_codec)
             return 0;
@@ -567,7 +574,7 @@ void* codec_request_buffer_callback(size_t *realsize, size_t reqsize)
     }
 
     while (copy_n > cur_ti->available) {
-        queue_post(&audio_queue, Q_AUDIO_CHECK_BUFFER, 0);
+        queue_post(&audio_queue, Q_AUDIO_FILL_BUFFER, 0);
         yield();
         if (ci.stop_codec || ci.reload_codec) {
             *realsize = 0;
@@ -624,7 +631,7 @@ static bool rebuffer_and_seek(size_t newpos)
 
     mutex_unlock(&mutex_bufferfill);
 
-    queue_post(&audio_queue, Q_AUDIO_CHECK_BUFFER, 0);
+    queue_post(&audio_queue, Q_AUDIO_FILL_BUFFER, 0);
 
     return true;
 }
@@ -802,17 +809,26 @@ static void pcmbuf_track_changed_callback(void)
     queue_post(&audio_queue, Q_AUDIO_TRACK_CHANGED, 0);
 }
 
-/* Give codecs or file buffering the right amount of processing time
-   to prevent pcm audio buffer from going empty. */
-static void yield_codecs(void)
+/* Yield to codecs for as long as possible if they are in need of data
+ * return true if the caller should break to let the audio thread process
+ * new events */
+static bool yield_codecs(void)
 {
     yield();
+    /* This indicates that we are in the initial buffer fill mode, or the
+     * playback recently skipped */
     if (!pcm_is_playing() && !pcm_is_paused())
         sleep(5);
+    
     while ((pcmbuf_is_crossfade_active() || pcmbuf_is_lowdata())
-            && !ci.stop_codec && playing && queue_empty(&audio_queue)
-            && filebufused > (128*1024))
+            && !ci.stop_codec && playing && !filebuf_is_lowdata())
+    {
         sleep(1);
+        if (!queue_empty(&audio_queue)) {
+            return true;
+        }
+    }
+    return false;
 }
 
 /* FIXME: This code should be made more generic and move to metadata.c */
@@ -855,9 +871,8 @@ void strip_id3v1_tag(void)
     }
 }
 
-static void audio_fill_file_buffer(void)
+static void audio_read_file(void)
 {
-    size_t size;
     size_t copy_n;
     int rc;
 
@@ -869,17 +884,24 @@ static void audio_fill_file_buffer(void)
         tracks[track_widx].codecsize = 0;
 
     mutex_lock(&mutex_bufferfill);
-    size = MIN(tracks[track_widx].filerem, AUDIO_FILL_CYCLE);
-    while (size > 0) {
-        /* Give codecs some processing time. */
-        yield_codecs();
+    while (tracks[track_widx].filerem > 0) {
+        /* Let the codec process until it is out of the danger zone, or there
+         * is an event to handle.  In the latter case, break this fill cycle
+         * immediately */
+        if (yield_codecs())
+            break;
 
         if (fill_bytesleft == 0)
             break ;
+
+        /* copy_n is the largest chunk that is safe to read */
         copy_n = MIN(conf_filechunk, filebuflen - buf_widx);
         copy_n = MIN(copy_n, fill_bytesleft);
+        /* rc is the actual amount read */
         rc = read(current_fd, &filebuf[buf_widx], copy_n);
+
         if (rc <= 0) {
+            /* Reached the end of the file */
             tracks[track_widx].filerem = 0;
             break ;
         }
@@ -892,15 +914,12 @@ static void audio_fill_file_buffer(void)
         tracks[track_widx].filepos += rc;
         filebufused += rc;
         fill_bytesleft -= rc;
-        if ((unsigned)rc > size) {
-            rc = size;
-            logf("audio_fill_file_buffer read past end of file\n");
-        }
-        size -= rc;
     }
 
     if (tracks[track_widx].filerem == 0) {
         strip_id3v1_tag();
+        close(current_fd);
+        current_fd = -1;
     }
 
     mutex_unlock(&mutex_bufferfill);
@@ -986,18 +1005,8 @@ static bool loadcodec(bool start_play)
 
     tracks[track_widx].codecsize = 0;
 
-    if (!start_play) {
-        prev_track = track_widx - 1;
-        if (prev_track < 0)
-            prev_track = MAX_TRACK-1;
-        if (track_count > 0 &&
-            get_codec_base_type(tracks[track_widx].id3.codectype) ==
-            get_codec_base_type(tracks[prev_track].id3.codectype))
-        {
-            logf("Reusing prev. codec");
-            return true;
-        }
-    } else {
+    if (start_play)
+    {
         /* Load the codec directly from disk and save some memory. */
         cur_ti = &tracks[track_widx];
         ci.filesize = cur_ti->filesize;
@@ -1009,14 +1018,31 @@ static bool loadcodec(bool start_play)
         queue_post(&codec_queue, Q_CODEC_LOAD_DISK, (void *)codec_path);
         return true;
     }
+    else
+    {
+        /* If the previous codec is the same as this one, there is no need
+         * to put another copy of it on the file buffer */
+        prev_track = track_widx - 1;
+        if (prev_track < 0)
+            prev_track = MAX_TRACK-1;
+        if (track_count > 0 &&
+            get_codec_base_type(tracks[track_widx].id3.codectype) ==
+            get_codec_base_type(tracks[prev_track].id3.codectype))
+        {
+            logf("Reusing prev. codec");
+            return true;
+        }
+    }
 
     fd = open(codec_path, O_RDONLY);
-    if (fd < 0) {
+    if (fd < 0)
+    {
         logf("Codec doesn't exist!");
         return false;
     }
 
     size = filesize(fd);
+    /* Never load a partial codec */
     if (fill_bytesleft < size + conf_watermark) {
         logf("Not enough space");
         /* Set codectype back to zero to indicate no codec was loaded. */
@@ -1026,8 +1052,12 @@ static bool loadcodec(bool start_play)
         return false;
     }
 
+    mutex_lock(&mutex_bufferfill);
     i = 0;
     while (i < size) {
+        /* FIXME: This will spin around pretty quickly, but still requires
+         * full read of the codec when an event is posted to the audio
+         * queue during this loop */
         yield_codecs();
 
         copy_n = MIN(conf_filechunk, filebuflen - buf_widx);
@@ -1041,6 +1071,8 @@ static bool loadcodec(bool start_play)
             buf_widx -= filebuflen;
         i += rc;
     }
+    mutex_unlock(&mutex_bufferfill);
+
     close(fd);
     logf("Done: %dB", i);
 
@@ -1115,10 +1147,10 @@ static bool audio_load_track(int offset, bool start_play, int peek_offset)
         return false;
 
     peek_again:
-    /* Get track name from current playlist read position. */
     logf("Buffering track:%d/%d", track_widx, track_ridx);
-    /* Handle broken playlists. */
+    /* Get track name from current playlist read position. */
     while ( (trackname = playlist_peek(peek_offset)) != NULL) {
+        /* Handle broken playlists. */
         fd = open(trackname, O_RDONLY);
         if (fd < 0) {
             logf("Open failed");
@@ -1141,13 +1173,9 @@ static bool audio_load_track(int offset, bool start_play, int peek_offset)
     tracks[track_widx].filesize = size;
     tracks[track_widx].filepos = 0;
     tracks[track_widx].available = 0;
-    //tracks[track_widx].taginfo_ready = false;
     tracks[track_widx].playlist_offset = peek_offset;
     last_peek_offset = peek_offset;
 
-    if (buf_widx >= filebuflen)
-        buf_widx -= filebuflen;
-
     /* Set default values */
     if (start_play) {
         int last_codec = current_codec;
@@ -1239,22 +1267,20 @@ static bool audio_load_track(int offset, bool start_play, int peek_offset)
     }
     current_fd = fd;
 
-    audio_fill_file_buffer();
+    audio_read_file();
 
     if (!start_play)
         track_count++;
 
-    /* Leave the file handle open for faster buffer refill. */
-    if (tracks[track_widx].filerem != 0) {
-        logf("Partially buf:%d", tracks[track_widx].available);
-    } else {
+    if (tracks[track_widx].filerem == 0) {
         logf("Completely buf.");
-        close(fd);
-
         if (++track_widx >= MAX_TRACK) {
             track_widx = 0;
         }
         tracks[track_widx].filesize = 0;
+        tracks[track_widx].filerem = 0;
+    } else {
+        logf("Partially buf:%d", tracks[track_widx].available);
     }
 
     return true;
@@ -1402,13 +1428,16 @@ static void initialize_buffer_fill(void)
     /* Save the current resume position once. */
     playlist_update_resume_info(audio_current_track());
 
-    fill_bytesleft = filebuflen - filebufused;
     cur_ti->start_pos = ci.curpos;
 
     pcmbuf_set_boost_mode(true);
 
     filling = true;
 
+    /* FIXME: This should be recalculated more often than once per fill cycle.
+     * This way we only fill up to the read point when filling started. */
+    fill_bytesleft = filebuflen - filebufused;
+
     /* Calculate real track count after throwing away old tracks. */
     cur_idx = track_ridx;
     for (i = 0; i < track_count; i++) {
@@ -1431,21 +1460,19 @@ static void initialize_buffer_fill(void)
     audio_clear_track_entries(true);
 }
 
-static void audio_check_buffer(void)
+static void audio_fill_file_buffer(void)
 {
-    /* Start buffer filling as necessary. */
-    if ((!conf_watermark || filebufused > conf_watermark || !playing
-                || ci.stop_codec || ci.reload_codec || playlist_end)
-            && !filling)
-        return;
+    if (!filling)
+        if (ci.stop_codec || ci.reload_codec || playlist_end)
+            return;
 
     mutex_lock(&mutex_bufferfill);
     initialize_buffer_fill();
     mutex_unlock(&mutex_bufferfill);
 
     /* Limit buffering size at first run. */
-    if (conf_bufferlimit && fill_bytesleft > conf_bufferlimit
-            - filebufused) {
+    if (conf_bufferlimit &&
+            fill_bytesleft > conf_bufferlimit - filebufused) {
         if (conf_bufferlimit > filebufused)
             fill_bytesleft = conf_bufferlimit - filebufused;
         else
@@ -1458,18 +1485,14 @@ static void audio_check_buffer(void)
         /* Try to load remainings of the file. */
         if (tracks[track_widx].filerem > 0)
         {
-            audio_fill_file_buffer();
+            audio_read_file();
         }
+        /* The current track is done, increase the track pointer */
         if (tracks[track_widx].filerem == 0) {
             if (++track_widx == MAX_TRACK)
                 track_widx = 0;
             tracks[track_widx].filesize = 0;
         }
-        else
-        {
-            /* Done filling, couldn't read that whole file */
-            fill_bytesleft = 0;
-        }
     }
     /* Otherwise, load a new track */
     else if (audio_load_track(0, false, last_peek_offset + 1))
@@ -1797,19 +1820,30 @@ void audio_thread(void)
 
     while (1) {
         if (play_pending)
+        {
             queue_wait_w_tmo(&audio_queue, &ev, 0);
+            if (ev.id == SYS_TIMEOUT)
+            {
+                ev.id = Q_AUDIO_PLAY;
+                ev.data = (bool *)1;
+            }
+        }
+        else if (filling)
+        {
+            queue_wait_w_tmo(&audio_queue, &ev, 0);
+            if (ev.id == SYS_TIMEOUT)
+            {
+                ev.id = Q_AUDIO_FILL_BUFFER;
+                ev.data = (bool *)1;
+            }
+        }
         else
             queue_wait(&audio_queue, &ev);
 
-        if (ev.id == SYS_TIMEOUT && play_pending)
-        {
-            ev.id = Q_AUDIO_PLAY;
-            ev.data = (bool *)1;
-        }
 
         switch (ev.id) {
-            case Q_AUDIO_CHECK_BUFFER:
-                audio_check_buffer();
+            case Q_AUDIO_FILL_BUFFER:
+                audio_fill_file_buffer();
                 break;
             case Q_AUDIO_PLAY:
                 /* Don't start playing immediately if user is skipping tracks