GNOME Bugzilla – Bug 625213
DACP Support in libdmapsharing
Last modified: 2010-08-20 15:17:52 UTC
Created attachment 166508 [details] [review] DACP Support for libdmapsharing, first version DACP Support is being implemented as part of the Google Summer of Code 2010. More information on the project can be found on http://live.gnome.org/SummerOfCode2010/AlexandreRosenfeld_Rhythmbox This is the current state of the project, I'm submitting here for an early review of the code. While not feature complete, everything should be in it's place, so it's important to get as much feedback about the code as possible.
Some initial comments follow (not exhaustive): > diff --git a/.gitignore b/.gitignore > new file mode 100644 > index 0000000..90c2def > --- /dev/null > +++ b/.gitignore [...] > +*.[oa] Please don't include .gitignore in your patch. > diff --git a/config.h.in b/config.h.in > index 4e5b5f2..d06e6a1 100644 > --- a/config.h.in > +++ b/config.h.in [...] > +/* Define to the home page for this package. */ > +#undef PACKAGE_URL I think that I should not have added config.h.in to the Git tree because it is generated by autoheader. Not your fault, but pull it out to be expedient because it is extraneous. > diff --git a/configure.ac b/configure.ac > index 141c479..9604dca 100644 > --- a/configure.ac > +++ b/configure.ac [...] > +m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])],) > + Extraneous? > diff --git a/docs/Makefile.am b/docs/Makefile.am > index bd6f455..38e8707 100644 > --- a/docs/Makefile.am > +++ b/docs/Makefile.am [...] > -HFILE_GLOB= > -CFILE_GLOB= > +HFILE_GLOB=../libdmapsharing/*.h > +CFILE_GLOB=../libdmapsharing/*.c I think you are right, but this belongs in separate patch. > diff --git a/libdmapsharing/daap-record.c b/libdmapsharing/daap-record.c > index 54e764b..6c3cd3a 100644 > --- a/libdmapsharing/daap-record.c > +++ b/libdmapsharing/daap-record.c [...] > + //FIXME: This is actually an enum > + g_object_interface_install_property (iface, > + g_param_spec_int ("mediakind", > + "Media kind", > + "Media kind", > + 0, > + G_MAXINT, > + 1, > + G_PARAM_READWRITE)); Use g_param_spec_enum()? > /* NOTE: experimenting with matching property name with > * DAAP protocol keyword to make some code simpler. */ Perhaps change this comment to "NOTE: property names must match DAAP protocol keywords. This is required by the current DMAPDb::dmap_db_apply_filter() and DMAPShare::dmap_share_build_filter()." > diff --git a/libdmapsharing/dmap-connection.c b/libdmapsharing/dmap-connection.c > index 34e43be..d9a3cb9 100644 > --- a/libdmapsharing/dmap-connection.c > +++ b/libdmapsharing/dmap-connection.c [...] > DMAPConnectionState state; > DMAPResponseHandler response_handler; > + gpointer response_handler_data; Can this be refactored? Can you add a gpointer user_data to the DAAPResponseData struct and use this instead of the above response_handler_data? You would need to change http_get() so that it allocates a DAAPResponseData and passes it to soup_session_queue_message(). Get the allocation code from http_response_handler() and modify http_response_handler() to take a DAAPResponseData as its third argument instead of allocating it. Modify the references to response_handler_data. I just don't like response_handler_data being a member of DMAPConnection because it is really just transient user_data. > - g_debug ("Finalize"); Extraneous. > - g_debug ("DAAP connection dispose"); > - Extraneous. > diff --git a/libdmapsharing/dmap-db.h b/libdmapsharing/dmap-db.h > index 686a4d2..06f7238 100644 > --- a/libdmapsharing/dmap-db.h > +++ b/libdmapsharing/dmap-db.h [...] > - * @fn: The function to apply to each record in the database. > + * @func: The function to apply to each record in the database. Extraneous. > diff --git a/libdmapsharing/dmap-mdns-browser-avahi.c b/libdmapsharing/dmap-mdns-browser-avahi.c > index bf67bd8..51b40fb 100644 > --- a/libdmapsharing/dmap-mdns-browser-avahi.c > +++ b/libdmapsharing/dmap-mdns-browser-avahi.c [...] > + switch (browser->priv->service_type) { > + case DMAP_MDNS_BROWSER_SERVICE_TYPE_DAAP: > + service_type = "_daap._tcp"; > + break; > + case DMAP_MDNS_BROWSER_SERVICE_TYPE_DPAP: > + service_type = "_dpap._tcp"; > + break; > + case DMAP_MDNS_BROWSER_SERVICE_TYPE_DACP: > + service_type = "_touch-remote._tcp"; > + break; > + default: > + break; > + } This code appears twice. Create a static function, const gchar *get_service_type_string(DMAPMdnsBrowserServiceType t) or a lookup table. > diff --git a/libdmapsharing/dmap-share.c b/libdmapsharing/dmap-share.c > index dd2490f..3e2e039 100644 > --- a/libdmapsharing/dmap-share.c > +++ b/libdmapsharing/dmap-share.c [...] > + g_debug ("URI: %s", soup_uri_to_string (soup_message_get_uri (message), FALSE)); Extraneous. > diff --git a/libdmapsharing/dmap-share.h b/libdmapsharing/dmap-share.h > index 3da79d5..88dc694 100644 > --- a/libdmapsharing/dmap-share.h > +++ b/libdmapsharing/dmap-share.h [...] > +void > +_dmap_share_ctrl_int_adapter (DMAPShare *share, > + SoupServer *server, > + SoupMessage *message, > + const char *path, > + GHashTable *query, > + SoupClientContext *context); Remove this.
(In reply to comment #1) > > +*.[oa] > > Please don't include .gitignore in your patch. Ok. > > +m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])],) > > + > > Extraneous? Both this and the above modification are included in Rhythmbox. Since libdmapsharing was extracted from Rhythmbox, it should have copied these also (mainly for developer convenience, it's much nicer with these options on). They will be removed from my patch, they weren't supposed to got in. > > +/* Define to the home page for this package. */ > > +#undef PACKAGE_URL > > I think that I should not have added config.h.in to the Git tree because > it is generated by autoheader. Not your fault, but pull it out to be > expedient because it is extraneous. OK, although it should be removed on master first, since if I remove on my branch it will still appear in a diff. > > -HFILE_GLOB= > > -CFILE_GLOB= > > +HFILE_GLOB=../libdmapsharing/*.h > > +CFILE_GLOB=../libdmapsharing/*.c > > I think you are right, but this belongs in separate patch. Haven't tested it either, will remove from my patch. > > + //FIXME: This is actually an enum > > + g_object_interface_install_property (iface, > > + g_param_spec_int ("mediakind", > > + "Media kind", > > + "Media kind", > > + 0, > > + G_MAXINT, > > + 1, > > + G_PARAM_READWRITE)); > > Use g_param_spec_enum()? Indeed, I had to integrate glib-mkenums to make it work. I haven't yet fixed this property, since I'm not sure which values should be used (DACP is using 1 and 32) > > DMAPConnectionState state; > > DMAPResponseHandler response_handler; > > + gpointer response_handler_data; > > Can this be refactored? Can you add a gpointer user_data to the > DAAPResponseData struct and use this instead of the above > response_handler_data? > > You would need to change http_get() so that it allocates a > DAAPResponseData and passes it to soup_session_queue_message(). Get > the allocation code from http_response_handler() and modify > http_response_handler() to take a DAAPResponseData as its third argument > instead of allocating it. Modify the references to response_handler_data. > > I just don't like response_handler_data being a member of DMAPConnection > because it is really just transient user_data. Done. Completely agree. I also refactored response_handler, since it's also transient and should never belong to the object private structure (thread-safety comes to mind but there is more issues). > > > + switch (browser->priv->service_type) { > > + case DMAP_MDNS_BROWSER_SERVICE_TYPE_DAAP: > > + service_type = "_daap._tcp"; > > + break; > > + case DMAP_MDNS_BROWSER_SERVICE_TYPE_DPAP: > > + service_type = "_dpap._tcp"; > > + break; > > + case DMAP_MDNS_BROWSER_SERVICE_TYPE_DACP: > > + service_type = "_touch-remote._tcp"; > > + break; > > + default: > > + break; > > + } > > This code appears twice. Create a static function, > const gchar *get_service_type_string(DMAPMdnsBrowserServiceType t) > or a lookup table. Done, used a lookup table. I think you asked for this a while ago, must have forgot it. Anything else not mentioned here have been fixed on my branch. A new patch will follow soon.
Created attachment 167778 [details] [review] DACP Support for libdmapsharing, second version This is a new patch with added functionality and fixes discussed above. I consider this patch to be feature-complete, since all known DACP actions have been implemented. However it may still contain bugs, especially since some new functionality haven't been fully tested or implemented in Rhythmbox yet. So the API may still break and things might still change.
Created attachment 167925 [details] [review] DACP Support for libdmapsharing, fourth version - Bump version to 2.2.0 - Fix regression in last patch
Some comments about the most recent patch: 1. As we discussed by email there is some functionality that needs to be fixed, either in this patch or in your Rhythmbox patch. 2. I don't like the hard dependency on gdk-pixbuf. I use libdmapsharing in an embedded environment with 32MB of memory and a tiny flash space. I don't want to add unnecessarily to the size of the library and its dependencies. Actual use of gdk-pixbuf must be moved to your Rhythmbox patch. You may need to add a simple pixbuf interface with get_width-, get_height-, scale_simple- and save_to_buffer-like functions that Rhythmbox would implement (using gdk-pixbuf). Libdmapsharing should reference this interface instead of using gdk-pixbuf directly. 3. Remove the PACKAGE_URL addition and double check that there are not any other extraneous things in the patch. 4. Let's call this version 2.1, a development version series with 2.2 being the stable release. The pkgconfig file should still be named "libdmapsharing-2.2.pc" because this is the API version we are working towards. This will require build system modification (see Makefile.am and configure.ac for how the .pc file is generated). I'm not too worried about you figuring out the build system -- if you can automate this quickly then do it, otherwise, I will do it. 5. We discussed the problem of supporting both DAAP and DACP in one process. This is clearly a bug, but more of an existing problem in libdmapsharing (its already a problem with DAAP + DACP). So, fixing this will NOT be a requirement to the acceptance of this patch. Instead, we will fix this later.
(In reply to comment #5) > Some comments about the most recent patch: > > 1. As we discussed by email there is some functionality that needs to be fixed, > either in this patch or in your Rhythmbox patch. Mostly done, the funtionality is there, with some quirks in some areas. Although this is one of the most complete DACP open-source implementations i've seen so far. > 2. I don't like the hard dependency on gdk-pixbuf. I use libdmapsharing in an > embedded environment with 32MB of memory and a tiny flash space. I don't want > to add unnecessarily to the size of the library and its dependencies. Actual > use of gdk-pixbuf must be moved to your Rhythmbox patch. You may need to add a > simple pixbuf interface with get_width-, get_height-, scale_simple- and > save_to_buffer-like functions that Rhythmbox would implement (using > gdk-pixbuf). Libdmapsharing should reference this interface instead of using > gdk-pixbuf directly. As discussed by email, this would be best to use a filename to a PNG in libdmapsharing and move the gdk-pixbuf code to Rhythmbox. This is still note done though. > 3. Remove the PACKAGE_URL addition and double check that there are not any > other extraneous things in the patch. Ok. > 4. Let's call this version 2.1, a development version series with 2.2 being the > stable release. The pkgconfig file should still be named > "libdmapsharing-2.2.pc" because this is the API version we are working towards. > This will require build system modification (see Makefile.am and configure.ac > for how the .pc file is generated). I'm not too worried about you figuring out > the build system -- if you can automate this quickly then do it, otherwise, I > will do it. I prefer that you do this, I'm not very good at editing autoconf related files. > 5. We discussed the problem of supporting both DAAP and DACP in one process. > This is clearly a bug, but more of an existing problem in libdmapsharing (its > already a problem with DAAP + DACP). So, fixing this will NOT be a requirement > to the acceptance of this patch. Instead, we will fix this later. This is definitely a serious bug, however I've not yet investigated the causes and will still try to complete this after GSoC.
Created attachment 168350 [details] [review] DACP Support for libdmapsharing, fifth version - Add delayed playstatusupdate, commands are much more responsive. - Fixed many bugs when queuing records.
Created attachment 168366 [details] [review] DACP Support for libdmapsharing, version 5.1 - Make gdk-pixbuf optional. The API now only includes a filename to a now playing artwork and optionally, if gdk-pixbuf is present, the image is loaded, scaled and saved to a PNG.
I have committed your DACP patch to libdmapsharing Git. As you stated the work is not complete, but your progress warrants committing the code to my development tree. Moving forward, please supply small, targeted patches (i.e., one patch per fix or change). We have completed the big, initial patch, so future patches must be simple and to the point. Please continue to attach patches to Bugzilla reports, one simple Bugzilla report for each simple patch.