playsink: handle after-the-fact changes in converters/volume booleans

The playsink was nastily poking a boolean in the structure.
Make those booleans properties, so we are told when they change,
and rebuild the conversion bin when they do.

Some cleanup to go with it too.

https://bugzilla.gnome.org/show_bug.cgi?id=661262

gst/playback/gstplaysink.c

@@ -1799,9 +1799,15 @@ gen_audio_chain (GstPlaySink * playsink, gboolean raw)
 
   if (!(playsink->flags & GST_PLAY_FLAG_NATIVE_AUDIO) || (!have_volume
           && playsink->flags & GST_PLAY_FLAG_SOFT_VOLUME)) {
-    GST_DEBUG_OBJECT (playsink, "creating audioconvert");
+    gboolean use_converters = !(playsink->flags & GST_PLAY_FLAG_NATIVE_AUDIO);
+    gboolean use_volume =
+        !have_volume && playsink->flags & GST_PLAY_FLAG_SOFT_VOLUME;
+    GST_DEBUG_OBJECT (playsink,
+        "creating audioconvert with use-converters %d, use-volume %d",
+        use_converters, use_volume);
     chain->conv =
-        g_object_new (GST_TYPE_PLAY_SINK_AUDIO_CONVERT, "name", "aconv", NULL);
+        g_object_new (GST_TYPE_PLAY_SINK_AUDIO_CONVERT, "name", "aconv",
+        "use-converters", use_converters, "use-volume", use_volume, NULL);
     gst_bin_add (bin, chain->conv);
     if (prev) {
       if (!gst_element_link_pads_full (prev, "src", chain->conv, "sink",
@@ -1812,11 +1818,6 @@ gen_audio_chain (GstPlaySink * playsink, gboolean raw)
     }
     prev = chain->conv;
 
-    GST_PLAY_SINK_AUDIO_CONVERT_CAST (chain->conv)->use_converters =
-        !(playsink->flags & GST_PLAY_FLAG_NATIVE_AUDIO);
-    GST_PLAY_SINK_AUDIO_CONVERT_CAST (chain->conv)->use_volume = (!have_volume
-        && playsink->flags & GST_PLAY_FLAG_SOFT_VOLUME);
-
     if (!have_volume && playsink->flags & GST_PLAY_FLAG_SOFT_VOLUME) {
       GstPlaySinkAudioConvert *conv =
           GST_PLAY_SINK_AUDIO_CONVERT_CAST (chain->conv);
@@ -1964,13 +1965,13 @@ setup_audio_chain (GstPlaySink * playsink, gboolean raw)
           G_CALLBACK (notify_mute_cb), playsink);
     }
 
-    GST_PLAY_SINK_AUDIO_CONVERT_CAST (chain->conv)->use_volume = FALSE;
+    g_object_set (chain->conv, "use-volume", FALSE, NULL);
   } else {
     GstPlaySinkAudioConvert *conv =
         GST_PLAY_SINK_AUDIO_CONVERT_CAST (chain->conv);
 
     /* no volume, we need to add a volume element when we can */
-    conv->use_volume = TRUE;
+    g_object_set (chain->conv, "use-volume", TRUE, NULL);
     GST_DEBUG_OBJECT (playsink, "the sink has no volume property");
 
     /* Disconnect signals */
@@ -3028,14 +3029,14 @@ caps_notify_cb (GstPad * pad, GParamSpec * unused, GstPlaySink * playsink)
 
   if (pad == playsink->audio_pad) {
     raw = is_raw_pad (pad);
-    reconfigure = (! !playsink->audio_pad_raw != ! !raw)
+    reconfigure = (!!playsink->audio_pad_raw != !!raw)
         && playsink->audiochain;
     GST_DEBUG_OBJECT (pad,
         "Audio caps changed: raw %d reconfigure %d caps %" GST_PTR_FORMAT, raw,
         reconfigure, caps);
   } else if (pad == playsink->video_pad) {
     raw = is_raw_pad (pad);
-    reconfigure = (! !playsink->video_pad_raw != ! !raw)
+    reconfigure = (!!playsink->video_pad_raw != !!raw)
         && playsink->videochain;
     GST_DEBUG_OBJECT (pad,
         "Video caps changed: raw %d reconfigure %d caps %" GST_PTR_FORMAT, raw,

gst/playback/gstplaysinkaudioconvert.c

@@ -34,15 +34,26 @@ GST_DEBUG_CATEGORY_STATIC (gst_play_sink_audio_convert_debug);
 G_DEFINE_TYPE (GstPlaySinkAudioConvert, gst_play_sink_audio_convert,
     GST_TYPE_PLAY_SINK_CONVERT_BIN);
 
-static gboolean
+enum
-gst_play_sink_audio_convert_add_conversion_elements (GstPlaySinkConvertBin *
-    cbin)
 {
-  GstPlaySinkAudioConvert *self = GST_PLAY_SINK_AUDIO_CONVERT (cbin);
+  PROP_0,
+  PROP_USE_CONVERTERS,
+  PROP_USE_VOLUME,
+};
+
+static gboolean
+gst_play_sink_audio_convert_add_conversion_elements (GstPlaySinkAudioConvert *
+    self)
+{
+  GstPlaySinkConvertBin *cbin = GST_PLAY_SINK_CONVERT_BIN (self);
   GstElement *el, *prev = NULL;
 
   g_assert (cbin->conversion_elements == NULL);
 
+  GST_DEBUG_OBJECT (self,
+      "Building audio conversion with use-converters %d, use-volume %d",
+      self->use_converters, self->use_volume);
+
   if (self->use_converters) {
     el = gst_play_sink_convert_bin_add_conversion_element_factory (cbin,
         "audioconvert", "conv");
@@ -73,6 +84,7 @@ gst_play_sink_audio_convert_add_conversion_elements (GstPlaySinkConvertBin *
     }
     prev = el;
   }
+
   return TRUE;
 
 link_failed:
@@ -90,6 +102,59 @@ gst_play_sink_audio_convert_finalize (GObject * object)
   G_OBJECT_CLASS (parent_class)->finalize (object);
 }
 
+static void
+gst_play_sink_audio_convert_set_property (GObject * object, guint prop_id,
+    const GValue * value, GParamSpec * pspec)
+{
+  GstPlaySinkAudioConvert *self = GST_PLAY_SINK_AUDIO_CONVERT_CAST (object);
+  gboolean v, changed = FALSE;
+
+  switch (prop_id) {
+    case PROP_USE_CONVERTERS:
+      v = g_value_get_boolean (value);
+      if (v != self->use_converters) {
+        self->use_converters = v;
+        changed = TRUE;
+      }
+      break;
+    case PROP_USE_VOLUME:
+      v = g_value_get_boolean (value);
+      if (v != self->use_volume) {
+        self->use_volume = v;
+        changed = TRUE;
+      }
+      break;
+    default:
+      break;
+  }
+
+  if (changed) {
+    GstPlaySinkConvertBin *cbin = GST_PLAY_SINK_CONVERT_BIN (self);
+    GST_DEBUG_OBJECT (self, "Rebuilding converter bin");
+    gst_play_sink_convert_bin_remove_elements (cbin);
+    gst_play_sink_audio_convert_add_conversion_elements (self);
+    gst_play_sink_convert_bin_cache_converter_caps (cbin);
+  }
+}
+
+static void
+gst_play_sink_audio_convert_get_property (GObject * object, guint prop_id,
+    GValue * value, GParamSpec * pspec)
+{
+  GstPlaySinkAudioConvert *self = GST_PLAY_SINK_AUDIO_CONVERT_CAST (object);
+
+  switch (prop_id) {
+    case PROP_USE_CONVERTERS:
+      g_value_set_boolean (value, self->use_converters);
+      break;
+    case PROP_USE_VOLUME:
+      g_value_set_boolean (value, self->use_volume);
+      break;
+    default:
+      break;
+  }
+}
+
 static void
 gst_play_sink_audio_convert_class_init (GstPlaySinkAudioConvertClass * klass)
 {
@@ -103,6 +168,18 @@ gst_play_sink_audio_convert_class_init (GstPlaySinkAudioConvertClass * klass)
   gstelement_class = (GstElementClass *) klass;
 
   gobject_class->finalize = gst_play_sink_audio_convert_finalize;
+  gobject_class->set_property = gst_play_sink_audio_convert_set_property;
+  gobject_class->get_property = gst_play_sink_audio_convert_get_property;
+
+  g_object_class_install_property (gobject_class, PROP_USE_CONVERTERS,
+      g_param_spec_boolean ("use-converters", "Use converters",
+          "Whether to use conversion elements", FALSE,
+          G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS));
+
+  g_object_class_install_property (gobject_class, PROP_USE_VOLUME,
+      g_param_spec_boolean ("use-volume", "Use volume",
+          "Whether to use a volume element", FALSE,
+          G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS));
 
   gst_element_class_set_details_simple (gstelement_class,
       "Player Sink Audio Converter", "Audio/Bin/Converter",
@@ -116,8 +193,6 @@ gst_play_sink_audio_convert_init (GstPlaySinkAudioConvert * self)
   GstPlaySinkConvertBin *cbin = GST_PLAY_SINK_CONVERT_BIN (self);
 
   cbin->audio = TRUE;
-  cbin->add_conversion_elements =
-      gst_play_sink_audio_convert_add_conversion_elements;
 
   /* FIXME: Only create this on demand but for now we need
    * it to always exist because of playsink's volume proxying
@@ -126,4 +201,7 @@ gst_play_sink_audio_convert_init (GstPlaySinkAudioConvert * self)
   self->volume = gst_element_factory_make ("volume", "volume");
   if (self->volume)
     gst_object_ref_sink (self->volume);
+
+  gst_play_sink_audio_convert_add_conversion_elements (self);
+  gst_play_sink_convert_bin_cache_converter_caps (cbin);
 }

gst/playback/gstplaysinkaudioconvert.h

@@ -36,7 +36,6 @@ G_BEGIN_DECLS
   (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GST_TYPE_PLAY_SINK_AUDIO_CONVERT))
 #define GST_IS_PLAY_SINK_AUDIO_CONVERT_CLASS(klass) \
   (G_TYPE_CHECK_CLASS_TYPE ((klass), GST_TYPE_PLAY_SINK_AUDIO_CONVERT))
-
 typedef struct _GstPlaySinkAudioConvert GstPlaySinkAudioConvert;
 typedef struct _GstPlaySinkAudioConvertClass GstPlaySinkAudioConvertClass;
 
@@ -46,8 +45,8 @@ struct _GstPlaySinkAudioConvert
 
   /* < pseudo public > */
   GstElement *volume;
-  gboolean use_volume;
   gboolean use_converters;
+  gboolean use_volume;
 };
 
 struct _GstPlaySinkAudioConvertClass

gst/playback/gstplaysinkconvertbin.c

@@ -147,6 +147,8 @@ gst_play_sink_convert_bin_set_targets (GstPlaySinkConvertBin * self,
   GstPad *pad;
   GstElement *head, *tail;
 
+  GST_DEBUG_OBJECT (self, "Setting pad targets with passthrough %d",
+      passthrough);
   if (self->conversion_elements == NULL || passthrough) {
     if (!passthrough) {
       GST_WARNING_OBJECT (self,
@@ -159,10 +161,12 @@ gst_play_sink_convert_bin_set_targets (GstPlaySinkConvertBin * self,
   }
 
   pad = gst_element_get_static_pad (head, "sink");
+  GST_DEBUG_OBJECT (self, "Ghosting bin sink pad to %" GST_PTR_FORMAT, pad);
   gst_ghost_pad_set_target (GST_GHOST_PAD_CAST (self->sinkpad), pad);
   gst_object_unref (pad);
 
   pad = gst_element_get_static_pad (tail, "src");
+  GST_DEBUG_OBJECT (self, "Ghosting bin src pad to %" GST_PTR_FORMAT, pad);
   gst_ghost_pad_set_target (GST_GHOST_PAD_CAST (self->srcpad), pad);
   gst_object_unref (pad);
 }
@@ -289,6 +293,7 @@ gst_play_sink_convert_bin_sink_setcaps (GstPad * pad, GstCaps * caps)
   gboolean reconfigure = FALSE;
   gboolean raw;
 
+  GST_DEBUG_OBJECT (pad, "setcaps: %" GST_PTR_FORMAT, caps);
   GST_PLAY_SINK_CONVERT_BIN_LOCK (self);
   s = gst_caps_get_structure (caps, 0);
   name = gst_structure_get_name (s);
@@ -299,6 +304,8 @@ gst_play_sink_convert_bin_sink_setcaps (GstPad * pad, GstCaps * caps)
     raw = g_str_has_prefix (name, "video/x-raw-");
   }
 
+  GST_DEBUG_OBJECT (self, "raw %d, self->raw %d, blocked %d",
+      raw, self->raw, gst_pad_is_blocked (self->sink_proxypad));
   if (raw) {
     if (!self->raw && !gst_pad_is_blocked (self->sink_proxypad)) {
       GST_DEBUG_OBJECT (self, "Changing caps from non-raw to raw");
@@ -376,24 +383,49 @@ gst_play_sink_convert_bin_getcaps (GstPad * pad)
   return ret;
 }
 
+void
+gst_play_sink_convert_bin_remove_elements (GstPlaySinkConvertBin * self)
+{
+  if (self->conversion_elements) {
+    g_list_foreach (self->conversion_elements,
+        (GFunc) gst_play_sink_convert_bin_remove_element, self);
+    g_list_free (self->conversion_elements);
+    self->conversion_elements = NULL;
+  }
+  if (self->identity) {
+    gst_element_set_state (self->identity, GST_STATE_NULL);
+    gst_bin_remove (GST_BIN_CAST (self), self->identity);
+    self->identity = NULL;
+  }
+  if (self->converter_caps) {
+    gst_caps_unref (self->converter_caps);
+    self->converter_caps = NULL;
+  }
+}
+
 static void
 gst_play_sink_convert_bin_finalize (GObject * object)
 {
   GstPlaySinkConvertBin *self = GST_PLAY_SINK_CONVERT_BIN_CAST (object);
 
+  gst_play_sink_convert_bin_remove_elements (self);
+
   gst_object_unref (self->sink_proxypad);
   g_mutex_free (self->lock);
 
   G_OBJECT_CLASS (parent_class)->finalize (object);
 }
 
-static void
+void
 gst_play_sink_convert_bin_cache_converter_caps (GstPlaySinkConvertBin * self)
 {
   GstElement *head;
   GstPad *pad;
 
-  self->converter_caps = NULL;
+  if (self->converter_caps) {
+    gst_caps_unref (self->converter_caps);
+    self->converter_caps = NULL;
+  }
 
   if (!self->conversion_elements) {
     GST_WARNING_OBJECT (self, "No conversion elements");
@@ -422,17 +454,6 @@ gst_play_sink_convert_bin_change_state (GstElement * element,
   GstPlaySinkConvertBin *self = GST_PLAY_SINK_CONVERT_BIN_CAST (element);
 
   switch (transition) {
-    case GST_STATE_CHANGE_NULL_TO_READY:
-      GST_PLAY_SINK_CONVERT_BIN_LOCK (self);
-      g_assert (self->add_conversion_elements);
-      if (!(*self->add_conversion_elements) (self)) {
-        GST_ELEMENT_ERROR (self, CORE, PAD,
-            (NULL), ("Failed to configure the converter bin."));
-      }
-      gst_play_sink_convert_bin_cache_converter_caps (self);
-      gst_play_sink_convert_bin_add_identity (self);
-      GST_PLAY_SINK_CONVERT_BIN_UNLOCK (self);
-      break;
     case GST_STATE_CHANGE_PAUSED_TO_READY:
       GST_PLAY_SINK_CONVERT_BIN_LOCK (self);
       if (gst_pad_is_blocked (self->sink_proxypad))
@@ -465,25 +486,6 @@ gst_play_sink_convert_bin_change_state (GstElement * element,
             (GDestroyNotify) gst_object_unref);
       GST_PLAY_SINK_CONVERT_BIN_UNLOCK (self);
       break;
-    case GST_STATE_CHANGE_READY_TO_NULL:
-      GST_PLAY_SINK_CONVERT_BIN_LOCK (self);
-      if (self->conversion_elements) {
-        g_list_foreach (self->conversion_elements,
-            (GFunc) gst_play_sink_convert_bin_remove_element, self);
-        g_list_free (self->conversion_elements);
-        self->conversion_elements = NULL;
-      }
-      if (self->identity) {
-        gst_element_set_state (self->identity, GST_STATE_NULL);
-        gst_bin_remove (GST_BIN_CAST (self), self->identity);
-        self->identity = NULL;
-      }
-      if (self->converter_caps) {
-        gst_caps_unref (self->converter_caps);
-        self->converter_caps = NULL;
-      }
-      GST_PLAY_SINK_CONVERT_BIN_UNLOCK (self);
-      break;
     default:
       break;
   }
@@ -547,4 +549,6 @@ gst_play_sink_convert_bin_init (GstPlaySinkConvertBin * self)
       GST_DEBUG_FUNCPTR (gst_play_sink_convert_bin_getcaps));
   gst_element_add_pad (GST_ELEMENT_CAST (self), self->srcpad);
   gst_object_unref (templ);
+
+  gst_play_sink_convert_bin_add_identity (self);
 }

gst/playback/gstplaysinkconvertbin.h

@@ -78,7 +78,6 @@ struct _GstPlaySinkConvertBin
 
   /* configuration for derived classes */
   gboolean audio;
-  gboolean (*add_conversion_elements)(GstPlaySinkConvertBin *);
 };
 
 struct _GstPlaySinkConvertBinClass
@@ -93,6 +92,10 @@ gst_play_sink_convert_bin_add_conversion_element_factory (GstPlaySinkConvertBin
 void
 gst_play_sink_convert_bin_add_conversion_element (GstPlaySinkConvertBin *self,
     GstElement *el);
+void
+gst_play_sink_convert_bin_cache_converter_caps (GstPlaySinkConvertBin * self);
+void
+gst_play_sink_convert_bin_remove_elements (GstPlaySinkConvertBin * self);
 
 G_END_DECLS
 #endif /* __GST_PLAY_SINK_CONVERT_BIN_H__ */

gst/playback/gstplaysinkvideoconvert.c

@@ -35,9 +35,10 @@ G_DEFINE_TYPE (GstPlaySinkVideoConvert, gst_play_sink_video_convert,
     GST_TYPE_PLAY_SINK_CONVERT_BIN);
 
 static gboolean
-gst_play_sink_video_convert_add_conversion_elements (GstPlaySinkConvertBin *
+gst_play_sink_video_convert_add_conversion_elements (GstPlaySinkVideoConvert *
-    cbin)
+    self)
 {
+  GstPlaySinkConvertBin *cbin = GST_PLAY_SINK_CONVERT_BIN (self);
   GstElement *el, *prev = NULL;
 
   el = gst_play_sink_convert_bin_add_conversion_element_factory (cbin,
@@ -87,6 +88,7 @@ gst_play_sink_video_convert_init (GstPlaySinkVideoConvert * self)
 {
   GstPlaySinkConvertBin *cbin = GST_PLAY_SINK_CONVERT_BIN (self);
   cbin->audio = FALSE;
-  cbin->add_conversion_elements =
+
-      gst_play_sink_video_convert_add_conversion_elements;
+  gst_play_sink_video_convert_add_conversion_elements (self);
+  gst_play_sink_convert_bin_cache_converter_caps (cbin);
 }

gst/playback/gstplaysinkvideoconvert.h

@@ -36,7 +36,6 @@ G_BEGIN_DECLS
   (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GST_TYPE_PLAY_SINK_VIDEO_CONVERT))
 #define GST_IS_PLAY_SINK_VIDEO_CONVERT_CLASS(klass) \
   (G_TYPE_CHECK_CLASS_TYPE ((klass), GST_TYPE_PLAY_SINK_VIDEO_CONVERT))
-
 typedef struct _GstPlaySinkVideoConvert GstPlaySinkVideoConvert;
 typedef struct _GstPlaySinkVideoConvertClass GstPlaySinkVideoConvertClass;