Skip to content

Image VFS support for JSON-RPC #958

Merged
merged 14 commits into from May 14, 2012

2 participants

@jmarshallnz
Team Kodi member

This adds support for trivial image:// wrapped URLs suitable for JSON-RPC use.

In CFile::Open() we check whether the texturecache has the image, and cache if not. Then we fall through to the cached version of the image.

This allows us to set image:// URLs in JSON-RPC, so that images are cached only if they're actually fetched by the client, not just if they request items that may have art that is not cached. Currently clients retrieve art via the VFS, so setting an image:// URL allows backward compatibility until such a time as JSON-RPC supports a direct image retrieval handler, whereby that handler can just use the VFS internally.

A couple of things to discuss:

  1. Currently for JSON-RPC we set only art that has been previously discovered (i.e. is in the database). Eden users updating to nightlies will therefore need to view the images in XBMC before they'll be available over JSON-RPC. We could change this by running the backward compatibility code via the thumbloader, however this is very slow for large numbers of items being retrieved.

  2. I've currently not fully implemented CFile::Stat() - in particular Stat() will fail if the image has not yet been cached. IMO this isn't an issue as it's unlikely that we'll be calling this. I could fall through to Stat() on the original image, but this ofcourse will give inaccurate information (size for example).

I consider this a fix rather than a feature (currently no art is available for video items via JSON-RPC).

@jmarshallnz
Team Kodi member

Slight issue with this in that microhttpd urldecodes the URLs sent to GET, thus our image:// get decoded and thus things fail.

Need to find a way to get the URl out of microhttpd without it decoding it first, or have a hacky workaround to urlencode it again in the vfs handler.

@Montellese
Team Kodi member

Looking good.

Not sure how to handle the first issue either. If we run the thumbloader when retrieving items through JSON-RPC it will slow things down a lot but it should only really slow things down on first retrieval right? I assume this problem only exists for people who upgrade with existing libraries?

Concerning the second issue we don't use Stat() in the webserver or any of its handlers so shouldn't be a problem.

I'll write an image handler for the webserver which will translate received URLs into an image://-based URL and dispatch them to this new image:// VFS handler. This way we can some day drop the webserver's VFS handler and by that close a huge security whole.

@jmarshallnz
Team Kodi member

I'll try factoring out the back-compat code from the thumbloader so that it only runs that bit of it (as we have video db items only, it'll only do it once) and see how slow it is.

If we want backcompat for old web clients, we'll need the urldecode issue fixed (or worked around).

@Montellese
Team Kodi member

I just looked at the code of the webinterface and it actually never url-encodes the VFS URLs it passes to http://localhost/vfs/ so that's why it doesn't work for now. By adding a simple encodeURI() around the paths retrieved from "fanart" and "thumbnail" it works perfectly fine, i.e. if JSON-RPC returns image://http%3a%2f%2fcf2%2eimgobject%2ecom%2ft%2fp%2foriginal%2ftkvBU9bwUIHv4MOc3BZuTfzKt8t%2ejpg in a "thumbnail" property, the webinterface will take that URL, URL-encode it (again) and will call http://localhost/vfs/image%3A%2F%2Fhttp%253a%252f%252fcf2%252eimgobject%252ecom%252ft%252fp%252foriginal%252ftkvBU9bwUIHv4MOc3BZuTfzKt8t%252ejpg

IMO that would be the correct behaviour because calling http://localhost/vfs/image://... is not right. It sounds and looks a bit odd to url-encode something that is already partly url-encoded but that's how it should be IMO.

When looking at how to implement the webservers image handler (which forwards to the VFS's CImageFile) I ran into the following problem which I would like to discuss:
JSON-RPC returns URLs of the form "image://" but a call to the image handler would already look like http://localhost/image/ so having the "image://" in there would be kind of duplicate (.../image/image://...). But not expecting "image://" would mean that clients would have to remove the "image://" part from the retrieved URLs which is not really user-friendly either. Any ideas?

@Montellese
Team Kodi member

I also noticed something else. When retrieving artwork for music (which is not yet integrated into the texture cache etc) the URLs for fanarts look like "special://....tbn" whereas the URLs for thumbnails look like "image://special%3a%2f%2f...tbn". So either one of those is wrong and if we don't want to break backwards-compatibility (until music artwork is integrated into the texture cache) the thumbnail's one looks wrong to me.

@jmarshallnz
Team Kodi member

Ah - that makes sense. Yes, agreed that to use the VFS it makes sense for the client to URL encode - I guess they got away with it till now as the URLs weren't already partially URL eoncdoed.

As for the image handler, I think it makes sense if we have an ID so that http://localhost/image/ is quite natural. Our ID at the moment is image:// stuff, so I think it still makes sense - basically whatever is set in the art fields should be what is passed as the ID. It looks odd at the moment, sure, but once things are in their ID form it will be more straight-forward. Essentially this is an interim step to moving to IDs (as the ID thing wouldn't work for VFS back-compat).

Will check the music - we just wrap directly though there's no real need to for that - a simple fix I think.

@jmarshallnz
Team Kodi member

I guess we should check what other clients are doing. I presume that if the default web interface is doing things "incorrectly" that the others probably are as well, which would break backwards compatibility anyway...

@freezy what are you doing for the android remote to receive image URLs from thumbs - are you URL encoding prior to appending to http://ip/vfs/ ?

@Montellese
Team Kodi member

For me this is the same as the invalid Content-Type header for JSON-RPC request. Sending a JSON-RPC request with a HTTP Content-Type set to "multipart/formdata" is simply wrong and you have to live with the consequences if the server changes the behaviour to follow the specification more strictly.

I have a fix for the webinterface's missing encodeURI calls and I have the webserver's image handler ready to be able to handle image://- and special://-based URLs. The latter currently doesn't work because the thumbnails are returned with image://special%3a%2f%2f (as mentioned above). Should I wait for your fix or send the pull request "blindly"?

@Montellese
Team Kodi member

Stupid brain of mine waits till I'm in bed to remind me that I forgot to adjust the Files.PrepareDownload implementation to point to the new image handler. Will do after some sleep and breakfast ;-)

@jmarshallnz
Team Kodi member

Merged Montellese' image:// handler. Now works fine in Global Search addon and over the web interface, and the back-compat is working fine.

One last thing to think about is whether or not we want to move to an ID based system, and if so, how to best do it. We need a URL, or at least a way for scripts that will be displaying art in the UI to transform anything set in the thumbnail or fanart properties to URLs. So question is whether we set the actual URL or the image:// URL or an ID derived from there. eg I could utilise the id key in the textures database for that, though some refactor would be needed to ensure that it would work - basically we'd need to add textures to the db before they're cached, and then have update routines rather than delete/add, which is slightly messier code-wise. Alternatively we could have an in-memory map to do the same thing if we don't care about volatility.

Jonathan Mar... and others added some commits May 12, 2012
@Montellese
Team Kodi member

Good to go from my side.

@jmarshallnz
Team Kodi member

@cptspiff you OK with this one going in? It's quite large for a fix, but better do it properly then hack something in.

@ghost
ghost commented May 14, 2012

haven't reviewed by in pricinple it's a refinement/fix so yes.

@jmarshallnz
Team Kodi member

Righto

@jmarshallnz jmarshallnz was assigned May 14, 2012
@jmarshallnz jmarshallnz merged commit 1e16f6f into xbmc:master May 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.