GNOME Bugzilla – Bug 698523
Adding Magnatune pluggin
Last modified: 2013-05-22 15:25:09 UTC
Created attachment 242093 [details] [review] Patch to basic implementation of Magnatune plugin This patch implements a search into database provided by magnatune website[0] The search is performed in Artist name and Albums and Tracks titles. - The plugin does not download the database - The search is not ordered in any specific way [0] http://magnatune.com/info/sqlite-normalized
Created attachment 242105 [details] [review] Patch to basic implementation of Magnatune plugin [2] I fixed a possible small leak in last patch. Also, I've changed the copyright from files (copy and paste error).
Review of attachment 242105 [details] [review]: In general the code looks good. But I'll wait until your next patches before committing it. One of the issues we need to deal with is downloading the Magnatune DB. We can't rely on the user to download it by hand. The approach I would use is the following one: 1) In plugin_init() I would check if DB is there or not; if not I would download it from Magnatune service. 2) If DB is present, I would check if I need to update it or not. IIRC, Magnatune updates the DB every week, and it has an available file on the web to check if the DB has changed or not. So we could use these facts to check if DB is up-to-date. 3) If it isn't up-to-date, download the new version. 4) After we have the new version, we instantiate and register the source. 5) If for some reason we can't get a DB, then no source will be registered. ::: src/magnatune/grl-magnatune.c @@ +88,3 @@ + +static GrlMedia * +build_media(gint track_id, I would re-organize the functions to follow the same convention used in other plugins. - Definitions, typdefs, structs... - Plugin initialization - GObject related code (_new(), _init(), _class_init(), ...) - Utilities (private functions used in the code) - Grilo related code (_search(), _browse(), ....) This makes easier to maintain the code and quickly find where are the relevant pieces of code. @@ +212,3 @@ + num_medias--; + } + One of the problems of using a mainloop instead of threads, is that while one function is running, the others are on the queue. In practical, it means that, for instance, while magnatune_execute_search() is running, the UI events aren't processed. So, if the function takes too much time to run, the effect for the user is that the UI application is totally blocked. Probably, this is not a big problem at this moment, because magnatune_execute_search() performs all the work in local, and it doesn't invoke any heavy function. But if were, we should split the function in smaller chunks. One of the way we usually split it is by running the creation & sending of each media in a idle callback. That is, in the idle we create and send the media, and return (keeping the idle function in the queue), so next time the idle function is invoked we sent the next item and repeat the step. As said, probably that is not needed at this moment, but just for keep in mind. @@ +238,3 @@ + err = g_error_new_literal(GRL_CORE_ERROR, + GRL_CORE_ERROR_QUERY_FAILED, + _("No database connection")); I don't know if you already did or not, but when adding a new string that must be translated, it could be a good idea to check if we can reuse any string already used in other places (of course, without forcing the situation). This would make the life easier for translators. @@ +301,3 @@ + + if(ret != SQLITE_OK) { + g_critical("Failed to open database '%s': %s", Let's use Grilo log messages instead. @@ +306,3 @@ + sqlite3_close(source->priv->db); + source->priv->db = NULL; + } I think it would be better to check for the DB in the plugin_init(), or in grl_magnatune_source_new(). If the DB is not present, then do not register the source. @@ +335,3 @@ + source = GRL_MAGNATUNE_SOURCE(object); + + sqlite3_close(source->priv->db); I think in your code, source->priv->db can be NULL.
Created attachment 243074 [details] [review] Patch to basic implementation of Magnatune plugin [3] Juan, Thanks for the review ! I have re-organized the code and applied some fixes, some highlights are: * I had to include some prototypes to follow the convention. * magnatune_execute_search() do a local search (response in < 0.1s from sql, on my computer) and that's why I kept it in mainloop. If you think is necessary to implement it as idle_callback, it's ok to me to do it. * if there is no database, the plugin_init will return FALSE. The check was made in plugin_init. The next patch handles downloading the database.
Created attachment 243078 [details] [review] Handling database download sync I used the libsoup to download the magnatune database (sync) as GrlNet uses it too (but async only). There were three concerns to this patch: 1-) Downloading sync when there is no database because if there is no database the plugin can't start. 2-) Downloading always the crc of database to check if there is a newer database. The file has 4Kb only so I this approach first. If the database exists, downloading or not the crc does not break the plugin_init. 3-) Downloading a newer database is done sync also. I guess its preferable to do it async - but how ? Using the current database while downloading a new one, then: A -) when the newer database is ready to use we restart the plugin; (how ?) B -) Using the newer database in next plugin_init only ? I kept it the simpler way for now.
(In reply to comment #3) > * magnatune_execute_search() do a local search (response in < 0.1s from sql, on > my computer) and that's why I kept it in mainloop. If you think is necessary to > implement it as idle_callback, it's ok to me to do it. > There are some plugins that do as you do, and others split the sends. I prefer to choose the simplest approach, and refactor it if that approach has performance problems. So in that sense, I'm fine with your approach. We can always refactor later if needed.
Review of attachment 243074 [details] [review]: ::: src/magnatune/grl-magnatune.c @@ +134,3 @@ + "source-id", SOURCE_ID, + "source-name", SOURCE_NAME, + "source-desc", SOURCE_DESC, Add also: "supported-media", GRL_MEDIA_TYPE_AUDIO, So clients know this source handles Audio content.
Review of attachment 243078 [details] [review]: Unfortunately, we can't go with the sync version. We need an asynchronous approach. Otherwise, everytime we load the plugins, magnatune will block the other plugins (and the client application too!) while the Magnatune DB is downloaded. Besides that, could it be possible to use the last time check before checking the CRC? Thus we avoid going to Magnatune service everytime we load the plugin. This could be done as follows: 1) If CRC file is older than 1 week, download the new CRC (1 week because Magnatune DB is usually updated weekly) 2) If the new CRC is different than the previous, then download the new DB.
Review of attachment 243078 [details] [review]: ::: src/magnatune/grl-magnatune.c @@ +314,3 @@ + } + + crr_path = g_strconcat(g_get_user_data_dir(), G_DIR_SEPARATOR_S, g_build_filename()
Review of attachment 243074 [details] [review]: As mentioned on IRC, the database should only be downloaded when the user browses or searches inside the Magnatune source. As to whether or not to show the source, I'd simply use GNetworkMonitor to determine whether the computer can connect to the Internet. If the database is not present and there's no network, hide it, otherwise, show it and download it when needed (and not on startup!).
Is a real problem downloading the database on startup? First, this download would happen as much as once per week (I'd say even less). It's totally transparent to the user (it shouldn't block what user is doing). And while DB is being download, we can use still the previous DB so user can always perform the operation with no delay. This downloading on startup isn't something really new; other applications also do it. And avoids the problem that when user starts to browse, it takes ages because it's downloading the DB, which gives a bad user experience, IMHO. I think you prefer to download it on first operation because probably you won't use the DB, so you want to avoid downloading the DB when you know you'll never use it. Correct me if I'm wrong. Maybe what we can do is having some configuration key to avoid this situation. Like "update-db = FALSE", or something similar.
(In reply to comment #10) > Is a real problem downloading the database on startup? > > First, this download would happen as much as once per week (I'd say even less). > It's totally transparent to the user (it shouldn't block what user is doing). > And while DB is being download, we can use still the previous DB so user can > always perform the operation with no delay. > > This downloading on startup isn't something really new; other applications also > do it. Like which one? The Magnatune plugin in Rhythmbox only starts the download when you use the web service, not before. > And avoids the problem that when user starts to browse, it takes ages > because it's downloading the DB, which gives a bad user experience, IMHO. > > I think you prefer to download it on first operation because probably you won't > use the DB, so you want to avoid downloading the DB when you know you'll never > use it. Correct me if I'm wrong. That's completely correct. There's no point loading nearly 5 megs of catalogue if you're not going to use it. There's no point downloading 5 megs of catalogue again and again if you're not going to use it. Why would I need this week's catalogue if I used it last week, won't this week, but will next week? > Maybe what we can do is having some configuration key to avoid this situation. > Like "update-db = FALSE", or something similar. As long as you default to FALSE.
I see your point. What I don't really like of this behaviour is that when user starts to browse or search, it will take ages to solve that operation, and he won't know why it happens: sometimes is faster (when DB is on disk), sometimes is very slow (when it's downloading). I'm fine with updating the DB when user starts to browse. We only need to download it before browse the first time, otherwise you have nothing to browse. Even in this case, I still think that in order to improve user experience, when user starts to browse/search and there's a new DB to download, starts the download in background, but use the current DB on disk to perform the browse. Thus, user won't see any delay on the operation. When the new DB is downloaded, we can switch to the new DB and use it from next operations on.
The best way to deal with the initial load delay would be for grilo to report the progress of the download to the application, which could present something to the user in the meanwhile.
(In reply to comment #13) > The best way to deal with the initial load delay would be for grilo to report > the progress of the download to the application, which could present something > to the user in the meanwhile. Yes, something that I had already considered, and that we need to add. In any case, this is what I think we should do: 1) If DB doesn't exist when loading plugin => download it in background, and register later the source 2) If DB exists, show the source 3) if user interacts with the source, and there is a new version => download the new version and meanwhile use the current one. Once it's downloaded, switch to the new version. I think this way we get almost your desired behaviour (DB is never updated unless you are interacting with it, except the very first time, which I think it's assumable), and also user don't need to wait for a new version when performing an operation, which is what I don't really like.
(In reply to comment #14) > (In reply to comment #13) > > The best way to deal with the initial load delay would be for grilo to report > > the progress of the download to the application, which could present something > > to the user in the meanwhile. > > Yes, something that I had already considered, and that we need to add. > > In any case, this is what I think we should do: > > 1) If DB doesn't exist when loading plugin => download it in background, and > register later the source Why is downloading multi-megabyte files needed? Why should users that launch Rhythmbox or GNOME music when connected to the internet through mobile broadband have to download a 5 meg file when they're likely going to play music locally? > 2) If DB exists, show the source > > 3) if user interacts with the source, and there is a new version => download > the new version and meanwhile use the current one. Once it's downloaded, switch > to the new version. > > I think this way we get almost your desired behaviour (DB is never updated > unless you are interacting with it, except the very first time, which I think > it's assumable), and also user don't need to wait for a new version when > performing an operation, which is what I don't really like. I don't think that we should be downloading multi-meg files like that. It sets a bad precedent. The Apple Trailers plugin doesn't do that, neither do any of the plugins that need to load data from the network.
Fair enough :)
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > The best way to deal with the initial load delay would be for grilo to report > > > the progress of the download to the application, which could present something > > > to the user in the meanwhile. > > > > Yes, something that I had already considered, and that we need to add. > > > > In any case, this is what I think we should do: > > > > 1) If DB doesn't exist when loading plugin => download it in background, and > > register later the source > > Why is downloading multi-megabyte files needed? Why should users that launch > Rhythmbox or GNOME music when connected to the internet through mobile > broadband have to download a 5 meg file when they're likely going to play music > locally? Indeed. > > > 2) If DB exists, show the source > > > > 3) if user interacts with the source, and there is a new version => download > > the new version and meanwhile use the current one. Once it's downloaded, switch > > to the new version. I think that is preferable download the newer database but use it next time that plugin starts. If we change database when application is up, there is a chance that we confuse the user [ making similar requests with different databases would lead to different results ] Unless that we can inform the application that the database has changed which is the same problem of informing the download status of database to the application.
(In reply to comment #17) > > > 3) if user interacts with the source, and there is a new version => download > > > the new version and meanwhile use the current one. Once it's downloaded, switch > > > to the new version. > > I think that is preferable download the newer database but use it next time > that plugin starts. If we change database when application is up, there is a > chance that we confuse the user [ making similar requests with different > databases would lead to different results ] > Unless that we can inform the application that the database has changed which > is the same problem of informing the download status of database to the > application. Well, that problem also happens with other sources: content can change for sure, so repeating the same operation can lead to different results. Also, we have that "content changed" notification, to notify that content has changed. In any case, I also agree with you, because it seems for me easier to switch in the next run. Just to add a new comment, let's follow Bastien suggestion, and let's download/update the DB when user interacts with the DB for first time.
Created attachment 243962 [details] [review] Patch to basic implementation of Magnatune plugin [4] Small changes from last review.
Created attachment 243963 [details] [review] Handling database download async - Using GNetworkMonitor to check if internet is available; - Using GrlNet lib to get database and crc files; - Downloading database only when user interacts (only by 'search', for now); - Get new database when: -- Database is older than 7 days AND Magnatune has a new db; -- to avoid multiples 'GET' to magnatune.com, a interval of 12 hours was included. Any new thoughts ? :)
Hey, just to avoid re-re-revision of patches, I'm almost done with browse operation. So, if it's okay for you, just wait a little more to apply those patches.
Good! I wait then
Created attachment 244482 [details] [review] Patch to basic implementation of Magnatune plugin
Created attachment 244483 [details] [review] Handling database download async
Created attachment 244484 [details] [review] Browse operation
Review of attachment 244483 [details] [review]: In one of the first versions of your patch, when you were downloading the DB from magnatune service, you did it directly to a file (correct me if I'm wrong). In the new version, you use GrlNet to fetch the DB, which means first loading it entirely in the memory, and later storing it in a file. This means we need to allocate memory for the full DB, which increases the requirement for memory in the source. I guess in the first version, downloading it directly to the disk, doesn't requires such amount of memory. Maybe someone with more knowledge about GIO interns can correct me. But if this is the case, maybe it could be better to use that first approach. ::: src/magnatune/grl-magnatune.c @@ +239,3 @@ } } else { + GRL_WARNING("No database was found. Download when user interact."); I think this shouldn't be a warning. It's a typical case not having the DB when you use the program for first time. So a GRL_DEBUG() fits better
Review of attachment 244484 [details] [review]: ::: src/magnatune/grl-magnatune.c @@ +1025,3 @@ + } else { + magnatune_execute_browse(os); + magnatune_check_update(); This applies to grl_magnatune_source_search() You are executing the check_update() everytime an operation is executed. I think that check_update() should only be executed once. This can be easily obtained by adding a static variable at the beginning of check_update(): magnatune_check_update() { static gboolean already_checked = FALSE; if (already_checked) { return; } /* Do the check */ already_checked = TRUE; }
(In reply to comment #26) > Review of attachment 244483 [details] [review]: > > In one of the first versions of your patch, when you were downloading the DB > from magnatune service, you did it directly to a file (correct me if I'm > wrong). > > In the new version, you use GrlNet to fetch the DB, which means first loading > it entirely in the memory, and later storing it in a file. This means we need > to allocate memory for the full DB, which increases the requirement for memory > in the source. I guess in the first version, downloading it directly to the > disk, doesn't requires such amount of memory. > > Maybe someone with more knowledge about GIO interns can correct me. But if this > is the case, maybe it could be better to use that first approach. 5 megs of temporary memory? Peanuts. It might be slightly more efficient to splice the data to disk (using g_io_stream_splice_async() for example), but the additional burden on code maintenance isn't really worth it given the use case.
(In reply to comment #28) > 5 megs of temporary memory? Peanuts. It might be slightly more efficient to > splice the data to disk (using g_io_stream_splice_async() for example), but the > additional burden on code maintenance isn't really worth it given the use case. Ok. So let's keep it as it is.
(In reply to comment #26) > Review of attachment 244483 [details] [review]: > > In one of the first versions of your patch, when you were downloading the DB > from magnatune service, you did it directly to a file (correct me if I'm > wrong). Actually the behavior was the same. soup_session_send_message() store the database in memory. ( magnatune_libsoup_get_file: https://bug698523.bugzilla-attachments.gnome.org/attachment.cgi?id=243078 ) > ::: src/magnatune/grl-magnatune.c > @@ +239,3 @@ > } > } else { > + GRL_WARNING("No database was found. Download when user interact."); > > I think this shouldn't be a warning. It's a typical case not having the DB when > you use the program for first time. So a GRL_DEBUG() fits better Ok, fixing.. (In reply to comment #27) > Review of attachment 244484 [details] [review]: > > ::: src/magnatune/grl-magnatune.c > @@ +1025,3 @@ > + } else { > + magnatune_execute_browse(os); > + magnatune_check_update(); > > This applies to grl_magnatune_source_search() > > You are executing the check_update() everytime an operation is executed. > > I think that check_update() should only be executed once. This can be easily > obtained by adding a static variable at the beginning of check_update(): > > magnatune_check_update() > { > static gboolean already_checked = FALSE; > > if (already_checked) { > return; > } > > /* Do the check */ > already_checked = TRUE; > } Indeed, this could be done, thanks. (In reply to comment #29) > (In reply to comment #28) > > 5 megs of temporary memory? Peanuts. It might be slightly more efficient to > > splice the data to disk (using g_io_stream_splice_async() for example), but the > > additional burden on code maintenance isn't really worth it given the use case. > > Ok. So let's keep it as it is. Ok. Thanks for the comments Bastien!
Created attachment 244952 [details] [review] Patch to basic implementation of Magnatune plugin GRL_DEBUG instead of GRL_WARNING
Created attachment 244957 [details] [review] Handling database download async check_update with static gboolean
Created attachment 244958 [details] [review] Browse operation one comment fix (copy paste mistake)
Created attachment 245018 [details] [review] Browse operation I've changed only the id format of each box/media in the browse operation. As Juan pointed, We could have two different IDs for the same track after the browse, and thats because how the browse was made. e.g: by genre / album / track -> could be something like 3/20/41 or artist / album / track -> could be something like 7/20/41 The ID's are different but the track is the same, which could lead in some confusion in the future. I'm concatenating one string to each id just to be clear enough ( genre-3/album-20/track-41 ) This way, we are always sure that track 41 is unique and is from album 20. PS: I was concatenating IDs like Jamendo does. I like this approach as the concat ID helps identify the whole process. Juan, I'm not sure that this was what you mean but I guess this solve the concern of being sure that a track found by Search and by Browse are the same or not.
(In reply to comment #34) > Juan, I'm not sure that this was what you mean but I guess this solve the > concern of being sure that a track found by Search and by Browse are the same > or not. Absolutely. As I commented with Victor in #grilo channel, the idea is that the ID of an artist/album/track/genre should be the same, no matter how you reach it, either browsing or searching.
After checking and testing the plugin, I think you didn't get my point correctly :) When you browse "Albums", and select the 3rd one ("10 Raptures"). It's ID is "root-1/album-1251". Why do you need to prepend "root-1/"? I think that "album-1251" should be enough to identify correctly that album. Moreover, if you select the first song of that album, named "Part of the Plan", it's ID is "root-1/album-1251/track-15751". But if you reach the same song through a search(), the ID would be just "15751". As you see, the same track has different IDs depending on how you get it. IMHO, in both cases the ID should be just "track-15751", as it identifies correctly the same song.
Created attachment 245030 [details] [review] Patch to basic implementation of Magnatune plugin changing media id to "track-<id>"
Created attachment 245031 [details] [review] Handling database download sync (nothing changed)
Created attachment 245032 [details] [review] Browse operation Ok! The code got simpler this way :) The box-id has only "genre-<id>" or "album-<id>" Thanks for the feedback Juan !
Created attachment 245033 [details] [review] Browse operation Ok! The code got simpler this way :) The box-id has only "genre-<id>" or "album-<id>" Thanks for the feedback Juan !
commit dc9dd8b10f08dcfd5d0711b695e2ab5cca92c751 Author: Victor Toso <me@victortoso.com> Date: Thu May 16 01:40:55 2013 -0300 magnatune: Implementing browse operation Application can browse throw Artists, Albums and Genres. https://bugzilla.gnome.org/show_bug.cgi?id=698523 src/magnatune/grl-magnatune.c | 260 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 260 insertions(+) commit 744174d64fba52b2ace9108aa6826ee196ef98fe Author: Victor Toso <me@victortoso.com> Date: Mon May 13 02:16:13 2013 -0300 magnatune: Adding support to download database All database and crc checks are made when user interact with the plugin. If there is no database and no internet connection, plugin_init fails. When current database has 7 days old the plugin starts to check for a newer database. To avoid checking crc to *every* user request a time interval of 12 hours is used. https://bugzilla.gnome.org/show_bug.cgi?id=698523 configure.ac | 7 +++++-- src/magnatune/grl-magnatune.c | 332 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 333 insertions(+), 6 deletions(-) commit d38fecb43d287bae844e4b80158cead538ddd252 Author: Victor Toso <me@victortoso.com> Date: Fri Apr 19 01:47:07 2013 -0300 magnatune: Adding Magnatune plugin Search implemented for Track title, Albums and Artists names. The function looks into database provided by magnatune website. A subset of keys are returned and could be incresead when implementing other functions. https://bugzilla.gnome.org/show_bug.cgi?id=698523 Makefile.am | 1 + configure.ac | 41 ++++++++++++++++++++++++++++++++ src/Makefile.am | 6 ++++- src/magnatune/Makefile.am | 34 +++++++++++++++++++++++++++ src/magnatune/grl-magnatune.c | 439 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/magnatune/grl-magnatune.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/magnatune/grl-magnatune.xml | 10 ++++++++ 7 files changed, 596 insertions(+), 1 deletion(-)