gnome-prototypes-team / gnome-music

Gnome Music
https://live.gnome.org/Music
Other
5 stars 13 forks source link

Album art cache rewrite #129

Closed kyoushuu closed 11 years ago

kyoushuu commented 11 years ago

NOTE THAT THIS IS NOT READY FOR MERGE YET This needs a lot of testing and feedback, so I created a pull request even if it's not yet ready.

This is supposed to fix the random crashes of GNOME Music because of nested methods. Then it ended up being a rewrite. I also merged lookup and resolving, because they are related anyway, and resolve is actually tried afterwards by users of lookup - having them separate only complicates things. If this proposal is accepted, I'll squash the related commits.

Unfortunately, this doesn't fully fix the problem - most of these random crashes are caused by grilo.tracker.resolve used to resolve the thumbnail url - but I don't know why this happens. Maybe, it's because albums are media boxes instead of audio objects of grilo? I'm still investigating it.

Also, I added a data parameter to the three affected methods, so that we could remove the other nested methods if necessary.

By the way, the first commit is kind of unrelated, but it is a small commit anyway, so I just added it here. Feel free to commit it separately if it's possible, so that it won't get delayed with the rest.

Error message for crashes fixed by this pull request:

(gnome-music:31519): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion 'G_IS_OBJECT (object)' failed

(gnome-music:31519): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion 'G_IS_OBJECT (object)' failed
Segmentation fault
vrutkovs commented 11 years ago

Awesome job! This PR fixes this spaghetti-code in album art, however I didn't test it, so I'll post comment here if it works for me.

I think we shall be able to investigate these random crashes, as GObject warnings are easier to investigate than random crashes :)

kyoushuu commented 11 years ago

Thanks! By the way, do you think I should move the lookup callbacks to LookupRequest too, just like in GetUriRequest? I think it's cleaner that way...

vrutkovs commented 11 years ago

Works okay for me, though I get random crashes on 'All Artists':

/opt/gnome/lib64/python3.3/site-packages/gi/overrides/Gtk.py:988: Warning: g_object_get_qdata: assertion 'G_IS_OBJECT (object)' failed
  for col_num, val in zip(columns, values):
/opt/gnome/lib64/python3.3/site-packages/gi/overrides/Gtk.py:988: Warning: g_object_set_qdata_full: assertion 'G_IS_OBJECT (object)' failed
  for col_num, val in zip(columns, values):
Segmentation fault (core dumped)
vrutkovs commented 11 years ago

Tested this, works nice, thanks!

kyoushuu commented 11 years ago

Media boxes are now skipped. This prevents loading and saving the media art unless we play - a possible work around would be to browse the box until we get a GrlMediaAudio, then extract the url from it.

Also, possible improvement would be to copy how tracker saves the media art (it saves album art in user's cache with album and artist keys, links it with just album key, then creates another copy to the cache on the file's location), and to load the media art from the location cache.

vrutkovs commented 11 years ago

Two major bugs:

Traceback (most recent call last):
  File "/opt/gnome/lib/python3.3/site-packages/gnomemusic/albumArtCache.py", line 113, in _on_resolve_ready
    self.get_from_uri(uri, self.artist, self.album, self.width, self.height,
AttributeError: 'LookupRequest' object has no attribute 'get_from_uri'
kyoushuu commented 11 years ago

Ah, must have been the move to LookupRequest of the other callbacks... Working on it...

kyoushuu commented 11 years ago

Fixed the second issue. As for the first one, we can't do anything for that I think - it is tracker who is extracting those media art, we only extract them if they are available online. To force tracker to re-extract, move the files to a different folder, then move them back.

kyoushuu commented 11 years ago

Reverted skipping of media boxes.