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
[JSON]Faster access to music library #14160
Conversation
Had a look at the code and it seems sane. The music database cpp got bumped by over 1,500 lines. It actually overtook VideoDatabase.cpp as the longest file. Maybe it's time to split into separate files so it can better scale to your future work? |
Thanks for looking @garbear. The length of the .cpp file is not really the issue, for example some of those extra lines are just long comments to document the design. What matters is how clear it is what each method does and why it does it like it does. So hound me for more Doxygen comments I guess. In my view it is better to have all the specific SQL built in one place, than dotted about, it encourages common queries. And to be honest I am really not sure how I would split it. Despite the initial "that's a lot of lines", once you read through the module it is more about string concantentation than complex code. Meanwhile it seems I have to tidy up some warnings to get the cross platform builds happy. |
5b8871b
to
3f3d766
Compare
xbmc/dbwrappers/Database.h
Outdated
: fetch(fetch), | ||
output(output), | ||
recno(recno), | ||
strField() |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
xbmc/dbwrappers/Database.cpp
Outdated
{ | ||
DatasetFieldInfo field(false, false, -1); | ||
fields.emplace_back(field); | ||
} |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
xbmc/dbwrappers/Database.cpp
Outdated
const std::string CDatabase::DatasetLayout::GetFields() | ||
{ | ||
std::string strSQL; | ||
for (auto field : fields) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
xbmc/dbwrappers/Database.cpp
Outdated
|
||
bool CDatabase::DatasetLayout::HasFilterFields() | ||
{ | ||
for (auto field : fields) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
xbmc/dbwrappers/Database.h
Outdated
bool HasFilterFields(); | ||
|
||
private: | ||
std::vector<DatasetFieldInfo> fields; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
itemUrl.AppendPath(path); | ||
CThumbLoader *thumbLoader = NULL; | ||
thumbLoader = new CMusicThumbLoader(); | ||
if (thumbLoader != NULL) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
{ | ||
CThumbLoader *thumbLoader = NULL; | ||
thumbLoader = new CMusicThumbLoader(); | ||
if (thumbLoader != NULL) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
xbmc/music/MusicDatabase.cpp
Outdated
|
||
// Count number of artists that satisfy selection criteria | ||
//(includes xsp limits from filter, but not sort limits) | ||
total = (int)strtol(GetSingleValue("SELECT COUNT(1) FROM artist " + strSQLExtra, m_pDS).c_str(), NULL, 10); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
xbmc/music/MusicDatabase.cpp
Outdated
|
||
// Count number of albums that satisfy selection criteria | ||
//(includes xsp limits from filter, but not sort limits) | ||
total = (int)strtol(GetSingleValue("SELECT COUNT(1) FROM album " + strSQLExtra, m_pDS).c_str(), NULL, 10); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
// Fields fetched by GetArtistsByWhereJSON, order same as in JSONtoDBArtist | ||
static enum _JoinToArtistFields | ||
{ | ||
joinToArtist_isSong = 0, |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Unless anyone else wishes to review the code, or is going to do some more JSON testing I would like to merge this Thursday so that it can be part of alpha testing in the wild. |
@DaveTBlake there's a MySQL (possibly SQLite too) issue with the following JSON query:
In kodi.log:
Reformatting the query produces:
Running this query in MySQL Workbench produces the error:
and indeed, in the |
Thanks @MilhouseVH, while SQLite happily gets same field twice obviously MySQL does not. Fix imminent. |
8817cd0
to
c5e14e9
Compare
Tested in millhouse raspberry pi build and seems to work as expected with the iPhone remote as far as I can tell. Hard to judge the speed but any improvement is most welcome in that area! |
c5e14e9
to
d65a09c
Compare
https://github.com/DaveTBlake/xbmc/blob/c5e14e91bddc077250b0b8272162fdaf0deb1da9/xbmc/music/MusicDatabase.cpp#L5802 |
Thanks @DaveTBlake that now works (it doesn't crash) but it does have some other issues. If you run:
on every song - including those which are part of an album.
I have one song in my library which is by two This is the song details as returned by
(Same response - pretty format):
No issues here - one song, both artists are present (and the thumbnail!) However, with
(Same response - pretty format):
With this PR, the |
d65a09c
to
e174d29
Compare
Thanks for further tests @MilhouseVH 3 is as you surmised, songs with the same name not being correctly grouped or ordered. Took me a while to repeat it, and I want to do some more checks myself |
e174d29
to
09d26f3
Compare
Like to try one more time @MilhouseVH? I have fixed the issues you found and done quite a lot more testing myself (to ensure I didn't add regressions). |
Thanks, the duplicate song and albumid issues are resolved. However there's still an issue with thumbnails when calling Query:
Response before (ie. without PR):
Response after (with latest PR):
There's also some other differences (the before/after file sizes are not the same, even when taking the lack of thumbnails into account), so I need to look at that and determine what is causing the difference. |
I think this is the final set of differences, at least as far as (all 3 of the following issues are most likely one and the same)
Query:
Response before (without PR):
Reponse after (with latest PR):
Response after, when calling
Query:
Before:
After:
Response after, when calling
Comparing random artist, before (-) and after (+):
Again, The only source of "RJD2" appears to be the 1, 2 and 3 are likely all related. It now appears this PR, when calling However |
Thanks for further tests @MilhouseVH
GetArtists
|
6a846c0
to
b97ce38
Compare
…implemented to be many times faster. Minor patch bump despite a significant speed improvement and fix of several bugs
b97ce38
to
db1caa0
Compare
Thanks @DaveTBlake - your latest updates now produce 100% identical results when compared with a build without this PR, ie. LibreELEC build I know you're keen to merge this, it's probably the best way to shake out any remaining issues (hopefully there aren't any). Performance comparison using MySQL 5.5.21 on HP N36L
Old/New timings are in seconds, averaged over 3 runs each. JSON data retrieved iteratively in chunks of 400 items - timings are for the complete data set. Tested using The following JSON calls methods/properties/sort orders were used: AudioLibrary.GetArtists
AudioLibrary.GetAlbums:
AudioLibrary.GetSongs:
RPi1 (512MB RAM, 10MB/s ethernet):
RPi3+ (1GB RAM, 30MB/s ethernet):
Skylake i5 NUC (8GB RAM, 30MB/s 5GHz WiFi):
|
Rework the way
AudioLibrary.GetArtists
,GetAlbums
andGetSongs
are implemented to be many times faster.Looking at up to a 500% increase in speed for processing JSON requests, attained by avoiding the use of fileitem lists, making inefficient queries per row for additional data and by sorting at the db rather than in memory.
Just a minor patch bump to JSON API version despite being a significant speed improvement and fix of several bugs.