Get rid of those horrible macros to protect against NULL reference when looking up the id3 info for tokens.

Change the way the wps playlist viewer gets the token values. All %i tokens are now supported (and a few others, experiment :) )


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

apps/gui/skin_engine/skin_display.c

@@ -169,17 +169,20 @@ bool audio_peek_track(struct mp3entry* id3, int offset);
 static void draw_playlist_viewer_list(struct gui_wps *gwps,
                                       struct playlistviewer *viewer)
 {
+    struct wps_state *state = gwps->state;
     int lines = viewport_get_nb_lines(viewer->vp);
     int line_height = font_get(viewer->vp->font)->height;
     int cur_playlist_pos = playlist_get_display_index();
     int start_item = MAX(0, cur_playlist_pos + viewer->start_offset);
     int i;
+    struct wps_token token;
     
     struct mp3entry *pid3;
 #if CONFIG_CODEC == SWCODEC
     struct mp3entry id3;
 #endif    
     char buf[MAX_PATH*2], tempbuf[MAX_PATH];
+    unsigned int buf_used = 0;
     
     
     gwps->display->set_viewport(viewer->vp);
@@ -187,11 +190,11 @@ static void draw_playlist_viewer_list(struct gui_wps *gwps,
     {
         if (i == cur_playlist_pos)
         {
-            pid3 = audio_current_track();
+            pid3 = state->id3;
         }
         else if (i == cur_playlist_pos+1)
         {
-            pid3 = audio_next_track();
+            pid3 = state->nid3;
         }
 #if CONFIG_CODEC == SWCODEC
         else if ((i>cur_playlist_pos) && audio_peek_track(&id3, i-cur_playlist_pos))
@@ -200,51 +203,56 @@ static void draw_playlist_viewer_list(struct gui_wps *gwps,
         }
 #endif
         else
+        {
             pid3 = NULL;
+        }
             
         int line = pid3 ? TRACK_HAS_INFO : TRACK_HAS_NO_INFO;        
-        int token = 0, cur_string = 0;
+        int j = 0, cur_string = 0;
         char *filename = playlist_peek(i-cur_playlist_pos);
         buf[0] = '\0';
-        while (token < viewer->lines[line].count)
+        buf_used = 0;
+        while (j < viewer->lines[line].count && (buf_used<sizeof(buf)))
         {
-            switch (viewer->lines[line].tokens[token])
+            const char *out = NULL;
+            token.type = viewer->lines[line].tokens[j];
+            token.value.i = 0;
+            token.next = false;
+            out = get_id3_token(&token, pid3, tempbuf, sizeof(tempbuf), -1, NULL);
+            if (out)
+            {
+                snprintf(&buf[buf_used], sizeof(buf)-buf_used, "%s", out);
+                buf_used += strlen(out);
+                j++;
+                continue;
+            }
+            switch (viewer->lines[line].tokens[j])
             {
                 case WPS_TOKEN_STRING:
                 case WPS_TOKEN_CHARACTER:
-                    strcat(buf, viewer->lines[line].strings[cur_string++]);
+                    snprintf(tempbuf, sizeof(tempbuf), "%s",
+                             viewer->lines[line].strings[cur_string]);
+                    cur_string++;
                     break;
                 case WPS_TOKEN_PLAYLIST_POSITION:
                     snprintf(tempbuf, sizeof(tempbuf), "%d", i);
-                    strcat(buf, tempbuf);
                     break;
                 case WPS_TOKEN_FILE_NAME:
                     get_dir(tempbuf, sizeof(tempbuf), filename, 0);
-                    strcat(buf, tempbuf);                    
                     break;
                 case WPS_TOKEN_FILE_PATH:
-                    strcat(buf, filename);
+                    snprintf(tempbuf, sizeof(tempbuf), "%s", filename);
-                    break;                
-                case WPS_TOKEN_METADATA_ARTIST:
-                    if (pid3)
-                        strcat(buf, pid3->artist ? pid3->artist : "");
                     break;
-                case WPS_TOKEN_METADATA_TRACK_TITLE:
-                    if (pid3)
-                        strcat(buf, pid3->title ? pid3->title : "");
-                    break;
-                case WPS_TOKEN_TRACK_LENGTH:
-                    if (pid3)
-                    {
-                        format_time(tempbuf, sizeof(tempbuf), pid3->length);
-                        strcat(buf, tempbuf);
-                    }
-                    break;                      
-                    
                 default:
+                    tempbuf[0] = '\0';
                     break;
             }
-            token++;
+            if (tempbuf[0])
+            {
+                snprintf(&buf[buf_used], sizeof(buf)-buf_used, "%s", tempbuf);
+                buf_used += strlen(tempbuf);
+            }
+            j++;
         }
             
         if (viewer->lines[line].scroll)

apps/gui/skin_engine/skin_tokens.c

@@ -62,6 +62,8 @@
 #include "language.h"
 #include "usb.h"
 
+extern struct wps_state wps_state;
+
 static char* get_codectype(const struct mp3entry* id3)
 {
     if (id3 && id3->codectype < AFMT_NUM_CODECS) {
@@ -148,8 +150,197 @@ static int pitch_speed_enum(int range, int32_t val, int32_t normval)
 }
 #endif
 
-/* Return the tag found at index i and write its value in buf.
+
-   The return value is buf if the tag had a value, or NULL if not.
+/* All tokens which only need the info to return a value go in here */
+const char *get_id3_token(struct wps_token *token, struct mp3entry *id3,
+                          char *buf, int buf_size, int limit, int *intval)
+{
+    struct wps_state *state = &wps_state;
+    if (id3)
+    {
+        switch (token->type)
+        {
+            case WPS_TOKEN_METADATA_ARTIST:
+                return id3->artist;
+            case WPS_TOKEN_METADATA_COMPOSER:
+                return id3->composer;
+            case WPS_TOKEN_METADATA_ALBUM:
+                return id3->album;
+            case WPS_TOKEN_METADATA_ALBUM_ARTIST:
+                return id3->albumartist;
+            case WPS_TOKEN_METADATA_GROUPING:
+                return id3->grouping;
+            case WPS_TOKEN_METADATA_GENRE:
+                return id3->genre_string;
+            case WPS_TOKEN_METADATA_DISC_NUMBER:
+                if (id3->disc_string)
+                    return id3->disc_string;
+                if (id3->discnum) {
+                    snprintf(buf, buf_size, "%d", id3->discnum);
+                    return buf;
+                }
+                return NULL;
+            case WPS_TOKEN_METADATA_TRACK_NUMBER:
+                if (id3->track_string)
+                    return id3->track_string;
+                if (id3->tracknum) {
+                    snprintf(buf, buf_size, "%d", id3->tracknum);
+                    return buf;
+                }
+                return NULL;
+            case WPS_TOKEN_METADATA_TRACK_TITLE:
+                return id3->title;
+            case WPS_TOKEN_METADATA_VERSION:
+                switch (id3->id3version)
+                {
+                    case ID3_VER_1_0:
+                        return "1";
+                    case ID3_VER_1_1:
+                        return "1.1";
+                    case ID3_VER_2_2:
+                        return "2.2";
+                    case ID3_VER_2_3:
+                        return "2.3";
+                    case ID3_VER_2_4:
+                        return "2.4";
+                    default:
+                        break;
+                }
+                return NULL;
+            case WPS_TOKEN_METADATA_YEAR:
+                if( id3->year_string )
+                    return id3->year_string;
+                if (id3->year) {
+                    snprintf(buf, buf_size, "%d", id3->year);
+                    return buf;
+                }
+                return NULL;
+            case WPS_TOKEN_METADATA_COMMENT:
+                return id3->comment;   
+            case WPS_TOKEN_FILE_PATH:
+                return id3->path;     
+            case WPS_TOKEN_FILE_BITRATE:
+                if(id3->bitrate)
+                    snprintf(buf, buf_size, "%d", id3->bitrate);
+                else
+                    return "?";
+                return buf;
+            case WPS_TOKEN_TRACK_TIME_ELAPSED:
+                format_time(buf, buf_size,
+                            id3->elapsed + state->ff_rewind_count);
+                return buf;
+
+            case WPS_TOKEN_TRACK_TIME_REMAINING:
+                format_time(buf, buf_size,
+                            id3->length - id3->elapsed -
+                            state->ff_rewind_count);
+                return buf;
+
+            case WPS_TOKEN_TRACK_LENGTH:
+                format_time(buf, buf_size, id3->length);
+                return buf;
+
+            case WPS_TOKEN_TRACK_ELAPSED_PERCENT:
+                if (id3->length <= 0)
+                    return NULL;
+
+                if (intval)
+                {
+                    *intval = limit * (id3->elapsed + state->ff_rewind_count)
+                              / id3->length + 1;
+                }
+                snprintf(buf, buf_size, "%d",
+                         100*(id3->elapsed + state->ff_rewind_count) / id3->length);
+                return buf;
+
+
+            case WPS_TOKEN_FILE_CODEC:
+                if (intval)
+                {
+                    if(id3->codectype == AFMT_UNKNOWN)
+                        *intval = AFMT_NUM_CODECS;
+                    else
+                        *intval = id3->codectype;
+                }
+                return get_codectype(id3);
+
+            case WPS_TOKEN_FILE_FREQUENCY:
+                snprintf(buf, buf_size, "%ld", id3->frequency);
+                return buf;
+            case WPS_TOKEN_FILE_FREQUENCY_KHZ:
+                /* ignore remainders < 100, so 22050 Hz becomes just 22k */
+                if ((id3->frequency % 1000) < 100)
+                    snprintf(buf, buf_size, "%ld", id3->frequency / 1000);
+                else
+                    snprintf(buf, buf_size, "%ld.%d",
+                            id3->frequency / 1000,
+                            (id3->frequency % 1000) / 100);
+                return buf;
+            case WPS_TOKEN_FILE_NAME:
+                if (get_dir(buf, buf_size, id3->path, 0)) {
+                    /* Remove extension */
+                    char* sep = strrchr(buf, '.');
+                    if (NULL != sep) {
+                        *sep = 0;
+                    }
+                    return buf;
+                }
+                return NULL;
+            case WPS_TOKEN_FILE_NAME_WITH_EXTENSION:
+                return get_dir(buf, buf_size, id3->path, 0);
+            case WPS_TOKEN_FILE_SIZE:
+                snprintf(buf, buf_size, "%ld", id3->filesize / 1024);
+                return buf;
+            case WPS_TOKEN_FILE_VBR:
+                return (id3->vbr) ? "(avg)" : NULL;
+            case WPS_TOKEN_FILE_DIRECTORY:
+                return get_dir(buf, buf_size, id3->path, token->value.i);
+                                
+#ifdef HAVE_TAGCACHE
+        case WPS_TOKEN_DATABASE_PLAYCOUNT:
+            if (intval)
+                *intval = id3->playcount + 1;
+            snprintf(buf, buf_size, "%ld", id3->playcount);
+            return buf;
+        case WPS_TOKEN_DATABASE_RATING:
+            if (intval)
+                *intval = id3->rating + 1;
+            snprintf(buf, buf_size, "%ld", id3->rating);
+            return buf;
+        case WPS_TOKEN_DATABASE_AUTOSCORE:
+            if (intval)
+                *intval = id3->score + 1;
+            snprintf(buf, buf_size, "%ld", id3->score);
+            return buf;
+#endif
+    
+            default:
+                return NULL;
+        }
+    }
+    else /* id3 == NULL, handle the error based on the expected return type */
+    {
+        switch (token->type)
+        {
+            /* Most tokens expect NULL on error so leave that for the default case,
+             * The ones that expect "0" need to be handled */
+            case WPS_TOKEN_FILE_FREQUENCY:
+            case WPS_TOKEN_FILE_FREQUENCY_KHZ:
+            case WPS_TOKEN_FILE_SIZE:
+            case WPS_TOKEN_DATABASE_PLAYCOUNT:
+            case WPS_TOKEN_DATABASE_RATING:
+            case WPS_TOKEN_DATABASE_AUTOSCORE:
+                if (intval)
+                    *intval = 0;
+                return "0";
+            default:
+                return NULL;
+        }
+    }
+    return buf;
+}
+
+/* Return the tags value as text. buf should be used as temp storage if needed.
 
    intval is used with conditionals/enums: when this function is called,
    intval should contain the number of options in the conditional/enum.
@@ -158,27 +349,6 @@ static int pitch_speed_enum(int range, int32_t val, int32_t normval)
    and the original value of *intval, inclusive).
    When not treating a conditional/enum, intval should be NULL.
 */
-
-/* a few convinience macros for the id3 == NULL case
- * depends on a few variable names in get_token_value() */
-
-#define HANDLE_NULL_ID3(id3field) (LIKELY(id3) ? (id3field) : NULL)
-
-#define HANDLE_NULL_ID3_NUM_ZERO { if (UNLIKELY(!id3)) return zero_str; }
-
-#define HANDLE_NULL_ID3_NUM_INTVAL(id3field) \
-            do { \
-                if (intval) { \
-                    *intval =  (LIKELY(id3) ? (id3field) + 1 : 0); \
-                } \
-            if (LIKELY(id3)) \
-            { \
-                snprintf(buf, buf_size, "%ld", (id3field)); \
-                return buf; \
-            } \
-            return zero_str; \
-            } while (0)
-
 const char *get_token_value(struct gui_wps *gwps,
                            struct wps_token *token,
                            char *buf, int buf_size,
@@ -189,30 +359,19 @@ const char *get_token_value(struct gui_wps *gwps,
 
     struct wps_data *data = gwps->data;
     struct wps_state *state = gwps->state;
-    int elapsed, length;
+    struct mp3entry *id3; /* Think very carefully about using this. 
-    static const char * const zero_str = "0";
+                             maybe get_id3_token() is the better place? */
+    const char *out_text = NULL;
 
     if (!data || !state)
         return NULL;
 
-    struct mp3entry *id3;
 
     if (token->next)
         id3 = state->nid3;
     else
         id3 = state->id3;
-
+        
-    if (id3)
-    {
-        elapsed = id3->elapsed;
-        length = id3->length;
-    }
-    else
-    {
-        elapsed = 0;
-        length = 0;
-    }
-
 #if CONFIG_RTC
     struct tm* tm = NULL;
 
@@ -234,6 +393,10 @@ const char *get_token_value(struct gui_wps *gwps,
         limit = *intval;
         *intval = -1;
     }
+    
+    out_text = get_id3_token(token, id3, buf, buf_size, limit, intval);
+    if (out_text)
+        return out_text;
 
     switch (token->type)
     {
@@ -248,21 +411,6 @@ const char *get_token_value(struct gui_wps *gwps,
         case WPS_TOKEN_TRANSLATEDSTRING:
             return (char*)P2STR(ID2P(token->value.i));
 
-        case WPS_TOKEN_TRACK_TIME_ELAPSED:
-            format_time(buf, buf_size,
-                        elapsed + state->ff_rewind_count);
-            return buf;
-
-        case WPS_TOKEN_TRACK_TIME_REMAINING:
-            format_time(buf, buf_size,
-                        length - elapsed -
-                        state->ff_rewind_count);
-            return buf;
-
-        case WPS_TOKEN_TRACK_LENGTH:
-            format_time(buf, buf_size, length);
-            return buf;
-
         case WPS_TOKEN_PLAYLIST_ENTRIES:
             snprintf(buf, buf_size, "%d", playlist_amount());
             return buf;
@@ -305,105 +453,6 @@ const char *get_token_value(struct gui_wps *gwps,
                 }
             }
             return buf;
-
-        case WPS_TOKEN_TRACK_ELAPSED_PERCENT:
-            if (length <= 0)
-                return NULL;
-
-            if (intval)
-            {
-                *intval = limit * (elapsed + state->ff_rewind_count)
-                          / length + 1;
-            }
-            snprintf(buf, buf_size, "%d",
-                     100*(elapsed + state->ff_rewind_count) / length);
-            return buf;
-
-        case WPS_TOKEN_METADATA_ARTIST:
-            return HANDLE_NULL_ID3(id3->artist);
-
-        case WPS_TOKEN_METADATA_COMPOSER:
-            return HANDLE_NULL_ID3(id3->composer);
-
-        case WPS_TOKEN_METADATA_ALBUM:
-            return HANDLE_NULL_ID3(id3->album);
-
-        case WPS_TOKEN_METADATA_ALBUM_ARTIST:
-            return HANDLE_NULL_ID3(id3->albumartist);
-
-        case WPS_TOKEN_METADATA_GROUPING:
-            return HANDLE_NULL_ID3(id3->grouping);
-
-        case WPS_TOKEN_METADATA_GENRE:
-            return HANDLE_NULL_ID3(id3->genre_string);
-
-        case WPS_TOKEN_METADATA_DISC_NUMBER:
-            if (LIKELY(id3)) {
-                if (id3->disc_string)
-                    return id3->disc_string;
-                if (id3->discnum) {
-                    snprintf(buf, buf_size, "%d", id3->discnum);
-                    return buf;
-                }
-            }
-            return NULL;
-
-        case WPS_TOKEN_METADATA_TRACK_NUMBER:
-            if (LIKELY(id3)) {
-                if (id3->track_string)
-                    return id3->track_string;
-
-                if (id3->tracknum) {
-                    snprintf(buf, buf_size, "%d", id3->tracknum);
-                    return buf;
-                }
-            }
-            return NULL;
-
-        case WPS_TOKEN_METADATA_TRACK_TITLE:
-            return HANDLE_NULL_ID3(id3->title);
-
-        case WPS_TOKEN_METADATA_VERSION:
-            if (LIKELY(id3))
-            {
-                switch (id3->id3version)
-                {
-                    case ID3_VER_1_0:
-                        return "1";
-
-                    case ID3_VER_1_1:
-                        return "1.1";
-
-                    case ID3_VER_2_2:
-                        return "2.2";
-
-                    case ID3_VER_2_3:
-                        return "2.3";
-
-                    case ID3_VER_2_4:
-                        return "2.4";
-
-                    default:
-                        break;
-                }
-            }
-            return NULL;
-
-        case WPS_TOKEN_METADATA_YEAR:
-            if (LIKELY(id3)) {
-                if( id3->year_string )
-                    return id3->year_string;
-
-                if (id3->year) {
-                    snprintf(buf, buf_size, "%d", id3->year);
-                    return buf;
-                }
-            }
-            return NULL;
-
-        case WPS_TOKEN_METADATA_COMMENT:
-            return HANDLE_NULL_ID3(id3->comment);
-
 #ifdef HAVE_ALBUMART
         case WPS_TOKEN_ALBUMART_FOUND:
             if (data->albumart) {
@@ -420,75 +469,6 @@ const char *get_token_value(struct gui_wps *gwps,
             return NULL;
 #endif
 
-        case WPS_TOKEN_FILE_BITRATE:
-            if(id3 && id3->bitrate)
-                snprintf(buf, buf_size, "%d", id3->bitrate);
-            else
-                return "?";
-            return buf;
-
-        case WPS_TOKEN_FILE_CODEC:
-            if (intval)
-            {
-                if (UNLIKELY(!id3))
-                    *intval = 0;
-                else if(id3->codectype == AFMT_UNKNOWN)
-                    *intval = AFMT_NUM_CODECS;
-                else
-                    *intval = id3->codectype;
-            }
-            return get_codectype(id3);
-
-        case WPS_TOKEN_FILE_FREQUENCY:
-            HANDLE_NULL_ID3_NUM_ZERO;
-            snprintf(buf, buf_size, "%ld", id3->frequency);
-            return buf;
-
-        case WPS_TOKEN_FILE_FREQUENCY_KHZ:
-            HANDLE_NULL_ID3_NUM_ZERO;
-            /* ignore remainders < 100, so 22050 Hz becomes just 22k */
-            if ((id3->frequency % 1000) < 100)
-                snprintf(buf, buf_size, "%ld", id3->frequency / 1000);
-            else
-                snprintf(buf, buf_size, "%ld.%d",
-                        id3->frequency / 1000,
-                        (id3->frequency % 1000) / 100);
-            return buf;
-
-        case WPS_TOKEN_FILE_NAME:
-            if (LIKELY(id3) && get_dir(buf, buf_size, id3->path, 0)) {
-                /* Remove extension */
-                char* sep = strrchr(buf, '.');
-                if (NULL != sep) {
-                    *sep = 0;
-                }
-                return buf;
-            }
-            else {
-                return NULL;
-            }
-
-        case WPS_TOKEN_FILE_NAME_WITH_EXTENSION:
-            if (LIKELY(id3))
-                return get_dir(buf, buf_size, id3->path, 0);
-            return NULL;
-
-        case WPS_TOKEN_FILE_PATH:
-            return HANDLE_NULL_ID3(id3->path);
-
-        case WPS_TOKEN_FILE_SIZE:
-            HANDLE_NULL_ID3_NUM_ZERO;
-            snprintf(buf, buf_size, "%ld", id3->filesize / 1024);
-            return buf;
-
-        case WPS_TOKEN_FILE_VBR:
-            return (LIKELY(id3) && id3->vbr) ? "(avg)" : NULL;
-
-        case WPS_TOKEN_FILE_DIRECTORY:
-            if (LIKELY(id3))
-                return get_dir(buf, buf_size, id3->path, token->value.i);
-            return NULL;
-
         case WPS_TOKEN_BATTERY_PERCENT:
         {
             int l = battery_level();
@@ -760,17 +740,7 @@ const char *get_token_value(struct gui_wps *gwps,
             return buf;
 #endif
 
-                
-#ifdef HAVE_TAGCACHE
-        case WPS_TOKEN_DATABASE_PLAYCOUNT:
-            HANDLE_NULL_ID3_NUM_INTVAL(id3->playcount);
 
-        case WPS_TOKEN_DATABASE_RATING:
-            HANDLE_NULL_ID3_NUM_INTVAL(id3->rating);
-
-        case WPS_TOKEN_DATABASE_AUTOSCORE:
-            HANDLE_NULL_ID3_NUM_INTVAL(id3->score);
-#endif
 
 #if (CONFIG_CODEC == SWCODEC)
         case WPS_TOKEN_CROSSFADE:

apps/gui/skin_engine/wps_internals.h

@@ -358,6 +358,8 @@ const char *get_token_value(struct gui_wps *gwps,
                            char *buf, int buf_size,
                            int *intval);
 
+const char *get_id3_token(struct wps_token *token, struct mp3entry *id3,
+                          char *buf, int buf_size, int limit, int *intval);
 
 
 struct gui_img* find_image(char label, struct wps_data *data);

apps/playback.c

@@ -640,8 +640,8 @@ bool audio_peek_track(struct mp3entry* id3, int offset)
 
     if (tracks[next_idx].id3_hid >= 0)
     {
-        bufread(tracks[next_idx].id3_hid, sizeof(struct mp3entry), id3);
+        return bufread(tracks[next_idx].id3_hid, sizeof(struct mp3entry), id3) 
-        return true;
+                    == sizeof(struct mp3entry);
     }
     return false;
 }