[rhythmbox] playbin2: fix filter add/remove while paused (bug #499048)

[Jonathan Matthew <jmatthew src gnome org>]

commit 2221a3885edb753013fbb6119ae3f092ee5698b5
Author: Jonathan Matthew <jonathan d14n org>
Date:   Tue Nov 10 23:04:00 2009 +1000

    playbin2: fix filter add/remove while paused (bug #499048)
    
    rb_player_playing() isn't enough to determine whether we need to use
    pad blocking.  The playbin2 player needs to use pad blocking when paused
    (which defers the filter add/remove until next time playback starts)
    because a streaming thread holds the stream lock when the pipeline is
    paused, so attempting to remove an element will deadlock.  The
    crossfading player doesn't have this restriction, since when it's
    paused, the whole output bin (including all filters and tee branches) is
    in READY state.

 backends/gstreamer/rb-player-gst-helper.c |   50 ++++++++++++++++++-----------
 backends/gstreamer/rb-player-gst-helper.h |    8 ++--
 backends/gstreamer/rb-player-gst-xfade.c  |   14 ++++++--
 backends/gstreamer/rb-player-gst.c        |   15 ++++++---
 4 files changed, 55 insertions(+), 32 deletions(-)
---
diff --git a/backends/gstreamer/rb-player-gst-helper.c b/backends/gstreamer/rb-player-gst-helper.c
index 7c8574e..61ba844 100644
--- a/backends/gstreamer/rb-player-gst-helper.c
+++ b/backends/gstreamer/rb-player-gst-helper.c
@@ -311,7 +311,7 @@ static gboolean
 pipeline_op (GObject *player,
 	     GstElement *fixture,
 	     GstElement *element,
-	     gboolean playing,
+	     gboolean use_pad_block,
 	     GstPadBlockCallback callback)
 {
 	RBGstPipelineOp *op;
@@ -327,7 +327,7 @@ pipeline_op (GObject *player,
 	block_pad = gst_pad_get_peer (fixture_pad);
 	gst_object_unref (fixture_pad);
 
-	if (playing) {
+	if (use_pad_block) {
 		char *whatpad;
 		whatpad = gst_object_get_path_string (GST_OBJECT (block_pad));
 		rb_debug ("blocking pad %s to perform an operation", whatpad);
@@ -338,7 +338,7 @@ pipeline_op (GObject *player,
 					   callback,
 					   op);
 	} else {
-		rb_debug ("sink not playing; calling op directly");
+		rb_debug ("not using pad blocking, calling op directly");
 		(*callback) (block_pad, FALSE, op);
 	}
 
@@ -524,17 +524,20 @@ really_remove_filter (GstPad *pad,
  * @player: player object (must implement @RBPlayerGstFilter interface)
  * @filterbin: the filter bin
  * @element: the filter to add
+ * @use_pad_block: if %TRUE, block the src pad connected to the filter bin
  *
- * Inserts a filter into the filter bin, using pad blocking (if necessary) to
- * avoid breaking the data flow.
+ * Inserts a filter into the filter bin, using pad blocking (if requested) to
+ * avoid breaking the data flow.  Pad blocking should be used when the pipeline
+ * is in PLAYING state, or when in PAUSED state where a streaming thread will
+ * be holding the stream lock for the filter bin.
  */
 gboolean
-rb_gst_add_filter (RBPlayer *player, GstElement *filterbin, GstElement *element)
+rb_gst_add_filter (RBPlayer *player, GstElement *filterbin, GstElement *element, gboolean use_pad_block)
 {
 	return pipeline_op (G_OBJECT (player),
 			    filterbin,
 			    element,
-			    rb_player_playing (player),
+			    use_pad_block,
 			    (GstPadBlockCallback) really_add_filter);
 }
 
@@ -543,17 +546,20 @@ rb_gst_add_filter (RBPlayer *player, GstElement *filterbin, GstElement *element)
  * @player: player object (must implement @RBPlayerGstFilter interface)
  * @filterbin: the filter bin
  * @element: the filter to remove
+ * @use_pad_block: if %TRUE, block the src pad connected to the filter bin
  *
- * Removes a filter from the filter bin, using pad blocking (if necessary) to
- * avoid breaking the data flow.
+ * Removes a filter from the filter bin, using pad blocking (if requested) to
+ * avoid breaking the data flow.  Pad blocking should be used when the pipeline
+ * is in PLAYING state, or when in PAUSED state where a streaming thread will
+ * be holding the stream lock for the filter bin.
  */
 gboolean
-rb_gst_remove_filter (RBPlayer *player, GstElement *filterbin, GstElement *element)
+rb_gst_remove_filter (RBPlayer *player, GstElement *filterbin, GstElement *element, gboolean use_pad_block)
 {
 	return pipeline_op (G_OBJECT (player),
 			    filterbin,
 			    element,
-			    rb_player_playing (player),
+			    use_pad_block,
 			    (GstPadBlockCallback) really_remove_filter);
 }
 
@@ -653,17 +659,20 @@ really_remove_tee (GstPad *pad, gboolean blocked, RBGstPipelineOp *op)
  * @player: player object (must implement @RBPlayerGstTee interface)
  * @tee: a tee element
  * @element: the tee branch to add
+ * @use_pad_block: if %TRUE, block the src pad connected to the filter bin
  *
- * Attaches a branch to the tee, using pad blocking (if necessary) to
- * avoid breaking the data flow.
+ * Attaches a branch to the tee, using pad blocking (if requested) to
+ * avoid breaking the data flow.  Pad blocking should be used when the pipeline
+ * is in PLAYING state, or when in PAUSED state where a streaming thread will
+ * be holding the stream lock for the filter bin.
  */
 gboolean
-rb_gst_add_tee (RBPlayer *player, GstElement *tee, GstElement *element)
+rb_gst_add_tee (RBPlayer *player, GstElement *tee, GstElement *element, gboolean use_pad_block)
 {
 	return pipeline_op (G_OBJECT (player),
 			    tee,
 			    element,
-			    rb_player_playing (player),
+			    use_pad_block,
 			    (GstPadBlockCallback) really_add_tee);
 }
 
@@ -672,17 +681,20 @@ rb_gst_add_tee (RBPlayer *player, GstElement *tee, GstElement *element)
  * @player: player object (must implement @RBPlayerGstTee interface)
  * @tee: a tee element
  * @element: the tee branch to remove
+ * @use_pad_block: if %TRUE, block the src pad connected to the filter bin
  *
- * Removes a branch from the tee, using pad blocking (if necessary) to
- * avoid breaking the data flow.
+ * Removes a branch from the tee, using pad blocking (if requested) to
+ * avoid breaking the data flow.  Pad blocking should be used when the pipeline
+ * is in PLAYING state, or when in PAUSED state where a streaming thread will
+ * be holding the stream lock for the filter bin.
  */
 gboolean
-rb_gst_remove_tee (RBPlayer *player, GstElement *tee, GstElement *element)
+rb_gst_remove_tee (RBPlayer *player, GstElement *tee, GstElement *element, gboolean use_pad_block)
 {
 	return pipeline_op (G_OBJECT (player),
 			    tee,
 			    element,
-			    rb_player_playing (player),
+			    use_pad_block,
 			    (GstPadBlockCallback) really_remove_tee);
 }
 
diff --git a/backends/gstreamer/rb-player-gst-helper.h b/backends/gstreamer/rb-player-gst-helper.h
index e2efc5b..58e96ca 100644
--- a/backends/gstreamer/rb-player-gst-helper.h
+++ b/backends/gstreamer/rb-player-gst-helper.h
@@ -53,11 +53,11 @@ gboolean	rb_gst_process_tag_string	(const GstTagList *taglist,
 
 GstElement *	rb_gst_create_filter_bin (void);
 
-gboolean	rb_gst_add_filter (RBPlayer *player, GstElement *filterbin, GstElement *element);
-gboolean	rb_gst_remove_filter (RBPlayer *player, GstElement *filterbin, GstElement *element);
+gboolean	rb_gst_add_filter (RBPlayer *player, GstElement *filterbin, GstElement *element, gboolean use_pad_block);
+gboolean	rb_gst_remove_filter (RBPlayer *player, GstElement *filterbin, GstElement *element, gboolean use_pad_block);
 
-gboolean	rb_gst_add_tee (RBPlayer *player, GstElement *tee, GstElement *element);
-gboolean	rb_gst_remove_tee (RBPlayer *player, GstElement *tee, GstElement *element);
+gboolean	rb_gst_add_tee (RBPlayer *player, GstElement *tee, GstElement *element, gboolean use_pad_block);
+gboolean	rb_gst_remove_tee (RBPlayer *player, GstElement *tee, GstElement *element, gboolean use_pad_block);
 
 G_END_DECLS
 
diff --git a/backends/gstreamer/rb-player-gst-xfade.c b/backends/gstreamer/rb-player-gst-xfade.c
index e3f6af2..17a789b 100644
--- a/backends/gstreamer/rb-player-gst-xfade.c
+++ b/backends/gstreamer/rb-player-gst-xfade.c
@@ -3853,6 +3853,12 @@ rb_player_gst_xfade_get_time (RBPlayer *iplayer)
 }
 
 static gboolean
+need_pad_block (RBPlayerGstXFade *player)
+{
+	return (player->priv->sink_state == SINK_PLAYING);
+}
+
+static gboolean
 rb_player_gst_xfade_add_tee (RBPlayerGstTee *iplayer, GstElement *element)
 {
 	RBPlayerGstXFade *player = RB_PLAYER_GST_XFADE (iplayer);
@@ -3861,7 +3867,7 @@ rb_player_gst_xfade_add_tee (RBPlayerGstTee *iplayer, GstElement *element)
 		return TRUE;
 	}
 
-	return rb_gst_add_tee (RB_PLAYER (player), player->priv->tee, element);
+	return rb_gst_add_tee (RB_PLAYER (player), player->priv->tee, element, need_pad_block (player));
 }
 
 static gboolean
@@ -3874,7 +3880,7 @@ rb_player_gst_xfade_remove_tee (RBPlayerGstTee *iplayer, GstElement *element)
 		return TRUE;
 	}
 
-	return rb_gst_remove_tee (RB_PLAYER (player), player->priv->tee, element);
+	return rb_gst_remove_tee (RB_PLAYER (player), player->priv->tee, element, need_pad_block (player));
 }
 
 
@@ -3887,7 +3893,7 @@ rb_player_gst_xfade_add_filter (RBPlayerGstFilter *iplayer, GstElement *element)
 		return TRUE;
 	}
 
-	return rb_gst_add_filter (RB_PLAYER (player), player->priv->filterbin, element);
+	return rb_gst_add_filter (RB_PLAYER (player), player->priv->filterbin, element, need_pad_block (player));
 }
 
 
@@ -3901,6 +3907,6 @@ rb_player_gst_xfade_remove_filter (RBPlayerGstFilter *iplayer, GstElement *eleme
 		return TRUE;
 	}
 
-	return rb_gst_remove_filter (RB_PLAYER (player), player->priv->filterbin, element);
+	return rb_gst_remove_filter (RB_PLAYER (player), player->priv->filterbin, element, need_pad_block (player));
 }
 
diff --git a/backends/gstreamer/rb-player-gst.c b/backends/gstreamer/rb-player-gst.c
index f7e7f3b..b89fea2 100644
--- a/backends/gstreamer/rb-player-gst.c
+++ b/backends/gstreamer/rb-player-gst.c
@@ -465,7 +465,6 @@ construct_pipeline (RBPlayerGst *mp, GError **error)
 
 		g_object_set (G_OBJECT (mp->priv->playbin), "audio-sink", mp->priv->sinkbin, NULL);
 
-
 		/* add any tees and filters that were waiting for us */
 		for (l = mp->priv->waiting_tees; l != NULL; l = g_list_next (l)) {
 			rb_player_gst_tee_add_tee (RB_PLAYER_GST_TEE (mp), GST_ELEMENT (l->data));
@@ -1026,6 +1025,12 @@ impl_get_time (RBPlayer *player)
 }
 
 static gboolean
+need_pad_blocking (RBPlayerGst *mp)
+{
+	return (mp->priv->playing || (mp->priv->uri != NULL));
+}
+
+static gboolean
 impl_add_tee (RBPlayerGstTee *player, GstElement *element)
 {
 	RBPlayerGst *mp = RB_PLAYER_GST (player);
@@ -1035,7 +1040,7 @@ impl_add_tee (RBPlayerGstTee *player, GstElement *element)
 		return TRUE;
 	}
 
-	return rb_gst_add_tee (RB_PLAYER (player), mp->priv->tee, element);
+	return rb_gst_add_tee (RB_PLAYER (player), mp->priv->tee, element, need_pad_blocking (mp));
 }
 
 static gboolean
@@ -1049,7 +1054,7 @@ impl_remove_tee (RBPlayerGstTee *player, GstElement *element)
 		return TRUE;
 	}
 
-	return rb_gst_remove_tee (RB_PLAYER (mp), mp->priv->tee, element);
+	return rb_gst_remove_tee (RB_PLAYER (mp), mp->priv->tee, element, need_pad_blocking (mp));
 }
 
 static gboolean
@@ -1061,7 +1066,7 @@ impl_add_filter (RBPlayerGstFilter *player, GstElement *element)
 		mp->priv->waiting_filters = g_list_prepend (mp->priv->waiting_filters, element);
 		return TRUE;
 	}
-	return rb_gst_add_filter (RB_PLAYER (mp), mp->priv->filterbin, element);
+	return rb_gst_add_filter (RB_PLAYER (mp), mp->priv->filterbin, element, need_pad_blocking (mp));
 }
 
 static gboolean
@@ -1075,7 +1080,7 @@ impl_remove_filter (RBPlayerGstFilter *player, GstElement *element)
 		return TRUE;
 	}
 
-	return rb_gst_remove_filter (RB_PLAYER (mp), mp->priv->filterbin, element);
+	return rb_gst_remove_filter (RB_PLAYER (mp), mp->priv->filterbin, element, need_pad_blocking (mp));
 }
 
 static void