GNOME.org
 

[rhythmbox] metadata: remove the need to load metadata before saving

commit f1751ab2bc5030ce911be953647aad107567b391
Author: Jonathan Matthew <jonathan d14n org>
Date:   Thu Jan 28 06:03:43 2010 +1000

    metadata: remove the need to load metadata before saving
    
    There's no need to load the metadata first, the tags from the file will
    be available in the tag writing pipeline anyway.  When saving metadata,
    we need a typefind element in the pipeline, since we don't already know
    the file type.
    
    We now have a method to reset the metadata state.  This previously
    happened at the start of the loading process, but we don't necessarily
    do that between save operations any more.

 bindings/python/rb.defs             |    7 ++
 metadata/rb-metadata-dbus-client.c  |   36 ++++++--
 metadata/rb-metadata-dbus-service.c |   12 ++-
 metadata/rb-metadata-gst.c          |  184 ++++++++++++++++++++++-------------
 metadata/rb-metadata.h              |    3 +
 rhythmdb/rhythmdb.c                 |    9 +--
 6 files changed, 167 insertions(+), 84 deletions(-)
---
diff --git a/bindings/python/rb.defs b/bindings/python/rb.defs
index 9121680..750c11e 100644
--- a/bindings/python/rb.defs
+++ b/bindings/python/rb.defs
@@ -2757,6 +2757,12 @@
   (return-type "char**")
 )
 
+(define-method reset
+  (of-object "RBMetaData")
+  (c-name "rb_metadata_reset")
+  (return-type "none")
+)
+
 (define-method load
   (of-object "RBMetaData")
   (c-name "rb_metadata_load")
@@ -2772,6 +2778,7 @@
   (c-name "rb_metadata_save")
   (return-type "none")
   (parameters
+    '("const-char*" "uri")
     '("GError**" "error")
   )
 )
diff --git a/metadata/rb-metadata-dbus-client.c b/metadata/rb-metadata-dbus-client.c
index 30e1760..f02193f 100644
--- a/metadata/rb-metadata-dbus-client.c
+++ b/metadata/rb-metadata-dbus-client.c
@@ -87,7 +87,6 @@ static char **saveable_types = NULL;
 
 struct RBMetaDataPrivate
 {
-	char       *uri;
 	char       *mimetype;
 	char      **missing_plugins;
 	char      **plugin_descriptions;
@@ -124,7 +123,6 @@ rb_metadata_finalize (GObject *object)
 
 	md = RB_METADATA (object);
 
-	g_free (md->priv->uri);
 	g_free (md->priv->mimetype);
 	if (md->priv->metadata)
 		g_hash_table_destroy (md->priv->metadata);
@@ -407,6 +405,24 @@ read_error_from_message (RBMetaData *md, DBusMessageIter *iter, GError **error)
 }
 
 /**
+ * rb_metadata_reset:
+ * @md: a #RBMetaData
+ *
+ * Resets the state of the metadata interface.  Call this before
+ * setting tags to be written to a file.
+ */
+void
+rb_metadata_reset (RBMetaData *md)
+{
+	if (md->priv->metadata)
+		g_hash_table_destroy (md->priv->metadata);
+	md->priv->metadata = g_hash_table_new_full (g_direct_hash,
+						    g_direct_equal,
+						    NULL,
+						    (GDestroyNotify)rb_value_free);
+}
+
+/**
  * rb_metadata_load:
  * @md: a #RBMetaData
  * @uri: URI from which to load metadata
@@ -440,14 +456,10 @@ rb_metadata_load (RBMetaData *md,
 	g_free (md->priv->mimetype);
 	md->priv->mimetype = NULL;
 
-	g_free (md->priv->uri);
-	md->priv->uri = g_strdup (uri);
 	if (uri == NULL)
 		return;
 
-	if (md->priv->metadata)
-		g_hash_table_destroy (md->priv->metadata);
-	md->priv->metadata = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, (GDestroyNotify)rb_value_free);
+	rb_metadata_reset (md);
 
 	g_static_mutex_lock (&conn_mutex);
 
@@ -746,7 +758,7 @@ rb_metadata_get_saveable_types (RBMetaData *md)
  * target URI.
  */
 void
-rb_metadata_save (RBMetaData *md, GError **error)
+rb_metadata_save (RBMetaData *md, const char *uri, GError **error)
 {
 	GError *fake_error = NULL;
 	DBusMessage *message = NULL;
@@ -776,6 +788,14 @@ rb_metadata_save (RBMetaData *md, GError **error)
 
 	if (*error == NULL) {
 		dbus_message_iter_init_append (message, &iter);
+		if (!dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &uri)) {
+			g_set_error (error,
+				     RB_METADATA_ERROR,
+				     RB_METADATA_ERROR_INTERNAL,
+				     _("D-BUS communication error"));
+		}
+	}
+	if (*error == NULL) {
 		if (!rb_metadata_dbus_add_to_message (md, &iter)) {
 			g_set_error (error,
 				     RB_METADATA_ERROR,
diff --git a/metadata/rb-metadata-dbus-service.c b/metadata/rb-metadata-dbus-service.c
index cdecfa5..21dbcae 100644
--- a/metadata/rb-metadata-dbus-service.c
+++ b/metadata/rb-metadata-dbus-service.c
@@ -241,30 +241,38 @@ rb_metadata_dbus_save (DBusConnection *connection,
 		       DBusMessage *message,
 		       ServiceData *svc)
 {
+	char *uri;
 	DBusMessageIter iter;
 	DBusMessage *reply;
 	GHashTable *data;
 	GError *error = NULL;
 
-	/* get metadata from message */
+	/* get URI and metadata from message */
 	if (!dbus_message_iter_init (message, &iter)) {
 		return DBUS_HANDLER_RESULT_NEED_MEMORY;
 	}
+	if (!rb_metadata_dbus_get_string (&iter, &uri)) {
+		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+	}
+
 	data = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, (GDestroyNotify)rb_value_free);
 	if (!rb_metadata_dbus_read_from_message (svc->metadata,
 						 data,
 						 &iter)) {
 		/* make translatable? */
+		g_free (uri);
 		return send_error (connection, message,
 				   RB_METADATA_ERROR_INTERNAL,
 				   "Unable to read metadata from message");
 	}
 
 	/* pass to real metadata instance, and save it */
+	rb_metadata_reset (svc->metadata);
 	g_hash_table_foreach_remove (data, (GHRFunc) _set_metadata, svc->metadata);
 	g_hash_table_destroy (data);
 
-	rb_metadata_save (svc->metadata, &error);
+	rb_metadata_save (svc->metadata, uri, &error);
+	g_free (uri);
 
 	if (error) {
 		DBusHandlerResult r;
diff --git a/metadata/rb-metadata-gst.c b/metadata/rb-metadata-gst.c
index 325f847..287422d 100644
--- a/metadata/rb-metadata-gst.c
+++ b/metadata/rb-metadata-gst.c
@@ -51,8 +51,6 @@ static void rb_metadata_finalize (GObject *object);
 
 struct RBMetaDataPrivate
 {
-	char *uri;
-
 	GHashTable *metadata;
 
 	GstElement *pipeline;
@@ -73,6 +71,43 @@ struct RBMetaDataPrivate
 
 #define RB_METADATA_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), RB_TYPE_METADATA, RBMetaDataPrivate))
 
+void
+rb_metadata_reset (RBMetaData *md)
+{
+	if (md->priv->metadata)
+		g_hash_table_destroy (md->priv->metadata);
+	md->priv->metadata = g_hash_table_new_full (g_direct_hash, g_direct_equal,
+						    NULL, (GDestroyNotify) rb_value_free);
+
+	if (md->priv->pipeline) {
+		gst_object_unref (md->priv->pipeline);
+		md->priv->pipeline = NULL;
+	}
+	if (md->priv->sink) {
+		gst_object_unref (md->priv->sink);
+		md->priv->sink = NULL;
+	}
+	md->priv->typefind_cb_id = 0;
+	if (md->priv->tags) {
+		gst_tag_list_free (md->priv->tags);
+		md->priv->tags = NULL;
+	}
+
+	g_free (md->priv->type);
+	md->priv->type = NULL;
+	md->priv->eos = FALSE;
+	md->priv->has_audio = FALSE;
+	md->priv->has_non_audio = FALSE;
+	md->priv->has_video = FALSE;
+	if (md->priv->missing_plugins != NULL) {
+		g_slist_foreach (md->priv->missing_plugins, (GFunc) gst_message_unref, NULL);
+		g_slist_free (md->priv->missing_plugins);
+		md->priv->missing_plugins = NULL;
+	}
+
+	g_clear_error (&md->priv->error);
+}
+
 
 static GstElement *
 flac_tagger (GstElement *pipeline, GstElement *link_to, GstTagList *tags)
@@ -314,18 +349,10 @@ rb_metadata_finalize (GObject *object)
 
 	md = RB_METADATA (object);
 
-	if (md->priv->metadata)
-		g_hash_table_destroy (md->priv->metadata);
-
-	if (md->priv->pipeline)
-		gst_object_unref (GST_OBJECT (md->priv->pipeline));
-
+	rb_metadata_reset (md);
 	if (md->priv->taggers)
 		g_hash_table_destroy (md->priv->taggers);
 
-	g_free (md->priv->type);
-	g_free (md->priv->uri);
-	g_clear_error (&md->priv->error);
 
 	G_OBJECT_CLASS (rb_metadata_parent_class)->finalize (object);
 }
@@ -455,6 +482,7 @@ rb_metadata_gst_typefind_cb (GstElement *typefind, guint probability, GstCaps *c
 	}
 
 	g_signal_handler_disconnect (typefind, md->priv->typefind_cb_id);
+	md->priv->typefind_cb_id = 0;
 }
 
 static void
@@ -689,32 +717,11 @@ rb_metadata_load (RBMetaData *md,
 	int change_timeout;
 	GstBus *bus;
 
-	g_free (md->priv->uri);
-	md->priv->uri = NULL;
-	g_free (md->priv->type);
-	md->priv->type = NULL;
-	md->priv->error = NULL;
-	md->priv->eos = FALSE;
-	md->priv->has_audio = FALSE;
-	md->priv->has_non_audio = FALSE;
-	md->priv->has_video = FALSE;
-	md->priv->missing_plugins = NULL;
-
-	if (md->priv->pipeline) {
-		gst_object_unref (GST_OBJECT (md->priv->pipeline));
-		md->priv->pipeline = NULL;
-	}
-
+	rb_metadata_reset (md);
 	if (uri == NULL)
 		return;
 
 	rb_debug ("loading metadata for uri: %s", uri);
-	md->priv->uri = g_strdup (uri);
-
-	if (md->priv->metadata)
-		g_hash_table_destroy (md->priv->metadata);
-	md->priv->metadata = g_hash_table_new_full (g_direct_hash, g_direct_equal,
-						    NULL, (GDestroyNotify) rb_value_free);
 
 	/* The main tagfinding pipeline looks like this:
  	 * <src> ! decodebin ! fakesink
@@ -913,7 +920,7 @@ rb_metadata_gst_add_tag_data (gpointer key, const GValue *val, RBMetaData *md)
 }
 
 static gboolean
-rb_metadata_file_valid (char *original, char *newfile)
+rb_metadata_file_valid (const char *original, const char *newfile)
 {
 	RBMetaData *md = rb_metadata_new ();
 	GError *error = NULL;
@@ -930,25 +937,75 @@ rb_metadata_file_valid (char *original, char *newfile)
 	return ret;
 }
 
+static void
+metadata_save_type_found (GstElement *typefind, guint probability, GstCaps *caps, RBMetaData *md)
+{
+	RBAddTaggerElem add_tagger_func;
+        GstElement *retag_end;
+	GstMessage *message;
+	const char *type;
+	GError *error = NULL;
+
+	g_signal_handler_disconnect (typefind, md->priv->typefind_cb_id);
+	md->priv->typefind_cb_id = 0;
+
+	if (gst_caps_is_empty (caps) || gst_caps_is_any (caps)) {
+		rb_debug ("got empty/no caps from typefind");
+		g_set_error (&error,
+			     RB_METADATA_ERROR,
+			     RB_METADATA_ERROR_UNSUPPORTED,
+			     _("Unable to identify file type"));
+		goto out_error;
+	}
+
+	type = gst_structure_get_name (gst_caps_get_structure (caps, 0));
+
+	/* Tagger element(s) */
+	add_tagger_func = g_hash_table_lookup (md->priv->taggers, type);
+	if (!add_tagger_func) {
+		rb_debug ("found unsupported type %s", type);
+		g_set_error (&error,
+			     RB_METADATA_ERROR,
+			     RB_METADATA_ERROR_UNSUPPORTED,
+			     _("Unsupported file type: %s"), type);
+		goto out_error;
+	}
+
+	rb_debug ("adding tagger for type %s", type);
+	retag_end = add_tagger_func (md->priv->pipeline, typefind, md->priv->tags);
+	if (!retag_end) {
+		g_set_error (&error,
+			     RB_METADATA_ERROR,
+			     RB_METADATA_ERROR_UNSUPPORTED,
+			     _("Unable to create tag-writing elements"));
+		goto out_error;
+	}
+
+	gst_element_link_many (retag_end, md->priv->sink, NULL);
+	return;
+
+out_error:
+	message = gst_message_new_error (GST_OBJECT (typefind), error, NULL);
+	gst_element_post_message (typefind, message);
+	g_clear_error (&error);
+}
+
 void
-rb_metadata_save (RBMetaData *md, GError **error)
+rb_metadata_save (RBMetaData *md, const char *uri, GError **error)
 {
 	GstElement *pipeline = NULL;
 	GstElement *source = NULL;
-        GstElement *retag_end = NULL; /* the last element after retagging subpipeline */
+	GstElement *typefind = NULL;
 	const char *plugin_name = NULL;
 	char *tmpname_prefix = NULL;
 	char *tmpname = NULL;
 	GOutputStream *stream = NULL;
 	GError *io_error = NULL;
-	RBAddTaggerElem add_tagger_func;
-
-	g_return_if_fail (md->priv->uri != NULL);
-	g_return_if_fail (md->priv->type != NULL);
 
-	rb_debug ("saving metadata for uri: %s", md->priv->uri);
+	g_return_if_fail (uri != NULL);
+	rb_debug ("saving metadata for uri: %s", uri);
 
-	tmpname_prefix = rb_uri_make_hidden (md->priv->uri);
+	tmpname_prefix = rb_uri_make_hidden (uri);
 	rb_debug ("temporary file name prefix: %s", tmpname_prefix);
 
 	rb_uri_mkstemp (tmpname_prefix, &tmpname, &stream, &io_error);
@@ -961,46 +1018,39 @@ rb_metadata_save (RBMetaData *md, GError **error)
 	md->priv->pipeline = pipeline;
 
 	/* Source */
-	source = gst_element_make_from_uri (GST_URI_SRC, md->priv->uri, "urisrc");
+	source = gst_element_make_from_uri (GST_URI_SRC, uri, "urisrc");
 	if (source == NULL) {
 		plugin_name = "urisrc";
 		goto missing_plugin;	
 	}
 	gst_bin_add (GST_BIN (pipeline), source);
 
+	/* typefind */
+	plugin_name = "typefind";
+	typefind = gst_element_factory_make (plugin_name, NULL);
+	if (typefind == NULL)
+		goto missing_plugin;
+	gst_bin_add (GST_BIN (pipeline), typefind);
+	md->priv->typefind_cb_id = g_signal_connect_object (typefind,
+							    "have_type",
+							    G_CALLBACK (metadata_save_type_found),
+							    md,
+							    0);
+
 	/* Sink */
 	plugin_name = "giostreamsink";
-	if (!(md->priv->sink = gst_element_factory_make (plugin_name, plugin_name)))
+	if (!(md->priv->sink = gst_element_factory_make (plugin_name, NULL)))
 		goto missing_plugin;
 
-	g_object_set (G_OBJECT (md->priv->sink), "stream", stream, NULL);
+	g_object_set (md->priv->sink, "stream", stream, NULL);
 
 	md->priv->tags = gst_tag_list_new ();
 	g_hash_table_foreach (md->priv->metadata,
 			      (GHFunc) rb_metadata_gst_add_tag_data,
 			      md);
 
-	/* Tagger element(s) */
-	add_tagger_func = g_hash_table_lookup (md->priv->taggers, md->priv->type);
-	if (!add_tagger_func) {
-		g_set_error (error,
-			     RB_METADATA_ERROR,
-			     RB_METADATA_ERROR_UNSUPPORTED,
-			     _("Unsupported file type: %s"), md->priv->type);
-		goto out_error;
-	}
-
-	retag_end = add_tagger_func (md->priv->pipeline, source, md->priv->tags);
-	if (!retag_end) {
-		g_set_error (error,
-			     RB_METADATA_ERROR,
-			     RB_METADATA_ERROR_UNSUPPORTED,
-			     _("Unable to create tag-writing elements"));
-		goto out_error;
-	}
-
 	gst_bin_add (GST_BIN (pipeline), md->priv->sink);
-	gst_element_link_many (retag_end, md->priv->sink, NULL);
+	gst_element_link (source, typefind);
 
 	gst_element_set_state (pipeline, GST_STATE_PLAYING);
 
@@ -1030,7 +1080,7 @@ rb_metadata_save (RBMetaData *md, GError **error)
 		stream = NULL;
 
 		/* check to ensure the file isn't corrupt */
-		if (!rb_metadata_file_valid (md->priv->uri, tmpname)) {
+		if (!rb_metadata_file_valid (uri, tmpname)) {
 			g_set_error (error,
 				     RB_METADATA_ERROR,
 				     RB_METADATA_ERROR_INTERNAL,
@@ -1039,7 +1089,7 @@ rb_metadata_save (RBMetaData *md, GError **error)
 		}
 
 		src = g_file_new_for_uri (tmpname);
-		dest = g_file_new_for_uri (md->priv->uri);
+		dest = g_file_new_for_uri (uri);
 		g_file_move (src, dest, G_FILE_COPY_OVERWRITE, NULL, NULL, NULL, &io_error);
 		if (io_error != NULL) {
 			goto gio_error;
diff --git a/metadata/rb-metadata.h b/metadata/rb-metadata.h
index bce8858..214e6da 100644
--- a/metadata/rb-metadata.h
+++ b/metadata/rb-metadata.h
@@ -126,11 +126,14 @@ RBMetaData *	rb_metadata_new		(void);
 gboolean	rb_metadata_can_save	(RBMetaData *md, const char *mimetype);
 char **		rb_metadata_get_saveable_types (RBMetaData *md);
 
+void		rb_metadata_reset	(RBMetaData *md);
+
 void		rb_metadata_load	(RBMetaData *md,
 					 const char *uri,
 					 GError **error);
 
 void		rb_metadata_save	(RBMetaData *md,
+					 const char *uri,
 					 GError **error);
 
 const char *	rb_metadata_get_mime	(RBMetaData *md);
diff --git a/rhythmdb/rhythmdb.c b/rhythmdb/rhythmdb.c
index e0095f6..82dd5f5 100644
--- a/rhythmdb/rhythmdb.c
+++ b/rhythmdb/rhythmdb.c
@@ -4628,16 +4628,11 @@ default_sync_metadata (RhythmDB *db,
 	GError *local_error = NULL;
 
 	uri = rhythmdb_entry_get_string (entry, RHYTHMDB_PROP_LOCATION);
-	rb_metadata_load (db->priv->metadata,
-			  uri, &local_error);
-	if (local_error != NULL) {
-		g_propagate_error (error, local_error);
-		return;
-	}
+	rb_metadata_reset (db->priv->metadata);
 
 	entry_to_rb_metadata (db, entry, db->priv->metadata);
 
-	rb_metadata_save (db->priv->metadata, &local_error);
+	rb_metadata_save (db->priv->metadata, uri, &local_error);
 	if (local_error != NULL) {
 		RhythmDBAction *load_action;
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]