On 9/18/14, 2:13 AM, David Henningsson wrote:
Calling snd_pcm_avail/delay causes a syscall to the kernel, which
communicates with the audio hardware, and can therefore be expensive
on some cards.

By only updating this value after a sleep and after unusual events,
we can reduce calls to update the hardware pointer.
In particular, if a write goes well, we will now assume that the
buffer has been filled up, instead of re-asking the hardware that
this is actually the case.

You can reduce the number of syscall by using snd_pcm_status, which provides all the information at once. I thought I contributed a patch for this long back. Making assumptions on how the write went is problematic in my opinion, and there can be a case where you want to write more data while awake if there is enough room. Maybe this ought to be split in two parts for testing/bisecting.

Signed-off-by: David Henningsson <david.hennings...@canonical.com>
---
  src/modules/alsa/alsa-sink.c | 92 +++++++++++++++++++++++++++++---------------
  1 file changed, 62 insertions(+), 30 deletions(-)

Hi,

This patch is not really finished, but I'd like you to test it and see
if you have any performance improvements, before I go ahead and try to make
it more robust.
When I tested it, I didn't see much performance gain, at least not consistently.
But maybe this is because the sound cards I tested with have quick hw pointer 
updates.

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index 9e9b863..5330c78 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -155,6 +155,11 @@ struct userdata {
      pa_reserve_monitor_wrapper *monitor;
      pa_hook_slot *monitor_slot;

+    snd_pcm_status_t *pcm_status;
+    snd_pcm_sframes_t pcm_status_delay;
+    snd_pcm_sframes_t pcm_status_avail;
+    bool pcm_status_valid;
+
      /* ucm context */
      pa_alsa_ucm_mapping_context *ucm_context;
  };
@@ -440,6 +445,7 @@ static int try_recover(struct userdata *u, const char 
*call, int err) {
      if (err == -ESTRPIPE)
          pa_log_debug("%s: System suspended!", call);

+    u->pcm_status_valid = false;
      if ((err = snd_pcm_recover(u->pcm_handle, err, 1)) < 0) {
          pa_log("%s: %s", call, pa_alsa_strerror(err));
          return -1;
@@ -450,6 +456,29 @@ static int try_recover(struct userdata *u, const char 
*call, int err) {
      return 0;
  }

+static bool ensure_pcm_status(struct userdata *u) {
+    int err;
+
+    if (u->pcm_status_valid)
+        return true;
+#ifdef DEBUG_TIMING
+    pa_log_debug("Calling snd_pcm_status (hwsync)");
+#endif
+    if (PA_UNLIKELY((err = pa_alsa_safe_delay(u->pcm_handle, u->pcm_status, 
&u->pcm_status_delay, u->hwbuf_size, &u->sink->sample_spec, false)) < 0)) {
+        pa_log_warn("Failed to retrieve current pcm status: %s", 
pa_alsa_strerror(err));
+        if (try_recover(u, "snd_pcm_status", err) < 0)
+            return false;
+        if ((err = pa_alsa_safe_delay(u->pcm_handle, u->pcm_status, &u->pcm_status_delay, 
u->hwbuf_size, &u->sink->sample_spec, false)) < 0) {
+            pa_log_warn("Failed to re-retrieve current pcm status: %s", 
pa_alsa_strerror(err));
+            return false;
+        }
+    }
+
+    u->pcm_status_avail = snd_pcm_status_get_avail(u->pcm_status); /* FIXME: 
This should be checked for weird values, just like delay */
+    u->pcm_status_valid = true;
+    return true;
+}
+
  static size_t check_left_to_play(struct userdata *u, size_t n_bytes, bool 
on_timeout) {
      size_t left_to_play;
      bool underrun = false;
@@ -521,23 +550,16 @@ static int mmap_write(struct userdata *u, pa_usec_t 
*sleep_usec, bool polled, bo
          hw_sleep_time(u, &max_sleep_usec, &process_usec);

      for (;;) {
-        snd_pcm_sframes_t n;
          size_t n_bytes;
          int r;
          bool after_avail = true;

          /* First we determine how many samples are missing to fill the
           * buffer up to 100% */
+        if (PA_UNLIKELY(!ensure_pcm_status(u)))
+            return -1;

-        if (PA_UNLIKELY((n = pa_alsa_safe_avail(u->pcm_handle, u->hwbuf_size, 
&u->sink->sample_spec)) < 0)) {
-
-            if ((r = try_recover(u, "snd_pcm_avail", (int) n)) == 0)
-                continue;
-
-            return r;
-        }
-
-        n_bytes = (size_t) n * u->frame_size;
+        n_bytes = (size_t) u->pcm_status_avail * u->frame_size;

  #ifdef DEBUG_TIMING
          pa_log_debug("avail: %lu", (unsigned long) n_bytes);
@@ -609,6 +631,7 @@ static int mmap_write(struct userdata *u, pa_usec_t 
*sleep_usec, bool polled, bo

              if (PA_UNLIKELY((err = pa_alsa_safe_mmap_begin(u->pcm_handle, &areas, &offset, 
&frames, u->hwbuf_size, &u->sink->sample_spec)) < 0)) {

+                u->pcm_status_valid = false;
                  if (!after_avail && err == -EAGAIN)
                      break;

@@ -647,6 +670,7 @@ static int mmap_write(struct userdata *u, pa_usec_t 
*sleep_usec, bool polled, bo
              pa_memblock_unref_fixed(chunk.memblock);

              if (PA_UNLIKELY((sframes = snd_pcm_mmap_commit(u->pcm_handle, 
offset, frames)) < 0)) {
+                u->pcm_status_valid = false;

                  if (!after_avail && (int) sframes == -EAGAIN)
                      break;
@@ -661,6 +685,8 @@ static int mmap_write(struct userdata *u, pa_usec_t 
*sleep_usec, bool polled, bo

              u->write_count += written;
              u->since_start += written;
+            u->pcm_status_avail -= sframes;
+            u->pcm_status_delay += sframes;

  #ifdef DEBUG_TIMING
              pa_log_debug("Wrote %lu bytes (of possible %lu bytes)", (unsigned 
long) written, (unsigned long) n_bytes);
@@ -706,20 +732,14 @@ static int unix_write(struct userdata *u, pa_usec_t 
*sleep_usec, bool polled, bo
          hw_sleep_time(u, &max_sleep_usec, &process_usec);

      for (;;) {
-        snd_pcm_sframes_t n;
          size_t n_bytes;
          int r;
          bool after_avail = true;

-        if (PA_UNLIKELY((n = pa_alsa_safe_avail(u->pcm_handle, u->hwbuf_size, 
&u->sink->sample_spec)) < 0)) {
+        if (PA_UNLIKELY(!ensure_pcm_status(u)))
+            return -1;

-            if ((r = try_recover(u, "snd_pcm_avail", (int) n)) == 0)
-                continue;
-
-            return r;
-        }
-
-        n_bytes = (size_t) n * u->frame_size;
+        n_bytes = (size_t) u->pcm_status_avail * u->frame_size;

  #ifdef DEBUG_TIMING
          pa_log_debug("avail: %lu", (unsigned long) n_bytes);
@@ -789,6 +809,7 @@ static int unix_write(struct userdata *u, pa_usec_t 
*sleep_usec, bool polled, bo

              if (PA_UNLIKELY(frames < 0)) {

+                u->pcm_status_valid = false;
                  if (!after_avail && (int) frames == -EAGAIN)
                      break;

@@ -815,6 +836,8 @@ static int unix_write(struct userdata *u, pa_usec_t 
*sleep_usec, bool polled, bo

              work_done = true;

+            u->pcm_status_avail -= frames;
+            u->pcm_status_delay += frames;
              u->write_count += written;
              u->since_start += written;

@@ -848,26 +871,19 @@ static int unix_write(struct userdata *u, pa_usec_t 
*sleep_usec, bool polled, bo
  }

  static void update_smoother(struct userdata *u) {
-    snd_pcm_sframes_t delay = 0;
      int64_t position;
-    int err;
      pa_usec_t now1 = 0, now2;
-    snd_pcm_status_t *status;
      snd_htimestamp_t htstamp = { 0, 0 };

-    snd_pcm_status_alloca(&status);
-
      pa_assert(u);
      pa_assert(u->pcm_handle);
-
      /* Let's update the time smoother */
-
-    if (PA_UNLIKELY((err = pa_alsa_safe_delay(u->pcm_handle, status, &delay, u->hwbuf_size, 
&u->sink->sample_spec, false)) < 0)) {
-        pa_log_warn("Failed to query DSP status data: %s", 
pa_alsa_strerror(err));
+    if (!ensure_pcm_status(u)) {
+        pa_log_warn("Failed to query DSP status data");
          return;
      }

-    snd_pcm_status_get_htstamp(status, &htstamp);
+    snd_pcm_status_get_htstamp(u->pcm_status, &htstamp);
      now1 = pa_timespec_load(&htstamp);

      /* Hmm, if the timestamp is 0, then it wasn't set and we take the current 
time */
@@ -879,7 +895,7 @@ static void update_smoother(struct userdata *u) {
          if (u->last_smoother_update + u->smoother_interval > now1)
              return;

-    position = (int64_t) u->write_count - ((int64_t) delay * (int64_t) 
u->frame_size);
+    position = (int64_t) u->write_count - ((int64_t) u->pcm_status_delay * 
(int64_t) u->frame_size);

      if (PA_UNLIKELY(position < 0))
          position = 0;
@@ -935,6 +951,7 @@ static int suspend(struct userdata *u) {

      /* Let's suspend -- we don't call snd_pcm_drain() here since that might
       * take awfully long with our long buffer sizes today. */
+    u->pcm_status_valid = false;
      snd_pcm_close(u->pcm_handle);
      u->pcm_handle = NULL;

@@ -1002,6 +1019,7 @@ static int update_sw_params(struct userdata *u) {

      pa_log_debug("setting avail_min=%lu", (unsigned long) avail_min);

+    u->pcm_status_valid = false;
      if ((err = pa_alsa_set_sw_params(u->pcm_handle, avail_min, !u->use_tsched)) 
< 0) {
          pa_log("Failed to set software parameters: %s", 
pa_alsa_strerror(err));
          return err;
@@ -1073,6 +1091,7 @@ static int unsuspend(struct userdata *u) {
          pa_snprintf(device_name, len, "%s,AES0=6", u->device_name);
      }

+    u->pcm_status_valid = false;
      if ((err = snd_pcm_open(&u->pcm_handle, device_name ? device_name : 
u->device_name, SND_PCM_STREAM_PLAYBACK,
                              SND_PCM_NONBLOCK|
                              SND_PCM_NO_AUTO_RESAMPLE|
@@ -1136,6 +1155,7 @@ static int unsuspend(struct userdata *u) {

  fail:
      if (u->pcm_handle) {
+        u->pcm_status_valid = false;
          snd_pcm_close(u->pcm_handle);
          u->pcm_handle = NULL;
      }
@@ -1653,6 +1673,7 @@ static int process_rewind(struct userdata *u) {

          in_frames = (snd_pcm_sframes_t) (rewind_nbytes / u->frame_size);
          pa_log_debug("before: %lu", (unsigned long) in_frames);
+        u->pcm_status_valid = false;
          if ((out_frames = snd_pcm_rewind(u->pcm_handle, (snd_pcm_uframes_t) 
in_frames)) < 0) {
              pa_log("snd_pcm_rewind() failed: %s", pa_alsa_strerror((int) 
out_frames));
              if (try_recover(u, "process_rewind", out_frames) < 0)
@@ -1727,6 +1748,7 @@ static void thread_func(void *userdata) {

                  if (u->first) {
                      pa_log_info("Starting playback.");
+                    u->pcm_status_valid = false;
                      snd_pcm_start(u->pcm_handle);

                      pa_smoother_resume(u->smoother, pa_rtclock_now(), true);
@@ -1811,6 +1833,7 @@ static void thread_func(void *userdata) {
                      (double) (real_sleep - rtpoll_sleep) / PA_USEC_PER_MSEC,
                      (double) (u->tsched_watermark_usec) / PA_USEC_PER_MSEC);
          }
+        u->pcm_status_valid = false;

          if (u->sink->flags & PA_SINK_DEFERRED_VOLUME)
              pa_sink_volume_change_apply(u->sink, NULL);
@@ -1827,11 +1850,14 @@ static void thread_func(void *userdata) {
              pollfd = pa_rtpoll_item_get_pollfd(u->alsa_rtpoll_item, &n);

              if ((err = snd_pcm_poll_descriptors_revents(u->pcm_handle, pollfd, n, 
&revents)) < 0) {
+                u->pcm_status_valid = false;
                  pa_log("snd_pcm_poll_descriptors_revents() failed: %s", 
pa_alsa_strerror(err));
                  goto fail;
              }

              if (revents & ~POLLOUT) {
+                u->pcm_status_valid = false;
+
                  if (pa_alsa_recover_from_poll(u->pcm_handle, revents) < 0)
                      goto fail;

@@ -2141,6 +2167,9 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, 
const char*driver, pa_ca
      if (reserve_monitor_init(u, dev_id) < 0)
          goto fail;

+    if (snd_pcm_status_malloc(&u->pcm_status) < 0)
+        goto fail;
+
      b = use_mmap;
      d = use_tsched;

@@ -2465,6 +2494,9 @@ static void userdata_free(struct userdata *u) {
          snd_pcm_close(u->pcm_handle);
      }

+    if (u->pcm_status)
+        snd_pcm_status_free(u->pcm_status);
+
      if (u->mixer_fdl)
          pa_alsa_fdlist_free(u->mixer_fdl);



_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Reply via email to