GNOME Bugzilla – Bug 740425
Thumbnail local items before remote ones
Last modified: 2018-01-15 14:33:07 UTC
All views populating with thumbnails seem particularly slow, especially with album thumbnails where individual images start showing up, rather than the whole album. I think we should be more aggressive with storage and cache a lot locally. Revisiting same albums seems just as slow as the initial time. Aperture on my mac uses gigabytes of view cache (not just for small previews, but even when opening large photo views). Sorry if this is a dupe, I am quite certain I filed this in the past, but for the life of me I cannot find it.
Seems like a basic thing to try and get right. Marking as a 3.16 target for now, just to keep it on the radar.
Created attachment 296448 [details] [review] Thumbnail local stuff before remote ones
Thanks for the patch, Pranav. Looks like Splinter is broken after the upgrade, so I will try to do this by hand. > --- a/src/photos-base-item.c > +++ b/src/photos-base-item.c > @@ -48,6 +48,7 @@ > #include "photos-selection-controller.h" > #include "photos-single-item-job.h" > #include "photos-utils.h" > +#include "photos-local-item.h" We might be getting close to circular includes - photos-base-item.c needs photos-local-item.h which needs photos-base-item.h Not a problem at the moment, and not a blocker for merging this patch. > +static gint > +photos_base_item_compare_items (gconstpointer a, gconstpointer b, > + gpointer > user_data) How about calling it photos_base_item_create_thumbnail_sort_func? It would be similar to photos_base_item_create_thumbnail_in_thread_func and make it clear what items we are comparing. > +{ > + GTask *task_a = G_TASK (a); > + GTask *task_b = G_TASK (b); > + > + PhotosBaseItem *item_a = PHOTOS_BASE_ITEM (g_task_get_source_object > + (task_a)); > + PhotosBaseItem *item_b = PHOTOS_BASE_ITEM (g_task_get_source_object > + (task_b)); For the sake of consistency, can you please put the function call and the definition on separate lines? > + if (PHOTOS_IS_LOCAL_ITEM (item_a)) > + return -1; > + if (PHOTOS_IS_LOCAL_ITEM (item_b)) > + return 1; > + > + return 0; > +} Using a 'gboolean ret_val ' initialized to 0 would be better, because then we won't need to have multiple return points for the function. > + g_thread_pool_set_sort_function (create_thumbnail_pool, (GCompareDataFunc) > + photos_base_item_compare_items, NULL); > + > g_assert (create_thumbnail_pool != NULL); > } The sort_function should be set after the assert. Now that I read the g_thread_pool_new code, using an assert on the return value is wrong. (I must have been stupid to use it in the first place.) In a separate patch, we should pass a GError ** to g_thread_pool_new and check that.
Created attachment 296496 [details] [review] Use GError when creating a new thread pool ... instead of directly using g_assert on returned value.
Created attachment 296497 [details] [review] Thumbnail local stuff before remote ones
Created attachment 296501 [details] [review] Thumbnail local stuff before remote ones corrected a minor typo
Created attachment 296506 [details] [review] Thumbnail local stuff before remote ones
Debarshi: Could you review the latest two patches here?
Created attachment 297648 [details] [review] base-item: Remove useless assertion
Review of attachment 296506 [details] [review]: ::: src/photos-base-item.c @@ +325,3 @@ +photos_base_item_create_thumbnail_sort_func (gconstpointer a, gconstpointer b, gpointer user_data) +{ + PhotosBaseItem *itemA, *itemB; No camel case for local variables, please. @@ +326,3 @@ +{ + PhotosBaseItem *itemA, *itemB; + gboolean ret_val = 0; gint, not gboolean.
Created attachment 297685 [details] [review] base-item: Thumbnail local items before remote ones
The next step would be to have a way to mark items as 'visible' and prioritize those that are visible over others.
dropping target
The move from GtkIconView to GtkFlowBox and a custom thumbnailer has vastly improved our thumbnail grid since 3.24. However, there are still a few things left to be done. (a) Album thumbnails: they don't scale as nicely as image thumbnails when the window's width changes. (b) Cache album thumbnails. (c) If an image with a missing thumbnail is opened, then we can short-circuit the thumbnailing by using the rendered pixels to create the thumbnail. (d) Better prioritization in the thumbnailing queue. Pranav took the first step by prioritizing local items over remote ones, since they tend to get thumbnailed faster. For example, items that are present in the current view should be thumbnailed before ones that aren't. I am sure there are more ... Among the above, (a) needs design input; (b) is what Jimmac's original complaint was about but it also depends on (a); (c) and (d) are independent of each other and everything else. How about we close this one and file a new bug for (a) and (b), and two separate ones for (c) and (d)?
(In reply to Debarshi Ray from comment #14) > (a) Album thumbnails: they don't scale as nicely as image thumbnails when > the window's width changes. https://gitlab.gnome.org/GNOME/gnome-photos/issues/3 > (b) Cache album thumbnails. On second thoughts, I left this one out because I wasn't sure whether it makes sense with the current implementation of album thumbnails. > (c) If an image with a missing thumbnail is opened, then we can > short-circuit the thumbnailing by using the rendered pixels to create the > thumbnail. https://gitlab.gnome.org/GNOME/gnome-photos/issues/4 > (d) Better prioritization in the thumbnailing queue. Pranav took the first > step by prioritizing local items over remote ones, since they tend to get > thumbnailed faster. For example, items that are present in the current view > should be thumbnailed before ones that aren't. https://gitlab.gnome.org/GNOME/gnome-photos/issues/5