ext: alsa: Fix fallback paths for setting buffer and period times

Below fallback paths were introduced in
https://github.com/GStreamer/gst-plugins-base/commit/9759810d8206b5f1aa199f98599caec3630a1813
if setting period time after buffer time failed :
1) Set period time and then buffer time if it doesn't work
2) Set only buffer time
3) Set only period time

These all were not functioning properly since they were using old
copy of snd_pcm_hw_params_t which already had some fields set
as per previous try and this was causing issues as driver was
referring to that old value while trying to set them again in
fallback paths.

So now we always use the initial copy of snd_pcm_hw_params_t
for every fallback  and same is also being done at
https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/commit/557c4295107dc7374c850b0bd5331dd35e8fdd0f

Also we change the sequence to set period time earlier than
buffer time since period bytes being the smaller unit, most of the times
if underlying alsa device has a dependency then it is of period bytes
to be a multiple of some value (as per underlying DMA constraint)
and rest of the parameters like buffer bytes need to be adjusted
as per period bytes.

The same sequence is also followed in alsa-utils at
https://github.com/alsa-project/alsa-utils/commit/9b621eeac4d55c4e881f093be5b163ca07d01b63

Fix 2) and 3) scenarios by returning success if the exclusive setting is passed
and not doing any further setting for buffer time or period time.

Add new fallback path of not setting any buffer time and period time
if all above fallback paths fail. The same is also being
followed at aforementioned pulseaudio commit.

In case of alsasink, remove the retry goto label, since it is not
required anymore as fallback paths take care of setting default
values if driver is not accepting any of the fallback paths.

Use separate label for exit to free params structs and return err
code. This also fixes leak in no_rate goto path in alsasink

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/1212>

2 files changed, 125 insertions, 135 deletions

diff --git a/ext/alsa/gstalsasink.c b/ext/alsa/gstalsasink.c
index 26cd1fca8..09ee1ea58 100644
--- a/ext/alsa/gstalsasink.c
+++ b/ext/alsa/gstalsasink.c
@@ -425,22 +425,16 @@ static int
 set_hwparams (GstAlsaSink * alsa)
 {
   guint rrate;
-  gint err;
-  snd_pcm_hw_params_t *params;
-  guint period_time, buffer_time;
+  gint err = 0;
+  snd_pcm_hw_params_t *params, *params_copy;
 
   snd_pcm_hw_params_malloc (&params);
+  snd_pcm_hw_params_malloc (&params_copy);
 
   GST_DEBUG_OBJECT (alsa, "Negotiating to %d channels @ %d Hz (format = %s) "
       "SPDIF (%d)", alsa->channels, alsa->rate,
       snd_pcm_format_name (alsa->format), alsa->iec958);
 
-  /* start with requested values, if we cannot configure alsa for those values,
-   * we set these values to -1, which will leave the default alsa values */
-  buffer_time = alsa->buffer_time;
-  period_time = alsa->period_time;
-
-retry:
   /* choose all parameters */
   CHECK (snd_pcm_hw_params_any (alsa->handle, params), no_config);
   /* set the interleaved read/write format */
@@ -466,7 +460,6 @@ retry:
   rrate = alsa->rate;
   CHECK (snd_pcm_hw_params_set_rate_near (alsa->handle, params, &rrate, NULL),
       no_rate);
-
 #ifndef GST_DISABLE_GST_DEBUG
   /* get and dump some limits */
   {
@@ -490,65 +483,66 @@ retry:
     GST_DEBUG_OBJECT (alsa, "periods min %u, max %u", min, max);
   }
 #endif
+  /* Keep a copy of initial params struct that can be used later */
+  snd_pcm_hw_params_copy (params_copy, params);
   if (!alsa->iec958) {
     /* Following pulseaudio's approach in
      * https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/commit/557c4295107dc7374c850b0bd5331dd35e8fdd0f
-     * we'll try various configuration to set the buffer time and period time as some
+     * we'll try various configuration to set the period time and buffer time as some
      * driver can be picky on the order of the calls.
      */
-    if (buffer_time != -1 && period_time != -1) {
-      if (((err = snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
-                      &buffer_time, NULL)) >= 0)
+    if (alsa->buffer_time != -1 && alsa->period_time != -1) {
+      if (((err = snd_pcm_hw_params_set_period_time_near (alsa->handle,
+                      params, &alsa->period_time, NULL)) >= 0)
           && ((err =
-                  snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
-                      &period_time, NULL)) >= 0)) {
-        GST_DEBUG_OBJECT (alsa, "buffer time %u period time %u set correctly",
-            buffer_time, period_time);
-        alsa->buffer_time = buffer_time;
-        alsa->period_time = period_time;
-        goto buffer_period_set;
+                  snd_pcm_hw_params_set_buffer_time_near (alsa->handle,
+                      params, &alsa->buffer_time, NULL)) >= 0)) {
+        GST_DEBUG_OBJECT (alsa, "period time %u buffer time %u set correctly",
+            alsa->period_time, alsa->buffer_time);
+        goto success;
       }
-      if (((err = snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
-                      &period_time, NULL)) >= 0)
+      /* Try the new order with previous params struct as current one might
+         have partial settings from the order that was tried unsuccessfully */
+      snd_pcm_hw_params_copy (params, params_copy);
+      if (((err = snd_pcm_hw_params_set_buffer_time_near (alsa->handle,
+                      params, &alsa->buffer_time, NULL)) >= 0)
           && ((err =
-                  snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
-                      &buffer_time, NULL)) >= 0)) {
-        GST_DEBUG_OBJECT (alsa, "period time %u buffer time %u set correctly",
-            period_time, buffer_time);
-        alsa->buffer_time = buffer_time;
-        alsa->period_time = period_time;
-        goto buffer_period_set;
+                  snd_pcm_hw_params_set_period_time_near (alsa->handle,
+                      params, &alsa->period_time, NULL)) >= 0)) {
+        GST_DEBUG_OBJECT (alsa, "buffer time %u period time %u set correctly",
+            alsa->buffer_time, alsa->period_time);
+        goto success;
       }
     }
-    /* now try to configure the buffer time and period time independently, if one
-     * of those fail, we fall back to the defaults and emit a warning. */
-    if (buffer_time != -1) {
-      /* set the buffer time */
-      if ((err = snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
-                  &buffer_time, NULL)) < 0) {
-        GST_ELEMENT_WARNING (alsa, RESOURCE, SETTINGS, (NULL),
-            ("Unable to set buffer time %i for playback: %s",
-                buffer_time, snd_strerror (err)));
-        /* disable buffer_time the next round */
-        buffer_time = -1;
-        goto retry;
+    /* now try to configure the period time and buffer time exclusively
+     * if both fail  we fall back to the defaults */
+    if (alsa->period_time != -1) {
+      snd_pcm_hw_params_copy (params, params_copy);
+      /* set the period time */
+      if ((err =
+              snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
+                  &alsa->period_time, NULL)) < 0) {
+        GST_DEBUG_OBJECT (alsa, "Unable to set period time %i for playback: %s",
+            alsa->period_time, snd_strerror (err));
+      } else {
+        GST_DEBUG_OBJECT (alsa, "period time %u set correctly",
+            alsa->period_time);
+        goto success;
       }
-      GST_DEBUG_OBJECT (alsa, "buffer time %u set correctly", buffer_time);
-      alsa->buffer_time = buffer_time;
     }
-    if (period_time != -1) {
-      /* set the period time */
-      if ((err = snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
-                  &period_time, NULL)) < 0) {
-        GST_ELEMENT_WARNING (alsa, RESOURCE, SETTINGS, (NULL),
-            ("Unable to set period time %i for playback: %s",
-                period_time, snd_strerror (err)));
-        /* disable period_time the next round */
-        period_time = -1;
-        goto retry;
+    if (alsa->buffer_time != -1) {
+      snd_pcm_hw_params_copy (params, params_copy);
+      /* set the buffer time */
+      if ((err =
+              snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
+                  &alsa->buffer_time, NULL)) < 0) {
+        GST_DEBUG_OBJECT (alsa, "Unable to set buffer time %i for playback: %s",
+            alsa->buffer_time, snd_strerror (err));
+      } else {
+        GST_DEBUG_OBJECT (alsa, "buffer time %u set correctly",
+            alsa->buffer_time);
+        goto success;
       }
-      GST_DEBUG_OBJECT (alsa, "period time %u set correctly", period_time);
-      alsa->period_time = period_time;
     }
   } else {
     /* Set buffer size and period size manually for SPDIF */
@@ -559,16 +553,20 @@ retry:
             &buffer_size), buffer_size);
     CHECK (snd_pcm_hw_params_set_period_size_near (alsa->handle, params,
             &period_size, NULL), period_size);
+    goto success;
   }
-buffer_period_set:
+  /* Set nothing if all above failed */
+  snd_pcm_hw_params_copy (params, params_copy);
+  GST_DEBUG_OBJECT (alsa, "Not setting period time and buffer time");
+
+success:
   /* write the parameters to device */
   CHECK (snd_pcm_hw_params (alsa->handle, params), set_hw_params);
-
   /* now get the configured values */
   CHECK (snd_pcm_hw_params_get_buffer_size (params, &alsa->buffer_size),
       buffer_size);
-  CHECK (snd_pcm_hw_params_get_period_size (params, &alsa->period_size, NULL),
-      period_size);
+  CHECK (snd_pcm_hw_params_get_period_size (params, &alsa->period_size,
+          NULL), period_size);
 
   GST_DEBUG_OBJECT (alsa, "buffer size %lu, period size %lu", alsa->buffer_size,
       alsa->period_size);
@@ -578,31 +576,26 @@ buffer_period_set:
   GST_DEBUG_OBJECT (alsa, "Hw support pause: %s",
       alsa->hw_support_pause ? "yes" : "no");
 
-  snd_pcm_hw_params_free (params);
-  return 0;
-
+  goto exit;
   /* ERRORS */
 no_config:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Broken configuration for playback: no configurations available: %s",
             snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 wrong_access:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Access type not available for playback: %s", snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 no_sample_format:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Sample format not available for playback: %s", snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 no_channels:
   {
@@ -620,35 +613,36 @@ no_channels:
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, ("%s", msg),
         ("%s", snd_strerror (err)));
     g_free (msg);
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 no_rate:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Rate %iHz not available for playback: %s",
             alsa->rate, snd_strerror (err)));
-    return err;
+    goto exit;
   }
 buffer_size:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Unable to get buffer size for playback: %s", snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 period_size:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Unable to get period size for playback: %s", snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 set_hw_params:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Unable to set hw params for playback: %s", snd_strerror (err)));
+  }
+exit:
+  {
     snd_pcm_hw_params_free (params);
+    snd_pcm_hw_params_free (params_copy);
     return err;
   }
 }
diff --git a/ext/alsa/gstalsasrc.c b/ext/alsa/gstalsasrc.c
index 00667eae6..72e54520a 100644
--- a/ext/alsa/gstalsasrc.c
+++ b/ext/alsa/gstalsasrc.c
@@ -367,10 +367,11 @@ static int
 set_hwparams (GstAlsaSrc * alsa)
 {
   guint rrate;
-  gint err;
-  snd_pcm_hw_params_t *params;
+  gint err = 0;
+  snd_pcm_hw_params_t *params, *params_copy;
 
   snd_pcm_hw_params_malloc (&params);
+  snd_pcm_hw_params_malloc (&params_copy);
 
   /* choose all parameters */
   CHECK (snd_pcm_hw_params_any (alsa->handle, params), no_config);
@@ -413,77 +414,88 @@ set_hwparams (GstAlsaSrc * alsa)
     GST_DEBUG_OBJECT (alsa, "periods min %u, max %u", min, max);
   }
 #endif
+  /* Keep a copy of initial params struct that can be used later */
+  snd_pcm_hw_params_copy (params_copy, params);
   /* Following pulseaudio's approach in
    * https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/commit/557c4295107dc7374c850b0bd5331dd35e8fdd0f
    * we'll try various configuration to set the buffer time and period time as some
    * driver can be picky on the order of the calls.
    */
   if (alsa->period_time != -1 && alsa->buffer_time != -1) {
+    if ((snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
+                &alsa->period_time, NULL) >= 0)
+        && (snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
+                &alsa->buffer_time, NULL) >= 0)) {
+      GST_DEBUG_OBJECT (alsa, "period time %u buffer time %u set correctly",
+          alsa->period_time, alsa->buffer_time);
+      goto success;
+    }
+    /* Try the new order with previous params struct as current one might
+       have partial settings from the order that was tried unsuccessfully */
+    snd_pcm_hw_params_copy (params, params_copy);
     if ((snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
                 &alsa->buffer_time, NULL) >= 0)
         && (snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
                 &alsa->period_time, NULL) >= 0)) {
       GST_DEBUG_OBJECT (alsa, "buffer time %u period time %u set correctly",
           alsa->buffer_time, alsa->period_time);
-      goto buffer_period_set;
+      goto success;
     }
+  }
+  if (alsa->period_time != -1) {
+    snd_pcm_hw_params_copy (params, params_copy);
+    /* set the period time only */
     if ((snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
-                &alsa->period_time, NULL) >= 0)
-        && (snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
-                &alsa->buffer_time, NULL) >= 0)) {
-      GST_DEBUG_OBJECT (alsa, "period time %u buffer time %u set correctly",
-          alsa->period_time, alsa->buffer_time);
-      goto buffer_period_set;
+                &alsa->period_time, NULL) >= 0)) {
+      GST_DEBUG_OBJECT (alsa, "period time %u set correctly",
+          alsa->period_time);
+      goto success;
     }
   }
   if (alsa->buffer_time != -1) {
-    /* set the buffer time */
-    CHECK (snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
-            &alsa->buffer_time, NULL), buffer_time);
-    GST_DEBUG_OBJECT (alsa, "buffer time %u", alsa->buffer_time);
-  }
-  if (alsa->period_time != -1) {
-    /* set the period time */
-    CHECK (snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
-            &alsa->period_time, NULL), period_time);
-    GST_DEBUG_OBJECT (alsa, "period time %u", alsa->period_time);
+    snd_pcm_hw_params_copy (params, params_copy);
+    /* set the buffer time only */
+    if ((snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
+                &alsa->buffer_time, NULL) >= 0)) {
+      GST_DEBUG_OBJECT (alsa, "buffer time %u set correctly",
+          alsa->buffer_time);
+      goto success;
+    }
   }
+  /* Set nothing if all above failed */
+  snd_pcm_hw_params_copy (params, params_copy);
+  GST_DEBUG_OBJECT (alsa, "Not setting period time and buffer time");
 
-buffer_period_set:
+success:
   /* write the parameters to device */
   CHECK (snd_pcm_hw_params (alsa->handle, params), set_hw_params);
-
   CHECK (snd_pcm_hw_params_get_buffer_size (params, &alsa->buffer_size),
       buffer_size);
+  GST_DEBUG_OBJECT (alsa, "buffer size : %lu", alsa->buffer_size);
+  CHECK (snd_pcm_hw_params_get_period_size (params, &alsa->period_size,
+          NULL), period_size);
+  GST_DEBUG_OBJECT (alsa, "period size : %lu", alsa->period_size);
 
-  CHECK (snd_pcm_hw_params_get_period_size (params, &alsa->period_size, NULL),
-      period_size);
-
-  snd_pcm_hw_params_free (params);
-  return 0;
-
+  goto exit;
   /* ERRORS */
 no_config:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Broken configuration for recording: no configurations available: %s",
             snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 wrong_access:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Access type not available for recording: %s", snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 no_sample_format:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Sample format not available for recording: %s", snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 no_channels:
   {
@@ -501,59 +513,43 @@ no_channels:
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, ("%s", msg),
         ("%s", snd_strerror (err)));
     g_free (msg);
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 no_rate:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Rate %iHz not available for recording: %s",
             alsa->rate, snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 rate_match:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Rate doesn't match (requested %iHz, get %iHz)", alsa->rate, err));
-    snd_pcm_hw_params_free (params);
-    return -EINVAL;
-  }
-buffer_time:
-  {
-    GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
-        ("Unable to set buffer time %i for recording: %s",
-            alsa->buffer_time, snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    err = -EINVAL;
+    goto exit;
   }
 buffer_size:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Unable to get buffer size for recording: %s", snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
-  }
-period_time:
-  {
-    GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
-        ("Unable to set period time %i for recording: %s", alsa->period_time,
-            snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 period_size:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Unable to get period size for recording: %s", snd_strerror (err)));
-    snd_pcm_hw_params_free (params);
-    return err;
+    goto exit;
   }
 set_hw_params:
   {
     GST_ELEMENT_ERROR (alsa, RESOURCE, SETTINGS, (NULL),
         ("Unable to set hw params for recording: %s", snd_strerror (err)));
+  }
+exit:
+  {
     snd_pcm_hw_params_free (params);
+    snd_pcm_hw_params_free (params_copy);
     return err;
   }
 }