GNOME Bugzilla – Bug 706019
Don't use the discoverer if we don't care of the results
Last modified: 2013-09-01 10:08:33 UTC
We're not using the result of discovering the media files (as grilo tracker-extract already did the job for us when the url was included in the db), we might just as well avoid hamming the disk for every track loaded.
Created attachment 251647 [details] [review] Don't use the discoverer if we don't care of the results
I think this is used to show the error icon when the track cannot be played, i.e. there is a missing gstreamer plugin.
(In reply to comment #2) > I think this is used to show the error icon when the track cannot be played, > i.e. there is a missing gstreamer plugin. What other players do in this case is to show an error only when you attempt to play it. Discovering in advance can potentially cause a lot of disk/network access, and should be avoided (especially synchronous in the main loop!)
(In reply to comment #3) > What other players do in this case is to show an error only when you attempt to > play it. > Discovering in advance can potentially cause a lot of disk/network access, and > should be avoided (especially synchronous in the main loop!) I agree. Could you update the patch to remove more code? After applying the patch, the code still checks if the icon is changed to error icon (like the try-catch block in Songs view). This would further decrease the time needed to add items. Then I'll ask the maintainers if we could merge this one.
This was discussed in bug 699762. Personally I'd prefer to have these icons shown up every time, unless it causes performance issues. Discovering the URI in mainloop is quite slow now, I think we should move this to a separate funciton and call it in idle_add (In reply to comment #0) > We're not using the result of discovering the media files We actually are - in Python the exception is being thrown, so we set an error sign when it occurs. Potentially we should fall back to error when any kind of problem occurs with this track (malformed URL, unreachable service etc.) (In reply to comment #3) > Discovering in advance can potentially cause a lot of disk/network access, and > should be avoided (especially synchronous in the main loop!) Agreed, we shouldn't do this for remote sources. On the patch itself - we should also remove the 'except:' branch and error icon constant
(In reply to comment #5) > This was discussed in bug 699762. Personally I'd prefer to have these icons > shown up every time, unless it causes performance issues. Discovering the URI > in mainloop is quite slow now, I think we should move this to a separate > funciton and call it in idle_add Ehm... idle_add() is still the mainloop, you're just dropping a different frame :)
(In reply to comment #6) > Ehm... idle_add() is still the mainloop, you're just dropping a different frame > :) But at least it is delayed, because item loading in gnome-music is slow, especially in Albums view :D. This might need some priority changes, so that discoverer is run in low priority.
(In reply to comment #7) > (In reply to comment #6) > > Ehm... idle_add() is still the mainloop, you're just dropping a different frame > > :) > > But at least it is delayed, because item loading in gnome-music is slow, > especially in Albums view :D. This might need some priority changes, so that > discoverer is run in low priority. It won't change a single bit. At some point the idle fires (no matter how low the priority), GStreamer stats/opens/reads the file and at that moment the mainloop is blocked. If that takes more than ~10ms (which is likely, if there is other IO activity like loading the icons, accessing the tracker DB or simply something else in the system), you're dropping frames, and the app feels less responsive. idle_add() is *never* a solution to sync IO problems. In this case, the solution is either not discovering, or discover_uri_async() (which does all the IO in a different thread, and only reports the result to the mainloop)
Also, with the tons of idle_add()s you have, you keep layouting and drawing over and over, as every frame changes something in the scene, and Gtk doesn't really like that (especially the layout is quite expensive)
Created attachment 252131 [details] [review] view: don't discover URIs synchronously Doing IO in the main loop is always a bad idea. But we don't need to, because GStreamer can be made to do all the processing in a background thread and only report the final result. Ok, this is the (partial) alternative, if you want to the keep the discoverer around for the error icon.
Review of attachment 252131 [details] [review]: I applied 706027 and this to master but when I select an album in the Albums view, a segmentation fault usually happens, otherwise wrong album information is shown. Sometimes when I play a song in the Songs view, a segmentation fault happens after a few seconds. I think the problem is in 706027 though. Maybe this patch and the other one could be reversed, so that it would apply above this patch? Album art cache is undergoing rewrite, but maybe this one could be applied soon. I agree that using async here is much better than the current one. ::: gnomemusic/view.py @@ +189,3 @@ + else: + self._model.set(itr, [8, 10], + [self.nowPlayingIconName, False]) I think setting the icon name to nowPlayingIconName is unnecessary here because _add_item already sets this. @@ +213,3 @@ + [str(item.get_id()), '', title, + artist, self._symbolicIcon, item, + -1, icon_name, False, icon_name == self.errorIconName]) I think we no longer need to check for errorIconName here and could directly put nowPlayingIconName and False as values.
Created attachment 252142 [details] [review] view: don't discover URIs synchronously Doing IO in the main loop is always a bad idea. But we don't need to, because GStreamer can be made to do all the processing in a background thread and only report the final result.
Review of attachment 252142 [details] [review]: Tested it. No more segmentation faults, but the error icon do not show up in Albums view and Artists view.
Review of attachment 252142 [details] [review]: The same here - unplayable tracks are not marked with the error sign. Since its gstreamer who reports 'OK' on them, I think its a gstreamer+pygi problem
Created attachment 253706 [details] [review] view: don't discover URIs synchronously Doing IO in the main loop is always a bad idea. But we don't need to, because GStreamer can be made to do all the processing in a background thread and only report the final result. Found the problem: GstDiscoverer doesn't like mixing sync with async IO. Solved by centralizing all discover calls in Player instead of ViewContainer (so they can be used by AlbumWidget and ArtistAlbumWidget) and making them all async.
Review of attachment 253706 [details] [review]: ack
Attachment 253706 [details] pushed as 5fdc332 - view: don't discover URIs synchronously