Idea: change the current loading strategy for progressive one.

[asermax]

This is a long term idea that have been on my head for a while now. The current loading strategy is quite slow on my collection + machine, and I suppose it may be the same for other people too (I would really love some feedback on that, lol). It would take a big refactoring to change the current strategy, but it would be nice to use a more progressive (or is it preemptive?) strategy, that would load into the model just the needed covers.

To make the idea a little clearer, I'll give some examples:

• At the beginning, set an initial loading cap, let's say 50 albums.
• When the user scrolls down and gets closer to the last album loaded, load another group (through an idle call)
• If the user inputs a search, use the same strategy as before, just show the first results and start adding more later.
• The same can be aplied on reordering.

Problems:

• Since the view is always sorted in some way, we should sort the albums manually, so the progressive loading makes sense. -> Maybe we should keep multiple lists with the differents orders and the current dict for lookup. This introduces another problem that is keeping the lists updated.
• Find a suitable algorithm and parameters to make the user interact with the view without noticing the progressive loading.
• If we want to implement this as an optional feature, it may be hard. Maybe we can transform the AlbumLoader into an interface and make two implementations, the current one for those who prefer the full loading and another one for those (like me) that have problems with the preformance of the plugin.

[fossfreedom]

I cant test this myself since I dont have the same size collection (and my laptop is quite good :) ...), but can we do a "quick" test stripping out the data manipulations such as coverart_album "load cover" - how much does this speed up the initial display?

As you say - maybe then look at some-sort of implementation that loads the covers according to the viewport size - and then subsequently in the background.

[asermax]

While implementing some other features, I got to test the loading without adding the albums to the model and the it was way faster (let's say about 10 or 15 times faster) if we also remove the cover loading, I supect an initial, limited load would be almost instantaneous no matter the collection size. The big problem I see, as I said above, is the ordering. Anyway, let's leave this one for later, when the developing is more stable, since trying to solve this while at the same time adding more features will be a little hard. When we are done implementing most of the features we intend for the plugin, I can concentrate into this one at my own time d:

[jrbastien]

@asermax, I hope your exams went well. I don't know if you have some availabilities now but that would be nice, if the loading could be improved. In my library (over a NAS and with more than 500 albums), I have about 15 seconds of covers shuffling before everything gets stable.

Would it be possible to keep a local database and update the sort in the background. I'm wondering how it is done with Banshee? We don't see the cover being sorted in real time.

[asermax]

Hihi! Most of my exams went well, all it's left are a couple of presentations next week and I'll have some time to spare d: so wait a little more and I'll try to tackle this problem since it bothers me too lol.

We do keep all the album's info in memory in an structure that we can manipulate in the way you say, but the problem lays on the slowness of the widget we use to show the album covers. If we do a full sort and re-render the view, the whole ui freezes for a noticeable time. I have some ideas I'll test later this week to try to go around that problem tho. For now, the most interesting one is using a progressive loading technique (as stated on the title of this issue), that means to only load a couple of albums (enough to fill the viewport) and dinamically load the rest as the user scrolls down. IIRC that's the way banshee used to do it (sometime). That way we can do the background sorting and the re-renderind wouldn't take so long. It would take some mayor refactoring to achieve this tho, so we'll have to evaluate other posibilities as well. I'll report through here if I make some progress :3

[jrbastien]

Thank you!

[fossfreedom]

@asermax - some notes whilst looking at this - I'll add to this as I investigate further ( so it wont make much sense until I've finished :-) )

Note 1:

coverart_album.py", line 143, in _on_notify_cover_size
    Album.update_unknown_cover(self.cover_size)
  File "/home/dad/.local/share/rhythmbox/plugins/coverart_browser/coverart_album.py", line 1236, in update_unknown_cover
    cls.UNKNOWN_COVER.resize(cover_size)
AttributeError: 'Cover' object has no attribute 'resize'

When renaming the Cover.resize method this error was thrown. Looks like the resize method is being called despite the normal album loading method. I can only surmise that when the gsettings cover_size signal is connected, _on_notify_cover_size is called automatically - seems loading and reloading methods are being called on initial startup. Suggest this may cause unnecessary excess time.

Note 2:

commenting out this signal connect:

self.connect('notify::cover-size', self._on_notify_cover_size)

then ...

def __init__(self, size, file_path=None, pixbuf=None):
        '''
        Initialises a cover, creating it's pixbuf or adapting a given one.
        Either a file path or a pixbuf should be given to it's correct
        initialization.
        '''
        if pixbuf:
            self.original = pixbuf
            self.pixbuf = pixbuf.scale_simple(size, size,
                 GdkPixbuf.InterpType.BILINEAR)
        else:
            self.original = file_path
            #self.pixbuf = GdkPixbuf.Pixbuf.new_from_file_at_size(file_path,
            #    size, size)
            self.pixbuf = None

commenting out the second self.pixbuf assignment as shown increases the speed of loading - but more importantly I didnt see any shuffling.

I suppose one "quick win" could be to add yet another idle method called after the model is fully populated that then creates the album cover icons. Probably the cover loading should be done respecting the sort-order chosen - that way the "flicker" will not occur.

Note 3:

I posed this Q on stackoverflow - thoughts?

• http://stackoverflow.com/questions/13299858/what-signals-should-i-catch-when-scrolling-in-a-gtk-scrolledwindow

[asermax]

I'll check out what you mentioned when I start working on this later this week foss. Sadly, I don't remember pretty well how we structured the plugin, so it'll take me some time to re-read the code and get the hang of it again xD

[asermax]

Just a heads up, I'm working on this, but I'm taking my time. I'm currently refactoring the album module, dividing it into more classes to make it more easy to understand and modify. I'm thinking of using a structure with a main AlbumManager (that would replace the AlbumLoader) and a couple of other helpers of managers that will each take care of a different aspect of the managment (e.g. AlbumLoader -> loads the albums and takes care of new entries or removed entries, CoverManager -> takes care of loading covers, updating covers). It currently is very unstable and it's kinda worse than before xD but I'll see if I can fix it up and make it presentable (so at least it behaves as the master branch's structure). I was thinking of uploading the changes once I'm sure it works (I don't want to upload something i will throw away later), but if you want me to put it up so you can see the progress, let me know.

Once I'm done with the refactoring, I'll go into the loading details. Gonna take your notes into account, and that answer on stackoverflow seems exaclty what we need. I'm not sure about the proxy pattern tho, since what we would need to do is a Pixbuf proxy, since that's what the view works with, not the cover itself, and idk how hard it would be to implement. Gonna look into it anyway, it's a good idea.

[asermax]

Note to myself: connecting the row_changed signal to update_iconview_callback is a BAD idea. It seems like that was one of the primary causes of slowness while loading covers. There haves to be a better solution to update covers (or at least a better workaround).

[fossfreedom]

The answer on the SO question here looks interesting,

• http://stackoverflow.com/a/10301611/1626229

could help - thoughts?

[asermax]

When resizing the view there's no problem, we can call update_iconview_callback without problem, so the current solution for that doesn't need to change for now. The problem arises when using the row-changed signal from the cover_model to trigger the update_iconview_callback. When loading the model, the row-changed signal is throwed multiple times, and the view is updated for each album added, even if it's not necesary, adding a lot of dead time. I'm working on it, I'll let you know how it goes. I think the refactoring is coming up pretty nicely now, gonna upload it soon enough.

[asermax]

Ok, I think this thing is ready to be uploaded d: I'm still working on adapting all the old functionality to the structure of the album module, but the basic is there, and working over the new structure seems a lot easier than modifying the old gigantic AlbumLoader. Some highlights:

• Album loading is done in 3 phases: 1. Load the albums from the query model. 2. Build the albums and add them to the model. 3. Load all the covers, taking into account the order of the albums.
• Sorting and filtering are now done on python strcutures instead of the liststore. That way we have more control over the proccess, including the posibility to manage filters more tidily (e.g. use multiple filters).
• There are no more reference of the ListStore on the source, all interaction with it is done through the AlbumsModel object that the AlbumsManager creates. That way we can encapsulate a lot of things there without worrying on how it should be accesed from outside.
• I cleaned up the Album class and made it a lot more concise. Now it just takes care of those things that it definetely haves (no more searching covers or filling models with entries).
• I decided to create a class for the entries themselves and encapsulate the access to it's properties from the DB. That way there are no more entry.get_string(...) and stuff like that all over the place.
• Another thing related to the entries: this "micro managment" we were doing over the changes on an entry values was a really pain in the ass and looked horrible. I decided that now when an entry gets changed (doesn't matter what changes), a signal is emitted on the Track and the responsible Album gets notified and updates itself acordingly. At the same time, the album informs it got updated and the AlbumModel takes care of updating the model itself. This way all the parts are really decoupled and is easy to add new functionality to any part of the process.
• I didn't do the progressive cover loading thing just yet since with the changes and fixes I made is working a lot better for me. Neverthless, I left that posibility open and adding a new ShowingPolicy (a class that decide what should be shown, when and how) should be really easy.

I'll add anything else that I find important to the list as it comes to mind d: for now, you can check the code on the loading branch. If there's something you don't understand or think is misplaced or poorly designed, let me know.

[fossfreedom]

Looks good :+1:

Lets merge this - yes I know this will cause instability for a while (the stuff below), but myself/you/both can work on this over the next few days/weeks to iron out the wrinkles.

The stuff you've written above is excellent - when I'll get some time I'll add a developer wiki entry so that others can also understand the structures better - hopefully will encourage commits from others.

ok - a quick wiz through the functionality - please dont take this as criticism - just some issues that I'm sure can be quickly resolved - in no particular order:

1. error on start:

   Traceback (most recent call last):
    File "/usr/lib/python2.7/dist-packages/gi/overrides/Gtk.py", line 348, in _full_callback
      raise AttributeError('Handler %s not found' % handler_name)
   AttributeError: Handler genre_fill_combo not found
   

2. rating sort - adding an album or track rating doesnt resort immediately - takes effect after toggling ascending/descending button the reordering was working only when sorting by name, but that was just luck. I fixed it to work doesn't matter which sort mode is set
3. calculated year sort doesnt work I don't know exactly what to look for - looks like other changes has fixed this :)

4. right click and choose coverart menu option for playqueue/playlists/music source library

   Traceback (most recent call last):
    File "/home/dad/.local/share/rhythmbox/plugins/coverart_browser/coverart_browser.py", line 146, in display_covers_for_source
      self.source.reset_coverview(model)
   AttributeError: 'CoverArtBrowserSource' object has no attribute 'reset_coverview'
   

5. Was on rating with descending - then clicked on album. The sort was changed "by album name" but it was in ascending order ... in-fact clicking around on the sort orders, the ascending/descending seems pretty random
6. Filter by tracks seem to empty the coverart view - i.e. doesnt filter by track title
7. Filter by track artists seems to search by track title instead the filter uses the album artist. Maybe it's because I changed the startegy to determine the album artist... check it out and let me know what you think ... ok happy.
8. Having the toolbar on the left hand pane - shutdown RB and restart - toolbar doesnt reappear in left hand pane.
9. Favourites threshold - right click to add to new playlist - seems to add random tracks to the playlist. I think it's fixed - yes - great :)

10. Double click album to play:

    Traceback (most recent call last):
      File "/home/dad/.local/share/rhythmbox/plugins/coverart_browser/coverart_browser_source.py", line 787, in item_activated_callback
        self.play_selected_album()
      File "/home/dad/.local/share/rhythmbox/plugins/coverart_browser/coverart_browser_source.py", line 815, in play_selected_album
        play_queue.clear()
    AttributeError: '__main__.RBPlayQueueSource' object has no attribute 'clear'
    

[asermax]

Wow, that's a lot of bugs xD I knew there were still things unfinished but hadn't though it was this many. Ok, I think we should first fix all this problems and any other big issue we may encounter before merging it into master, I prefer to keep the inestability on this branch only for now. Some notes about the issues you mentioned:

• Fixed the signal error on start up, I had forgot to remove the signal from the sidebar ui file.
• I was thinking how to reimplement the playlist thing. Maybe in tabs? Using a 'playlist filter' would be nice too (it would work really good with the idea you propose on #102). Now that the AlbumsModel accepts multiple filters, this would be easy to do.
• The year thing seems to be working good for me.
• The ascending and descending is broken yup. I though I had nailed it but is not working right, gonna check it out.

The other issues I will investigate and fix, you can dive in as well. Gonna be posting the fixes over here!

EDIT: Oh btw, I removed a lot of debug strings, I was getting crazy looking for what I needed on the sea of prints. We should just keep some of them, the most importants and those that don't repeat constantly, otherwise it renders the whole log kinda useless :/ And the whole module needs new docstrings xD

EDIT 2: Gonna mark down those issues I think that are resolved on your post!

[asermax]

Sorry foss, but that's what I didn't want to do with the playlists :( cleaning and reloading are heavy procedures, at least on my collection. I think that we should use what's already there instead of recreating the structure (actually, the problem isn't when choosing a playlist to show in the coverview, but when going back to the base_model, since it haves to reload all the albums again). I took the liberty to make some changes and implement it as a filter (as I proposed before) and I think is working really well, with the exception that in this way, Tracks aren't filtered. I really don't have a problem with that, since the plugin is made to deal with albums, not individual tracks; but if it's a must-be, I can find another way to implement it or go back to the previous strategy.

[asermax]

Well, I think that's it, the branch seems a lot more stable now with lots of bugfixes. I'll clean up the settings (take out those options that doesn't make sense anymore, and merge the branch into master.

[jrbastien]

I don't know if it was ready yet but I updated my installation with the latest master branch and RB can no longer load the coverart plugin. Here are the logs from Ubuntu 12.10 32 bits:

jrbastien@jrbastien-Vostro-1500:~$ rhythmbox Traceback (most recent call last): File "/home/jrbastien/.local/share/rhythmbox/plugins/coverart_browser/coverart_browser.py", line 36, in from coverart_album_search import CoverAlbumSearch File "/home/jrbastien/.local/share/rhythmbox/plugins/coverart_browser/coverart_album_search.py", line 34, in import discogs_client as discogs File "/home/jrbastien/.local/share/rhythmbox/plugins/coverart_browser/discogs_client.py", line 4, in import requests ImportError: No module named requests

(rhythmbox:3562): libpeas-WARNING **: Error loading plugin 'coverart_browser'

(rhythmbox:3562): libdmapsharing-WARNING **: DACP browsing not started

(rhythmbox:3562): Gtk-CRITICAL **: gtk_builder_get_object: assertion `GTK_IS_BUILDER (builder)' failed

[asermax]

That problem isn't related with the changes we made here, but with the discogs album provider that foss is working on. You need to install python-requests to be able to use it (sudo apt-get install python-requests).

Btw, if you get to make it work, I would really apreciate your feedback on the overall behavior of this new approach. I'm liking it a lot more (more responsive, no shuffling), but I would like to hear what you think about it, since you probably will see things that I do not xD

[jrbastien]

Ok, thank you again for your prompt response. Installing python-requests fixed the issue.

My first assessment is yes, this is faster and the shuffling is gone. I will test further and let you know of any issue.

[jrbastien]

On Ubuntu 12.04 64 bits, I am less successful. RB does not crash but does not display any cover. I went to the usual procedure: downloading the master branch, installing and then commenting out the changed_Id string.

In the log I have an error with /dev/radio0 but that was working in the same session before the update so I suspect it was there also before.

(rhythmbox:17158): Rhythmbox-WARNING **: Could not open device /dev/radio0

Other then this, nothing else in the terminal.

[fossfreedom]

@jrbastien - rhythmbox v2.98 on ubuntu 12.04? i.e. the one from the webupd8 PPA? I despair with the webupd8 guys releasing that for 12.04 - it just doesnt work with python plugins!

You see the plugin name in the side-bar but on clicking - just an empty screen?

If you switch on in the preferences to display the cover text below the cover icon - do you then see your albums text but with no covers icons?

[jrbastien]

Yes, ubuntu 12.04 64 bits with RB v2.98 from webupd8 PPA. And yes, the plugin name is there but the album pane is empty. Switching the text under the album ON and OFF has no effect. Still nothing.

I did a ppa-purge and rollbacked to RB 2.96 and the problem persists.

[fossfreedom]

I found that ppa-purge doesnt actually remove all the offending bits of v2.98 :(

I wrote this up on Ask Ubuntu. Can you try this?

http://askubuntu.com/questions/201093/how-do-i-downgrade-rhythmbox-v2-98

[jrbastien]

OK, I have followed your procedure that you have given on Askubunut.com. Still no luck on 12.04 with now RB 2.96. Notice that if I revert to the release 0.6, it is working fine.

[fossfreedom]

@jrbastien - then I'm stumped - @asermax any ideas?

@jrbastien - please can you paste.ubuntu.com a link with the output of

rhythmbox -D coverart

Lets see if there is anything obvious in the debug statements in the code.

[jrbastien]

I'm currently building a fresh Virtual Machine to make sure I'm not making you loosing time. I will send a pastebin if the problem is in this vm too.

[asermax]

Nup, I don't know why this can be happening. The only thing that comes to mind is the missing pixbuf-column property for the iconview on the ui file, but it doesn't seem to be the case on the master branch, so I'm out of ideas o-o

[jrbastien]

Well, I have to admit that the problem might be in my system. My virtual machine does not have this issue. It is either 3 things:

1 - My system is corrupted 2 - The plugin does not work on a 64 bits system 3 - Something needs to be cleaned up before upgrading from release 0.6 to this new version.

I have pasted the log here but I don't see any problem in it: http://paste.ubuntu.com/1426585/

[fossfreedom]

I think this should really be in a separate thread...

anyway

I'm using 12.04 64bits - so dont think 2) is correct re 3) - I've tested this on my 12.04 moving from 0.6 to 0.7 (development) - so again dont think so. 1) agree - but what?

ok - lets try some stuff.

• disable all plugins except coverart-browser

If that doesnt work then try the following

• fully clean up your previous installs

rm -rf ~/.local/share/rhythmbox/plugins/coverart_browser
nautilus ~/.local/share/rhythmbox/plugins

check through nautilus that there are no similar coverart-browser plugins installed.

Check if you have installed the deb package previously i.e.

sudo apt-get purge rhythmbox-plugin-coverart-browser
nautilus /usr/lib/rhythmbox/plugins/
nautilus /usr/share/rhythmbox/plugins

re-install the plugin

then reset gsettings

gsettings reset-recursively org.gnome.rhythmbox.plugins.coverart_browser

• If the above doesnt work, then lets look at potential corruption in the rhythmbox cache

rm -rf ~/.cache/rhythmbox

This will remove all cached covers & ratings - so you'll need to reload this afterwards and redo ratings.

• if this doesnt work then try removing both the cache above and both xml files in ~/.local/share/rhythmbox/ - Note this will effectively reset rhythmbox - you'll need to reimport your music again

[jrbastien]

OK thanks a lot for the extensive support. Resetting gsettings fixed my issue. I could not make it work until I got to that step.

Actually, if I simply re-install, the issue re-appear. I definitely needs to reset gsettings every time to have it working.

[fossfreedom]

@jrbastien - thanks for the feedback. I've created a wiki page with the above so at least this is now more visible.

I'll create an issue to remember to add to the install routine a gsettings reset procedure.