audioconvert: fix setting of mix matrix at run time

There were two main issues:

The mix matrix was not protected with the object lock

The code was mistakenly assuming that after updating the mix matrix
a reconfigure event sent upstream would be enough to cause upstream to
send caps again, and the converter was only reconstructed in ->set_caps.

That was not actually enough, as if the new matrix didn't affect the
number of input / output channels there was no reason for upstream to do
anything after getting the unchanged caps.

The fix for this was to have ->transform also recreate the converter
when needed, with the added subtlety that depending on the mix matrix
the element could be set to passthrough. This means that when setting
the mix matrix the converter also had to be recreated immediately to
check if the element had to be switched back to non-passthrough.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7399>

diff --git a/subprojects/gst-plugins-base/gst/audioconvert/gstaudioconvert.c b/subprojects/gst-plugins-base/gst/audioconvert/gstaudioconvert.c
index 4cf55ffc71927f651fdf5fcc91785adfa8bc4d9d..c4d2eeda55b03c451dfbccd714f92641a28dd1e4 100644 (file)
--- a/subprojects/gst-plugins-base/gst/audioconvert/gstaudioconvert.c
+++ b/subprojects/gst-plugins-base/gst/audioconvert/gstaudioconvert.c
@@ -395,6 +395,8 @@ gst_audio_convert_transform_caps (GstBaseTransform * btrans,
   gst_caps_map_in_place (tmp, remove_layout_from_structure, NULL);
   gst_caps_map_in_place (tmp, remove_channels_from_structure, btrans);
 
+  GST_OBJECT_LOCK (this);
+
   /* We can infer the required input / output channels based on the
    * matrix dimensions */
   if (gst_value_array_get_size (&this->mix_matrix)) {
@@ -411,6 +413,7 @@ gst_audio_convert_transform_caps (GstBaseTransform * btrans,
     gst_caps_map_in_place (tmp, add_other_channels_to_structure,
         GINT_TO_POINTER (other_channels));
   }
+  GST_OBJECT_UNLOCK (this);
 
   if (filter) {
     tmp2 = gst_caps_intersect_full (filter, tmp, GST_CAPS_INTERSECT_FIRST);
@@ -766,27 +769,25 @@ gst_audio_convert_fixate_caps (GstBaseTransform * base,
 }
 
 static gboolean
-gst_audio_convert_set_caps (GstBaseTransform * base, GstCaps * incaps,
-    GstCaps * outcaps)
+gst_audio_convert_ensure_converter (GstBaseTransform * base,
+    GstAudioInfo * in_info, GstAudioInfo * out_info)
 {
   GstAudioConvert *this = GST_AUDIO_CONVERT (base);
-  GstAudioInfo in_info;
-  GstAudioInfo out_info;
-  gboolean in_place;
   GstStructure *config;
+  gboolean in_place;
+  gboolean ret = TRUE;
 
-  GST_DEBUG_OBJECT (base, "incaps %" GST_PTR_FORMAT ", outcaps %"
-      GST_PTR_FORMAT, incaps, outcaps);
-
+  GST_OBJECT_LOCK (this);
   if (this->convert) {
-    gst_audio_converter_free (this->convert);
-    this->convert = NULL;
+    GST_TRACE_OBJECT (this, "We already have a converter");
+    goto done;
   }
 
-  if (!gst_audio_info_from_caps (&in_info, incaps))
-    goto invalid_in;
-  if (!gst_audio_info_from_caps (&out_info, outcaps))
-    goto invalid_out;
+  if (!GST_AUDIO_INFO_IS_VALID (in_info) || !GST_AUDIO_INFO_IS_VALID (out_info)) {
+    GST_LOG_OBJECT (this,
+        "No format information (yet), not creating converter");
+    goto done;
+  }
 
   config = gst_structure_new ("GstAudioConverterConfig",
       GST_AUDIO_CONVERTER_OPT_DITHER_METHOD, GST_TYPE_AUDIO_DITHER_METHOD,
@@ -796,41 +797,82 @@ gst_audio_convert_set_caps (GstBaseTransform * base, GstCaps * incaps,
       GST_AUDIO_CONVERTER_OPT_NOISE_SHAPING_METHOD,
       GST_TYPE_AUDIO_NOISE_SHAPING_METHOD, this->ns, NULL);
 
-  if (this->mix_matrix_is_set)
+  if (this->mix_matrix_is_set) {
     gst_structure_set_value (config, GST_AUDIO_CONVERTER_OPT_MIX_MATRIX,
         &this->mix_matrix);
 
-  this->convert = gst_audio_converter_new (0, &in_info, &out_info, config);
+    this->convert = gst_audio_converter_new (0, in_info, out_info, config);
+  } else {
+    this->convert = gst_audio_converter_new (0, in_info, out_info, config);
+  }
 
   if (this->convert == NULL)
     goto no_converter;
 
   in_place = gst_audio_converter_supports_inplace (this->convert);
+  GST_OBJECT_UNLOCK (this);
+
   gst_base_transform_set_in_place (base, in_place);
 
   gst_base_transform_set_passthrough (base,
       gst_audio_converter_is_passthrough (this->convert));
 
-  this->in_info = in_info;
-  this->out_info = out_info;
+  GST_OBJECT_LOCK (this);
 
-  return TRUE;
+done:
+  GST_OBJECT_UNLOCK (this);
+  return ret;
+
+no_converter:
+  GST_ERROR_OBJECT (this, "Failed to make converter");
+  ret = FALSE;
+  goto done;
+}
+
+static gboolean
+gst_audio_convert_set_caps (GstBaseTransform * base, GstCaps * incaps,
+    GstCaps * outcaps)
+{
+  GstAudioConvert *this = GST_AUDIO_CONVERT (base);
+  GstAudioInfo in_info;
+  GstAudioInfo out_info;
+  gboolean ret;
+
+  GST_DEBUG_OBJECT (base, "incaps %" GST_PTR_FORMAT ", outcaps %"
+      GST_PTR_FORMAT, incaps, outcaps);
+
+  if (this->convert) {
+    gst_audio_converter_free (this->convert);
+    this->convert = NULL;
+  }
+
+  if (!gst_audio_info_from_caps (&in_info, incaps))
+    goto invalid_in;
+  if (!gst_audio_info_from_caps (&out_info, outcaps))
+    goto invalid_out;
+
+  ret = gst_audio_convert_ensure_converter (base, &in_info, &out_info);
+
+  if (ret) {
+    this->in_info = in_info;
+    this->out_info = out_info;
+  }
+
+done:
+  return ret;
 
   /* ERRORS */
 invalid_in:
   {
     GST_ERROR_OBJECT (base, "invalid input caps");
-    return FALSE;
+    ret = FALSE;
+    goto done;
   }
 invalid_out:
   {
     GST_ERROR_OBJECT (base, "invalid output caps");
-    return FALSE;
-  }
-no_converter:
-  {
-    GST_ERROR_OBJECT (base, "could not make converter");
-    return FALSE;
+    ret = FALSE;
+    goto done;
   }
 }
 
@@ -849,6 +891,13 @@ gst_audio_convert_transform (GstBaseTransform * base, GstBuffer * inbuf,
   if (gst_buffer_get_size (inbuf) == 0)
     return GST_FLOW_OK;
 
+  gst_audio_convert_ensure_converter (base, &this->in_info, &this->out_info);
+
+  if (!this->convert) {
+    GST_ERROR_OBJECT (this, "No audio converter at transform time");
+    return GST_FLOW_ERROR;
+  }
+
   if (inbuf != outbuf) {
     inbuf_writable = gst_buffer_is_writable (inbuf)
         && gst_buffer_n_memory (inbuf) == 1
@@ -998,6 +1047,74 @@ gst_audio_convert_prepare_output_buffer (GstBaseTransform * base,
   return ret;
 }
 
+static void
+gst_audio_convert_set_mix_matrix (GstAudioConvert * this, const GValue * value)
+{
+  gboolean mix_matrix_was_set;
+  GstAudioConverter *old_converter;
+  GValue old_mix_matrix = G_VALUE_INIT;
+  gboolean restore = FALSE;
+
+  g_value_init (&old_mix_matrix, GST_TYPE_ARRAY);
+
+  GST_OBJECT_LOCK (this);
+
+  mix_matrix_was_set = this->mix_matrix_is_set;
+  old_converter = this->convert;
+  if (mix_matrix_was_set) {
+    g_value_copy (&this->mix_matrix, &old_mix_matrix);
+  }
+
+  if (this->convert) {
+    this->convert = NULL;
+  }
+
+  if (!gst_value_array_get_size (value)) {
+    g_value_copy (value, &this->mix_matrix);
+    this->mix_matrix_is_set = TRUE;
+  } else {
+    const GValue *first_row = gst_value_array_get_value (value, 0);
+
+    if (gst_value_array_get_size (first_row)) {
+      g_value_copy (value, &this->mix_matrix);
+      this->mix_matrix_is_set = TRUE;
+    } else {
+      g_warning ("Empty mix matrix's first row.");
+      restore = TRUE;
+      goto done;
+    }
+  }
+
+  GST_OBJECT_UNLOCK (this);
+
+  /* We need to call this here already because gst_audio_convert_transform
+   * might never get called otherwise if the element was set to passthrough.
+   *
+   * In any case if this succeeds we still want to reconfigure the sink to give
+   * upstream a chance to renegotiate channels.
+   */
+  if (gst_audio_convert_ensure_converter (GST_BASE_TRANSFORM (this),
+          &this->in_info, &this->out_info)) {
+    gst_base_transform_reconfigure_sink (GST_BASE_TRANSFORM (this));
+  } else {
+    g_warning ("Cannot build converter with this mix matrix");
+    restore = TRUE;
+    goto done;
+  }
+
+done:
+  if (restore) {
+    this->mix_matrix_is_set = mix_matrix_was_set;
+    if (mix_matrix_was_set) {
+      g_value_copy (&old_mix_matrix, &this->mix_matrix);
+    }
+    this->convert = old_converter;
+  } else if (old_converter) {
+    gst_audio_converter_free (old_converter);
+  }
+  g_value_unset (&old_mix_matrix);
+}
+
 static void
 gst_audio_convert_set_property (GObject * object, guint prop_id,
     const GValue * value, GParamSpec * pspec)
@@ -1015,23 +1132,7 @@ gst_audio_convert_set_property (GObject * object, guint prop_id,
       this->dither_threshold = g_value_get_uint (value);
       break;
     case PROP_MIX_MATRIX:
-      if (!gst_value_array_get_size (value)) {
-        g_value_copy (value, &this->mix_matrix);
-        this->mix_matrix_is_set = TRUE;
-      } else {
-        const GValue *first_row = gst_value_array_get_value (value, 0);
-
-        if (gst_value_array_get_size (first_row)) {
-          g_value_copy (value, &this->mix_matrix);
-          this->mix_matrix_is_set = TRUE;
-
-          /* issue a reconfigure upstream */
-          gst_base_transform_reconfigure_sink (GST_BASE_TRANSFORM (this));
-        } else {
-          g_warning ("Empty mix matrix's first row.");
-          this->mix_matrix_is_set = FALSE;
-        }
-      }
+      gst_audio_convert_set_mix_matrix (this, value);
       break;
     default:
       G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
@@ -1056,8 +1157,10 @@ gst_audio_convert_get_property (GObject * object, guint prop_id,
       g_value_set_uint (value, this->dither_threshold);
       break;
     case PROP_MIX_MATRIX:
+      GST_OBJECT_LOCK (object);
       if (this->mix_matrix_is_set)
         g_value_copy (&this->mix_matrix, value);
+      GST_OBJECT_UNLOCK (object);
       break;
     default:
       G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);