Created attachment 245826 [details] [review]
Patch

I tried creating a patch for this.

Review of attachment 245826 [details] [review]:

Very good start Arnel :D
Thanks for the amazing effort. Can you take care of the small comments I made?

::: src/albumArtCache.js
@@ +54,3 @@
 
+    try_load: function(size, artist, album, i, callback) {
+        var key, path, file;

CamelCase :P and I assume this is a private method so append an underscore ==> _tryLoad

@@ +58,2 @@
+        if (i >= this._keybuilder_funcs.length) {
+            callback (null);

No need to do a callback. Just ignore it and do a return

::: src/widgets.js
@@ +257,3 @@
+                            if (pixbuf == null)
+                                pixbuf = albumArtCache.makeDefaultIcon(256, 256);
+                            this.ui.get_object("cover").set_from_pixbuf (pixbuf);

You are waiting for the callback to set the default icon. The default icon should be set before we call lookup (since we know it exists and does not require lots of crawling)

Created attachment 245835 [details] [review]
Updated patch

(In reply to comment #2)
> Review of attachment 245826 [details] [review]:
> 
> Very good start Arnel :D
> Thanks for the amazing effort. Can you take care of the small comments I made?
> 
> ::: src/albumArtCache.js
> @@ +54,3 @@
> 
> +    try_load: function(size, artist, album, i, callback) {
> +        var key, path, file;
> 
> CamelCase :P and I assume this is a private method so append an underscore ==>
> _tryLoad

Fixed. Vala habit :D

> @@ +58,2 @@
> +        if (i >= this._keybuilder_funcs.length) {
> +            callback (null);
> 
> No need to do a callback. Just ignore it and do a return

Are you sure? The code in view.js checks only the cache then uses grilo to get the cover art if not found. Also, see below.

> ::: src/widgets.js
> @@ +257,3 @@
> +                            if (pixbuf == null)
> +                                pixbuf = albumArtCache.makeDefaultIcon(256,
> 256);
> +                            this.ui.get_object("cover").set_from_pixbuf
> (pixbuf);
> 
> You are waiting for the callback to set the default icon. The default icon
> should be set before we call lookup (since we know it exists and does not
> require lots of crawling)

I moved the default icon part to before the new is inserted to the model. It is then inserted as default value. After loading, only sets the icon if successful.

Is the scaling part alright? I didn't use async there because there is no async function for just scaling which would make reloading from the stream required, and I think streams could be used only once.

Created attachment 245836 [details] [review]
Updated patch: show default in toolbar too

Updated patch to show default icon in toolbar too.

Now only ArtistAlbumWidget._updateAlbumArt and ViewContainer._updateAlbumArt remains users of that callback (null).

Created attachment 245837 [details] [review]
Updated patch rebased against master

I think my patch needs further improvement, so don't push it yet unless you think the improvements should be or could be moved to a different patch.

My concern is with the default icons. Seems like they are still causing a small delay in the playback (especially the default image in the toolbar). I am still investigating which ones are causing these.

I think we should cache the default image. Since calling albumArtCache.makeDefaultIcon is not asynchronous. I think the small delay will be further minimized by doing so - we just call that function once, causing a small delay, then reuse it while adding tracks. Because we add the tracks after loading the default icon, I don't think making albumArtCache.makeDefaultIcon asynchronous is a good thing - it will add another layer of callbacks.

I can't easily say these into words, so I'll just show what the current situation is:

=== Views ===
Current situation in views:
- Look for tracks
  - Create default icon synchronously  <- This causes delay
  - Load the cover art synchronously   <- Causes a lot of delay

Situation in views after current patch:
- Look for tracks
  - Create default icon synchronously  <- This causes delay
  - Load the cover art asynchronously

Possible improvement for views:
- Create default icon synchronously    <- Causes delay, but smaller
- Look for tracks
  - Load cover art asynchronously

=== Toolbar ===
Current situation in toolbar:
- Load track
- Load cover art synchronously         <- Causes a lot of delay
- Set cover art                        <- Might be causing some delay
- Set state to playing

Situation in toolbar after current patch:
- Load track
- Load default icon                    <- Causes some delay
- Set cover art                        <- Might be causing some delay
- Load cover art asynchronously
- Set state to playing

Possible improvement for toolbar:
- Load default icon in _init           <- Causes delay, but smaller
- Load track
- Set cover art                        <- Might be causing some delay
- Load cover art asynchronously
- Set state to playing

Setting the cover art might be causing some delay, maybe make an idle function which sets the default icon and loads the cover art asynchronously? So that the state would be changed to playing immediately. (I already tried this, but there are still some delay, especially when the next song is shorter than the current one, the scrollbar jumps to start, to end then to start again. I'm still investigating it).

What do you think? Do you have a better idea?

Created attachment 245853 [details] [review]
Updated patch to "cache" default icon

Awesome, worked like charm for me, thanks!

The following fix has been pushed:
e579c23 Make loading of cover art asynchronous

Created attachment 245981 [details] [review]
Make loading of cover art asynchronous