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

Below fallback paths were introduced in
9759810d82
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
557c429510

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
9b621eeac4

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>

ext/alsa/gstalsasink.c

@@ -425,22 +425,16 @@ static int
 set_hwparams (GstAlsaSink * alsa)
 {
   guint rrate;
-  gint err;
+  gint err = 0;
-  snd_pcm_hw_params_t *params;
+  snd_pcm_hw_params_t *params, *params_copy;
-  guint period_time, buffer_time;
 
   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 (alsa->buffer_time != -1 && alsa->period_time != -1) {
-      if (((err = snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
+      if (((err = snd_pcm_hw_params_set_period_time_near (alsa->handle,
-                      &buffer_time, NULL)) >= 0)
+                      params, &alsa->period_time, NULL)) >= 0)
           && ((err =
-                  snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
+                  snd_pcm_hw_params_set_buffer_time_near (alsa->handle,
-                      &period_time, NULL)) >= 0)) {
+                      params, &alsa->buffer_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;
-      }
-      if (((err = snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
-                      &period_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->period_time, alsa->buffer_time);
-        alsa->buffer_time = buffer_time;
+        goto success;
-        alsa->period_time = period_time;
+      }
-        goto buffer_period_set;
+      /* 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_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
+    /* now try to configure the period time and buffer time exclusively
-     * of those fail, we fall back to the defaults and emit a warning. */
+     * if both fail  we fall back to the defaults */
-    if (buffer_time != -1) {
+    if (alsa->period_time != -1) {
-      /* set the buffer time */
+      snd_pcm_hw_params_copy (params, params_copy);
-      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;
-      }
-      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,
+      if ((err =
-                  &period_time, NULL)) < 0) {
+              snd_pcm_hw_params_set_period_time_near (alsa->handle, params,
-        GST_ELEMENT_WARNING (alsa, RESOURCE, SETTINGS, (NULL),
+                  &alsa->period_time, NULL)) < 0) {
-            ("Unable to set period time %i for playback: %s",
+        GST_DEBUG_OBJECT (alsa, "Unable to set period time %i for playback: %s",
-                period_time, snd_strerror (err)));
+            alsa->period_time, snd_strerror (err));
-        /* disable period_time the next round */
+      } else {
-        period_time = -1;
+        GST_DEBUG_OBJECT (alsa, "period time %u set correctly",
-        goto retry;
+            alsa->period_time);
+        goto success;
+      }
+    }
+    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),
+  CHECK (snd_pcm_hw_params_get_period_size (params, &alsa->period_size,
-      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);
+  goto exit;
-  return 0;
-
   /* 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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;
   }
 }

ext/alsa/gstalsasrc.c

@@ -367,10 +367,11 @@ static int
 set_hwparams (GstAlsaSrc * alsa)
 {
   guint rrate;
-  gint err;
+  gint err = 0;
-  snd_pcm_hw_params_t *params;
+  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_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;
-    }
     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;
+      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 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)) {
+      GST_DEBUG_OBJECT (alsa, "period time %u set correctly",
+          alsa->period_time);
+      goto success;
     }
   }
   if (alsa->buffer_time != -1) {
-    /* set the buffer time */
+    snd_pcm_hw_params_copy (params, params_copy);
-    CHECK (snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
+    /* set the buffer time only */
-            &alsa->buffer_time, NULL), buffer_time);
+    if ((snd_pcm_hw_params_set_buffer_time_near (alsa->handle, params,
-    GST_DEBUG_OBJECT (alsa, "buffer time %u", alsa->buffer_time);
+                &alsa->buffer_time, NULL) >= 0)) {
-  }
+      GST_DEBUG_OBJECT (alsa, "buffer time %u set correctly",
-  if (alsa->period_time != -1) {
+          alsa->buffer_time);
-    /* set the period time */
+      goto success;
-    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);
   }
+  /* 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),
+  goto exit;
-      period_size);
-
-  snd_pcm_hw_params_free (params);
-  return 0;
-
   /* 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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);
+    goto exit;
-    return err;
   }
 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);
+    err = -EINVAL;
-    return -EINVAL;
+    goto exit;
-  }
-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;
   }
 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);
+    goto exit;
-    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;
   }
 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);
+    goto exit;
-    return err;
   }
 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;
   }
 }