Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't use rand() for choosing a random song for partymode #2046

Merged
merged 1 commit into from Feb 3, 2013

Conversation

jmarshallnz
Copy link
Contributor

Ideally for Frodo. rand() typically returns a number between 0..32767, so for larger databases doesn't hit every possible row.

@koying: please test mysql. I have a feeling it will be quite slow, but not sure. Easy to test - just start partymode then skip a song.

@Fneufneu
Copy link
Member

Fneufneu commented Jan 8, 2013

hum RANDOM() does not exist in MySQL
and i always heard ORDER BY RAND() have some perfs issues.

@jmarshallnz
Copy link
Contributor Author

Our db layer maps it to rand() for mysql.

@bobo1on1
Copy link
Member

bobo1on1 commented Jan 8, 2013

If it gets mapped to rand(), how does it improve anything?

@Montellese
Copy link
Member

@bobo1on1: That mapping only applies to the use of RANDOM() / RAND() in SQL not in C++.

@jmarshallnz
Copy link
Contributor Author

@koying ping :)

@koying
Copy link
Contributor

koying commented Feb 1, 2013

Oups, sorry, slipped through...

Unfortunately, mysql uses "rand()", not "random()": http://dev.mysql.com/doc/refman/5.0/en/mathematical-functions.html#function_rand

@jmarshallnz
Copy link
Contributor Author

Read up :) - we already take care of that in the database layer. The question is whether this makes things unacceptably slow. Easiest test is partymode with a decent number of songs. The length to queue the next song (i.e. after the first song is over) in the list is of interest.

@koying
Copy link
Contributor

koying commented Feb 2, 2013

Ok, so:

  • Initial party mode is ok (default number of songs)
  • Doing first "next" brings an error message and I get this in the log:
11:17:55 T:8092   DEBUG: CMusicDatabase::GetRandomSong query = select * from songview  WHERE songview.idSong not in (6788,9063,9224,6602,9159,7857,7281,8978,7909,9602) ORDER BY RANDOM() LIMIT 1
11:17:55 T:8092   ERROR: SQL: Undefined MySQL error: Code (1305)
                                            Query: select * from songview  WHERE songview.idSong not in (6788,9063,9224,6602,9159,7857,7281,8978,7909,9602) ORDER BY RANDOM() LIMIT 1
11:17:55 T:8092   ERROR: CMusicDatabase::GetRandomSong(songview.idSong not in (6788,9063,9224,6602,9159,7857,7281,8978,7909,9602)) failed

but still works without noticeable delay

  • Further "next" works ok.

@jmarshallnz
Copy link
Contributor Author

Hmm, there doesn't seem to be anything wrong with the query, right? Mind running it in mysql directly and seeing if you get a bit more information from the error msg?

EDIT: Hmm, why isn't RANDOM() -> RAND() being detected...

@koying
Copy link
Contributor

koying commented Feb 2, 2013

Exactly ;) I didn't dig further, but I can if you want...

@jmarshallnz
Copy link
Contributor Author

Ah, I see the problem. The RANDOM() -> RAND() conversion is done only in vprepare, and we're using BuildSQL() which assumes pre-formatted.

@jmarshallnz
Copy link
Contributor Author

Ok, should work now :)

@koying
Copy link
Contributor

koying commented Feb 3, 2013

Yep. working fine and no slowdowns :)

jmarshallnz added a commit that referenced this pull request Feb 3, 2013
don't use rand() for choosing a random song for partymode
@jmarshallnz jmarshallnz merged commit 3756b57 into xbmc:master Feb 3, 2013
@jmarshallnz
Copy link
Contributor Author

Thanks :)

@veggiematts
Copy link

Hi !

I'm wondering if the proposed patch also fixes the issue for music party mode, or just for video party mode ?

@t-nelson
Copy link
Contributor

It's not proposed, it's merged. Try any build from the last 10mo and you'll know whether or not it works.

FYI. Commenting on github spams ~50 dev inboxs. Please keep support requests, like this, to the forums in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants