Created attachment 303453 [details] [review]
filesystem: copy xml to libs dir.

Created attachment 303454 [details] [review]
gstreamer plugin

+ Exposes a GstDiscovererInfo through a new key.
+ Persistent through gom.
+ Adds a test.

Review of attachment 303453 [details] [review]:

No "." at the end of the commit subject. Looks good otherwise.

Review of attachment 303454 [details] [review]:

Use dashes for list items in the log message.

The blocker right now is the fact that you don't populate any generic keys, see review for details.

::: configure.ac
@@ +129,3 @@
+PKG_CHECK_MODULES(GSTREAMER, gstreamer-1.0, HAVE_GSTREAMER=yes, HAVE_GSTREAMER=no)
+
+PKG_CHECK_MODULES(GST_PBUTILS, gstreamer-pbutils-1.0, HAVE_GST_PBUTILS="yes", HAVE_GST_PBUTILS="no")

I know the other plugins do it that way, but pkg-config will avoid double-linking, and make sure ordering is correct when checking for multiple .pc files at once.

I'd replace both those lines with:
PKG_CHECK_MODULES(GSTREAMER, gstreamer-1.0 gstreamer-pbutils-1.0, HAVE_GSTREAMER=yes, HAVE_GSTREAMER=no)

And make sure to check for the version number as well.

::: src/gstreamer/grl-gstreamer.c
@@ +109,3 @@
+  GrlConfig *config;
+  gint config_count;
+  gint timeout = 1;

Add a #define for DEFAULT_DISCOVERER_TIMEOUT, to avoid magic numbers here and in the property definition later.

@@ +386,3 @@
+  } else if (keys_need_discovering (rs->keys)) {
+    queue_uri_discovery (gstsource, rs);
+  } else {                      /* FIXME can that happen ? */

Probably not, you might want to add a "g_assert_not_reached()" if you're sure it cannot.

@@ +397,3 @@
+  static GList *keys = NULL;
+  if (!keys) {
+    keys = grl_metadata_key_list_new (GRL_GSTREAMER_METADATA_KEY_DISCOVERY,

This is really unusable for applications though...

This is probably good enough to have this plugin inside your application, but we cannot really have an application requesting "a blob of data" from a supposedly generic plugin.

This might be good enough if it at least populated the rest of the generic info GrlMediaVideo can hold for the "default" tracks, or something like that, *and* also allowed giving away a representation of the data as defined in <mention location>. But it cannot be the only key supported by the plugin.

@@ +500,3 @@
+  path = g_build_filename (g_get_user_data_dir (), "grilo-plugins", NULL);
+  if (!g_file_test (path, G_FILE_TEST_IS_DIR)) {
+    g_mkdir_with_parents (path, 0775);

Do a g_mkdir_with_parents() directly, it won't fail if the directory already exists.
Also use 0700 as the mode.

@@ +519,3 @@
+  object_types =
+      g_list_prepend (NULL, GINT_TO_POINTER (DISCOVERY_INFO_TYPE_RESOURCE));
+  gom_repository_automatic_migrate_async (source->priv->repository, 2,

Why version 2 already? Maybe we could start with version 1?

From bug 709910:
"
The plugin could gather things like:
- bitrate
- duration
- framerate
- width, height
- orientation
"

It should export this information, in addition to the more complete GstDiscovererInfo. If there are multiple tracks (audio or video), we should select the information for the default track (or the first one). It's not 100% correct, and doesn't exactly map all the information available, but it's pretty much what the Tracker plugin would gather given the same file, for example, and application that want exact information can use the GstDiscovererInfo key.

*** Bug 709910 has been marked as a duplicate of this bug. ***

Comment on attachment 303453 [details] [review]
filesystem: copy xml to libs dir.

Attachment 303453 [details] pushed as 65300fb - filesystem: copy xml to libs dir.

Any plans to update this?

Add to the "request" component.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/58.