[Tolriq]

I can confirm that the patch gets better perfs but there's still some things strange with albums and song without thumb.

Will try to have a full trace when I got time but my previous comment is still there.

It seems there's case where the art table does not fill itself on scan and will rescan every time. And since for songs that don't have thumbs it failback to try albums this mades 2 request / song + 1 file access on every request.
With the last PR this is still 4 file open/close less but it's still a lot and the code is hard for me to fix but I'm sure there's something to fix about this

[Koying]

1. The separate queries for art.
[...]
The first can't really be - after all, art is a 1:many map, so you can't retrieve it all in one go.

But can there be multiple "thumb"'s for 1 item?
Because, mainly, the performance issue is because the remotes want to retrieve the thumb of a list of items.

If there is only 1 thumb (which seems to be the case, unless mistaken), we add it to the views and performance is great in this case.

Not really multiple thumbs but multiple pieces of artwork. Currently we only have thumbnail and fanart which is why they are directly supported in the JSON-RPC calls but in the future xbmc will support clearart, logos, and all that stuff and we don't plan to add a single field for every one of those into the database view so while this approach might work right now it will utterly fail in the future.

IMHO, problem lies in the design of the "art" table, which is actually multiple tables merged in one.
It is, at the same time:
- a ThumbUrl and a FanartUrl table (differentiated with the "type" field)
- the joining tables between above tables and the entities tables (differentiated with the "media_type" field).

In actuality, we have a "1-to-[0..1]" relationship between, e.g. the "movie" table and the implied "ThumbUrl" table.
For performance and resource reasons, standard database designs ask for denormalizing (i.e. flattening) [0..1] relationship in the corresponding entities, so we should have a "thumbUrl" field in each corresponding tables.

Same goes fo fanarts, cleararts, logos, ..., as long as there is a "1-to-[0..1]" relationship.

If there are other reasons for having the "art" table the way it is that I don't know, at least having those "thumbUrl", ... fields in the DB views seems completely valid to me.

Some numbers from my database:

Code:

mysql> select count(*), art.url from movie left join art on movie.idMovie = art.media_id and art.media_type="movie" and art.type="thumb" where art.url IS NULL;
+----------+------+
| count(*) | url  |
+----------+------+
|        0 | NULL |
+----------+------+

=> Every movie has a thumb

Code:

mysql> select idMovie, cntUrl from (select movie.idMovie, count(*) as cntUrl from movie left join art on movie.idMovie = art.media_id and art.media_type="movie" and art.type="thumb" group by movie.idMovie) as T where cntUrl > 1;
Empty set (0.02 sec)

=> Only 1 thumb per movie

Code:

mysql> select count(*) from art left join movie on art.media_id = movie.idMovie where art.media_type="movie" and art.type="thumb" and movie.idMovie IS NULL;                        
+----------+
| count(*) |
+----------+
|      109 |
+----------+

=> Dangling "thumb" records, no referencial integrity => waste of resources

[Montellese]

Actually it has become quite common to have a table with the main entities and then a table with arbitrary properties which are then linked to the main entities with a foreign key (and if there are multiple different types of main entities with a field defining the type) because it allows for a much more flexible system i.e. you don't have to change the SQL layout everytime a new property has to be added. On the other hand it will result in a lot more JOIN queries but that's the price you have to pay if you want to be flexible.

In the future we don't want to dictate scrapers which fields to get and which not. Ideally there'll be a set of well-defined properties but it will also be possible to add additional properties without having to change the code or the database layout. Artwork is a perfect example for this. Banners, posters, landscape, clerart etc have all been there more or less poorly hacked into XBMC through skins (but every skin has to do it by itself) just because the design of XBMC does not allow for easy extension of the existing schema. Everytime a new type of artwork comes to live XBMC would have to change the database layout, the code that stores and retrieves the data in/from the database and the way those fields are exposed to "clients" like skins, python addons, JSON-RPC clients etc. When using a single art table with an arbitrary "type" it is possible to retrieve all the artwork belonging to a single item with the same logic and it doesn't have to be changed everytime a new type of artwork surfaces.

Obviously there seems to be a problem with re-scanning artwork for already scanned songs etc which needs to be addressed but IMO going down the route of adding a new column to every main entity table (movie, tvshow, episode, musicvideo, ...) everytime a new type of artwork surfaces is the wrong way to go.

[Tolriq]

While I do endorse the global point of view of Art table and choices that have been made, I indeed think that some times best practices and rationalization comes to way too high price for some things.

While the global concept is OK, the main point of Xbmc is to have at least one thumb per item to have a beautiful display in the interface. And since this is the basis of the app near all addons / remotes and system that get the data in batch will query for this thumb that is a total part of the item. (Since even when not present you add a default one). Even if you don't force scrapers to get all data you will get a thumb or find a way to generate one from one of the other art.

In the mean time new needs have surfaced for more arts that skins have hacked and Xbmc tries to get into (cdart, ....), and the way that was chosen is good, but it the cost that is associated with it is way too much for the thumb part.

I do agree that Xbmc should not add a field for each art but for me thumb is so tied to the item that it should been included and this is the only one, even fanart are not so much tied to item and not mandatory.

My English is bad but I suppose the idea is here Best is the worst enemy of good, this is an often seen problem in my job even if nothing to do with databases optimizations :p

[Koying]

I can understand the reasoning, but I can hardly understand that this vision degrades performances to the point of being unusable.

Once again, the problem we have is limited to thumbnails in lists.

I can hardly see the point of fetching the fanarts, cleararts, logos, ... in lists of e.g. movies. Those should be limited to "Details" fetching.
OTOH, thumbnails are here forever, will be requested forever in basically 99% of all item lists fetched from the DB and basically have a 1-to-1 relationship with their associated entities.

So, if adding a "thumbUrl" field to the tables is not acceptable (which I disagree with, again specifically for thumbnails), adding the JOIN to the thumbnails in the DB views once and for all will keep the vision while keeping performances. No need to add the other, "details" art, to the views.

---

In the mean time new needs have surfaced for more arts that skins have hacked and Xbmc tries to get into (cdart, ....), and the way that was chosen is good, but it the cost that is associated with it is way too much for the thumb part.

Hehe, we are synchronized, here

[Montellese]

Ah seems like I misunderstood you. I thought you proposed to add a new field/column for every artwork type. IMO thumbnail and fanart have a special position among all the different types of artwork because they are the ones used and usable in most views/situations. I'll talk to jmarshall about adding those two to the respective views.

[Tolriq]

I'm playing with File.GetDirectory with addons and databases path since some users asked me to implement the trick Millencolin007 found.

But it seems there's some problem with the return type value for lot's of items.

For example :

Code:

{"id":1,"jsonrpc":"2.0","method":"Files.GetDirectory","params":{"media":"video","directory":"videodb:\/\/3\/3\/","sort":{"order":"ascending","method":"label"},"properties":["title","thumbnail","fanart","rating","genre","artist","track","season","episode","year","duration","album","showtitle","playcount","file"]}}

returns

Code:

{"id":1,"jsonrpc":"2.0","result":{"files":[{"album":"","artist":[],"episode":-1,"fanart":"","file":"videodb://3/3/2011/","filetype":"directory","genre":[],"label":"2011","playcount":0,"rating":0,"season":-1,"showtitle":"","thumbnail":"","title":"","track":-1,"type":"movie","year":0}],"limits":{"end":1,"start":0,"total":1}}}

As you can see the type sent is movie while this is a Year listing so should be a year value or the standard "unknown" value.

This is the same for actors / genres / countries / directors / ...

This would be cool if we can have the real type for those or a least the unknown to display correctly the returned data.

Remark :
Strangely the Years for Tv Shows are correctly tagged as unknown :

Code:

{"id":1,"jsonrpc":"2.0","method":"Files.GetDirectory","params":{"media":"video","directory":"videodb:\/\/2\/3\/","sort":{"order":"ascending","method":"label"},"properties":["title","thumbnail","fanart","rating","genre","artist","track","season","episode","year","duration","album","showtitle","playcount","file"]}}

returns

Code:

{"id":1,"jsonrpc":"2.0","result":{"files":[{"fanart":"","file":"videodb://2/3/2001/","filetype":"directory","label":"2001","thumbnail":"","title":"","type":"unknown"},{"fanart":"","file":"videodb://2/3/2004/","filetype":"directory","label":"2004","thumbnail":"","title":"","type":"unknown"},{"fanart":"","file":"videodb://2/3/2005/","filetype":"directory","label":"2005","thumbnail":"","title":"","type":"unknown"},{"fanart":"","file":"videodb://2/3/2010/","filetype":"directory","label":"2010","thumbnail":"","title":"","type":"unknown"},{"fanart":"","file":"videodb://2/3/2011/","filetype":"directory","label":"2011","thumbnail":"","title":"","type":"unknown"},{"fanart":"","file":"videodb://2/3/2012/","filetype":"directory","label":"2012","thumbnail":"","title":"","type":"unknown"}],"limits":{"end":6,"start":0,"total":6}}}

[Montellese]

The problem is that these paths are not meant for external access and JSON-RPC doesn't support year listings etc yet. I'm not sure why it falls back to "movie" for some paths and to "unknown" for others but keep in mind that you are using something that you are not meant to use

[Tolriq]

Well it's there and it's quite powerful and doesn't harm to use

A simple correction to send back unknown would be perfect Since it send back movie this mean I should switch all returned movie items to generic ones since I can't distinguish movies anymore

And since this is in a generic Files browsers this also impact all the rest :p

If I find the problem and submit a PR to send back unknow have it a chance to be accepted ?

[Montellese]

Sure just returning "unknown" properly is a valid fix but I won't do any changes to return "year" or something like that although with my latest work from GSoC it would probably be pretty easy. Might even be that I can use that to easier handle the returned type.

[Tolriq]

Oki cool I'll check on this if not out of my comp

But found another bug that is more impacting

Present in Eden and Frodo :

Code:

{"id":2,"jsonrpc":"2.0","method":"Player.GetProperties","params":{"playerid":0,"properties":["audiostreams","canseek","currentaudiostream","currentsubtitle","partymode","percentage","playlistid","position","repeat",
"shuffled","speed","subtitleenabled","subtitles","time","totaltime","type"]}}

Will return empty string (no json response) so not valid for internet radio streaming.

This is due to requesting the percentage value.
If not asking it works and since when it works the total time sent back is 0 I suspect a simple divide by zero error not handled

[Montellese]

All JSON-RPC does is ask the player to provide the proper percentage value (which uses the same code which should be available to skinners) so if something's broken there it's not in JSON-RPC code but somewhere else.

[Tolriq]

There's no error in Xbmc logs

22:51:18 T:2958019440 DEBUG: JSONRPC: Incoming request: [{"id":2,"jsonrpc":"2.0","method":"Player.GetProperties","params":{"playerid":0,"properties":["audiostreams","canseek","currentaudiostream","currentsubtitle","partymode","percentage","playlistid","position","repeat","shuffled","speed","subtitleenabled","subtitles","time","totaltime","type"]}},{"id":3,"jsonrpc":"2.0","method":"Player.GetItem","params":{"playerid":0,"properties":["album","albumartist","artist","director","episode","fanart","file","genre","plot","rating","season","showtitle","studio","imdbnumber","tagline","thumbnail","title","track","writer","year"]}}]
22:51:18 T:2958019440 DEBUG: JSONRPC: Calling player.getproperties
22:51:18 T:2958019440 DEBUG: JSONRPC: Calling player.getitem
22:51:18 T:2958019440 DEBUG: CFileCurl::GetMimeType - http://radio1-se.digitalgunfire.com:8000/ -> audio/mpeg

And the json answer is :

HTTP/1.1 200 OK
Content-Length: 0
Content-Type: application/json
Date: Wed, 26 Sep 2012 21:33:36 GMT

So perhaps there's still something to do on Json side to not send back a 200 result with a 0 content lenght ?

Edit during the read after post, there really no error since I do batch query and JSON does handle the second one seeing the player.getitem. So there's definitively something wrong at Json side.

[Montellese]

https://github.com/xbmc/xbmc/commit/69f4...a7ab272d36 should fix the bad "type" information.

[Montellese]

And the issue with the "percentage" property for music streams (i.e. no totaltime) should be fixed with https://github.com/xbmc/xbmc/commit/63b6...5ed221f094 (and as I said it's not really in the JSON-RPC code . It just didn't return a response because the JSON serializer can't handle infinity values).