[Feature] playlist_viewer id3 title display

Not sure this is a great idea from disk and battery standpoint
but there is no reason you can't..

using the name buffer to fill title data
prevent hitting the disk for each screen scroll

add get_metadata_ex to allow flags
 METADATA_EXCLUDE_ID3_PATH
  prevent copying the filename to the ID3 struct
 METADATA_CLOSE_FD_ON_EXIT
  instead of seeking to the beginning the file is closed before
  get_metadata returns

add logic to allow a invalid fd to signal that get_metadata
should open and close the file within its call

bugfix per Chris_s don't use the tagcache for the trackinfo

Change-Id: Ic7a595b39a8d7a57f975312bc9c8bb4111f22a88

9 files changed, 145 insertions, 89 deletions

diff --git a/apps/buffering.c b/apps/buffering.c
index bf41544c56..b9f30fc6df 100644
--- a/apps/buffering.c
+++ b/apps/buffering.c
@@ -652,11 +652,9 @@ static bool buffer_handle(int handle_id, size_t to_buffer)
     trigger_cpu_boost();
 
     if (h->type == TYPE_ID3) {
-        if (!get_metadata(ringbuf_ptr(h->data), h->fd, h->path)) {
-            /* metadata parsing failed: clear the buffer. */
-            wipe_mp3entry(ringbuf_ptr(h->data));
-        }
-        close_fd(&h->fd);
+        get_metadata_ex(ringbuf_ptr(h->data),
+                        h->fd, h->path, METADATA_CLOSE_FD_ON_EXIT);
+        h->fd = -1; /* with above, behavior same as close_fd */
         h->widx = ringbuf_add(h->data, h->filesize);
         h->end  = h->filesize;
         send_event(BUFFER_EVENT_FINISHED, &handle_id);
diff --git a/apps/iap/iap-core.c b/apps/iap/iap-core.c
index da04a67311..3faf8237bf 100644
--- a/apps/iap/iap-core.c
+++ b/apps/iap/iap-core.c
@@ -685,7 +685,6 @@ bool iap_getc(const unsigned char x)
 void iap_get_trackinfo(const unsigned int track, struct mp3entry* id3)
 {
     int tracknum;
-    int fd;
     struct playlist_track_info info;
 
     tracknum = track;
@@ -699,10 +698,8 @@ void iap_get_trackinfo(const unsigned int track, struct mp3entry* id3)
     if(playlist_next(0) != tracknum)
     {
         playlist_get_track_info(NULL, tracknum, &info);
-        fd = open(info.filename, O_RDONLY);
-        memset(id3, 0, sizeof(*id3));
-        get_metadata(id3, fd, info.filename);
-        close(fd);
+        /* memset(id3, 0, sizeof(*id3)) --get_metadata does this for us */
+        get_metadata(id3, -1, info.filename);
     } else {
         memcpy(id3, audio_current_track(), sizeof(*id3));
     }
diff --git a/apps/iap/iap-lingo4.c b/apps/iap/iap-lingo4.c
index e1ab150fd9..6eb63003a5 100644
--- a/apps/iap/iap-lingo4.c
+++ b/apps/iap/iap-lingo4.c
@@ -1787,7 +1787,6 @@ void iap_handlepkt_mode4(const unsigned int len, const unsigned char *buf)
         {
             unsigned char data[70] = {0x04, 0x00, 0xFF};
             struct mp3entry id3;
-            int fd;
             size_t len;
             long tracknum = get_u32(&buf[3]);
 
@@ -1802,10 +1801,8 @@ void iap_handlepkt_mode4(const unsigned int len, const unsigned char *buf)
             {
                 struct playlist_track_info info;
                 playlist_get_track_info(NULL, tracknum, &info);
-                fd = open(info.filename, O_RDONLY);
-                memset(&id3, 0, sizeof(struct mp3entry));
-                get_metadata(&id3, fd, info.filename);
-                close(fd);
+                /* memset(&id3, 0, sizeof(struct mp3entry)); --get_metadata does this for us */
+                get_metadata(&id3, -1, info.filename);
             }
             /* Return the requested track data */
             switch(cmd)
diff --git a/apps/playback.c b/apps/playback.c
index 18f62fb69b..8a30af5199 100644
--- a/apps/playback.c
+++ b/apps/playback.c
@@ -2022,8 +2022,7 @@ static int audio_load_track(void)
         if (fd >= 0)
         {
             id3_mutex_lock();
-            if(!get_metadata(ub_id3, fd, path))
-                wipe_mp3entry(ub_id3);
+            get_metadata(ub_id3, fd, path);
             id3_mutex_unlock();
         }
 
diff --git a/apps/playlist_viewer.c b/apps/playlist_viewer.c
index 579eba5a61..d06a3f3df6 100644
--- a/apps/playlist_viewer.c
+++ b/apps/playlist_viewer.c
@@ -51,6 +51,10 @@
 #include "menus/exported_menus.h"
 #include "yesno.h"
 
+#if defined(HAVE_TC_RAMCACHE) && defined(HAVE_DIRCACHE)
+#include "tagcache.h"
+#endif
+
 /* Maximum number of tracks we can have loaded at one time */
 #define MAX_PLAYLIST_ENTRIES 200
 
@@ -60,11 +64,11 @@
 
 /* Information about a specific track */
 struct playlist_entry {
-    char *name;                 /* Formatted track name                     */
+    char *name;                 /* track path                               */
+    char *title;                /* Formatted track name                     */
     int index;                  /* Playlist index                           */
     int display_index;          /* Display index                            */
-    bool queued;                /* Is track queued?                         */
-    bool skipped;               /* Is track marked as bad?                  */
+    int attr;                  /* Is track queued?; Is track marked as bad?*/
 };
 
 enum direction
@@ -86,6 +90,8 @@ enum pv_onplay_result {
 
 struct playlist_buffer
 {
+    struct playlist_entry tracks[MAX_PLAYLIST_ENTRIES];
+
     char *name_buffer;        /* Buffer used to store track names */
     int buffer_size;          /* Size of name buffer */
 
@@ -97,7 +103,6 @@ struct playlist_buffer
                                  the buffer has a real index < to the
                                  real index of the the first track)*/
 
-    struct playlist_entry tracks[MAX_PLAYLIST_ENTRIES];
     int num_loaded;           /* Number of track entries loaded in buffer */
 };
 
@@ -136,7 +141,6 @@ static bool playlist_viewer_init(struct playlist_viewer * viewer,
                                  const char* filename, bool reload,
                                  int *most_recent_selection);
 
-static void format_name(char* dest, const char* src);
 static void format_line(const struct playlist_entry* track, char* str,
                         int len);
 
@@ -221,6 +225,40 @@ static void playlist_buffer_load_entries_screen(struct playlist_buffer * pb,
     }
 }
 
+static bool retrieve_track_metadata(struct mp3entry *id3, const char *filename, int flags)
+{
+    bool success = true;
+#if defined(HAVE_TC_RAMCACHE) && defined(HAVE_DIRCACHE)
+    /* try to get the id3 data from the database */
+    /* the database, doesn't store frequency, file size or codec (g4470) ChrisS*/
+    if ((flags & METADATA_EXCLUDE_ID3_PATH) || !tagcache_fill_tags(id3, filename))
+#endif
+    /* fall back to reading the file from disk */
+    {
+        success = get_metadata_ex(id3, -1, filename, flags);
+    }
+    return success;
+}
+
+static int load_track_title(char* buffer, size_t bufsz, char *filename)
+{
+    struct mp3entry id3;
+    if (retrieve_track_metadata(&id3, filename, METADATA_EXCLUDE_ID3_PATH)
+       && id3.title && id3.title[0] != '\0')
+    {
+        const char *artist = id3.artist;
+        if (!artist)
+            artist = id3.albumartist;
+        if(!artist)
+            artist = str(LANG_TAGNAVI_UNTAGGED);
+
+        size_t len = snprintf(buffer, bufsz, "%s - %s", artist, id3.title) + 1;
+        if (len < bufsz)
+            return len;
+    }
+    return 0; /*Failure*/
+}
+
 static int playlist_entry_load(struct playlist_entry *entry, int index,
                                char* name_buffer, int remaining_size)
 {
@@ -234,17 +272,29 @@ static int playlist_entry_load(struct playlist_entry *entry, int index,
     if (playlist_get_track_info(viewer.playlist, index, &info) < 0)
         return -1;
 
-    len = strlen(info.filename) + 1;
+    len = strlcpy(name_buffer, info.filename, remaining_size) + 1;
 
     if (len <= remaining_size)
     {
-        strcpy(name_buffer, info.filename);
-
         entry->name = name_buffer;
+        entry->title = name_buffer;
         entry->index = info.index;
         entry->display_index = info.display_index;
-        entry->queued = info.attr & PLAYLIST_ATTR_QUEUED;
-        entry->skipped = info.attr & PLAYLIST_ATTR_SKIPPED;
+        entry->attr = info.attr & (PLAYLIST_ATTR_SKIPPED | PLAYLIST_ATTR_QUEUED);
+
+        if (global_settings.playlist_viewer_track_display == 2) /* title */
+        {
+            /* advance buffer position past filename, adjust length remaining */
+            name_buffer += len;
+            remaining_size -= len;
+            int tlen = load_track_title(name_buffer, remaining_size, info.filename);
+            if (tlen > 0)
+            {
+                entry->title = name_buffer;
+                len += tlen;
+            }
+        }
+
         return len;
     }
     return -1;
@@ -433,7 +483,7 @@ static bool playlist_viewer_init(struct playlist_viewer * viewer,
 }
 
 /* Format trackname for display purposes */
-static void format_name(char* dest, const char* src)
+static void format_name(char* dest, const char* src, size_t bufsz)
 {
     switch (global_settings.playlist_viewer_track_display)
     {
@@ -442,17 +492,16 @@ static void format_name(char* dest, const char* src)
         {
             /* Only display the filename */
             char* p = strrchr(src, '/');
-
-            strcpy(dest, p+1);
-
+            strlcpy(dest, p+1, bufsz);
             /* Remove the extension */
             strrsplt(dest, '.');
-
             break;
         }
+        case 2: /* Artist - Title */
+            /*fall-through*/
         case 1:
             /* Full path */
-            strcpy(dest, src);
+            strlcpy(dest, src, bufsz);
             break;
     }
 }
@@ -463,10 +512,9 @@ static void format_line(const struct playlist_entry* track, char* str,
 {
     char name[MAX_PATH];
     char *skipped = "";
+    format_name(name, track->title, sizeof(name));
 
-    format_name(name, track->name);
-
-    if (track->skipped)
+    if (track->attr & PLAYLIST_ATTR_SKIPPED)
         skipped = "(ERR) ";
 
     if (global_settings.playlist_viewer_indices)
@@ -524,13 +572,7 @@ static enum pv_onplay_result show_track_info(const struct playlist_entry *curren
     }
     else
     {
-        int fd = open(current_track->name, O_RDONLY);
-        if (fd >= 0)
-        {
-            if (get_metadata(&id3, fd, current_track->name))
-                id3_retrieval_successful = true;
-            close(fd);
-        }
+        id3_retrieval_successful = retrieve_track_metadata(&id3, current_track->name, 0);
     }
 
     return id3_retrieval_successful &&
@@ -755,7 +797,7 @@ static enum themable_icons playlist_callback_icons(int selected_item,
         /* Track we are moving */
         return Icon_Moving;
     }
-    else if (track->queued)
+    else if (track->attr & PLAYLIST_ATTR_QUEUED)
     {
         /* Queued track */
         return Icon_Queued;
@@ -776,17 +818,27 @@ static int playlist_callback_voice(int selected_item, void *data)
             talk_id(LANG_NOW_PLAYING, true);
         if (track->index == local_viewer->moving_track)
             talk_id(VOICE_TRACK_TO_MOVE, true);
-        if (track->queued)
+        if (track->attr & PLAYLIST_ATTR_QUEUED)
             talk_id(VOICE_QUEUED, true);
     }
-    if (track->skipped)
+    if (track->attr & PLAYLIST_ATTR_SKIPPED)
         talk_id(VOICE_BAD_TRACK, true);
     if (global_settings.playlist_viewer_indices)
         talk_number(track->display_index, true);
 
-    if(global_settings.playlist_viewer_track_display)
-        talk_fullpath(track->name, true);
-    else talk_file_or_spell(NULL, track->name, NULL, true);
+    switch(global_settings.playlist_viewer_track_display)
+    {
+        case 1: /*full path*/
+            talk_fullpath(track->name, true);
+            break;
+        case 2: /*title*/
+            talk_spell(track->title, true);
+            break;
+        default:
+        case 0: /*filename only*/
+            talk_file_or_spell(NULL, track->name, NULL, true);
+            break;
+    }
     if (viewer.moving_track != -1)
         talk_ids(true,VOICE_PAUSE, VOICE_MOVING_TRACK);
 
@@ -1132,11 +1184,11 @@ static void close_playlist_viewer(void)
 static const char* playlist_search_callback_name(int selected_item, void * data,
                                                  char *buffer, size_t buffer_len)
 {
-    (void)buffer_len; /* this should probably be used */
     int *found_indicies = (int*)data;
     static struct playlist_track_info track;
     playlist_get_track_info(viewer.playlist, found_indicies[selected_item], &track);
-    format_name(buffer, track.filename);
+
+    format_name(buffer, track.filename, buffer_len);
     return buffer;
 }
 
@@ -1145,7 +1197,7 @@ static int say_search_item(int selected_item, void *data)
     int *found_indicies = (int*)data;
     static struct playlist_track_info track;
     playlist_get_track_info(viewer.playlist,found_indicies[selected_item],&track);
-    if(global_settings.playlist_viewer_track_display)
+    if(global_settings.playlist_viewer_track_display == 1)
         talk_fullpath(track.filename, false);
     else talk_file_or_spell(NULL, track.filename, NULL, false);
     return 0;
diff --git a/apps/settings_list.c b/apps/settings_list.c
index 09a45f1faa..60a2735e90 100644
--- a/apps/settings_list.c
+++ b/apps/settings_list.c
@@ -1415,9 +1415,9 @@ const struct settings_list settings[] = {
     OFFON_SETTING(0,playlist_viewer_indices,LANG_SHOW_INDICES,true,
                   "playlist viewer indices",NULL),
     CHOICE_SETTING(0, playlist_viewer_track_display, LANG_TRACK_DISPLAY, 0,
-                   "playlist viewer track display","track name,full path",
-                   NULL, 2, ID2P(LANG_DISPLAY_TRACK_NAME_ONLY),
-                   ID2P(LANG_DISPLAY_FULL_PATH)),
+                   "playlist viewer track display","track name,full path,id3 title",
+                   NULL, 3, ID2P(LANG_DISPLAY_TRACK_NAME_ONLY),
+                   ID2P(LANG_DISPLAY_FULL_PATH), ID2P(LANG_ID3_TITLE)),
     CHOICE_SETTING(0, recursive_dir_insert, LANG_RECURSE_DIRECTORY , RECURSE_ON,
                    "recursive directory insert", off_on_ask, NULL , 3 ,
                    ID2P(LANG_OFF), ID2P(LANG_ON), ID2P(LANG_ASK)),
diff --git a/apps/tagcache.c b/apps/tagcache.c
index 5413e2def0..1412647368 100644
--- a/apps/tagcache.c
+++ b/apps/tagcache.c
@@ -2193,7 +2193,6 @@ static void NO_INLINE add_tagcache(char *path, unsigned long mtime)
     struct mp3entry id3;
     struct temp_file_entry entry;
     bool ret;
-    int fd;
     int idx_id = -1;
     char tracknumfix[3];
     int offset = 0;
@@ -2266,21 +2265,16 @@ static void NO_INLINE add_tagcache(char *path, unsigned long mtime)
         }
     }
 
-    fd = open(path, O_RDONLY);
-    if (fd < 0)
-    {
-        logf("open fail: %s", path);
-        return ;
-    }
-
-    memset(&id3, 0, sizeof(struct mp3entry));
+    /*memset(&id3, 0, sizeof(struct mp3entry)); -- get_metadata does this for us */
     memset(&entry, 0, sizeof(struct temp_file_entry));
     memset(&tracknumfix, 0, sizeof(tracknumfix));
-    ret = get_metadata(&id3, fd, path);
-    close(fd);
+    ret = get_metadata_ex(&id3, -1, path, METADATA_EXCLUDE_ID3_PATH);
 
     if (!ret)
+    {
+        logf("get_metadata fail: %s", path);
         return ;
+    }
 
     logf("-> %s", path);
 
diff --git a/lib/rbcodec/metadata/metadata.c b/lib/rbcodec/metadata/metadata.c
index 19147ccdb3..28cef46d92 100644
--- a/lib/rbcodec/metadata/metadata.c
+++ b/lib/rbcodec/metadata/metadata.c
@@ -394,25 +394,19 @@ unsigned int probe_file_format(const char *filename)
 /* Note, that this returns false for successful, true for error! */
 bool mp3info(struct mp3entry *entry, const char *filename)
 {
-    int fd;
-    bool result;
-
-    fd = open(filename, O_RDONLY);
-    if (fd < 0)
-        return true;
-
-    result = !get_metadata(entry, fd, filename);
-
-    close(fd);
-
-    return result;
+    return !get_metadata(entry, -1, filename);
 }
 
 /* Get metadata for track - return false if parsing showed problems with the
- * file that would prevent playback.
+ * file that would prevent playback. supply a filedescriptor <0 and the file will be opened
+ * and closed automatically within the get_metadata call
+ * get_metadata_ex allows flags to change the way get_metadata behaves
+ * METADATA_EXCLUDE_ID3_PATH  won't copy filename path to the id3 path buffer 
+ * METADATA_CLOSE_FD_ON_EXIT closes the open filedescriptor on exit
  */
-bool get_metadata(struct mp3entry* id3, int fd, const char* trackname)
+bool get_metadata_ex(struct mp3entry* id3, int fd, const char* trackname, int flags)
 {
+    bool success = true;
     const struct afmt_entry *entry;
     int logfd = 0;
     DEBUGF("Read metadata for %s\n", trackname);
@@ -426,10 +420,20 @@ bool get_metadata(struct mp3entry* id3, int fd, const char* trackname)
             close(logfd);
         }
     }
-
     /* Clear the mp3entry to avoid having bogus pointers appear */
     wipe_mp3entry(id3);
 
+    if (fd < 0)
+    {
+        fd = open(trackname, O_RDONLY);
+        flags |= METADATA_CLOSE_FD_ON_EXIT;
+        if (fd < 0)
+        {
+            DEBUGF("Error opening %s\n", trackname);
+            return false; /*Failure*/
+        }
+    }
+
     /* Take our best guess at the codec type based on file extension */
     id3->codectype = probe_file_format(trackname);
 
@@ -443,19 +447,31 @@ bool get_metadata(struct mp3entry* id3, int fd, const char* trackname)
     if (!entry->parse_func)
     {
         DEBUGF("nothing to parse for %s (format %s)\n", trackname, entry->label);
-        return false;
+        success = false;
     }
-
-    if (!entry->parse_func(fd, id3))
+    else if (!entry->parse_func(fd, id3))
     {
         DEBUGF("parsing %s failed (format: %s)\n", trackname, entry->label);
-        return false;
+        success = false;
+        wipe_mp3entry(id3); /* ensure the mp3entry is clear */
     }
 
-    lseek(fd, 0, SEEK_SET);
-    strlcpy(id3->path, trackname, sizeof(id3->path));
-    /* We have successfully read the metadata from the file */
-    return true;
+    if ((flags & METADATA_CLOSE_FD_ON_EXIT))
+        close(fd);
+    else
+        lseek(fd, 0, SEEK_SET);
+
+    if (success && (flags & METADATA_EXCLUDE_ID3_PATH) == 0)
+    {
+        strlcpy(id3->path, trackname, sizeof(id3->path));
+    }
+    /* have we successfully read the metadata from the file? */
+    return success;
+}
+
+bool get_metadata(struct mp3entry* id3, int fd, const char* trackname)
+{
+    return get_metadata_ex(id3, fd, trackname, 0);
 }
 
 #define MOVE_ENTRY(x) if (x) x += offset;
diff --git a/lib/rbcodec/metadata/metadata.h b/lib/rbcodec/metadata/metadata.h
index b30979334a..a0ba0376c6 100644
--- a/lib/rbcodec/metadata/metadata.h
+++ b/lib/rbcodec/metadata/metadata.h
@@ -24,6 +24,8 @@
 
 #include "platform.h"
 
+#define METADATA_EXCLUDE_ID3_PATH (0x01) /* don't copy filename path to the id3 path buffer */
+#define METADATA_CLOSE_FD_ON_EXIT (0x02) /* close the filedescriptor when finished */
 /* Audio file types. */
 /* NOTE: The values of the AFMT_* items are used for the %fc tag in the WPS
          - so new entries MUST be added to the end to maintain compatibility.
@@ -323,6 +325,7 @@ struct mp3entry {
 
 unsigned int probe_file_format(const char *filename);
 bool get_metadata(struct mp3entry* id3, int fd, const char* trackname);
+bool get_metadata_ex(struct mp3entry* id3, int fd, const char* trackname, int flags);
 bool mp3info(struct mp3entry *entry, const char *filename);
 void adjust_mp3entry(struct mp3entry *entry, void *dest, const void *orig);
 void copy_mp3entry(struct mp3entry *dest, const struct mp3entry *orig);