Merge branch 't/ao_fixes'
authorAndre Noll <maan@systemlinux.org>
Sun, 25 May 2014 13:31:07 +0000 (15:31 +0200)
committerAndre Noll <maan@systemlinux.org>
Sun, 25 May 2014 13:32:39 +0000 (15:32 +0200)
Cooking since 2014-03-22.

* t/ao_fixes:
  ao_write: Call ao_initialize() only once.
  ao_write: Join threads before returning an error from aow_post_select().
  ao_write: Simplify locking.
  Don't unlock and lock the thread mutex unnecessarily.
  ao_write: Check return value of pthread functions.
  ao_write: Avoid segfault on exit.
  ao_write: Avoid pthread_join().
  ao_write: Enforce a 20ms timeout.
  ao_write: Fix spurious segfault.

19 files changed:
Makefile.real
NEWS
alsa_mix.c
alsa_write.c
buffer_tree.c
client.c
client.h
client_common.c
configure.ac
flac_afh.c
interactive.c
m4/gengetopt/alsa_write.m4
oggdec_filter.c
opusdec_filter.c
t/makefile.test
t/t0005-man.sh [new file with mode: 0755]
t/test-lib.sh
version.c
wma_afh.c

index d6ffe0c..2973637 100644 (file)
@@ -86,7 +86,6 @@ DEBUG_CPPFLAGS += -Wredundant-decls
 DEBUG_CPPFLAGS += -Wall -Wno-sign-compare -Wno-unknown-pragmas
 DEBUG_CPPFLAGS += -Wformat-security
 DEBUG_CPPFLAGS += -Wmissing-format-attribute
-DEBUG_CPPFLAGS += -Wdeclaration-after-statement
 
 ifeq ($(uname_s),Linux)
        CPPFLAGS += -fdata-sections -ffunction-sections
diff --git a/NEWS b/NEWS
index ab53a17..642c1fc 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -1,9 +1,19 @@
 NEWS
 ====
 
----------------------------------------------
-0.5.2 (to be announced) "orthogonal interior"
----------------------------------------------
+-------------------------------------------------
+0.5.3 (to be released) "symbolic synchronization"
+-------------------------------------------------
+
+       - Various alsa-related fixes, mostly for the raspberry pi.
+       - The test suite has been extended to include sanity checks
+         for the generated man pages.
+       - ao_writer fixes. This writer was in a quite bad shape. Many
+         serious bugs have been fixed.
+
+----------------------------------------
+0.5.2 (2014-04-11) "orthogonal interior"
+----------------------------------------
 
 The new sync filter, the AES_CTR128 stream cipher and the overhauled
 network code are the highlights of this release. It also includes a
@@ -23,9 +33,13 @@ fair number of smaller fixes and improvements not mentioned here.
        - The man pages of para_audiod, para_filter, para_recv, and
          para_write contain the relevant options for receivers, filters,
          writers. This broke in 0.5.0.
+       - ogg/vorbis latency improvements.
        - Improved user manual.
        - Minor fixes to avoid clang warnings.
 
+Downloads: ./releases/paraslash-0.5.2.tar.bz2 (tarball),
+./releases/paraslash-0.5.2.tar.bz2.asc (signature)
+
 ------------------------------------------
 0.5.1 (2013-12-20) "temporary implication"
 ------------------------------------------
index be38e88..c860efc 100644 (file)
@@ -150,7 +150,7 @@ static int alsa_mix_set_channel(struct mixer_handle *h,
                        mixer_channel, h->card, snd_strerror(ret));
                return -E_ALSA_MIX_RANGE;
        }
-       if (h->pmin < 0 || h->pmax < 0 || h->pmin >= h->pmax) {
+       if (h->pmin >= h->pmax) {
                PARA_NOTICE_LOG("alsa reported %s range %ld-%ld (%s)\n",
                        mixer_channel, h->pmin, h->pmax, h->card);
                return -E_ALSA_MIX_RANGE;
index ad59a82..3759306 100644 (file)
@@ -34,17 +34,24 @@ struct private_alsa_write_data {
        snd_pcm_t *handle;
        /** Determined and set by alsa_init(). */
        int bytes_per_frame;
-       /**
-        * The sample rate given by command line option or the decoder
-        * of the writer node group.
+       /*
+        * If the sample rate is not given at the command line and no wav
+        * header was detected, the btr exec mechanism is employed to query the
+        * ancestor buffer tree nodes for this information. In a typical setup
+        * the decoder passes the sample rate back to the alsa writer.
+        *
+        *  \sa \ref btr_exec_up().
         */
        unsigned sample_rate;
-       snd_pcm_format_t sample_format;
-       /**
-        * The number of channels, given by command line option or the
-        * decoder of the writer node group.
+       /*
+        * The sample format (8/16 bit, signed/unsigned, little/big endian) is
+        * determined in the same way as the \a sample_rate.
         */
+       snd_pcm_format_t sample_format;
+       /* The number of channels, again determined like \a sample_rate. */
        unsigned channels;
+       /* time until buffer underrun occurs, in milliseconds */
+       unsigned buffer_time;
        struct timeval drain_barrier;
        /* File descriptor for select(). */
        int poll_fd;
@@ -72,9 +79,9 @@ static int alsa_init(struct private_alsa_write_data *pad,
        snd_pcm_uframes_t start_threshold, stop_threshold;
        snd_pcm_uframes_t buffer_size, period_size;
        snd_output_t *output_log;
-       unsigned buffer_time;
        int ret;
        const char *msg;
+       unsigned period_time;
 
        PARA_INFO_LOG("opening %s\n", conf->device_arg);
        msg = "unable to open pcm";
@@ -107,18 +114,21 @@ static int alsa_init(struct private_alsa_write_data *pad,
                &pad->sample_rate, NULL);
        if (ret < 0)
                goto fail;
-       msg = "unable to get buffer time";
-       ret = snd_pcm_hw_params_get_buffer_time_max(hwparams, &buffer_time,
-               NULL);
-       if (ret < 0 || buffer_time == 0)
-               goto fail;
-       /* buffer at most 500 milliseconds */
-       buffer_time = PARA_MIN(buffer_time, 500U * 1000U);
+       /* alsa wants microseconds */
+       pad->buffer_time = conf->buffer_time_arg * 1000;
        msg = "could not set buffer time";
        ret = snd_pcm_hw_params_set_buffer_time_near(pad->handle, hwparams,
-               &buffer_time, NULL);
+               &pad->buffer_time, NULL);
+       if (ret < 0)
+               goto fail;
+       pad->buffer_time /= 1000; /* we prefer milliseconds */
+       period_time = pad->buffer_time * 250; /* buffer time / 4 */
+       msg = "could not set period time";
+       ret = snd_pcm_hw_params_set_period_time_near(pad->handle, hwparams,
+               &period_time, 0);
        if (ret < 0)
                goto fail;
+
        msg = "unable to install hw params";
        ret = snd_pcm_hw_params(pad->handle, hwparams);
        if (ret < 0)
@@ -166,15 +176,16 @@ static int alsa_init(struct private_alsa_write_data *pad,
        if (ret == 0) {
                char *buf, *p;
                size_t sz;
-               PARA_INFO_LOG("dumping alsa configuration\n");
+               PARA_DEBUG_LOG("dumping alsa configuration\n");
                snd_pcm_dump(pad->handle, output_log);
+               snd_pcm_hw_params_dump(hwparams, output_log);
                sz = snd_output_buffer_string(output_log, &buf);
                for (p = buf; p < buf + sz;) {
                        char *q = memchr(p, '\n', buf + sz - p);
                        if (!q)
                                break;
                        *q = '\0';
-                       PARA_INFO_LOG("%s\n", p);
+                       PARA_DEBUG_LOG("%s\n", p);
                        p = q + 1;
                }
                snd_output_close(output_log);
@@ -207,11 +218,12 @@ static void alsa_write_pre_select(struct sched *s, struct task *t)
                sched_request_barrier_or_min_delay(&pad->drain_barrier, s);
                return;
        }
+       /* wait at most 50% of the buffer time */
+       sched_request_timeout_ms(pad->buffer_time / 2, s);
        ret = snd_pcm_poll_descriptors(pad->handle, &pfd, 1);
        if (ret < 0) {
                PARA_ERROR_LOG("could not get alsa poll fd: %s\n",
                        snd_strerror(-ret));
-               t->error = -E_ALSA;
                return;
        }
        pad->poll_fd = pfd.fd;
@@ -296,21 +308,13 @@ again:
                wn->min_iqs = pad->bytes_per_frame;
                goto again;
        }
-       if (pad->poll_fd < 0 || !FD_ISSET(pad->poll_fd, &s->rfds))
-               return 0;
        frames = bytes / pad->bytes_per_frame;
        frames = snd_pcm_writei(pad->handle, data, frames);
        if (frames == 0 || frames == -EAGAIN) {
-               /*
-                * The alsa poll fd was ready for IO but we failed to write to
-                * the alsa handle. This means another event is pending. We
-                * don't care about that but we have to dispatch the event in
-                * order to avoid a busy loop. So we simply read from the poll
-                * fd.
-                */
                char buf[100];
-               if (read(pad->poll_fd, buf, 100))
-                       do_nothing;
+               if (pad->poll_fd >= 0 && FD_ISSET(pad->poll_fd, &s->rfds))
+                       if (read(pad->poll_fd, buf, 100))
+                               do_nothing;
                return 0;
        }
        if (frames > 0) {
index 5afa3ad..44b73c9 100644 (file)
@@ -771,6 +771,12 @@ void btr_drain(struct btr_node *btrn)
                btr_drop_buffer_reference(br);
 }
 
+static void btr_free_node(struct btr_node *btrn)
+{
+       free(btrn->name);
+       free(btrn);
+}
+
 /**
  * Remove a node from a buffer tree.
  *
@@ -798,8 +804,7 @@ void btr_remove_node(struct btr_node **btrnp)
        btr_drain(btrn);
        if (btrn->parent)
                list_del(&btrn->node);
-       free(btrn->name);
-       free(btrn);
+       btr_free_node(btrn);
 out:
        *btrnp = NULL;
 }
@@ -862,6 +867,7 @@ void btr_splice_out_node(struct btr_node **btrnp)
                        list_del(&ch->node);
        }
        assert(list_empty(&btrn->children));
+       btr_free_node(btrn);
        *btrnp = NULL;
 }
 
index c1ef521..b39a8b0 100644 (file)
--- a/client.c
+++ b/client.c
@@ -114,7 +114,6 @@ static int execute_client_command(const char *cmd, char **result)
        schedule(&command_sched);
        *result = exec_task.result_buf;
        btr_remove_node(&exec_task.btrn);
-       client_disconnect(ct);
        ret = 1;
 out:
        btr_remove_node(&exec_task.btrn);
@@ -444,7 +443,6 @@ static int client_i9e_line_handler(char *line)
 {
        int ret;
 
-       client_disconnect(ct);
        PARA_DEBUG_LOG("line: %s\n", line);
        ret = make_client_argv(line);
        if (ret <= 0)
index 82dbc03..e304f09 100644 (file)
--- a/client.h
+++ b/client.h
@@ -52,7 +52,6 @@ struct client_task {
        char **features;
 };
 
-void client_disconnect(struct client_task *ct);
 void client_close(struct client_task *ct);
 int client_parse_config(int argc, char *argv[], struct client_task **ct_ptr,
                int *loglevel);
index 900d365..8212abb 100644 (file)
 /** The size of the receiving buffer. */
 #define CLIENT_BUFSIZE 4000
 
-/**
- * Close the connection to para_server and deallocate per-command resources.
- *
- * \param ct The client task.
- *
- * This frees all resources of the current command but keeps the configuration
- * in \p ct->conf.
- *
- * \sa \ref client_close().
- */
-void client_disconnect(struct client_task *ct)
-{
-       if (!ct)
-               return;
-       if (ct->scc.fd >= 0)
-               close(ct->scc.fd);
-       free_argv(ct->features);
-       ct->features = NULL;
-       sc_free(ct->scc.recv);
-       ct->scc.recv = NULL;
-       sc_free(ct->scc.send);
-       ct->scc.send = NULL;
-       btr_remove_node(&ct->btrn[0]);
-       btr_remove_node(&ct->btrn[1]);
-}
-
 /**
  * Close the connection to para_server and free all resources.
  *
  * \param ct Pointer to the client data.
  *
- * \sa \ref client_open(), \ref client_disconnect().
+ * \sa \ref client_open().
  */
 void client_close(struct client_task *ct)
 {
        if (!ct)
                return;
-       client_disconnect(ct);
        free(ct->user);
        free(ct->config_file);
        free(ct->key_file);
@@ -476,6 +449,16 @@ out:
        btr_remove_node(&ct->btrn[1]);
        if (ret != -E_SERVER_CMD_SUCCESS && ret != -E_SERVER_CMD_FAILURE)
                PARA_ERROR_LOG("%s\n", para_strerror(-ret));
+       if (ct->scc.fd >= 0) {
+               close(ct->scc.fd);
+               ct->scc.fd = -1;
+       }
+       free_argv(ct->features);
+       ct->features = NULL;
+       sc_free(ct->scc.recv);
+       ct->scc.recv = NULL;
+       sc_free(ct->scc.send);
+       ct->scc.send = NULL;
        return ret;
 }
 
index 07ecffd..30bce8e 100644 (file)
@@ -757,7 +757,15 @@ if test "$have_readline" = "yes"; then
 fi
 
 if test "$have_readline" = "yes"; then
-       :
+       AC_CHECK_DECL(
+               [rl_free_keymap],
+               [AC_DEFINE(RL_FREE_KEYMAP_DECLARED, 1, readline >= 6.3)],
+               [],
+               [
+                       #include <stdio.h>
+                       #include <readline/readline.h>
+               ]
+       )
        AC_SUBST(readline_cppflags)
        AC_SUBST(readline_ldflags)
        AC_DEFINE(HAVE_READLINE, 1, define to 1 to turn on readline support)
index 21f86f4..eeb0e86 100644 (file)
@@ -80,7 +80,7 @@ static FLAC__int64 meta_tell_cb(FLAC__IOHandle handle)
 static int meta_eof_cb(FLAC__IOHandle handle)
 {
        struct private_flac_afh_data *pfad = handle;
-       return pfad->fpos == pfad->map_bytes - 1;
+       return pfad->fpos == pfad->map_bytes;
 }
 
 static int meta_close_cb(FLAC__IOHandle __a_unused handle)
index ee953bf..9f2b719 100644 (file)
@@ -227,6 +227,7 @@ static void wipe_bottom_line(void)
        fprintf(i9ep->stderr_stream, "\r");
 }
 
+#ifndef RL_FREE_KEYMAP_DECLARED
 /**
  * Free all storage associated with a keymap.
  *
@@ -237,6 +238,7 @@ static void wipe_bottom_line(void)
  * \param keymap The keymap to deallocate.
  */
 void rl_free_keymap(Keymap keymap);
+#endif
 
 /**
  * Reset the terminal and save the in-memory command line history.
index be56c98..b2c5621 100644 (file)
@@ -8,11 +8,29 @@ include(header.m4)
 option "device" d
 #~~~~~~~~~~~~~~~~
 "set PCM device"
-string typestr="device"
-default="default"
+string typestr = "device"
+default = "default"
 optional
-details="
-       On systems with dmix, a better choice than the default
-       value might be to use \"plug:swmix\".
+details = "
+       Check for the presence of a /proc/asound/ directory to see if
+       ALSA is present in your kernel. The file /proc/asound/devices
+       contains all devices ALSA knows about.
 "
+
+option "buffer-time" B
+#~~~~~~~~~~~~~~~~~~~~~
+"duration of the ALSA buffer"
+int typestr = "milliseconds"
+default = "170"
+optional
+details = "
+       This is only a hint as ALSA might pick a slightly different
+       time, depending on the sound hardware. The chosen value is
+       shown in debug output as BUFFER_TIME.
+
+       If synchronization between multiple clients is desired,
+       the same buffer time should be configured for all clients.
+"
+
 </qu>
+
index 1eaa793..3222b4a 100644 (file)
@@ -136,14 +136,13 @@ open:
                0, /* no initial bytes */
                ovc); /* the ov_open_callbacks */
        if (oret == OV_ENOTVORBIS || oret == OV_EBADHEADER) {
-               /* this might be due to the input buffer being too small */
+               /* maybe the input buffer is too small */
                if (!btr_no_parent(btrn)) {
                        fn->min_iqs += 1000;
                        iqs = btr_get_input_queue_size(btrn);
                        ret = 0;
                        if (iqs < fn->min_iqs)
                                goto out;
-                       PARA_CRIT_LOG("iqs: %zu\n", iqs);
                        btr_merge(btrn, fn->min_iqs);
                        pod->converted = 0;
                        goto open;
@@ -236,13 +235,8 @@ static int ogg_post_select(__a_unused struct sched *s, struct task *t)
                        break;
                fn->min_iqs = 0;
                have += ret;
-               if (have < OGGDEC_OUTPUT_CHUNK_SIZE)
-                       continue;
-               if (btr_get_output_queue_size(btrn) > OGGDEC_MAX_OUTPUT_SIZE)
+               if (have >= OGGDEC_OUTPUT_CHUNK_SIZE)
                        break;
-               btr_add_output(buf, have, btrn);
-               buf = para_malloc(OGGDEC_OUTPUT_CHUNK_SIZE);
-               have = 0;
        }
        pod->have_more = (ret > 0);
        if (have > 0) {
index d748985..9022fba 100644 (file)
@@ -277,7 +277,7 @@ static void opusdec_pre_select(struct sched *s, struct task *t)
 
        if (ret != 0)
                return sched_min_delay(s);
-       if (ctx->have_more)
+       if (!ctx->have_more)
                return;
        if (btr_get_output_queue_size(fn->btrn) <= OPUSDEC_MAX_OUTPUT_SIZE)
                return sched_min_delay(s);
index a2bd4e5..6ef9ac8 100644 (file)
@@ -8,6 +8,7 @@ test_options += --results-dir $(results_dir)
 test_options += --trash-dir $(trash_dir)
 test_options += --executables "$(prefixed_executables)"
 test_options += --objects "$(basename $(all_objs))"
+test_options += --man-dir $(man_dir)
 
 ifdef V
        ifeq ("$(origin V)", "command line")
diff --git a/t/t0005-man.sh b/t/t0005-man.sh
new file mode 100755 (executable)
index 0000000..7a627aa
--- /dev/null
@@ -0,0 +1,64 @@
+#!/usr/bin/env bash
+
+test_description='Parse generated man pages.
+
+Simple sanity checks of some man pages.
+
+* para_audiod: Check that list of audiod commands is present
+* para_server: Check that list of server/afs commands is present
+* para_play: Check that list of play commands is present
+
+* para_{recv,filter,write,audiod}: Check for presence of
+filter/receiver/writer options as appropriate '
+
+. ${0%/*}/test-lib.sh
+
+rfw_regex='Options for .\{100,\}Options for ' # recv/filter/writer
+
+grep_man()
+{
+       local regex="$1" exe="$2"
+       tr '\n' ' ' < "$o_man_dir/para_$exe.1" | grep -q "$regex"
+}
+
+# check that options of all reveivers/filters/writers are contained
+# in the man pages
+
+regex="$rfw_regex"
+test_expect_success 'para_recv: receiver options' "grep_man '$regex' recv"
+test_expect_success 'para_filter: filter options' "grep_man '$regex' filter"
+test_expect_success 'para_write: writer options' "grep_man '$regex' write"
+test_require_objects "audiod"
+if [[ -n "$result" ]]; then
+       test_skip 'para_audiod' "missing object(s): $result"
+else
+       test_expect_success 'para_audiod: recv/filter/writer options' \
+               "grep_man '$regex' audiod"
+fi
+
+# check various command lists
+
+test_require_objects "audiod"
+if [[ -n "$result" ]]; then
+       test_skip 'para_audiod' "missing object(s): $result"
+else
+       regex='LIST OF AUDIOD COMMANDS.\{200,\}'
+       test_expect_success 'para_audiod: command list' \
+               "grep_man '$regex' audiod"
+fi
+
+test_require_objects "server"
+missing_objects="$result"
+if [[ -n "$missing_objects" ]]; then
+       test_skip "para_server" "missing object(s): $missing_objects"
+else
+       regex='LIST OF SERVER COMMANDS.\{100,\}LIST OF AFS COMMANDS'
+       test_expect_success 'para_server: server/afs commands' \
+               "grep_man '$regex' server"
+fi
+
+# para_play is always built
+regex='LIST OF COMMANDS.\{100,\}'
+test_expect_success 'para_play: play commands' "grep_man '$regex' play"
+regex="$rfw_regex"
+test_done
index f1bb8cf..0e702b5 100644 (file)
@@ -21,22 +21,22 @@ say_color()
        if [[ "$o_nocolor" != "true" && -n "$1" ]]; then
                export TERM=$ORIGINAL_TERM
                case "$1" in
-                       error) tput bold; tput setaf 1;;
-                       skip)  tput setaf 5;;
+                       error) tput $C_BOLD; tput $C_SETAF 1;;
+                       skip)  tput $C_SETAF 5;;
                        ok)
                                (($o_verbose == 0)) && return
-                               tput setaf 2;;
-                       pass)  tput bold; tput setaf 2;;
-                       info)  tput setaf 3;;
+                               tput $C_SETAF 2;;
+                       pass)  tput $C_BOLD; tput $C_SETAF 2;;
+                       info)  tput $C_SETAF 3;;
                        run)
                                (($o_verbose == 0)) && return
-                               tput setaf 6;;
+                               tput $C_SETAF 6;;
                esac
        fi
        shift
        printf "%s\n" "$*"
        if [[ "$o_nocolor" != "true" && -n "$1" ]]; then
-               tput sgr0
+               tput $C_SGR0
                export TERM=dumb
        fi
 }
@@ -218,9 +218,21 @@ can_use_colors()
        result="false"
        [[ "$TERM" == "dumb" ]] && return
        [[ -t 1 ]] || return
-       tput bold >/dev/null 2>&1 || return
-       tput setaf 1 >/dev/null 2>&1 || return
-       tput sgr0 >/dev/null 2>&1 || return
+       C_BOLD='bold'
+       tput $C_BOLD &>/dev/null || {
+               C_BOLD='md'
+               tput $C_BOLD &>/dev/null
+       } || return
+       C_SETAF='setaf'
+       tput $C_SETAF 1 &>/dev/null || {
+               C_SETAF='AF'
+               tput $C_SETAF 1 &>/dev/null
+       } || return
+       C_SGR0='sgr0'
+       tput $C_SGR0 >/dev/null 2>&1 || {
+               C_SGR0='me'
+               tput $C_SGR0 &>/dev/null
+       } || return
        result="true"
 }
 
@@ -235,6 +247,7 @@ parse_options()
                -v=1|--verbose=1) o_verbose="1"; shift;;
                -v|--verbose|-v=2|--verbose=2) o_verbose="2"; shift;;
                --no-color) o_nocolor="true"; shift;;
+               --man-dir) export o_man_dir="$2"; shift; shift;;
                --results-dir) o_results_dir="$2"; shift; shift;;
                --trash-dir) o_trash_dir="$2"; shift; shift;;
                --executables-dir) export o_executables_dir="$2"; shift; shift;;
@@ -265,11 +278,13 @@ fixup_dirs()
        [[ -z "$o_results_dir" ]] && o_results_dir="$test_dir/test-results"
        [[ -z "$o_executables_dir" ]] && o_executables_dir="$test_dir/.."
        [[ -z "$o_trash_dir" ]] && o_trash_dir="$test_dir/trashes"
+       [[ -z "$o_man_dir" ]] && o_man_dir="$test_dir/../build/man/man1"
 
        # we want alsolute paths because relative paths become invalid
        # after changing to the trash dir
        [[ -n "${o_results_dir##/*}" ]] && o_results_dir="$wd/$o_results_dir"
        [[ -n "${o_executables_dir##/*}" ]] && o_executables_dir="$wd/$o_results_dir"
+       [[ -n "${o_man_dir##/*}" ]] && o_man_dir="$wd/$o_man_dir"
        [[ -n "${o_trash_dir##/*}" ]] && o_trash_dir="$wd/$o_trash_dir"
 
        mkdir -p "$o_results_dir"
index 8526bbc..3a190e6 100644 (file)
--- a/version.c
+++ b/version.c
@@ -57,7 +57,7 @@ const char *version_text(const char *pfx)
        static char buf[512];
 
        snprintf(buf, sizeof(buf) - 1, "%s\n"
-               "Copyright (C) 2013-2014 Andre Noll\n"
+               "Copyright (C) 2014 Andre Noll\n"
                "This is free software with ABSOLUTELY NO WARRANTY."
                " See COPYING for details.\n"
                "Report bugs to <maan@systemlinux.org>.\n"
index 7ac1ba6..c429020 100644 (file)
--- a/wma_afh.c
+++ b/wma_afh.c
@@ -132,7 +132,7 @@ static const char album_tag_header[] = { /* WM/AlbumTitle */
 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, len5;
+       uint16_t len1, len2, len3, len4;
 
        p = search_pattern(comment_header, sizeof(comment_header),
                buf, buf_size);
@@ -149,7 +149,7 @@ static void read_asf_tags(const char *buf, int buf_size, struct taginfo *ti)
        p += 2;
        len4 = read_u16(p);
        p += 2;
-       len5 = read_u16(p);
+       /* ignore length of the rating information */
        p += 2;
        if (p + len1 >= end)
                goto next;
@@ -158,10 +158,10 @@ static void read_asf_tags(const char *buf, int buf_size, struct taginfo *ti)
        if (p + len2 >= end)
                goto next;
        ti->artist = get_str16(p, len2);
-       p += len2 + len3 + len4;
-       if (p + len5 >= end)
+       p += len2 + len3;
+       if (p + len4 >= end)
                goto next;
-       ti->comment = get_str16(p, len5);
+       ti->comment = get_str16(p, len4);
 next:
        p = search_pattern(extended_content_header, sizeof(extended_content_header),
                buf, buf_size);