Upnp cache notify #1582

Merged
merged 4 commits into from Oct 11, 2012

Conversation

Projects
None yet
3 participants
Contributor

alcoheca commented Oct 9, 2012

This should hopefully go in to - as it stands we never tell clients that items have been updated. They must restart XBMC in order to get a refreshed listing.

This is a ContentDirectory:1 compliant (though not particularly efficient) way of handling this.

Everytime an item is updated and announced, we increment the container's updateid and set a modified bit. Then we update the ContainerUpdateIDs statevariable in Platinum. Clients are then notified.

ContentDirectory:3 has a better way to achieve this - only updating the actual object's metadata, not the entire container but that is a lot more work to achieve.

Users can disable this if they find it annoying (enabled by default).

We should look at the list of which containers are updated for particular events (ie VideoDb scan), MusicItem being updated, as currently every item is updated after a play event - and therefore the Songs / Movies container is updated - which is a performance nightmare. These are to be found in CUPnPServer::Announce and UPNP::audio_containers UPNP::video_containers. There is probably a happy medium to be found.

I'm also going to add a commit which adds a context menu entry (Refresh Listing) to UPNP listings, for the case where user has disabled announcements from XBMC. (NB: that as it stands, updates from other devices are respected).

@alcoheca alcoheca Platinum: allow some statevariables to reset to default value after s…
…ending completed (needed for ContainerUpdateIDs usage)
b1a28da

@Montellese Montellese and 1 other commented on an outdated diff Oct 10, 2012

xbmc/network/upnp/UPnPServer.cpp
+ if (type == AudioLibrary) {
+ for (size_t i = 0; i < sizeof(audio_containers)/sizeof(audio_containers[0]); i++)
+ UpdateContainer(audio_containers[i]);
+ }
+ else if (type == VideoLibrary) {
+ for (size_t i = 0; i < sizeof(video_containers)/sizeof(video_containers[0]); i++)
+ UpdateContainer(video_containers[i]);
+ }
+ else
+ return;
+ m_scanning = false;
+ PropagateUpdates();
+}
+
+/*----------------------------------------------------------------------
+| CUPnPServer::AddUpdate
@Montellese

Montellese Oct 10, 2012

Owner

Wrong method name. Should be UpdateContainer.

@alcoheca

alcoheca Oct 10, 2012

Contributor

fixed

@Montellese Montellese and 1 other commented on an outdated diff Oct 10, 2012

xbmc/video/VideoDatabase.cpp
@@ -2730,13 +2730,14 @@ void CVideoDatabase::DeleteMovie(const CStdString& strFilenameAndPath, bool bKee
m_pDS->exec(strSQL.c_str());
}
+ if (!bKeepId)
@Montellese

Montellese Oct 10, 2012

Owner

Why does this have to happen here? What if the call to CommitTransaction() fails and throws an exception? In that case the movie wouldn't have been removed but the announcement has been sent out.

EDIT: Same problem further down.

@alcoheca

alcoheca Oct 10, 2012

Contributor

It's because I need to grab the tvshowID from the passed removed episode db
ID. (so I can mark the tvshow as updated).

Same with a song, needing to grab the album ID.

See in CUPnPServer::Announce()

Actually removing one song, or episode is pretty unlikely, but there's no
other way for me to spot a deleted album/tvshow.

I could actually ditch handling Removes altogether and just monitor for
Adds?

On 10 October 2012 14:09, Sascha Montellese notifications@github.comwrote:

In xbmc/video/VideoDatabase.cpp:

@@ -2730,13 +2730,14 @@ void CVideoDatabase::DeleteMovie(const CStdString& strFilenameAndPath, bool bKee
m_pDS->exec(strSQL.c_str());
}

  • if (!bKeepId)

Why does this have to happen here? What if the call to CommitTransaction()
fails and throws an exception? In that case the movie wouldn't have been
removed but the announcement has been sent out.


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

@alcoheca

alcoheca Oct 10, 2012

Contributor

OR Instead we could pass the 'parent' id (which would be the album or tvshow) as a param to the Announcement ? That's all I need.

@Montellese

Montellese Oct 10, 2012

Owner

The problem is that everything we throw into the data object of an announcement is also sent out to JSON-RPC and a parentid is not part of the API specification right now. But it might be worth adding an extra object to every announcement where we can store internal data that should be ignored by JSON-RPC etc.

@alcoheca

alcoheca Oct 10, 2012

Contributor

yeah that could work - I realise I've jumped onto the json-rpc and started messing with it, but there's really no other way to get this sort of info.

My future plans for the remote libraies will need even tighter integration with the library, so eventually I'll stop vandalizing your code :)

@Montellese

Montellese Oct 10, 2012

Owner

Well the chance for a failed CommitTransaction() is very very small so IMO this change is acceptable but can you add a comment like

//TODO: move this below CommitTransaction() once UPnP doesn't really on this anymore

so we don't forget why it is there and that it would ideally be placed differently.

@alcoheca

alcoheca Oct 10, 2012

Contributor

Ah right, that's also a good approach. I'll go with that

On 10 October 2012 14:54, Sascha Montellese notifications@github.comwrote:

In xbmc/video/VideoDatabase.cpp:

@@ -2730,13 +2730,14 @@ void CVideoDatabase::DeleteMovie(const CStdString& strFilenameAndPath, bool bKee
m_pDS->exec(strSQL.c_str());
}

  • if (!bKeepId)

Well the chance for a failed CommitTransaction() is very very small so IMO
this change is acceptable but can you add a comment like

//TODO: move this below CommitTransaction() once UPnP doesn't really on this anymore

so we don't forget why it is there and that it would ideally be placed
differently.


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

Contributor

alcoheca commented Oct 11, 2012

@elupus what do you think about this going in?

Member

elupus commented Oct 11, 2012

Cosmetics. The indentation is 4 spaces in that file.

Style: NPT_CHECK with early returns should be used where possible to keep it in line with the other code.

Also while i looked at the code, there is a bad bug in NPT_String CUPnPServer::BuildSafeResourceUri. It uses a NPT_String as return type but uses a NPT_CHECK in there. That mean it can return some odd number formatted as string. It must use NPT_CHECK_LABEL.

Member

elupus commented Oct 11, 2012

Note.. that bug is probably a bug i introduced way back. so should be solved separate from this pull.

Member

elupus commented Oct 11, 2012

Also we should always try to keep changes to platinum internals as separate commits. Why is it needed btw?

Contributor

alcoheca commented Oct 11, 2012

There's no support for this type of statevariable which must clear after being evented. Another way of doing it would be to clear while adding another container_id/update_id pair after it had been evented, but there's no way to determine when that has happened either.

Contributor

alcoheca commented Oct 11, 2012

about the BuildSafeResouceURI bug, NPT_Map::Put() only ever returns NPT_SUCCESS so I can just remove that check.

Contributor

alcoheca commented Oct 11, 2012

updated according to your comments

Member

elupus commented Oct 11, 2012

Ok. Green button it is.

Contributor

alcoheca commented Oct 11, 2012

amazing

alcoheca merged commit 59ca873 into xbmc:master Oct 11, 2012

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