Metadata #1569

Merged
merged 13 commits into from Oct 11, 2012

Projects

None yet

4 participants

@alcoheca
Contributor
alcoheca commented Oct 8, 2012

This is a load of commits which make the metadata available to upnp clients as rich as the inbuilt library views.

Fixes include: tv show season artwork, playcounts, resume points, watched status, correct director/actor info, missing dates.

I know it's late, but this was a total PITA to get my head round the season numbering and the lack of some metainfo in particular video db nodes.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 8, 2012
xbmc/filesystem/UPnPDirectory.cpp
@@ -449,6 +457,9 @@ bool CUPnPDirectory::GetResource(const CURL& path, CFileItem &item)
}
}
+ if (pItem->HasVideoInfoTag() && pItem->GetVideoInfoTag()->m_resumePoint.IsSet())
+ pItem->m_lStartOffset = STARTOFFSET_RESUME; // resume point set in the resume item, so force resume
+
@jmarshallnz
jmarshallnz Oct 8, 2012 Member

This shouldn't be required I think - i.e. the user should be prompted if the resume point is set?

@alcoheca
alcoheca Oct 8, 2012 Contributor

oh I thought it was, I'll check

On 8 October 2012 22:25, jmarshallnz notifications@github.com wrote:

In xbmc/filesystem/UPnPDirectory.cpp:

@@ -449,6 +457,9 @@ bool CUPnPDirectory::GetResource(const CURL& path, CFileItem &item)
}
}

  •        if (pItem->HasVideoInfoTag() && pItem->GetVideoInfoTag()->m_resumePoint.IsSet())
    
  •            pItem->m_lStartOffset = STARTOFFSET_RESUME; // resume point set in the resume item, so force resume
    

This shouldn't be required I think - i.e. the user should be prompted if
the resume point is set?


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1569/files#r1789224.

@alcoheca
alcoheca Oct 9, 2012 Contributor

yeah you're totally right - sorry

@jmarshallnz jmarshallnz and 3 others commented on an outdated diff Oct 8, 2012
xbmc/network/upnp/UPnPInternal.cpp
for (unsigned int index = 0; index < tag.m_writingCredits.size(); index++)
object.m_People.authors.Add(tag.m_writingCredits[index].c_str());
object.m_Description.description = tag.m_strTagLine;
object.m_Description.long_description = tag.m_strPlot;
- if (resource) resource->m_Duration = tag.m_streamDetails.GetVideoDuration();
- if (resource) resource->m_Resolution = NPT_String::FromInteger(tag.m_streamDetails.GetVideoWidth()) + "x" + NPT_String::FromInteger(tag.m_streamDetails.GetVideoHeight());
+ object.m_Description.rating = tag.m_strMPAARating;
+ object.m_MiscInfo.last_position = tag.m_resumePoint.timeInSeconds;
+ object.m_MiscInfo.last_time = tag.m_lastPlayed.GetAsDBDate();
+ object.m_MiscInfo.play_count = tag.m_playCount;
+ if (resource) {
+ if (!tag.HasStreamDetails()) {
+ CVideoDatabase vdb;
+ if (vdb.Open()) {
+ vdb.GetStreamDetails(tag);
+ }
+ }
@jmarshallnz
jmarshallnz Oct 8, 2012 Member

This is going to slow things down somewhat I suspect - any idea how slow it gets?

@elupus
elupus Oct 8, 2012 Member

Yea, the db must be cached somehow.

@alcoheca
alcoheca Oct 8, 2012 Contributor

I don't know how to improve this, as it stands the only way to get a file's duration is make this call. i.e. GetTvShow/MovieInfo doesn't populate m_strRuntime.

The biggest slow down is the fact file size is never saved in the db either, we stat for the file size currently.

@jmarshallnz
jmarshallnz Oct 8, 2012 Member

IMO file size is kinda useless - unless there's a bunch of clients that rely on it, I'd drop that.

@alcoheca
alcoheca Oct 8, 2012 Contributor

Sounds fair. As to clients relying on this, I'm not aware of any, but I've not been able to test this beyond a few linux clients.

@Montellese
Montellese Oct 9, 2012 Member

This is a general problem in xbmc. In JSON-RPC I also have to manually retrieve the stream details but there I have the advantage that I only have to do it if requested by the client. Will this metadata be cached by upnp until something changes or is it retrieved everytime a user goes into his video listing?

@alcoheca
alcoheca Oct 9, 2012 Contributor

it's cached by platinum. I'd be much happier if this sort of thing was handled by the VideoDb. Is it a bad an idea to call GetStreamDetails during a directory fetch?

@Montellese
Montellese Oct 9, 2012 Member

The problem is that the number of SQL queries during a directory fetch jumps from 1 to 1 + n where n is the number of items retrieved. If you retrieve 1000+ movies you'll end up with 1001 SQL queries instead of 1 which obviously results in quite a noticeable slowdown.

@alcoheca
alcoheca Oct 9, 2012 Contributor

Yep, so what do you think is the solution? Populate the StreamDetails during the directory fetch? Is there a reason why this isn't happening right now?

@Montellese
Montellese Oct 9, 2012 Member

Based on my last comment and description of the problem it should be obvious that retrieving streamdetails during a directory fetch is out of the question. IMO right now the way you've done it is the only real way to go although it is anything but ideal but ideal doesn't exist in this area right now.

@alcoheca
alcoheca Oct 9, 2012 Contributor

I was thinking about a grabbing the first item (video stream) from a JOIN with streamdetails where idfile=idfile etc, but probably the best option is to wait until the db can return the duration of a file without any extra lookups.

It's pretty weird that duration and filesize are unavailable from the db, maybe in the future we can add those during the scan?

After switching to using ThumbLoader for thumbs/fanart the speedup is incredible, so I'm going to ditch this call and be done with it.

@jmarshallnz jmarshallnz and 2 others commented on an outdated diff Oct 8, 2012
xbmc/network/upnp/UPnPInternal.cpp
+ }
+
+ // determine the correct artwork for this item
+ if (item.IsMusicDb()) {
+ if (!mdb.Open() || !item.HasMusicInfoTag()) goto failure;
+ thumb = mdb.GetArtForItem(item.GetMusicInfoTag()->GetAlbumId(), "album", "thumb");
+ fanart = mdb.GetArtistArtForItem(item.GetMusicInfoTag()->GetDatabaseId(), "song", "fanart");
+ } else if (item.IsVideoDb()) {
+ if (!vdb.Open() || !item.HasVideoInfoTag()) goto failure;
+ if (item.GetVideoInfoTag()->m_iEpisode >= 0) {
+ thumb = vdb.GetArtForItem(item.GetVideoInfoTag()->m_iDbId, "episode", "thumb");
+ fanart = vdb.GetArtForItem(item.GetVideoInfoTag()->m_iIdShow, "tvshow", "fanart");
+ } else {
+ thumb = vdb.GetArtForItem(item.GetVideoInfoTag()->m_iDbId, "movie", "thumb");
+ fanart = vdb.GetArtForItem(item.GetVideoInfoTag()->m_iDbId, "movie", "fanart");
+ }
@jmarshallnz
jmarshallnz Oct 8, 2012 Member

Hopefully @Montellese has some tricks to make this no longer necessary.

Note that you can just do GetArtForItem(item.GetVideoInfoTag()->m_iDbId, item.GetVideoInfoTag()->m_type) to get the art maps directly. With the exception of episodes + seasons (dunno if you play with them) where the fanart needs to be fetched from the show. The advantage is that this will also give you other art types, should they be available.

A nicer alternative might just be to call the FillLibraryArt() function from the music and video thumbloaders. One thing to keep in mind is that you want to initiate the thumbloader via OnLoaderStart() to make sure it only opens the db once.

@alcoheca
alcoheca Oct 9, 2012 Contributor

we should have a chat about the best way to do this - as you say I need to use m_iIdshow to get fanart for episode & seasons. I don't know if using the thumbloader is viable, as it would be open/instantiated for the duration of someone browsing the listings (ie possibly hours)?

@jmarshallnz
jmarshallnz Oct 9, 2012 Member

Ignoring the problem that the function call is per-item (and thus you can't save the db open/close between items) you could:

CVideoThumbLoader loader;
loader.FillLibraryArt(item);

Same for the music art. It's still got a db open per item, but it's then unified so you make sure you get all the art.

If you can persist your CVideoThumbLoader members across calls (JSON-RPC does it by passing the loader into the function that formats stuff up), then you can save the db open/close by first executing CVideoThumbLoader::OnLoaderStart().

@alcoheca
alcoheca Oct 9, 2012 Contributor

ah ok, that's a much better option, I can pass in the loader without too much problem. will do tomorrow.

@Montellese
Montellese Oct 9, 2012 Member

You can take a look at 0dddb48#diff-2 for how I did it in JSON-RPC (FileItemHandler.cpp). It's not perfect but a lot better.

@alcoheca
alcoheca Oct 10, 2012 Contributor

fixed using ThumbLoader in latest push. there's an issue with ImageFile.cpp opening thumbs but hopefully that's an easy fix. listings are much faster now.

@jmarshallnz jmarshallnz and 2 others commented on an outdated diff Oct 8, 2012
xbmc/network/upnp/UPnPInternal.cpp
+ PLT_AlbumArtInfo art;
+ art.uri = upnp_server->BuildSafeResourceUri(
+ rooturi,
+ (*ips.GetFirstItem()).ToString(),
+ CTextureCache::GetWrappedImageURL(thumb));
+ // Set DLNA profileID by extension, defaulting to JPEG.
+ NPT_String ext = URIUtils::GetExtension(thumb).c_str();
+ if (strcmp(ext, ".png") == 0) {
+ art.dlna_profile = "PNG_TN";
+ } else {
+ art.dlna_profile = "JPEG_TN";
+ }
+ object->m_ExtraInfo.album_arts.Add(art);
+ }
+ if (!fanart.empty()) {
+ upnp_server->AddSafeResourceUri(object, rooturi, ips, fanart.c_str(), "xbmc.org:*:fanart:*");
@jmarshallnz
jmarshallnz Oct 8, 2012 Member

Should this one be wrapped in GetWrappedImageURL() ? Otherwise the URL may not be readable to the client (it could be local to the server).

@elupus
elupus Oct 8, 2012 Member

safe resource url is for the upnp server's http server. it then used internal filesystem of server for access. so unless i missunderstand, it should be fine.

@jmarshallnz
jmarshallnz Oct 8, 2012 Member

I think it still needs wrapping, as otherwise the server is going to fetch the URL directly rather than via the texture cache?

@alcoheca
alcoheca Oct 8, 2012 Contributor

sorry you're right, Id thought these were already wrapped for some reason

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 8, 2012
xbmc/network/upnp/UPnPServer.cpp
+ }
+
+ if (item->GetVideoInfoTag()->m_type == "season") {
+ // season tags don't contain all the info we need
+ CVideoInfoTag* season = item->GetVideoInfoTag();
+ CVideoInfoTag show;
+
+ VIDEODATABASEDIRECTORY::CQueryParams params;
+ VIDEODATABASEDIRECTORY::CDirectoryNode::GetDatabaseInfo((const char*)path, params);
+ CVideoDatabase db;
+ if (!db.Open() || params.GetTvShowId() < 0 ) return NULL;
+ db.GetTvShowInfo((const char*)path, show, params.GetTvShowId());
+ season->m_strPlot = show.m_strPlot;
+ season->m_firstAired = show.m_firstAired;
+ season->m_premiered = show.m_premiered;
+ }
@jmarshallnz
jmarshallnz Oct 8, 2012 Member

Ideally these would be hooked up so you didn't need the extra query per item (the items will all be the same, so at worse it's a single query for all of them, right?)

@alcoheca
alcoheca Oct 8, 2012 Contributor

Yeah that's right, this is called for every season of a tvshow. CUPnPServer::Build() is not currently well positioned for holding state between calls though.

Could the db call populate these instead in GetSeasonsNav perhaps? This would mean seasons could have different m_firstaired/premiered dates (I'm surprised they don't anyway).

@jmarshallnz
jmarshallnz Oct 8, 2012 Member

Yeah, that would be the best fix, as you could tack them straight onto the season fetch easily enough.

@alcoheca
alcoheca Oct 9, 2012 Contributor

thanks, I'll give that a crack.

@alcoheca
alcoheca Oct 9, 2012 Contributor

got that working, will be in the next push of this PR. I shouldn't be so wary of touching the DB code...

@elupus elupus and 1 other commented on an outdated diff Oct 8, 2012
xbmc/filesystem/UPnPDirectory.cpp
@@ -355,10 +355,18 @@ bool CUPnPDirectory::GetResource(const CURL& path, CFileItem &item)
}
#endif
+
+ // Browse and wait for result
+ PLT_MediaObjectListReference list;
+ NPT_Result res;
+ if (device->m_ModelName.Find("XBMC Media Center", 0, true) >= 0)
+ // we want all properties
+ res = upnp->m_MediaBrowser->BrowseSync(device, object_id, list, false, 0, 0, "");
+ else
+ res = upnp->m_MediaBrowser->BrowseSync(device, object_id, list);
+
@elupus
elupus Oct 8, 2012 Member

preferable not special cased to xbmc if it doesn't break anything.

@alcoheca
alcoheca Oct 8, 2012 Contributor

it won't break, just means more metadata flying over the wires. I'm also trying to find the original bug in platinum's filtering.

@alcoheca
alcoheca Oct 9, 2012 Contributor

now we grab all properties, regardless of server

@jmarshallnz
Member

This could probably go in as-is, or close to it once the simple fixes above are taken care of or commented on.

While the additional queries are troublesome, there's no easy way to solve them - some of the art ones can be eliminated with work that @Montellese has done, but not all of them (e.g. episode/season fanart needs to reference the tvshow node, though theoretically we could use the same mechanism to grab them at query time as well, though it'd be clunky). How many times does a full retrieval of the movies list occur on the server when browsing on the client? i.e. does the client cache the listings at all? If so, it might not be too bad?

IMO getting the functionality into Frodo is essential - this is one thing we do really badly (multi-client) and this is a nice way to get around that - the user could replace the video library nodes with a upnp node, and then have a "dumb" client that looks the same as the server (give or take).

@elupus elupus commented on the diff Oct 8, 2012
xbmc/network/upnp/UPnPInternal.cpp
container->m_Recorded.series_title = tag.m_strShowTitle;
- container->m_Recorded.episode_number = tag.m_iSeason * 100 + tag.m_iEpisode;
- container->m_Title = container->m_Recorded.series_title + " - " + container->m_Recorded.program_title;
+ container->m_Recorded.episode_number = tag.m_iEpisode;
@elupus
elupus Oct 8, 2012 Member

This is different from how episode_number is set in the fill from PopulateObjectFromTag.

@alcoheca
alcoheca Oct 8, 2012 Contributor

this is only for the season/tvshow containers, not the actual episodes. here episode_number means number of episodes. (PopulateObjectFromTag is not called on videodb folders).

@elupus elupus commented on the diff Oct 8, 2012
xbmc/network/upnp/UPnPRenderer.cpp
// prevent hackers from accessing files outside of our root
if ((filepath.Find("/..") >= 0) || (filepath.Find("\\..") >=0)) {
return NPT_FAILURE;
}
// open the file
- NPT_File file(filepath);
+ CStdString path = CURL::Decode((const char*) filepath);
+ NPT_File file(path.c_str());
@elupus
elupus Oct 8, 2012 Member

This looks weird. Doesn't query.GetField() url decode already? you can't just for the heck of it do it again as that breaks rars/zips and so on.

@alcoheca
alcoheca Oct 8, 2012 Contributor

I'll check, could be a mistake

@alcoheca
alcoheca Oct 9, 2012 Contributor

no it doesn't, they're still urlencoded after GetField().

FYI the url part of the imageurl is still urlencoded after CURL::Decode(filepath)

eg:
path = image://http%3a%2f%2ffanart.tv%2ffanart%2fmusic%2f66c662b6-6e2f-4930-8610-912e24c63ed1%2falbumcover%2f14010%2fhigh-voltage-4ec1a0bb2a96e.jpg

without this, then the image:// part is urlencoded, and we can't open the file.

@Montellese
Montellese Oct 9, 2012 Member

Shouldn't this be up to our VFS handler (ImageFile.h/cpp) to deal with? I don't know the context here but ideally you'd use the image:// URL as you get it (which includes the url-encoded path to the original source of the image but not the locally cached one) and let the VFS handle the lookup in the db to map the url-encoded path to the path pointing to the locally cached image.

@elupus
elupus Oct 9, 2012 Member

I checked source, you are right. The return of GetField is encoded. So it must be decoded once before use to get back to the original value.

@elupus
Member
elupus commented Oct 8, 2012

we should get this is this window yes, we can bug fix it during the month.

@alcoheca
Contributor
alcoheca commented Oct 8, 2012

@jmarshallnz Platinum caches all server results, and waits for a containerupdate event - which we do not send currently.

I have a branch I'm about to PR, where we send updates for containers when items are updated. It works well, though obviously a lot more data is flying across. But this is definitely needed, as currently you need to restart the client to see the updates.

UPNP's ContentDirectory:3 (we currently implement ContentDirectory:1) fixes the constant resending of metadata, by introducing a list of what has changed, rather than just which container. If this inefficient (but standards compliant) approach could go in for frodo, then I can then move onto implementing ContentDirectory:3 properly. (We need to be backwards compliant so it makes sense to do it this way).

@Montellese
Member

The work @jmarshallnz mentioned I'm doing in the artwork area is still WIP and needs more testing but it would eliminate the need to query artwork seperately (except for things like fanart for seasons/episodes etc but that might be solveable as well).

@alcoheca
Contributor
alcoheca commented Oct 9, 2012

@Montellese cheers, for now I'll try using the Thumbloaders. I think if you're working on improving this side of xbmc, then it should handle all cases, ie fanart for seasons/episodes, otherwise we'd still need to code weird segments like this all over the place.

@elupus elupus commented on an outdated diff Oct 10, 2012
xbmc/network/upnp/UPnPInternal.cpp
return object;
failure:
delete object;
+ if (single_loader) {
+ delete thumb_loader;
+ thumb_loader = NULL;
+ }
@elupus
elupus Oct 10, 2012 Member

shared_ptr would have avoided this.. i'll merge as is, but it would be nice to fix.

@elupus elupus commented on an outdated diff Oct 10, 2012
xbmc/network/upnp/UPnPServer.cpp
@@ -425,6 +444,8 @@ static NPT_String TranslateWMPObjectId(NPT_String id)
NPT_CHECK(action->SetArgumentValue("NumberReturned", NPT_String::FromInteger(count)));
NPT_CHECK(action->SetArgumentValue("TotalMatches", NPT_String::FromInteger(items.Size())));
NPT_CHECK(action->SetArgumentValue("UpdateId", "0"));
+ if (thumb_loader)
+ delete thumb_loader;
@elupus
elupus Oct 10, 2012 Member

This can leak.. NPT_CHECK can return early.. Must be a shared pointer.

@alcoheca
Contributor

Going in with the shared pointer fixes

@alcoheca alcoheca merged commit 967a050 into xbmc:master Oct 11, 2012
@elupus
Member
elupus commented Nov 3, 2012

Why did we use FillLibraryArt here and not LoadItem in the thumbloader? It
seems art doesn't show up until we have touched the item in gui currently.

@alcoheca
Contributor
alcoheca commented Nov 3, 2012

I was just following rpc and advice. I noticed it too, will change it.
On Nov 3, 2012 3:37 PM, "Joakim Plate" notifications@github.com wrote:

Why did we use FillLibraryArt here and not LoadItem in the thumbloader? It
seems art doesn't show up until we have touched the item in gui currently.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1569#issuecomment-10040506.

@elupus
Member
elupus commented Nov 3, 2012

It doesn't solve the issue completely thou. Album art doesn't show up if
you haven't visited that node in xbmc. Not been able to figure out why yet.

On Sat, Nov 3, 2012 at 4:50 PM, Alasdair Campbell
notifications@github.comwrote:

I was just following rpc and advice. I noticed it too, will change it.
On Nov 3, 2012 3:37 PM, "Joakim Plate" notifications@github.com wrote:

Why did we use FillLibraryArt here and not LoadItem in the thumbloader?
It
seems art doesn't show up until we have touched the item in gui
currently.


Reply to this email directly or view it on GitHub<
https://github.com/xbmc/xbmc/pull/1569#issuecomment-10040506>.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1569#issuecomment-10040642.

@jmarshallnz
Member

As they're library items, FillLibraryArt is all you need anyway.

Assuming that you're piping the art through CImageFile, it'll be cached as required.

@elupus
Member
elupus commented Nov 3, 2012

I suspect we are not. Currently no art show up on albums, unless I have
touched them with a the local system.
This is an upgraded music db if that make a difference?

@jmarshallnz
Member

Nope, shouldn't make a difference for music.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment