GNOME Bugzilla – Bug 681259
audioscrobbler: use single image download for multiple data items
Last modified: 2012-09-17 18:09:22 UTC
Created attachment 220402 [details] [review] use single image download for multiple data items Sometimes last.fm uses the same image for all the tracks on one album. When we fixed https://bugzilla.gnome.org/show_bug.cgi?id=645015, we made it so that the image would only be downloaded once, leaving most of the displayed tracks without an image. Rather than mapping from src file to the one data item the image is for, this patch keeps a map from src file to a list of data items which want the image. When a new data item wants an image which is already being downloaded, append it to the list. When the download finishes, notify all the data items in the list. Still to do: The image is still only saved in the cache for one data item, meaning it'll download again when refreshed. To fix this we can do some local file copies after the download finishes. I'm also not 100% happy with some of the code in this file, so I might see if I can make some improvements if I get a bit of time. Although I'm not sure how much better I'm really going to be able to do. A glib last.fm library would be nice, or a higher level language. But that's not really related to this.
Review of attachment 220402 [details] [review]: Generally looks good. One minor potential leak that should be fixed (one way or another) and some other mostly harmless comments. OK to commit without the leak. ::: plugins/audioscrobbler/rb-audioscrobbler-user.c @@ +359,3 @@ + /* do not provide a value_destroy_func, as we do not want the + * list to be freed when replacing its head to append to it */ I found this comment a bit confusing at first. I don't think this really needs to be explained, so maybe just remove the comment. Alternatively, you could store GQueues as the hash table values rather than GLists, so if the key already exists, you can just append to the queue rather than updating the list and re-inserting it. This way you could use the destroy function to take care of cleaning up the queues and their contents. @@ +438,3 @@ + if (user->priv->file_to_data_list_map != NULL) { + g_list_foreach (g_hash_table_get_values (user->priv->file_to_data_list_map), the list returned by g_hash_table_get_values needs to be freed @@ +1639,1 @@ + if (data->refcount > 1) { if (data->refcount < 1) { continue; } would reduce the indent level of the rest of the loop, which is getting a bit excessive. @@ +1709,3 @@ /* cleanup the file and data */ + free_data_list (data_list); + g_hash_table_remove (user->priv->file_to_data_list_map, src_file); I'd be slightly happier if the list was removed from the hash table before being freed, but I'm being paranoid and/or picky here.
(In reply to comment #1) > ::: plugins/audioscrobbler/rb-audioscrobbler-user.c > @@ +359,3 @@ > > + /* do not provide a value_destroy_func, as we do not want the > + * list to be freed when replacing its head to append to it */ > > I found this comment a bit confusing at first. I don't think this really needs > to be explained, so maybe just remove the comment. > > Alternatively, you could store GQueues as the hash table values rather than > GLists, so if the key already exists, you can just append to the queue rather > than updating the list and re-inserting it. This way you could use the destroy > function to take care of cleaning up the queues and their contents. Using GQueue seems to fix most of the issues here, so I've switched to that and, hopefully not introduced any other problems in doing so. (Didn't know that GQueue existed, cheers for that.) > @@ +438,3 @@ > > + if (user->priv->file_to_data_list_map != NULL) { > + g_list_foreach (g_hash_table_get_values > (user->priv->file_to_data_list_map), > > the list returned by g_hash_table_get_values needs to be freed Fixed by GQueue > @@ +1639,1 @@ > + if (data->refcount > 1) { > > if (data->refcount < 1) { > continue; > } > > would reduce the indent level of the rest of the loop, which is getting a bit > excessive. Sure. It's data->refcount <= 1, I think. Because the download reference doesn't count. This is the code I have in mind when I say I want to clean it up. Especially now when we might be emitting the signal for every recent track when we only really need to emit it once. > @@ +1709,3 @@ > /* cleanup the file and data */ > + free_data_list (data_list); > + g_hash_table_remove (user->priv->file_to_data_list_map, src_file); > > I'd be slightly happier if the list was removed from the hash table before > being freed, but I'm being paranoid and/or picky here. GQueue
Created attachment 220736 [details] [review] use single image download for multiple data items
Review of attachment 220736 [details] [review]: looks good now.
Comment on attachment 220736 [details] [review] use single image download for multiple data items Pushed that patch. Keeping the bug open until we save a copy of the image in the cache for every data item. Should hopefully find some time to get that done in the next week or so.
Created attachment 221327 [details] [review] copy cached images for every interested data item This was actually really simple, because we don't care if this operation fails or not, and we don't have to do anything when it finishes.
Hmm, the queue iteration introduced by the previous patch is segfaulting for me now, pretty sure it wasn't a few days ago... for (data_i = g_queue_peek_head (data_queue); data_i != NULL; data_i = g_list_next (data_i)) { RBAudioscrobblerUserData *data = data_i->data; if (data->refcount <= 1) { // segfault when we dereference data. That's perhaps not the correct way to iterate through a GQueue. Maybe I'm missing the correct way to iterate the underlying list? We probably don't want to use peek_nth() for efficiency reasons. But because the loop contents needs both "user" (the this pointer), and the "dest_image_path" (the image to load for each item), we can't simply move the contents to a function and use g_queue_foreach. We could probably split it into a few functions though (load the images, emit signals, and copy images), and use multiple g_queue_foreach calls.
I use 'for (l = queue->head; l != NULL; l = l->next) { ... }' for GQueues, but that's just a shorter way of saying what you're doing now.
Ah! It should be g_queue_peek_head_link(). peek_head() returns the data not the list node. Am I okay just to push that fix?
sure, go for it.
Review of attachment 221327 [details] [review]: looks ok to me.
Fix pushed to master.