Made ID3 code store all comments in a list (API change in mpg123_id3v2 struct!) and while doing that seen what I did wrong last time: I ignored the encoding of the description field.

git-svn-id: svn://scm.orgis.org/mpg123/trunk@1204 35dc7657-300d-0410-a2e5-dc2837fedb53

doc/examples/Makefile

@@ -8,7 +8,7 @@ SND_CFLAGS     := $(shell pkg-config --cflags sndfile)
 SND_LDFLAGS    := $(shell pkg-config --libs   sndfile)
 
 # Oder of libs not that important here...
-compile = $(CC) $(CFLAGS) $(LDFLAGS) $(MPG123_CFLAGS) $(MPG123_LDFLAGS)
+compile = $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(MPG123_CFLAGS) $(MPG123_LDFLAGS)
 
 mpg123_to_wav: mpg123_to_wav.c
 	$(compile) $(SND_CFLAGS) $(SND_LDFLAGS)-o mpg123_to_wav mpg123_to_wav.c

doc/examples/id3dump.c

@@ -42,14 +42,16 @@ void print_lines(const char* prefix, mpg123_string *inlines)
 {
 	size_t i;
 	int hadcr = 0, hadlf = 0;
-	char *lines = "";
+	char *lines = NULL;
-	char *line;
+	char *line  = NULL;
-	size_t len = 1;
+	size_t len = 0;
-	if(inlines->fill)
+	if(inlines != NULL && inlines->fill)
 	{
 		lines = inlines->p;
 		len   = inlines->fill;
 	}
+	else return;
+
 	line = lines;
 	for(i=0; i<len; ++i)
 	{
@@ -89,7 +91,7 @@ void print_v2(mpg123_id3v2 *v2)
 	sources[1] = &v2->artist;
 	sources[2] = &v2->album;
 	sources[3] = &v2->year;
-	sources[4] = &v2->comment;
+	sources[4] =  v2->generic_comment;
 	sources[5] = &v2->genre;
 	for(i=0; i<V1FIELDS; ++i)
 	{
@@ -109,6 +111,7 @@ int main(int argc, char **argv)
 	}
 	mpg123_init();
 	m = mpg123_new(NULL, NULL);
+mpg123_param(m, MPG123_VERBOSE, 4, 0);
 	
 	for(i=1; i < argc; ++i)
 	{

src/id3print.c

@@ -36,7 +36,7 @@ void print_id3_tag(mpg123_handle *mh, int long_id3, FILE *out)
 		transform(&tag[TITLE],   &v2->title);
 		transform(&tag[ARTIST],  &v2->artist);
 		transform(&tag[ALBUM],   &v2->album);
-		transform(&tag[COMMENT], &v2->comment);
+		transform(&tag[COMMENT],  v2->generic_comment);
 		transform(&tag[YEAR],    &v2->year);
 		transform(&tag[GENRE],   &v2->genre);
 	}

src/libmpg123/id3.c

@@ -38,8 +38,55 @@ void init_id3(mpg123_handle *fr)
 	mpg123_init_string(&fr->id3v2.artist);
 	mpg123_init_string(&fr->id3v2.album);
 	mpg123_init_string(&fr->id3v2.year);
-	mpg123_init_string(&fr->id3v2.comment);
 	mpg123_init_string(&fr->id3v2.genre);
+	fr->id3v2.comments = 0;
+	fr->id3v2.comment  = NULL;
+	fr->id3v2.generic_comment = NULL;
+}
+
+static void free_comment(mpg123_handle *mh)
+{
+	size_t i;
+	mpg123_id3v2 *id = &mh->id3v2;
+	for(i=0; i<id->comments; ++i)
+	{
+		mpg123_free_string(&id->comment[i].description);
+		mpg123_free_string(&id->comment[i].text);
+	}
+	free(id->comment);
+	id->comment  = NULL;
+	id->comments = 0;
+	id->generic_comment = NULL;
+}
+
+static mpg123_comment *add_comment(mpg123_handle *mh)
+{
+	mpg123_id3v2 *id = &mh->id3v2;
+	mpg123_comment *x = safe_realloc(id->comment, sizeof(mpg123_comment)*(id->comments+1));
+	if(x == NULL) return NULL; /* bad */
+
+	id->comment   = x;
+	id->comments += 1;
+	id->comment[id->comments-1].lang[0] = 0; /* empty... */
+	mpg123_init_string(&id->comment[id->comments-1].description);
+	mpg123_init_string(&id->comment[id->comments-1].text);
+	return &id->comment[id->comments-1]; /* Return pointer to the added comment. */
+}
+
+static void pop_comment(mpg123_handle *mh)
+{
+	mpg123_comment *x;
+	mpg123_id3v2 *id = &mh->id3v2;
+	if(id->comments < 1) return;
+
+	mpg123_free_string(&id->comment[id->comments-1].description);
+	mpg123_free_string(&id->comment[id->comments-1].text);
+	x = safe_realloc(id->comment, sizeof(mpg123_comment)*(id->comments-1));
+	if(x != NULL)
+	{
+		id->comment   = x;
+		id->comments -= 1;
+	}
 }
 
 void exit_id3(mpg123_handle *fr)
@@ -48,8 +95,8 @@ void exit_id3(mpg123_handle *fr)
 	mpg123_free_string(&fr->id3v2.artist);
 	mpg123_free_string(&fr->id3v2.album);
 	mpg123_free_string(&fr->id3v2.year);
-	mpg123_free_string(&fr->id3v2.comment);
 	mpg123_free_string(&fr->id3v2.genre);
+	free_comment(fr);
 }
 
 void reset_id3(mpg123_handle *fr)
@@ -59,8 +106,8 @@ void reset_id3(mpg123_handle *fr)
 	fr->id3v2.artist.fill = 0;
 	fr->id3v2.album.fill = 0;
 	fr->id3v2.year.fill = 0;
-	fr->id3v2.comment.fill = 0;
 	fr->id3v2.genre.fill = 0;
+	free_comment(fr);
 }
 
 /*
@@ -107,6 +154,157 @@ void store_id3_text(mpg123_string *sb, char *source, size_t source_size)
 	else error("unable to convert string to UTF-8 (out of memory, junk input?)!");
 }
 
+char *next_text(char* prev, int encoding, size_t limit)
+{
+	char *text = prev;
+	unsigned long neednull = encoding_widths[encoding];
+	/* So I go lengths to find zero or double zero... */
+	while(text-prev < limit)
+	{
+		if(text[0] == 0)
+		{
+			if(neednull <= limit-(text-prev))
+			{
+				unsigned long i = 1;
+				for(; i<neednull; ++i) if(text[i] != 0) break;
+
+				if(i == neednull) /* found a null wide enough! */
+				{
+					text += neednull;
+					break;
+				}
+			}
+			else{ text = NULL; break; }
+		}
+		++text;
+	}
+	if(text-prev == limit) text = NULL;
+
+	return text;
+}
+
+/* Store a new comment that perhaps is a RVA / RVA_ALBUM/AUDIOPHILE / RVA_MIX/RADIO one */
+static void process_comment(mpg123_handle *fr, char *realdata, size_t realsize, int tt)
+{
+	/* Text encoding          $xx */
+	/* Language               $xx xx xx */
+	/* Short description (encoded!)      <text> $00 (00) */
+	/* Then the comment text (encoded) ... */
+	char  encoding = realdata[0];
+	char *lang    = realdata+1; /* I'll only use the 3 bytes! */
+	char *descr   = realdata+4;
+	char *text;
+	mpg123_comment *xcom = add_comment(fr);
+	if(xcom == NULL)
+	{
+		if(NOQUIET) error("Unable to attach new comment!");
+		return;
+	}
+	memcpy(xcom->lang, lang, 3);
+	xcom->lang[3] = 0;
+	/* Now I can abuse a byte from lang for the encoding. */
+	descr[-1] = encoding;
+	/* Be careful with finding the end of description, I have to honor encoding here. */
+	text = next_text(descr, encoding, realsize-(descr-realdata));
+	if(text == NULL)
+	{
+		if(NOQUIET) error("No comment text / valid description?");
+		pop_comment(fr);
+		return;
+	}
+	store_id3_text(&xcom->description, descr-1, text-descr+1);
+	text[-1] = encoding;
+	store_id3_text(&xcom->text, text-1, realsize-(text-realdata)+1);
+
+	if(VERBOSE4)
+	{
+		fprintf(stderr, "Note: ID3 comment desc: %s\n", xcom->description.fill > 0 ? xcom->description.p : "");
+		fprintf(stderr, "Note: ID3 comment text: %s\n", xcom->text.fill > 0 ? xcom->text.p : "");
+	}
+	if(xcom->description.fill > 0 && xcom->text.fill > 0)
+	{
+		int rva_mode = -1; /* mix / album */
+		if(   !strcasecmp(xcom->description.p, "rva")
+			 || !strcasecmp(xcom->description.p, "rva_mix")
+			 || !strcasecmp(xcom->description.p, "rva_track")
+			 || !strcasecmp(xcom->description.p, "rva_radio"))
+		rva_mode = 0;
+		else if(   !strcasecmp(xcom->description.p, "rva_album")
+						|| !strcasecmp(xcom->description.p, "rva_audiophile")
+						|| !strcasecmp(xcom->description.p, "rva_user"))
+		rva_mode = 1;
+		if((rva_mode > -1) && (fr->rva.level[rva_mode] <= tt+1))
+		{
+			fr->rva.gain[rva_mode] = atof(xcom->text.p);
+			if(VERBOSE3) fprintf(stderr, "Note: RVA value %fdB\n", fr->rva.gain[rva_mode]);
+			fr->rva.peak[rva_mode] = 0;
+			fr->rva.level[rva_mode] = tt+1;
+		}
+	}
+	/* Mark the last found generic comment. */
+	if(xcom->description.fill == 0 || xcom->description.p[0] == 0)
+	fr->id3v2.generic_comment = &xcom->text;
+}
+
+void process_extra(mpg123_handle *fr, char* realdata, size_t realsize, int tt)
+{
+	/* Text encoding          $xx */
+	/* Description        ... $00 (00) */
+	/* Text ... */
+	mpg123_string work;
+	char encoding = realdata[0];
+	char *descr  = realdata+1; /* remember, the encoding is descr[-1] */
+	char *text   = next_text(descr, encoding, realsize-(descr-realdata));
+	if(text == NULL)
+	{
+		if(NOQUIET) error("No extra frame text / valid description?");
+		return;
+	}
+	mpg123_init_string(&work);
+	store_id3_text(&work, descr-1, text-descr+1);
+	if(work.fill > 0)
+	{
+		int is_peak = 0;
+		int rva_mode = -1; /* mix / album */
+
+		if(!strncasecmp(work.p, "replaygain_track_",17))
+		{
+			debug("ID3v2: track gain/peak");
+			rva_mode = 0;
+			if(!strcasecmp(work.p, "replaygain_track_peak")) is_peak = 1;
+			else if(strcasecmp(work.p, "replaygain_track_gain")) rva_mode = -1;
+		}
+		else
+		if(!strncasecmp(work.p, "replaygain_album_",17))
+		{
+			debug("ID3v2: album gain/peak");
+			rva_mode = 1;
+			if(!strcasecmp(work.p, "replaygain_album_peak")) is_peak = 1;
+			else if(strcasecmp(work.p, "replaygain_album_gain")) rva_mode = -1;
+		}
+		if((rva_mode > -1) && (fr->rva.level[rva_mode] <= tt+1))
+		{
+			text[-1] = encoding;
+			store_id3_text(&work, text-1, realsize-(text-realdata)+1);
+			if(work.fill > 0)
+			{
+				if(is_peak)
+				{
+					fr->rva.peak[rva_mode] = atof(work.p);
+					if(VERBOSE3) fprintf(stderr, "Note: RVA peak %fdB\n", fr->rva.peak[rva_mode]);
+				}
+				else
+				{
+					fr->rva.gain[rva_mode] = atof(work.p);
+					if(VERBOSE3) fprintf(stderr, "Note: RVA gain %fdB\n", fr->rva.gain[rva_mode]);
+				}
+				fr->rva.level[rva_mode] = tt+1;
+			}
+		}
+	}
+	mpg123_free_string(&work);
+}
+
 /*
 	trying to parse ID3v2.3 and ID3v2.4 tags...
 
@@ -288,104 +486,11 @@ int parse_new_id3(mpg123_handle *fr, unsigned long first4bytes)
 								pos = 0; /* now at the beginning again... */
 								switch(tt)
 								{
-									case comment: /* a comment that perhaps is a RVA / fr->rva.ALBUM/AUDIOPHILE / fr->rva.MIX/RADIO one */
+									case comment:
-									{
+										process_comment(fr, (char*)realdata, realsize, tt);
-										/* Text encoding          $xx */
-										/* Language               $xx xx xx */
-										/* policy about encodings: do not care for now here */
-										/* if(realdata[0] == 0)  */
-										{
-											/* don't care about language */
-											pos = 4;
-											if(   !strcasecmp((char*)realdata+pos, "rva")
-											   || !strcasecmp((char*)realdata+pos, "fr->rva.mix")
-											   || !strcasecmp((char*)realdata+pos, "fr->rva.radio"))
-											rva_mode = 0;
-											else if(   !strcasecmp((char*)realdata+pos, "fr->rva.album")
-											        || !strcasecmp((char*)realdata+pos, "fr->rva.audiophile")
-											        || !strcasecmp((char*)realdata+pos, "fr->rva.user"))
-											rva_mode = 1;
-											if((rva_mode > -1) && (fr->rva.level[rva_mode] <= tt+1))
-											{
-												char* comstr;
-												size_t comsize = realsize-4-(strlen((char*)realdata+pos)+1);
-												if(VERBOSE3) fprintf(stderr, "Note: evaluating %s data for RVA\n", realdata+pos);
-												if((comstr = (char*) malloc(comsize+1)) != NULL)
-												{
-													memcpy(comstr,realdata+realsize-comsize, comsize);
-													comstr[comsize] = 0;
-													/* hm, what about utf16 here? */
-													fr->rva.gain[rva_mode] = atof(comstr);
-													if(VERBOSE3) fprintf(stderr, "Note: RVA value %fdB\n", fr->rva.gain[rva_mode]);
-													fr->rva.peak[rva_mode] = 0;
-													fr->rva.level[rva_mode] = tt+1;
-													free(comstr);
-												}
-												else error("could not allocate memory for rva comment interpretation");
-											}
-											else
-											{
-												if(!strcasecmp((char*)realdata+pos, ""))
-												{
-													/* only add general comments */
-													realdata[pos] = realdata[pos-4]; /* the encoding field copied */
-													debug("storing a comment");
-													store_id3_text(&fr->id3v2.comment, (char*)realdata+pos, realsize-4);
-												}
-											}
-										}
-									}
 									break;
 									case extra: /* perhaps foobar2000's work */
-									{
+										process_extra(fr, (char*)realdata, realsize, tt);
-										/* Text encoding          $xx */
-										/* unicode would hurt in string comparison... */
-										if(realdata[0] == 0)
-										{
-											int is_peak = 0;
-											pos = 1;
-											
-											if(!strncasecmp((char*)realdata+pos, "replaygain_track_",17))
-											{
-												debug("ID3v2: track gain/peak");
-												rva_mode = 0;
-												if(!strcasecmp((char*)realdata+pos, "replaygain_track_peak")) is_peak = 1;
-												else if(strcasecmp((char*)realdata+pos, "replaygain_track_gain")) rva_mode = -1;
-											}
-											else
-											if(!strncasecmp((char*)realdata+pos, "replaygain_album_",17))
-											{
-												debug("ID3v2: album gain/peak");
-												rva_mode = 1;
-												if(!strcasecmp((char*)realdata+pos, "replaygain_album_peak")) is_peak = 1;
-												else if(strcasecmp((char*)realdata+pos, "replaygain_album_gain")) rva_mode = -1;
-											}
-											if((rva_mode > -1) && (fr->rva.level[rva_mode] <= tt+1))
-											{
-												char* comstr;
-												size_t comsize = realsize-1-(strlen((char*)realdata+pos)+1);
-												if(VERBOSE3) fprintf(stderr, "Note: evaluating %s data for RVA\n", realdata+pos);
-												if((comstr = (char*) malloc(comsize+1)) != NULL)
-												{
-													memcpy(comstr,realdata+realsize-comsize, comsize);
-													comstr[comsize] = 0;
-													if(is_peak)
-													{
-														fr->rva.peak[rva_mode] = atof(comstr);
-														if(VERBOSE3) fprintf(stderr, "Note: RVA peak %fdB\n", fr->rva.peak[rva_mode]);
-													}
-													else
-													{
-														fr->rva.gain[rva_mode] = atof(comstr);
-														if(VERBOSE3) fprintf(stderr, "Note: RVA gain %fdB\n", fr->rva.gain[rva_mode]);
-													}
-													fr->rva.level[rva_mode] = tt+1;
-													free(comstr);
-												}
-												else error("could not allocate memory for rva comment interpretation");
-											}
-										}
-									}
 									break;
 									case rva2: /* "the" RVA tag */
 									{

src/libmpg123/mpg123.h.in

@@ -566,6 +566,13 @@ EXPORT int  mpg123_add_string(mpg123_string* sb, char* stuff);
  *  \return 0 on error, 1 on success */
 EXPORT int  mpg123_set_string(mpg123_string* sb, char* stuff);
 
+/** Sub data structure for ID3v2, for storing comments. */
+typedef struct
+{
+	char lang[4]; /**< Three-letter language code (null-terminated). */
+	mpg123_string description; /**< Empty for the generic comment... */
+	mpg123_string text;        /**< ... */
+} mpg123_comment;
 
 /** Data structure for storing IDV3v2 tags.
  *  This structure is not a direct binary mapping with the file contents.
@@ -579,8 +586,10 @@ typedef struct
 	mpg123_string artist;  /**< Artist string. */
 	mpg123_string album;   /**< Album string. */
 	mpg123_string year;    /**< The year as a string. */
-	mpg123_string comment; /**< Comment string. */
 	mpg123_string genre;   /**< Genre String. The genre string(s) may very well need postprocessing, esp. for ID3v2.3. */
+	size_t comments;         /**< Number of comments. */
+	mpg123_comment *comment; /**< Array of comments. */
+	mpg123_string *generic_comment; /**< Pointer to last encountered comment text with empty description. */
 } mpg123_id3v2;
 
 /** Data structure for ID3v1 tags (the last 128 bytes of a file).

src/libmpg123/mpg123lib_intern.h

@@ -151,6 +151,7 @@ typedef unsigned char byte;
 #define VERBOSE  (NOQUIET && fr->p.verbose)
 #define VERBOSE2 (NOQUIET && fr->p.verbose > 1)
 #define VERBOSE3 (NOQUIET && fr->p.verbose > 2)
+#define VERBOSE4 (NOQUIET && fr->p.verbose > 3)
 
 int decode_update(mpg123_handle *mh);
 

src/libmpg123/stringbuf.c

@@ -53,6 +53,10 @@ int mpg123_resize_string(mpg123_string* sb, size_t new)
 
 int mpg123_copy_string(mpg123_string* from, mpg123_string* to)
 {
+	if(to == NULL) return -1;
+
+	if(from == NULL) return mpg123_set_string(to, "");
+
 	if(mpg123_resize_string(to, from->fill))
 	{
 		memcpy(to->p, from->p, to->size);