Created attachment 253924 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

This plugin uses the dleyna-server service to interact with DMS (DLNA
Media Server, eg. Rygel) through an high level DBus API and it is meant
to deprecate the UPnP plugin.

It implements all the functionalities exposed by GrlSource: resolve(),
browse(), search(), query(), store(), store_metadata(), remove(),
cancel() and change notifications.

This adds a soft-dependency on the deyna-renderer DBus service: if the
service is not activatable no DLNA server sources will be shown.

The required GLib version has been bumped to 2.36 to use GTask and
the Grilo one to 0.2.7 for grl_operation_set_data_full().

Created attachment 253951 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

Re-reading the documentation it turned out that using GBinding in this case was
plainly wrong, as the properties to be set are construct-only.

Now it should hopefully work fine on GLib 2.36.x.

Hey Emanuelle,

you asked me on IRC for a quick review of the dbusmock bits:

tests/dleyna/dbusmock/dleyna-server-mock:

+# DbusMock does not seem to like to be torn down after each fixture

That works fine, dbusmock's test suite uses that all the time, and I also use it in application tests that use dbusmock. What does work poorly is to shut down/restart the test D-BUS daemon for each individual test, but that's not what this script does.

+# logfile to /dev/null as we don't use it
+os.system("python3 -m dbusmock -t dleynamanager.py --logfile /dev/null")

This would be easier and more efficiently written as a shell script, BTW. But in fact you don't need it at all really, you could just put this into dleyna-server-mock.service.in with something like

  Exec=python3 -m dbusmock -t @abs_top_srcdir@/tests/.../dleynamanager.py --logfile /dev/null


dleynamanager.py etc.: LGTM; elegant chaining of template loading, I haven't done/seen that before :)

Otherwise it seems that you don't run these tests on a private test bus, but on the actual user session bus? Doesn't that get in the way of a running dleyna service?

(In reply to comment #3)

> you asked me on IRC for a quick review of the dbusmock bits:

Awesome, thanks! :D
 
> tests/dleyna/dbusmock/dleyna-server-mock:
> 
> +# DbusMock does not seem to like to be torn down after each fixture
> 
> That works fine, dbusmock's test suite uses that all the time, and I also use
> it in application tests that use dbusmock. What does work poorly is to shut
> down/restart the test D-BUS daemon for each individual test, but that's not
> what this script does.

When not setting `--logfile /dev/null` the output fd gets closed by the first instance and later ones are going to fail with BrokenPipeError when trying to write to it.

> +# logfile to /dev/null as we don't use it
> +os.system("python3 -m dbusmock -t dleynamanager.py --logfile /dev/null")
> 
> This would be easier and more efficiently written as a shell script, BTW. But
> in fact you don't need it at all really, you could just put this into
> dleyna-server-mock.service.in with something like
> 
>   Exec=python3 -m dbusmock -t @abs_top_srcdir@/tests/.../dleynamanager.py
> --logfile /dev/null

I'm also changing the current directory to the one containing the templates, to be able to load them easily.

Since it was all python I wrote it in python, for no other particular reason, but I'd be happy to have a one-liner to include directly in the .service file.

Maybe I should add a command line flag to dbusmock to change the current directory? Or it should simply chdir() to the directory of the template, if one is specified?

> dleynamanager.py etc.: LGTM; elegant chaining of template loading, I haven't
> done/seen that before :)

Heh, it looked like a nice way to abuse dbusmock. :)

> Otherwise it seems that you don't run these tests on a private test bus, but on
> the actual user session bus? Doesn't that get in the way of a running dleyna
> service?

Nope, in test_dleyna_utils.c:test_dleyna_setup() I'm using GTestDBus to run a private session bus, so it should not interfere with existing services (hopefully).

Hope that this helps clearing some of your doubts, and thanks for the review!

I am trying this plugin and so far I have found one problem: There is no name or description on the source.

After debugging it a bit, it seems that this is being caused by a race inside GObject:

source = g_object_new (GRL_DLEYNA_SOURCE_TYPE, "server", server, NULL);

g_object_new sets the "server" property here, which in turn sets the "source-name" and "source-description", but those properties are marked as CONSTRUCT in grilo and the GObject constructor overrides them with empty strings right afterwards.

Created attachment 255794 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

Rebased on top of current master and fix the issue with the contruct-only
property initialization order. Thanks George for spotting the cause of
your error!

Another bug found. grl_operation_options_get_count() returns -1 if the count is not set, however the MediaContainer2.ListChildren method expects 0 for "unlimited" and on -1 it returns an error. Probably the intended behavior is to get unlimited results, so here is a quick patch that fixes the problem:

--- a/src/dleyna/grl-dleyna-source.c
+++ b/src/dleyna/grl-dleyna-source.c
@@ -1056,6 +1056,8 @@ grl_dleyna_source_browse (GrlSource *source,
   filter = build_properties_filter (bs->keys);
   offset = grl_operation_options_get_skip (bs->options);
   limit = grl_operation_options_get_count (bs->options);
+  if (limit == -1)
+    limit = 0;

   object_path = grl_dleyna_source_media_get_object_path (bs->container);
   if (object_path == NULL)

Created attachment 256114 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

Fixed the bug spotted by George about the "unlimited" representation mismatch
between Grilo (which uses a signed integer) and dLeyna (which uses an
unsigned integer).

I've also fixed all the other occurrences of the bug and also fixed the testcase.

Thanks again George!

And another minor problem: grl_source_get_caps() documentation says:

* Returns: (transfer none): The capabilities

and indeed, with other plugins if you unref the caps you get criticals/crashes at some point, but grl-dleyna always returns a new caps instance and transfers ownership to the caller.

Created attachment 256398 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

I've done what all the other plugins do an save the caps in a static local variable.
I think this is still a memory leak as the caps never get unref'ed, but definitely less serious.

George, thanks again!

Created attachment 256401 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

Now that a python-dbusmock package is available for python2, drop the python3
machinery and make the dbusmock'ed test run on whatever python version is available.

This makes `make check` under JHBuild happy again without fiddling with $PYTHONPATH.

Review of attachment 256401 [details] [review]:

::: src/dleyna/Makefile.am
@@ +54,3 @@
+
+grl-dleyna-proxy-mediaserver2.h grl-dleyna-proxy-mediaserver2.c: org.gnome.UPnP.MediaServer2.xml
+	$(AM_V_GEN)gdbus-codegen \

Instead of hardcoding gdbus-codegen, use pkg-config in configure.ac to assign in a variable, and then use that variable instead here.

::: src/dleyna/grl-dleyna-server.c
@@ +88,3 @@
+                                GParamSpec *pspec)
+{
+    G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);

Any reason to not read properties? Specially when you later declare them as readable.

::: src/dleyna/grl-dleyna-servers-manager.c
@@ +102,3 @@
+                                 "com.intel.dleyna-server",
+                                 object_path,
+                                 NULL, /* GCancellable */

Put comments above, not at the side.

@@ +250,3 @@
+
+gboolean
+grl_dleyna_servers_manager_is_available (void)

This function is not used at all.

::: src/dleyna/grl-dleyna-source.c
@@ +85,3 @@
+
+G_DEFINE_TYPE (GrlDleynaSource, grl_dleyna_source, GRL_TYPE_SOURCE)
+

Would you mind to reorder the functions and group them as we do in other plugins? Like first all gobject related functions, then the private utility functions, and at the end all the Grilo functions (browse, search, and so on). This would help to find where is the code.

@@ +109,3 @@
+  const gchar *id;
+
+  if (media == NULL)

Minor style question. Try to use always brackets, even for one single line code

@@ +184,3 @@
+
+  if (g_strcmp0 (key, "Path") == 0)
+    {

Minor style issue. Put bracket after the condition. Check other plugins.

@@ +494,3 @@
+
+  if (ss == NULL)
+    return;

Shouldn't call the callback()?

@@ +703,3 @@
+      case GRL_METADATA_KEY_BITRATE:
+      case GRL_METADATA_KEY_PUBLICATION_DATE:
+        return TRUE;

In order to make things easier to maintain, this function should rely on grl_dleyna_source_supported_keys() instead of listing here the supported keys. You could use g_list_find() to see if the key is supported or not.

@@ +775,3 @@
+    {
+      GRL_DEBUG ("%s error:%s", G_STRFUNC, error->message);
+      callback (source, operation_id, NULL, 0, user_data, error);

It seems for me that you are not using the errors defined by Grilo. For instance, if browse fail we need to send a GRL_CORE_ERROR_BROWSE_FAILED. Ensure that you are sending the proper errors.

@@ +878,3 @@
+   * exit early if the media has no id or is from a different source */
+  media_id = grl_media_get_id (rs->media);
+  if (media_id == NULL || !g_str_has_prefix (media_id, "dleyna:"))

So, for the root container the only key we can use is the ID, right?

@@ +1190,3 @@
+static void
+grl_dleyna_source_store (GrlSource *source,
+                         GrlSourceStoreSpec *ss)

Like in the notify, what happens if the server doesn't support storing? Shouldn't it be detected and update supported_operations() accordingly?

@@ +1233,3 @@
+  if (container_object_path == NULL)
+    {
+      gchar const * const child_types[] = { "*", NULL };

As child_types is defined in here and also in the else, let's move it to before the conditional.

@@ +1516,3 @@
+  device = grl_dleyna_server_get_media_device (self->priv->server);
+  g_signal_connect_object (device, "changed", G_CALLBACK (grl_dleyna_source_changed_cb),
+                           self, G_CONNECT_SWAPPED);

What happens if the DLNA server doesn't support notifications change?

If dleyna can be used to know if a server is able to notify changes, wouldn't it make sense to add the notification in the list of supported operations if server really supports it?

@@ +1553,3 @@
+    {
+      if (GRL_DLEYNA_SOURCE (source)->priv->search_enabled)
+        grl_caps_set_type_filter (caps, GRL_TYPE_FILTER_ALL);

I think you should use different caps for each operation. Else, if your server has browse_filtered_enabled, but not search_enabled, and the developer gets the caps first for the browse operation, it turns out that later if they ask for caps for search operation it will return the same caps than browse, with the filter by type active.

@@ +1586,3 @@
+  g_object_class_install_property (gobject_class,
+                                   PROP_SERVER,
+                                   g_param_spec_object ("server",

Why this property? I haven't seen it used in any place

::: src/dleyna/grl-dleyna.c
@@ +98,3 @@
+      if (error != NULL)
+        {
+          g_warning ("Failed to unregister source %s: %s", udn, error->message);

Let's use GRL_WARNING instead.

@@ +119,3 @@
+  bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8");
+
+  servers = grl_dleyna_servers_manager_dup_singleton ();

I think it would be better if you check if everything goes fine, and if not, return FALSE to tell the plugin failed to initialize.

Actually, I think you have a function not used at all (manager is available) that could be used here.

Created attachment 257264 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

(In reply to comment #12)
> Review of attachment 256401 [details] [review]:
> 
> ::: src/dleyna/Makefile.am
> @@ +54,3 @@
> +
> +grl-dleyna-proxy-mediaserver2.h grl-dleyna-proxy-mediaserver2.c:
> org.gnome.UPnP.MediaServer2.xml
> +    $(AM_V_GEN)gdbus-codegen \
> 
> Instead of hardcoding gdbus-codegen, use pkg-config in configure.ac to assign
> in a variable, and then use that variable instead here.

Done.

> ::: src/dleyna/grl-dleyna-server.c
> @@ +88,3 @@
> +                                GParamSpec *pspec)
> +{
> +    G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
> 
> Any reason to not read properties? Specially when you later declare them as
> readable.

Ehrm, no, fixed.

> ::: src/dleyna/grl-dleyna-servers-manager.c
> @@ +102,3 @@
> +                                 "com.intel.dleyna-server",
> +                                 object_path,
> +                                 NULL, /* GCancellable */
> 
> Put comments above, not at the side.

I've dropped the comment and reformatted the block.

> @@ +250,3 @@
> +
> +gboolean
> +grl_dleyna_servers_manager_is_available (void)
> 
> This function is not used at all.

Reused in grl_dleyna_plugin_init(), even if it doesn't add really much
 (see below).

> ::: src/dleyna/grl-dleyna-source.c
> @@ +85,3 @@
> +
> +G_DEFINE_TYPE (GrlDleynaSource, grl_dleyna_source, GRL_TYPE_SOURCE)
> +
> 
> Would you mind to reorder the functions and group them as we do in other
> plugins? Like first all gobject related functions, then the private utility
> functions, and at the end all the Grilo functions (browse, search, and so on).
> This would help to find where is the code.

Sure. I'll do that it after having applied all the suggested changes, to
somewhat preserve references.

> @@ +109,3 @@
> +  const gchar *id;
> +
> +  if (media == NULL)
> 
> Minor style question. Try to use always brackets, even for one single line code

Done.

> @@ +184,3 @@
> +
> +  if (g_strcmp0 (key, "Path") == 0)
> +    {
> 
> Minor style issue. Put bracket after the condition. Check other plugins.

Done.

> @@ +494,3 @@
> +
> +  if (ss == NULL)
> +    return;
> 
> Shouldn't call the callback()?

Nope, if `ss` is NULL we don't have a valid pointer to the callback. But that's
fine, since it means that we simply got an upload status notification for an
upload that was not started by us and that we can safely ignore. :)

Added a comment.
 
> @@ +703,3 @@
> +      case GRL_METADATA_KEY_BITRATE:
> +      case GRL_METADATA_KEY_PUBLICATION_DATE:
> +        return TRUE;
> 
> In order to make things easier to maintain, this function should rely on
> grl_dleyna_source_supported_keys() instead of listing here the supported keys.
> You could use g_list_find() to see if the key is supported or not.

Done.

> @@ +775,3 @@
> +    {
> +      GRL_DEBUG ("%s error:%s", G_STRFUNC, error->message);
> +      callback (source, operation_id, NULL, 0, user_data, error);
> 
> It seems for me that you are not using the errors defined by Grilo. For
> instance, if browse fail we need to send a GRL_CORE_ERROR_BROWSE_FAILED. Ensure
> that you are sending the proper errors.

Mh, does this means that I should drop the original error and instantiate a new
one instead of propagating it?

> @@ +878,3 @@
> +   * exit early if the media has no id or is from a different source */
> +  media_id = grl_media_get_id (rs->media);
> +  if (media_id == NULL || !g_str_has_prefix (media_id, "dleyna:"))
> 
> So, for the root container the only key we can use is the ID, right?

Sorry, I'm not sure what you mean.

> @@ +1190,3 @@
> +static void
> +grl_dleyna_source_store (GrlSource *source,
> +                         GrlSourceStoreSpec *ss)
> 
> Like in the notify, what happens if the server doesn't support storing?
> Shouldn't it be detected and update supported_operations() accordingly?

I've asked the dLeyna developers and unfortunately it seem that there's no
clear mechanism to detect beforehand if a server handles uploads or not. :(

> @@ +1233,3 @@
> +  if (container_object_path == NULL)
> +    {
> +      gchar const * const child_types[] = { "*", NULL };
> 
> As child_types is defined in here and also in the else, let's move it to before
> the conditional.

Right.

> @@ +1516,3 @@
> +  device = grl_dleyna_server_get_media_device (self->priv->server);
> +  g_signal_connect_object (device, "changed", G_CALLBACK
> (grl_dleyna_source_changed_cb),
> +                           self, G_CONNECT_SWAPPED);
> 
> What happens if the DLNA server doesn't support notifications change?
> 
> If dleyna can be used to know if a server is able to notify changes, wouldn't
> it make sense to add the notification in the list of supported operations if
> server really supports it?

Unfortunately again, it does not seems to exist any mechanism to be able to
detect if change notifications are properly supported or not. :/

> @@ +1553,3 @@
> +    {
> +      if (GRL_DLEYNA_SOURCE (source)->priv->search_enabled)
> +        grl_caps_set_type_filter (caps, GRL_TYPE_FILTER_ALL);
> 
> I think you should use different caps for each operation. Else, if your server
> has browse_filtered_enabled, but not search_enabled, and the developer gets the
> caps first for the browse operation, it turns out that later if they ask for
> caps for search operation it will return the same caps than browse, with the
> filter by type active.

Ouch! I was doing exacly that, but as George pointed out in comment #9, it
means I would leak all the instantiated caps objects. :/

I've resorted to using two static variables, one for the browse case, and one
for all the other ones.
 
> @@ +1586,3 @@
> +  g_object_class_install_property (gobject_class,
> +                                   PROP_SERVER,
> +                                   g_param_spec_object ("server",
> 
> Why this property? I haven't seen it used in any place

It is a construct-only property used in grl_dleyna_source_new():
  source = g_object_new (GRL_DLEYNA_SOURCE_TYPE, "server", server, "source-id", id,                                     
                         "source-name", friendly_name, "source-desc", desc, NULL);

> ::: src/dleyna/grl-dleyna.c
> @@ +98,3 @@
> +      if (error != NULL)
> +        {
> +          g_warning ("Failed to unregister source %s: %s", udn,
> error->message);
> 
> Let's use GRL_WARNING instead.

Done, and I've took the chance to revise my usage of GRL_DEBUG too, which
required some hackery in the test program as G_LOG_LEVEL_WARNING is printable
by default and also considered fatal by GTester.

> @@ +119,3 @@
> +  bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8");
> +
> +  servers = grl_dleyna_servers_manager_dup_singleton ();
> 
> I think it would be better if you check if everything goes fine, and if not,
> return FALSE to tell the plugin failed to initialize.
> 
> Actually, I think you have a function not used at all (manager is available)
> that could be used here.

I've amended the code to return the result of
grl_dleyna_servers_manager_is_available() but it won't be immensely useful
since most of the errors will be detected only when the underlying async
DBus calls will complete, and since grl_dleyna_plugin_init() is called only
once there's no chance it will get called when this already happened.

Created attachment 257280 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

Reordered and grouped functions as suggested: GObject-related first, then
private stuff and Grilo API implementations last.

(In reply to comment #13)
> > It seems for me that you are not using the errors defined by Grilo. For
> > instance, if browse fail we need to send a GRL_CORE_ERROR_BROWSE_FAILED. Ensure
> > that you are sending the proper errors.
> 
> Mh, does this means that I should drop the original error and instantiate a new
> one instead of propagating it?

Yes. You can though re-use the message, but the domain and code must be specific for Grilo.

> 
> > @@ +878,3 @@
> > +   * exit early if the media has no id or is from a different source */
> > +  media_id = grl_media_get_id (rs->media);
> > +  if (media_id == NULL || !g_str_has_prefix (media_id, "dleyna:"))
> > 
> > So, for the root container the only key we can use is the ID, right?
> 
> Sorry, I'm not sure what you mean.
> 

I meant, what if a invokes resolve() for root container asking for keys like title? root container has the id as NULL, and I see that under that case you invoke the callback without changing the media at all. So it could mean two cases:

a) It's wrong, and you aren't solving the keys asked by user
b) In the case of root container, only ID is the available key, so any other key requested by user can't be solve, and that's why we send the element untouched.


> > Like in the notify, what happens if the server doesn't support storing?
> > Shouldn't it be detected and update supported_operations() accordingly?
> 
> I've asked the dLeyna developers and unfortunately it seem that there's no
> clear mechanism to detect beforehand if a server handles uploads or not. :(
> 

Hmm.

That is very unfortunate. Because if we implement upload() the user expects upload works. Dunno how to deal with it. 

The least we can do is returning GRL_CORE_ERROR_STORE_FAILED when it fails, telling that the Container where we want to save the element is like read-only.

In UPnP there's something called FeatureList, which should return all the features supported by the server. I'm not a UPnP expert, so not sure if that list could be used to know if sources support or not uploading content.

Because there're lso a upnp:createClass, upnp:createClass@name, and upnp:createClass@includeDerived that are properties of the ContentDirectory service, but don't know neither if those can be used to detect or not.


> > What happens if the DLNA server doesn't support notifications change?
> > 
> > If dleyna can be used to know if a server is able to notify changes, wouldn't
> > it make sense to add the notification in the list of supported operations if
> > server really supports it?
> 
> Unfortunately again, it does not seems to exist any mechanism to be able to
> detect if change notifications are properly supported or not. :/
> 

Again, very bad from UPnP designers :(

> > Why this property? I haven't seen it used in any place
> 
> It is a construct-only property used in grl_dleyna_source_new():
>   source = g_object_new (GRL_DLEYNA_SOURCE_TYPE, "server", server, "source-id",
> id,                                     
>                          "source-name", friendly_name, "source-desc", desc,
> NULL);

Yes, but it is used or required for something?
> > I think it would be better if you check if everything goes fine, and if not,
> > return FALSE to tell the plugin failed to initialize.
> > 
> > Actually, I think you have a function not used at all (manager is available)
> > that could be used here.
> 
> I've amended the code to return the result of
> grl_dleyna_servers_manager_is_available() but it won't be immensely useful
> since most of the errors will be detected only when the underlying async
> DBus calls will complete, and since grl_dleyna_plugin_init() is called only
> once there's no chance it will get called when this already happened.

Yeah, but at least we can detect if we have dleyna running, right?

Review of attachment 257280 [details] [review]:

I understand that this patch doesn't address yet the latest comments. Other than that looks good.

(In reply to comment #0)
> Note that the BrowseObjects() and Changed API in dleyna-server are not
> finalized yet, but I'm posting the patch for a first round of reviews.

They’ve been finalised and released in dleyna-server 0.4.0, so we need to make sure that the plugin gracefully handles the BrowseObjects and Changed D-Bus methods/signals not existing.

(In reply to comment #15)

> > Mh, does this means that I should drop the original error and instantiate a new
> > one instead of propagating it?
> 
> Yes. You can though re-use the message, but the domain and code must be
> specific for Grilo.

Fixed. Patch will follow.

> > > @@ +878,3 @@
> > > +   * exit early if the media has no id or is from a different source */
> > > +  media_id = grl_media_get_id (rs->media);
> > > +  if (media_id == NULL || !g_str_has_prefix (media_id, "dleyna:"))
> > > 
> > > So, for the root container the only key we can use is the ID, right?
> > 
> > Sorry, I'm not sure what you mean.
> > 
> 
> I meant, what if a invokes resolve() for root container asking for keys like
> title? root container has the id as NULL, and I see that under that case you
> invoke the callback without changing the media at all. So it could mean two
> cases:
> 
> a) It's wrong, and you aren't solving the keys asked by user
> b) In the case of root container, only ID is the available key, so any other
> key requested by user can't be solve, and that's why we send the element
> untouched.

Oh, I wasn't aware of the fact that even for resolve if the id is missing the root container is assumed.
I fixed it by always resolving the id even if not in the list of requested keys. Hopefully this is not a problem.

> > > Like in the notify, what happens if the server doesn't support storing?
> > > Shouldn't it be detected and update supported_operations() accordingly?
> > 
> > I've asked the dLeyna developers and unfortunately it seem that there's no
> > clear mechanism to detect beforehand if a server handles uploads or not. :(
> 
> Hmm.
> 
> That is very unfortunate. Because if we implement upload() the user expects
> upload works. Dunno how to deal with it. 

Me neither. I have also been told that upload may only be allowed for certain folders while other may disallow it, a finer mechanism would be needed for this to work properly.

> The least we can do is returning GRL_CORE_ERROR_STORE_FAILED when it fails,
> telling that the Container where we want to save the element is like read-only.

Fixed.

> In UPnP there's something called FeatureList, which should return all the
> features supported by the server. I'm not a UPnP expert, so not sure if that
> list could be used to know if sources support or not uploading content.
>
> Because there're lso a upnp:createClass, upnp:createClass@name, and
> upnp:createClass@includeDerived that are properties of the ContentDirectory
> service, but don't know neither if those can be used to detect or not.

I'm far from being an expert of UPnP/DLNA stuff either (and that's why I found this plugin so interesting :), here's an excerpt of a conversation with Mark Ryan, main architect of dLeyna:

<mryan> AFAIK, I know there is no way to tell whether a UPnP server supports upload or not.
<mryan> Checking for the existence of CreateObject is not enough.
<mryan> For DLNA, there is a capability that indicates whether upload to the any container is supported.
<mryan> But I guess a DLNA DMS could support upload but not upload to the any container.
<mryan> in DLNA, you can however check to see if a given container supports upload
<mryan> via the dlnaManaged flag

My understanding is that the "image-upload", "av-upload" and "audio-upload" caps are not exported if the server does not support uploading to "AnyContainer" (ie. the server decides where to store the uploaded content) even if uploading to specific folders.

A server could also specify custom methods to be called for uploading specific mimetypes (feature which is not yest supported by dLeyna).

<mryan> So to summarise, there is no way to definitively say that a DMS supports upload or not.
<mryan> Unless it is a DLNA server that supports the AnyContainer or content-synchronisation.
<mryan> Then you know.
<mryan> The best heuristic that would work with both DLNA and UPnP servers would be to check for the existence of CreateObject
<mryan> Just because CreateObject exists doesn't mean the servers supports Upload, but it's a good bet.
<mryan> The problem is that dLeyna-server doesn't currently perform any introspection.
<mryan> So dLeyna clients have no way of knowing whether the server supports CreateObject or not.

> > > What happens if the DLNA server doesn't support notifications change?
> > > 
> > > If dleyna can be used to know if a server is able to notify changes, wouldn't
> > > it make sense to add the notification in the list of supported operations if
> > > server really supports it?
> > 
> > Unfortunately again, it does not seems to exist any mechanism to be able to
> > detect if change notifications are properly supported or not. :/
> 
> Again, very bad from UPnP designers :(

Honestly, UPnP has never striked me as an example of good design. Unfortunately it's the only thing a lot of devices support and the main competitor is even less specified (DAAP). Sigh.

> > > Why this property? I haven't seen it used in any place
> > 
> > It is a construct-only property used in grl_dleyna_source_new():
> >   source = g_object_new (GRL_DLEYNA_SOURCE_TYPE, "server", server, "source-id", id,                                     
> >                          "source-name", friendly_name, "source-desc", desc, NULL);
> 
> Yes, but it is used or required for something?

The self->priv->server pointer points to the DLNA server represented by the source .

> > > I think it would be better if you check if everything goes fine, and if not,
> > > return FALSE to tell the plugin failed to initialize.
> > > 
> > > Actually, I think you have a function not used at all (manager is available)
> > > that could be used here.
> > 
> > I've amended the code to return the result of
> > grl_dleyna_servers_manager_is_available() but it won't be immensely useful
> > since most of the errors will be detected only when the underlying async
> > DBus calls will complete, and since grl_dleyna_plugin_init() is called only
> > once there's no chance it will get called when this already happened.
> 
> Yeah, but at least we can detect if we have dleyna running, right?

How we can detect it synchronously? Isn't every check is going to be async, thus not improving anything?

(In reply to comment #17)

> > Note that the BrowseObjects() and Changed API in dleyna-server are not
> > finalized yet, but I'm posting the patch for a first round of reviews.
> 
> They’ve been finalised and released in dleyna-server 0.4.0, so we need to make
> sure that the plugin gracefully handles the BrowseObjects and Changed D-Bus
> methods/signals not existing.

No special care has been currently put, other than handling the DBus errors that using _resolve() and _notify_change_start() on older dleyna-servers will produce.

Both could be reimplemented in a less efficient way using other methods if there's interest, but at this point I'd honestly say the extra code is not worth the effort.

Created attachment 259454 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

Address the issues found in comment #15.

(In reply to comment #18)
> Oh, I wasn't aware of the fact that even for resolve if the id is missing the
> root container is assumed.

Yeah, the container with NULL ID is the "virtual" root container. Because there must be a well-known container where user can start to browse, which is this one (like the '/' in a harddisk).

You could use it in the resolve() if you want to know , for instance, how many elements are in the root (if you support childcount).

Review of attachment 259454 [details] [review]:

There's a repeating leak pattern across the code. Easy to fix though :)

How is the state of the work? Besides fixing the comments, do you think it is ready for upstreaming? or do you plan to do more work/fixes?

::: src/dleyna/grl-dleyna-source.c
@@ +334,3 @@
+  if (error != NULL) {
+    GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+    error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_STORE_FAILED);

I don't think it is a good idea to reuse the error variable. I think you are leaking the error passed as parameter

@@ +705,3 @@
+  if (error != NULL) {
+    GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+    error = grl_dleyna_source_convert_error (error, code);

Same  here, not a good idea to reuse the same error variable (leak in potential)

@@ +852,3 @@
+  if (error != NULL) {
+    GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+    error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_RESOLVE_FAILED);

You are leaking here the error

@@ +952,3 @@
+  if (error != NULL) {
+    GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+    error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_STORE_FAILED);

As said, leaking error here

@@ +1049,3 @@
+  if (error != NULL) {
+    GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+    error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_STORE_FAILED);

Leak

@@ +1078,3 @@
+  if (error != NULL) {
+    GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+    error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_REMOVE_FAILED);

Leak :)

@@ +1298,3 @@
+   * exit early if the media is from a different source */
+  media_id = grl_media_get_id (rs->media);
+  if (media_id != NULL && !g_str_has_prefix (media_id, "dleyna:")) {

I understand that you check the prefix to discard content coming from other sources.

Well, you don't need to do it: as you don't re-implement may_resolve(), the default one already does this check, so the media passed to resolve() is only media coming from this source.

@@ +1495,3 @@
+    if (error != NULL) {
+      GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+      error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_STORE_FAILED);

Leaking error

@@ +1531,3 @@
+    if (error != NULL) {
+      GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+      error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_STORE_FAILED);

Leaking error

@@ +1579,3 @@
+  if (error != NULL) {
+    GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+    error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_STORE_METADATA_FAILED);

leak

@@ +1620,3 @@
+  if (error != NULL) {
+    GRL_WARNING ("%s error:%s", G_STRFUNC, error->message);
+    error = grl_dleyna_source_convert_error (error, GRL_CORE_ERROR_REMOVE_FAILED);

Leak

(In reply to comment #22)
> Review of attachment 259454 [details] [review]:
> 
> There's a repeating leak pattern across the code. Easy to fix though :)

Maybe it's a bit too confusing, but grl_dleyna_source_convert_error() creates a new error reusing the message from the original one and then proceed to unref the original error, so there shouldn't be any leak (at least here :)

> How is the state of the work? Besides fixing the comments, do you think it is
> ready for upstreaming? or do you plan to do more work/fixes?

Sorry for the looong delay. I just added support for the "source-icon" property introduced by Bastien in https://git.gnome.org/browse/grilo/commit/?id=c8423dac9 and I think that, pending your approval, it should be ready to be pushed to master and replace the UPnP plugin.

> @@ +1298,3 @@
> +   * exit early if the media is from a different source */
> +  media_id = grl_media_get_id (rs->media);
> +  if (media_id != NULL && !g_str_has_prefix (media_id, "dleyna:")) {
> 
> I understand that you check the prefix to discard content coming from other
> sources.
> 
> Well, you don't need to do it: as you don't re-implement may_resolve(), the
> default one already does this check, so the media passed to resolve() is only
> media coming from this source.

In a previous version of the patch I was probably overriding may_resolve() and that's why I remember observing such a unexpected behaviour. That said, I dropped the check and indeed the plugin seem to work fine. :)

Updated patch to follow shortly. Thanks!

Created attachment 267264 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

Drop redundant check, add support for "source-icon" property.

I get those errors when browsing a local media with grilo-test-ui-0.2:
(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key tracker-urn

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key rating

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key last-played-time

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key lyrics

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key creation-date

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key keyword

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key framerate

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key season

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key bookmark-date

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key original-title

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key orientation

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key certificate

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key tmdb-id

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key flash-used

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key performer

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key artist-avatar

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key director

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key exposure-time

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key playback-interrupted-time

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key tmdb-imdb-id

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key tmdb-backdrop

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key description

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key favourite

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key site

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key studio

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key license

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key external-player

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key tracker-category

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key iso-speed

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key region

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key play-count

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key source

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key producer

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key camera-model

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key start-time

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key tmdb-poster

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key episode

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key show

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key thumbnail-binary

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key external-url

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key modification-date

(grilo-test-ui-0.2:31902): Grilo-WARNING **: [dleyna] grl-dleyna-source.c:463: properties_add_for_key ignored non-supported key author-avatar

Bastien, I literally had a mid-air collision with your message while submitting this comment. :)

Testing with grilo-test-ui-0.2 results in a lot of warnings because it requests all known keys and not only the supported one, and the dleyna source prints a warning when a unsupported key is requested.

Juan, should I drop the warning and silently ignore unsupported keys or fix grilo-test-ui-0.2 to only request the keys returned by grl_source_supported_keys()? Bastien, what Totem does in this regard?

Btw, I just submitted bug #723077 with the patch for grilo-test-ui-0.2 that I used to test my handling of the GrlSource::source-icon property.

Also:
diff --git a/src/local-metadata/grl-local-metadata.c b/src/local-metadata/grl-local-metadata.c
index e300433..a1eaa58 100644
--- a/src/local-metadata/grl-local-metadata.c
+++ b/src/local-metadata/grl-local-metadata.c
@@ -842,6 +842,8 @@ has_compatible_media_url (GrlMedia *media)
 
     if (g_str_has_prefix (source, "grl-upnp-uuid:"))
       return FALSE;
+    if (g_str_has_prefix (source, "grl-dleyna-uuid:"))
+      return FALSE;
   }
 
   url = grl_media_get_url (media);

Created attachment 267386 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

Skip dleyna media items in the local-metadata source as reported by Bastien.

Created attachment 272186 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

A fly-by attempt to rebase it on current master. Just wanted to read the code in a text editor. Hope I got it right.

Created attachment 272684 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

rebased the above written patch on current master

Seems one of the tests are broken, specifically the one about notification changes.

More specifically, in build_media_from_variant() function it is searching for "Type", when the current variant doesn't contain it (it is "{'Path': <objectpath '/com/intel/dLeynaServer/server/0/33'>, 'ChangeType': <uint32 3>}").

Created attachment 275317 [details] [review]
dleyna: Plugin to access DLNA contents using the dLeyna DBus service

In the change notification test the `/com/intel/dLeynaServer/server/0/33`
object is deleted, so the fact that no `Type` information is set has been done
on purpose as the DMS is not required to send it.

The error was that I was expecting `g_variant_lookup()` to set the out
parameter to NULL if the field was not present, which is not the case. The
`type` string was left to uninitialized garbage, which in my tests simply went
through all the `g_str_has_prefix()` checks falling back to the default case,
hiding the bug.

I explicitly initialized the `type` variable to `NULL` and explicitly handled
the `NULL` case before all the `g_str_has_prefix()` checks.

Attachment 275317 [details] pushed as 4990fa3 - dleyna: Plugin to access DLNA contents using the dLeyna DBus service