Bug 198381 - Race condition when fetching lyrics
Summary: Race condition when fetching lyrics
Status: RESOLVED LATER
Alias: None
Product: amarok
Classification: Applications
Component: Context View/Lyrics (show other bugs)
Version: 2.4-GIT
Platform: Gentoo Packages Unspecified
: NOR normal
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
: 211242 215958 222885 225852 234134 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-06-30 03:17 UTC by Elias Probst
Modified: 2012-12-04 09:49 UTC (History)
12 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Elias Probst 2009-06-30 03:17:02 UTC
Version:           2.1.1 (using KDE 4.2.4)
Installed from:    Gentoo Packages

I've seen several times the following happen:

- Track A is played
- The lyrics for track A are fetched (but not showed yet - the script is still fetching them...)
- I switch to track B before the lyrics of track A are shown
- Amarok fetches now + shows the lyrics for track B
- After some seconds Amarok finishes fetching the lyrics of track A
- Amarok shows now the lyrics of track A instead of the currently playing track B

This happens especially if you're on a slow internet connection - it's not really easy to reproduce but I think the problem in the code logic shouldn't be that hard to find.
Comment 1 Myriam Schweingruber 2009-06-30 19:42:32 UTC
Could this be related to bug 186021?
Comment 2 Myriam Schweingruber 2009-10-21 09:52:58 UTC
Hm, just had a duplicate with Amarok 2.2, changing version
Comment 3 Myriam Schweingruber 2009-10-21 09:53:32 UTC
*** Bug 211242 has been marked as a duplicate of this bug. ***
Comment 4 Myriam Schweingruber 2009-11-06 14:38:32 UTC
Leo, any news on this?
Comment 5 Myriam Schweingruber 2009-11-24 23:01:08 UTC
*** Bug 215958 has been marked as a duplicate of this bug. ***
Comment 6 Myriam Schweingruber 2009-12-17 16:15:51 UTC
Changing version from duplicate.
Comment 7 Myriam Schweingruber 2010-02-07 13:45:16 UTC
Is this still valid for Amarok 2.2.2 or in current git?
Comment 8 Claus Appel 2010-02-07 15:17:45 UTC
Yes. I updated from git a few days ago, and I can still confirm the problem. It is a bit hard to reproduce (you have to change track twice in rapid succession), but I have managed to reproduce it three times just now.
Comment 9 Myriam Schweingruber 2010-02-07 15:25:32 UTC
Thank you for the fast feedback, updating version.
Comment 10 Myriam Schweingruber 2010-02-08 00:31:28 UTC
*** Bug 225852 has been marked as a duplicate of this bug. ***
Comment 11 Sven Krohlas 2010-04-07 10:32:43 UTC
*** Bug 222885 has been marked as a duplicate of this bug. ***
Comment 12 Thomas Klausner 2010-04-07 10:44:59 UTC
I see this as well, on amarok-2.3.0.
Comment 13 Sven Krohlas 2010-04-12 09:38:24 UTC
*** Bug 234134 has been marked as a duplicate of this bug. ***
Comment 14 Jamie 2010-04-12 11:02:04 UTC
(In reply to comment #13)
> *** Bug 234134 has been marked as a duplicate of this bug. ***

Sorry, that dupe was mine. Didn't find this when I searched originally.

Just a note on reproducing the bug. As mentioned in my original bug, it's very easy to reproduce by writing a small script which calls qdbus org.kde.Amarok /TrackList AddTrack filename.mp3 multiple times in quick succession. Doing this causes all the lyrics to become mismatched to the wrong songs.
Comment 15 Martin Blumenstingl 2010-08-15 17:30:21 UTC
Hi,

the real issue is in the lyrics script:
the lyrics script calls Amarok.Lyrics.showLyrics() unconditionally (other Amarok.Lyrics.* may also be called unconditionally).

The problem is: Amarok.Lyrics.showLyrics() can not know the current track is still the same track for which the lyrics were fetched.
The safest way is probably removing Amarok.Lyrics.showLyrics() and using Amarok.Lyrics.setLyricsForTrack() instead (that method also takes the track's URL - so it's not affected by the described race condition).

Unfortunately there's a small issue:
The current code would not show the lyrics when just calling setLyricsForTrack.
I already fixed that in my "improve-lyrics-handling" branch on gitorious, but I cannot merge this to amarok's master (I have to wait until 2.3.2 is released).
My branch can be found here: http://gitorious.org/~xdarklight/amarok/xdarklights-clone/commits/improve-lyrics-handling

So here's a quick summary of what would have to be done in the lyrics script that is shipped with amarok:
-one has to remember the track's URL when getLyrics() is called
-onLyricsReceived() and onHelpReceived() need to know to which track that event belongs. If the event is not for the current track then Amarok.Lyrics.showLyricsError (or any other Amarok.Lyrics.* method) should not be called
-in any case: if lyrics were found Amarok.Lyrics.setLyricsForTrack() has to be called (only after my branch has been merged). The trackUrl has to be taken from step #1.

Regards,
Martin
Comment 16 Samuel Brack 2010-12-07 17:01:58 UTC
I could reproduce it under 2.4-GIT, too, so the bug isn't fixed so far.
Comment 17 Myriam Schweingruber 2010-12-07 20:00:49 UTC
bumping version.
Comment 18 Myriam Schweingruber 2012-06-23 16:10:43 UTC
Is this still reproducible with current git? I tried to reproduce it but maybe my internet connection is too fast.
Comment 19 Elias Probst 2012-06-24 16:16:45 UTC
I'm no Amarok user anymore, but reproducing this could be done by using trickle to simulate a slow internet connection.

trickle is available here: http://monkey.org/~marius/pages/?page=trickle

It could be used like this:
trickle -u 5 -d 5 amarok
to restrict the up-/download bandwidth of Amarok to 5kB/s.
Comment 20 Myriam Schweingruber 2012-10-21 09:13:55 UTC
Martin: do you still have that fix? Please submit it to reviewboard.kde.org
Comment 21 Martin Blumenstingl 2012-10-21 09:49:26 UTC
Hi Myriam,
I never wrote this code for the default lyrics script (as I was using the UltimateLyrics script back then instead of the default lyrics script we're talking about here).
I think the UltimateLyrics developer used my steps from above to implement a fix.

If anyone wants to take a look (I'm not using Amarok anymore, so I can't fix it):
- Amarok.Lyrics.showLyrics() only works for the current track (that is *playing* at the time the lyrics are received)
- if there is a short song where fetching lyrics takes a long time Amarok continues from track A (for which the lyrics were requested) to track B (which is now playing, where showLyrics is called).
- using Amarok.Lyrics.setLyricsForTrack() is a safe way, because it sets the lyrics for a specific track (of which you need to know the URL). The lyrics applet is automatically updated if setLyricsForTrack() is called for the "currently playing" track (that was the fix I mentioned back then).
Comment 22 Myriam Schweingruber 2012-12-04 09:49:34 UTC
Thank you for the feedback. I change this to LATER so we don't miss the information in it.