Merge branch 'refs/heads/t/afh_improvements'

Was cooking for about a week.

	mp3_afh: Detect both v1 and v2 tags.
	ogg: Improve documentation of struct ogg_afh_callback_info.
	ogg_afh: Add a comment about the three vorbis header packets.
	afh_common.c: Avoid ifdefs.
	afh loglevel adjustments.
	wma: Rename comment_header.
	afh: Fix fd leak.
	afh: Unmap the audio file on errors.

diff --git a/NEWS b/NEWS
index b9d032fcc2e9cbf5b04af7ca35b86ce33d90a1fe..ddd70a13d5ace03cb614551d79a27cc1f18a123d 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,7 @@ Another cleanup and bugfix release.
        - audiod memory leak fixes.
        - Miscellaneous improvements to the build system.
        - oss_writer improvements.
+       - Improved handling of mp3 files with both id3v1 and id3v2 tags.
 
 Download: ./releases/paraslash-git.tar.bz2 (tarball)
 

diff --git a/afh.c b/afh.c
index d06e4420ff8364ae0401c9de111f3cfe208213c5..f3c25a261ea5cc9c3b49398c850dba00e82afd35 100644 (file)
--- a/afh.c
+++ b/afh.c
@@ -102,16 +102,16 @@ int main(int argc, char **argv)
                }
                ret = compute_afhi(conf.inputs[i], audio_file_data, audio_file_size,
                        fd, &afhi);
-               if (ret < 0)
-                       goto out;
-
-               audio_format_num = ret;
-               printf("File %d: %s\n", i + 1, conf.inputs[i]);
-               print_info(audio_format_num, &afhi);
-               if (conf.chunk_table_given)
-                       print_chunk_table(&afhi);
-               printf("\n");
-               clear_afhi(&afhi);
+               if (ret >= 0) {
+                       audio_format_num = ret;
+                       printf("File %d: %s\n", i + 1, conf.inputs[i]);
+                       print_info(audio_format_num, &afhi);
+                       if (conf.chunk_table_given)
+                               print_chunk_table(&afhi);
+                       printf("\n");
+                       clear_afhi(&afhi);
+               }
+               close(fd);
                ret2 = para_munmap(audio_file_data, audio_file_size);
                if (ret2 < 0 && ret >= 0)
                        ret = ret2;

diff --git a/afh_common.c b/afh_common.c
index 914d1a321af35bae395cc365b14537e86991b42d..edfc8d1d08e79f333afce711aa814a374c8f1faf 100644 (file)
--- a/afh_common.c
+++ b/afh_common.c
@@ -15,27 +15,10 @@
 #include "string.h"
 #include "afh.h"
 
-/* The mp3 audio format handler does not need any libs. */
-void mp3_init(struct audio_format_handler *);
-
-#ifdef HAVE_OGGVORBIS
-       void ogg_init(struct audio_format_handler *);
-#endif
-#ifdef HAVE_FAAD
-       void aac_afh_init(struct audio_format_handler *);
-#endif
-#ifdef HAVE_SPEEX
-       void spx_afh_init(struct audio_format_handler *);
-#endif
-#ifdef HAVE_FLAC
-       void flac_afh_init(struct audio_format_handler *);
-#endif
-
-#ifdef HAVE_OPUS
-       void opus_afh_init(struct audio_format_handler *);
-#endif
-
-void wma_afh_init(struct audio_format_handler *);
+typedef void afh_init_func(struct audio_format_handler *);
+/* It does not hurt to declare init functions which are not available. */
+extern afh_init_func mp3_init, ogg_init, aac_afh_init, wma_afh_init,
+       spx_afh_init, flac_afh_init, opus_afh_init;
 
 /** The list of all status items */
 const char *status_item_list[] = {STATUS_ITEM_ARRAY};
@@ -118,9 +101,9 @@ void afh_init(void)
 {
        int i;
 
-       PARA_INFO_LOG("supported audio formats: %s\n", AUDIO_FORMAT_HANDLERS);
+       PARA_NOTICE_LOG("supported audio formats: %s\n", AUDIO_FORMAT_HANDLERS);
        FOR_EACH_AUDIO_FORMAT(i) {
-               PARA_NOTICE_LOG("initializing %s handler\n",
+               PARA_INFO_LOG("initializing %s handler\n",
                        audio_format_name(i));
                afl[i].init(&afl[i]);
        }

diff --git a/mp3_afh.c b/mp3_afh.c
index cc3f49ffbb2b704af955e77e944a72250d3d309c..3b540c0b84a8ca081bce71a3dd1015f42fb8b97b 100644 (file)
--- a/mp3_afh.c
+++ b/mp3_afh.c
@@ -123,20 +123,11 @@ static char *get_strings(struct id3_frame *fr)
        return NULL;
 }
 
-static void mp3_get_id3(__a_unused unsigned char *map,
-               __a_unused size_t numbytes, int fd, struct taginfo *tags)
+/* this only sets values which are undefined so far */
+static void parse_frames(struct id3_tag *id3_t, struct taginfo *tags)
 {
        int i;
-       struct id3_tag *id3_t;
-       struct id3_file *id3_f = id3_file_fdopen(fd, ID3_FILE_MODE_READONLY);
-
-       if (!id3_f)
-               return;
-       id3_t = id3_file_tag(id3_f);
-       if (!id3_t) {
-               id3_file_close(id3_f);
-               return;
-       }
+
        for (i = 0; i < id3_t->nframes; i++) {
                struct id3_frame *fr = id3_t->frames[i];
                if (!strcmp(fr->id, ID3_FRAME_TITLE)) {
@@ -165,7 +156,31 @@ static void mp3_get_id3(__a_unused unsigned char *map,
                        continue;
                }
        }
-       id3_file_close(id3_f);
+}
+
+static int mp3_get_id3(unsigned char *map, size_t numbytes, __a_unused int fd,
+               struct taginfo *tags)
+{
+       int ret = 0;
+       struct id3_tag *id3_t;
+
+       /* id3v2 tags are usually located at the beginning. */
+       id3_t = id3_tag_parse(map, numbytes);
+       if (id3_t) {
+               parse_frames(id3_t, tags);
+               ret |= 2;
+               id3_tag_delete(id3_t);
+       }
+       /* Also look for an id3v1 tag at the end of the file. */
+       if (numbytes >= 128) {
+               id3_t = id3_tag_parse(map + numbytes - 128, 128);
+               if (id3_t) {
+                       parse_frames(id3_t, tags);
+                       ret |= 1;
+                       id3_tag_delete(id3_t);
+               }
+       }
+       return ret;
 }
 
 #else /* HAVE_LIBID3TAG */
@@ -181,7 +196,7 @@ static char *unpad(char *string)
        return string;
 }
 
-static void mp3_get_id3(unsigned char *map, size_t numbytes, __a_unused int fd,
+static int mp3_get_id3(unsigned char *map, size_t numbytes, __a_unused int fd,
        struct taginfo *tags)
 {
        char title[31], artist[31], album[31], year[5], comment[31];
@@ -189,7 +204,7 @@ static void mp3_get_id3(unsigned char *map, size_t numbytes, __a_unused int fd,
 
        if (numbytes < 128 || strncmp("TAG", (char *)map + numbytes - 128, 3)) {
                PARA_DEBUG_LOG("no id3 v1 tag\n");
-               return;
+               return 0;
        }
        fpos = numbytes - 125;
        memcpy(title, map + fpos, 30);
@@ -216,6 +231,7 @@ static void mp3_get_id3(unsigned char *map, size_t numbytes, __a_unused int fd,
        tags->year = para_strdup(year);
        tags->album = para_strdup(album);
        tags->comment = para_strdup(comment);
+       return 1;
 }
 #endif /* HAVE_LIBID3TAG */
 
@@ -387,6 +403,7 @@ static int mp3_read_info(unsigned char *map, size_t numbytes, int fd,
        unsigned chunk_table_size = 1000; /* gets increased on demand */
        off_t fpos = 0;
        struct mp3header header;
+       const char *tag_versions[] = {"no", "id3v1", "id3v2", "id3v1+id3v2"};
 
        afhi->chunks_total = 0;
        afhi->chunk_table = para_malloc(chunk_table_size * sizeof(uint32_t));
@@ -445,9 +462,9 @@ static int mp3_read_info(unsigned char *map, size_t numbytes, int fd,
        tv_divide(afhi->chunks_total, &total_time, &afhi->chunk_tv);
        PARA_DEBUG_LOG("%lu chunks, each %lums\n", afhi->chunks_total,
                tv2ms(&afhi->chunk_tv));
-       afhi->techinfo = make_message("%cbr, %s", vbr? 'v' : 'c',
-               header_mode(&header));
-       mp3_get_id3(map, numbytes, fd, &afhi->tags);
+       ret = mp3_get_id3(map, numbytes, fd, &afhi->tags);
+       afhi->techinfo = make_message("%cbr, %s, %s tags", vbr? 'v' : 'c',
+               header_mode(&header), tag_versions[ret]);
        return 1;
 err_out:
        PARA_ERROR_LOG("%s\n", para_strerror(-ret));

diff --git a/ogg_afh.c b/ogg_afh.c
index 1e389d9e5770f021a2a86aa97e77479709335f92..9dfb028d0a2f9e14dac54cbc5a4ca200cd2dcf6b 100644 (file)
--- a/ogg_afh.c
+++ b/ogg_afh.c
@@ -20,6 +20,20 @@ struct private_vorbis_data {
        vorbis_comment vc;
 };
 
+/*
+ * Vorbis uses three header packets, all of which are required: the
+ * identification header, the comments header, and the setup header.
+ *
+ * The identification header identifies the bitstream as Vorbis. It contains
+ * the Vorbis version and simple audio characteristics of the stream such as
+ * sample rate and number of channels.
+ *
+ * The comment header includes user text comments (tags) and a vendor string
+ * for the application/library that produced the bitstream.
+ *
+ * The setup header includes extensive CODEC setup information as well as the
+ * complete VQ and Huffman codebooks needed for decoding.
+ */
 static int vorbis_packet_callback(ogg_packet *packet, int packet_num,
                __a_unused int serial, struct afh_info *afhi, void *private_data)
 {

diff --git a/ogg_afh_common.h b/ogg_afh_common.h
index 09870a99078a492565e80318f34d917e07651d72..47e133bfcf61c15542c9133b61e113ebdce335f8 100644 (file)
--- a/ogg_afh_common.h
+++ b/ogg_afh_common.h
@@ -10,22 +10,26 @@
  */
 
 /**
- * Callback structure provided by vorbis/speex audio format handlers.
+ * Callback structure provided by audio format handlers.
  *
- * Both audio formats utilize the ogg container format. Meta info about
- * the audio file is contained in the first three ogg packets.
+ * All audio formats which utilize the ogg container format define a callback
+ * function whose purpose is to extract the meta information about the audio
+ * file from the first few ogg packets of the bitstream.
  */
 struct ogg_afh_callback_info {
        /**
-         * ogg_get_file_info() calls this function for each of the three
-         * header packets. If this callback returns a negative value, the
-         * audio file is considered invalid and the chunk table is not
-         * created. If it returns zero, the end of the header has been
-         * reached and no further ogg packets should be processed.
-         */
+        * ogg_get_file_info() calls this function for the first three ogg
+        * packets, or until the callback returns a non-positive value. If it
+        * returns negative, the audio file was not recognized by the audio
+        * format handler. If it returns zero, the audio file is considered
+        * valid, and no further processing by the callback is needed. In this
+        * case the header chunk, i.e. chunk number zero, is defined as the
+        * concatenation of all packets that have been processed by the
+        * callback.
+        */
        int (*packet_callback)(ogg_packet *packet, int packet_num,
                int serial, struct afh_info *afhi, void *private_data);
-       /** Vorbis/speex specific data. */
+       /** Data specific to the audio format handler. */
        void *private_data;
 };
 

diff --git a/wma_afh.c b/wma_afh.c
index 3df9b5fb818bf3e9a406eb175f7090a796502337..f1edacf07aa56b2d26e54e54641c1fd3d6bc1f11 100644 (file)
--- a/wma_afh.c
+++ b/wma_afh.c
@@ -100,7 +100,7 @@ static char *get_str16(const char *in, int len)
        return out;
 }
 
-static const char comment_header[] = {
+static const char content_description_header[] = {
        0x33, 0x26, 0xb2, 0x75, 0x8E, 0x66, 0xCF, 0x11,
        0xa6, 0xd9, 0x00, 0xaa, 0x00, 0x62, 0xce, 0x6c
 };
@@ -127,10 +127,10 @@ static void read_asf_tags(const char *buf, int buf_size, struct taginfo *ti)
        const char *p, *end = buf + buf_size, *q;
        uint16_t len1, len2, len3, len4;
 
-       p = search_pattern(comment_header, sizeof(comment_header),
-               buf, buf_size);
+       p = search_pattern(content_description_header,
+               sizeof(content_description_header), buf, buf_size);
        if (!p || p + 34 >= end) {
-               PARA_NOTICE_LOG("comment header not found\n");
+               PARA_NOTICE_LOG("content description header not found\n");
                goto next;
        }
        p += 24;