GNOME Bugzilla – Bug 652516
[PATCH] Grilo DMAP plugin
Last modified: 2012-09-12 17:10:29 UTC
Created attachment 189866 [details] DMAP plugin for Grilo I am working on a DMAP plugin for Grilo. This should allow interaction with Apple's DMAP family of protocols. Please see also: http://mail.gnome.org/archives/grilo-list/2011-May/msg00057.html This patch implements enough of the plugin so that I am able to browse a dmapd share (http://www.flyn.org/projects/dmapd/index.html) and play a file using the Grilo-enabled totem (bug #628648).
Created attachment 190244 [details] [review] Additive patch; adds search support and related work
Created attachment 194630 [details] [review] DMAP plugin from grilo
Does this patch include support for password-protected shares?
(In reply to comment #3) > Does this patch include support for password-protected shares? Not yet. Of course, libdmapsharing supports this. I have not yet investigated the Grilo plugin API for password support.
Review of attachment 194630 [details] [review]: First of all, thank you for this contribution, and sorry for taking so long to review it. I've left some comments in the code. But I would like to do a couple of more general comments too: - Avoid using Vala. It isn't that I'm against it, but I think it is too much overkill to use here. Doing it would include a new dependency, Vala, and other Vala-dependencies that are well-written in the Makefile.am: gstreamer and libsoup. And even libgee is also needed, and all of them to implement the DB and the DAAP record. I think we can go with a simpler approach using C. I've notice that, for instance, you already have a GHashtable-based DB and a record in the dmapd demon. Can we borrow the code and put here? - Showing the shares. I've notice a global "DMAP" source is shown, and when browsing through it the first things it shows are the found shares. I would like to propose a different approach, following what UPnP is doing: create a grilo source per each found shared. That is, when the dmap plugin is initialized, do not create any source. Instead, everytime a new share ("service-added") is found, create a new source that will handle the content in that share, and register it into plugin-registry. When the service goes away, just remove the service. All data needed to handle the service (host, port, name, ....) can be stored in the private structure of that source, not needing to do that encoding you do into the URL field. ::: configure.ac @@ -314,0 +317,13 @@ +# BUILD DMAP PLUGIN +# ---------------------------------------------------------- + ... 10 more ... if test "x$HAVE_XML" = "xno"; then ::: src/media/dmap/Makefile.am @@ +27,3 @@ +libgrldmap_la_LIBADD += \ + $(XML_LIBS) + the later 4 lines are not needed: they are already well-defined above @@ +40,3 @@ + +libdir = $(GRL_PLUGINS_DIR) +dmapxmldir = $(GRL_PLUGINS_CONF_DIR) In the latest version, XML files are installed in the same place as the libraries. So it should be now: dmapxmldir = $(GRL_PLUGINS_DIR) ::: src/media/dmap/grl-dmap.c @@ +238,3 @@ + NULL); + g_assert (title); + g_assert (url); I think you should be very careful about the assertions. In this case, I have a song that doesn't have a title, and the g_assert() makes the whole application crash.
I would also add that seems main distros are not including Vala bindings for libdmapsharing. So using Vala would make things a bit complicated to package. I've done the check using jhbuild, and even so, i have several issues to make things working.
*** Bug 651305 has been marked as a duplicate of this bug. ***
W. Michael, were you able to rework on patches based on the comments ?
I have not recently had the time, but would like to do this work eventually. I would not object if someone else took this work to completion, but more likely it will have to wait.
Created attachment 216077 [details] [review] DMAP plugin for grilo The latest patch replaces the Vala code with C and implements the share list technique described by Juan.
Review of attachment 216077 [details] [review]: You are handling when a new service is added ("service-added" signal), but not when it is removed ("service-removed" signal). ::: configure.ac @@ +118,3 @@ +PKG_CHECK_MODULES(DMAP, libdmapsharing-3.0 >= 2.9.12 gee-1.0, HAVE_DMAP=yes, HAVE_DMAP=no) + Split libdmapsharing and gee check (see other sources). This helps to notify user why dmap plugin does not build. ::: src/media/dmap/Makefile.am @@ +7,3 @@ + +lib_LTLIBRARIES = libgrldmap.la + Use ext_LTLIBRARIES @@ +20,3 @@ +libgrldmap_la_LDFLAGS = \ + -module \ + -avoid-version -no-undefined @@ +38,3 @@ + simple-dmap-db.h + +libdir = $(GRL_PLUGINS_DIR) extdir (see Jamendo as example) ::: src/media/dmap/grl-dmap.c @@ +124,3 @@ + G_CALLBACK (service_added_cb), + NULL); + I think you need to pass "plugin" as the last parameter, instead of NULL. Because you are using it in service_added_cb() callback (else, it is crashing). @@ +321,3 @@ + GRL_METADATA_KEY_URL, + GRL_METADATA_KEY_MIME, + GRL_METADATA_KEY_DATE, GRL_METADATA_KEY_DATE is an invalid key @@ +328,3 @@ + GRL_METADATA_KEY_CHILDCOUNT, + GRL_METADATA_KEY_THUMBNAIL, + NULL); Are you really handling all these keys? I'm doing some tests, and most of these keys are never exposed by this plugin. Just to clarify: supported_keys() returns a list of keys that this source is able to handle. The keys ignored by the source aren't supported. @@ +377,3 @@ + cb_and_db->cb.predicate_data = NULL; + cb_and_db->cb.user_data = bs->user_data; + It seems you are ignoring completely the values of skip and count. These parameters are very important, specially when the server contains hundred or thousand of files, as the user application uses them to implement pagination. Same happens in search() function @@ +406,3 @@ + } + + for (list = services; list; list = list->next) { It seems you are searching in all DMAP sources, instead of in the specific "source". search(), as well as other operations like browse(), are to be used with the specific source passed as parameter.
Another issue (not sure if you are able to reproduce it): when I'm running the program with no avahi daemon, it crashes at the beginning. And it doesn't seem to be involved with the plugin itself.
Created attachment 218579 [details] [review] DMAP plugin for grilo This patch should address the issues raised in comment #11.
Created attachment 223991 [details] [review] Add DMAP plugin, using Grilo 0.2 API.
Created attachment 224094 [details] [review] New version, fixing the service_removed_cb() callback. It seems the parameter received is not a Service, but the service name being removed. So we need to index the hashtable by service_name.
I'm seeing in connected_cb() that if there is an error, you simply ignore it. The point is that in this case, when building the hashtable with the results to send back, there are no results, and you aren't calling the callback to notify it. I think the proper place to do is in connected_cb(): if there is an error, just invoke the callback setting the proper Grilo error.
Created attachment 224107 [details] [review] Fix for issues in previous comment
Created attachment 224115 [details] [review] dmap: Prefix source id with "grl-dmap-" prefix Purpose is double: use the same policy as other sources, and to avoid conflicts with other sources having exactly the same title. This fixes
commit 4eb11d4e7f5eaa0547f31b0ce7afca3f13e1e79c Author: Juan A. Suarez Romero <jasuarez@igalia.com> Date: Wed Sep 12 15:27:40 2012 +0200 dmap: Prefix source id with "grl-dmap-" prefix Purpose is double: use the same policy as other sources, and to avoid conflicts with other sources having exactly the same title. This fixes https://bugzilla.gnome.org/show_bug.cgi?id=652516 src/dmap/grl-dmap.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) commit 48f415fa6562012a996395c11420d746e45cf7ea Author: Juan A. Suarez Romero <jasuarez@igalia.com> Date: Wed Sep 12 11:27:18 2012 +0000 dmap: Notify when there are no results This fixes https://bugzilla.gnome.org/show_bug.cgi?id=652516 src/dmap/grl-dmap.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) commit fafa1bea66a0486b483d2fbc4ad880a14367795e Author: W. Michael Petullo <mike@flyn.org> Date: Wed Jul 11 12:46:07 2012 -0500 Add DMAP plugin This fixes https://bugzilla.gnome.org/show_bug.cgi?id=652516 Signed-off-by: W. Michael Petullo <mike@flyn.org> Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com> configure.ac | 40 +++++++++++ src/Makefile.am | 8 ++- src/dmap/Makefile.am | 45 +++++++++++++ src/dmap/grl-dmap.c | 459 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/dmap/grl-dmap.h | 76 +++++++++++++++++++++ src/dmap/grl-dmap.xml | 10 +++ src/dmap/simple-daap-record-factory.c | 58 ++++++++++++++++ src/dmap/simple-daap-record-factory.h | 72 ++++++++++++++++++++ src/dmap/simple-daap-record.c | 326 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/dmap/simple-daap-record.h | 78 ++++++++++++++++++++++ src/dmap/simple-dmap-db.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/dmap/simple-dmap-db.h | 82 +++++++++++++++++++++++ 12 files changed, 1444 insertions(+), 2 deletions(-)