GNOME Bugzilla – Bug 676366
Add Ampache plugin
Last modified: 2018-09-24 09:20:47 UTC
From the officiel web site[1]: « Ampache is a web based audio/video streaming application and file manager allowing you to access your music & videos from anywhere, using almost any internet enabled device» Ampache can be using as a streaming server for musics. Streaming music from this server is already integrated in Amarok and Owncloud also use this server for share their musics. It would be cool to can stream tracks from server and send tracks to server. All informations for dev are there: http://ampache.org/wiki/ [1] http://ampache.org/
There are plugins for Banshee and Rhythmbox which could be used for inspiration: * Banshee - https://gitorious.org/banshee-community-extensions/banshee-community-extensions/trees/master/src/Ampache * Rhythmbox - http://code.google.com/p/rhythmbox-ampache/
Created attachment 278958 [details] [review] Intial version of the ampache plugin.
Review of attachment 278958 [details] [review]: The sources needs cleaning up, or the implementation finishing before this can be merged. I also don't like the fact that we require pre-usage configuration, instead of the username/password being requested when needed, and the server URL through mDNS or similar. ::: src/ampache/grl-ampache.c @@ +131,3 @@ +static const GList *grl_ampache_source_supported_keys (GrlSource *source); + +// static void grl_ampache_source_resolve (GrlSource *source, You'll need to clean that up. @@ +187,3 @@ + source_class->cancel = grl_ampache_source_cancel; + source_class->supported_keys = grl_ampache_source_supported_keys; + // source_class->resolve = grl_ampache_source_resolve; Ditto. @@ +225,3 @@ +xml_count_children (xmlNodePtr node) +{ +#if (LIBXML2_VERSION >= 20700) 2.7.0: Aug 30 2008 I don't think we need pre-2.7.0 support. @@ +357,3 @@ + data->album_name = + (gchar *) xmlNodeListGetString (doc, node->xmlChildrenNode, 1); + // data->album_id = (gchar *) xmlGetProp (node, (const xmlChar *) "id"); What are those? @@ +571,3 @@ + gchar *pass_hash = g_compute_checksum_for_string(G_CHECKSUM_SHA256, grl_config_get_password(config), -1); + gchar *passphrase = g_compute_checksum_for_string(G_CHECKSUM_SHA256, g_strconcat(current_time,pass_hash, NULL), -1); + gchar *url = g_strdup_printf(AMPACHE_GENERATE_AUTH, grl_config_get_string(config, "host_url"), passphrase, current_time, AMPACHE_VERSION, grl_config_get_username(config)); space before parenthesis (everywhere) And that line could do with some indentation. @@ +626,3 @@ + update_media_from_artists (media); + break; + case 1: skip 1? That looks wrong. @@ +963,3 @@ + bindtextdomain (GETTEXT_PACKAGE, LOCALEDIR); + bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8"); + empty lines.
Created attachment 284117 [details] [review] Attachment to the patch.
Review of attachment 284117 [details] [review]: I'm not too confident that this doesn't leak all over the place. Could you please run grl-launch-0.2 in a variety of situations with the ampache plugin to make sure that it doesn't leak. The cancellation and error paths are areas where this could be a problem. ::: src/ampache/grl-ampache.c @@ +240,3 @@ + xmlDocPtr doc; + xmlNodePtr node; + struct PluginInfo *plugininfo = (struct PluginInfo *) user_data; You're leaking the plugininfo that you allocated in generate_api_key() @@ +561,3 @@ + g_get_real_time ()/1000000); + gchar *pass_hash = g_compute_checksum_for_string (G_CHECKSUM_SHA256, + grl_config_get_password (config), Leaking the password. @@ +564,3 @@ + -1); + gchar *passphrase = g_compute_checksum_for_string (G_CHECKSUM_SHA256, + g_strconcat (current_time,pass_hash, NULL), You're leaking the output of g_strconcat() @@ +567,3 @@ + -1); + gchar *url = g_strdup_printf (AMPACHE_GENERATE_AUTH, + grl_config_get_string (config, "host_url"), Leaking the host_url. @@ +570,3 @@ + passphrase, current_time, + AMPACHE_VERSION, + grl_config_get_username (config)); Leaking the username. @@ +966,3 @@ + bindtextdomain (GETTEXT_PACKAGE, LOCALEDIR); + bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8"); + Too many linefeeds here. @@ +985,3 @@ + return FALSE; + } + GRL_DEBUG ("no ampache plugin"); "no ampache plugin" ?
Comment on attachment 284117 [details] [review] Attachment to the patch. As discussed on IRC, the support would be implemented through lua instead.
Add to the "request" component.
Created attachment 329190 [details] [review] lua-factory: Support GoaPasswordBased Allows retrieving password from account objects implementing PasswordBased interface
Review of attachment 329190 [details] [review]: Looks fine to land when there's something actually using it (so we can test it).
Created attachment 329768 [details] [review] lua-factory: add get_ampache_passphrase method
Created attachment 329770 [details] [review] lua-factory: add goa_music_uri method
Created attachment 329771 [details] [review] lua-factory: add owncloud lua plugin
Review of attachment 329768 [details] [review]: We really don't want anything this specific in the lua-factory. Either it needs to be in GOA, or it needs to be in the source.
Review of attachment 329770 [details] [review]: ::: src/lua-factory/grl-lua-library.c @@ +1716,3 @@ + + if (object != NULL) { + music = goa_object_peek_music (object); Is "Music" the only account type that can use this? videos_uri? @@ +1720,3 @@ + if (music == NULL) { + GRL_WARNING ("Source is broken as it tries to access gnome-online-accounts " + "information, but it doesn't declare what account data it " That's not the right error here. @@ +1726,3 @@ + } + + gchar *uri = NULL; No C-99 declarations.
Review of attachment 329771 [details] [review]: You need some information in the commit message. You'll also want to make sure to capitalise "ownCloud" in the same way as upstream, and add links to API docs in the code itself. ::: src/lua-factory/sources/grl-owncloud.lua @@ +42,3 @@ + }, + supported_media = 'audio', + tags = { 'net:internet', 'net:plaintext'} It's over the internet, and in plaintext? You need to disallow creating those sorts of setup in gnome-online-accounts. Either it's plaintext and local network only, or it needs to be encrypted. You don't want your owncloud password in cleartext over the internet. @@ +49,3 @@ + +function grl_source_init (config) + if (config) then What is it you're trying to implement here? It needs to either use configuration data, or GOA, not both. @@ +60,3 @@ + + ldata.username, ldata.ampache_password = grl.goa_password ("ampache_password") + ldata.username, ldata.password = grl.goa_password ("password") Why are you setting the username twice? @@ +77,3 @@ + local passphrase, timestamp = grl.get_ampache_passphrase (ldata.ampache_password) + auth_endpoint = + ldata.ampache_uri .. string.format ( how about string.format (ldata.ampache_uri .. "?action... @@ +87,3 @@ + loglevel = 1, + headers = { + Authorization = string.format ("Basic %s", base64_credential) Any way to pass this in GET instead? @@ +131,3 @@ + + for i, song in pairs (res.root.song) do + count = count - 1 Remove one. @@ +143,3 @@ + media.duration = song.time.xml + + count = count - 1 And again. I think you might come short. @@ +156,3 @@ + +-- base64 encoding +-- refer to http://lua-users.org/wiki/BaseSixtyFour It's LGPLv2 and has an author, best update the copyright. @@ +159,3 @@ +local b='ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/' + +function enc(data) base64_enc()
Created attachment 329947 [details] [review] lua-factory: Support GoaPasswordBased
Created attachment 329948 [details] [review] lua-factory: add checksum method
Created attachment 329949 [details] [review] lua-factory: add goa_get_uri method
Created attachment 329950 [details] [review] lua-factory: add time method
Created attachment 329951 [details] [review] lua-factory: add ownCloud source
Created attachment 332408 [details] [review] lua-factory: implement query operation in ownCloud source
Created attachment 333943 [details] [review] lua-factory: implement search operation in ownCloud source
Created attachment 333944 [details] [review] lua-factory: factor out code to build media object from ownCloud source
Created attachment 333945 [details] [review] lua-factory: document ownCloud source
Is the new patch set good for landing? I'd like very much to see this plugin in grilo.
JFYI, adding this to my todo to review early next week. Sorry for the delay!
-- 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/23.