Local thumbnails are inaccessible when Kodi requires authentication

[christianprescott]

Some media thumbnails (movie posters, album art, etc.) have remote locations (e.g. http://image.tmdb.org/t/p/original/vDwphkloD7ToaDpKASAXGgHOclN.jpg) but some are served from a local path on the Kodi web server. These local images encode the path in the URL and specify an "image://" protocol. (e.g. http://server:port/image/image://%252fhome%252fchristian%252f%252fMusic%252fArctic%2520Monkeys%252fAM%252fFolder.jpg/)

Local thumbnails may become unavailable if Kodi is using authentication. The app uses HTTP Basic auth headers in RPC requests, but since no credentials are supplied when BitmapImage attempts to get the image the response is 401 Unauthorized and the image is not displayed.

[christianprescott]

No idea how to solve this one yet, still thinking. Some ideas:

• Implement auth for all requests made by BitmapImage. Perhaps BitmapImage can be extended so we can write our own request for the UriSource? How will it differentiate between local and remote images?
• Implement INotifyPropertyChanged across all thumbnail-able models. When thumbnail path is modified download the image via HttpRequestMessage with Authorization header and bind XAML to an ImageSource member of the model (instead of a URI member).

Low priority, though, I'm sure...

[akshay2000]

Thanks a lot for pointing this out. I don't know how it went unnoticed for so long!

Frankly speaking, analytics show that not many people are using the authentication with Kodi. Still, this is one glaring fault. I'll look into the whole architecture and then think about proper solution. I don't want to go with duct tape solution here.

[christianprescott]

Yep, understood.

I am using it, though, so I have incentive to sort this out the right way. :)

[akshay2000]

I could use a simple solution to format the URL into format http://username:password@host:port/path/to/resource that is accepted by most browsers. This works with Chrome and Firefox. But evidently, Internet Explorer (and consequently, the app's automatic image loading) doesn't support these kind of URLs.

[christianprescott]

Yes, I was disappointed to find that didn't work. It would have been so simple!

[akshay2000]

Moreover the solutions we try to implement might end up breaking lazy loading or background loading or image caching or all of it!

[akshay2000]

I've modified the ImagePathConverter. So now, all the images require authentication. Everything is loaded from the cache of Kodi server. No need to handle the URLs differently.

[akshay2000]

Another idea: In the converter, we generate a unique string from the URI. Pass this string to some external method that downloads and saves image locally using this string as file name. Converter returns URI to the local file (even before download is finished) and image is bound to it. This method returns file if it exists and is new, or redownloads if it's beyond the 'expirey date'. Not sure if this thing will work, but this approach has many advantages. No need to replace half the XAML of app. No need to reimplement the whole BitmapImage class. Most of the app remains unharmed and unaltered.

[christianprescott]

Good thinking. (+) Fewer changes to existing code (+) Option to to decode/resize once on cache write (-) Storage IO is slow (but I'm not sure exactly how slow) especially on cache miss, when download, write and read must occur. Maybe not any different from existing method, though. (-) App storage will grow significantly to cache resources. And if all resources are coming from server now, some of those are pretty large (e.g. fanart) (-) Building the image cache is not trivial - significant effort to develop (?) Is the value converter called lazily? If not, hundreds or thousands of cache/network requests will be fired at once. (?) Will BitmapImage load the image from storage when it's saved for the first time? If not we'll need a way to "refresh" UriSource

[christianprescott]

In other news: had time to run a few performance tests today on the ThumbnailImage implementation. Haven't been able to compare to BitmapImage yet, but it feels like performance isn't suffering.

[akshay2000]

1. Storage IO can't possibly be slower than the WiFi network. Secondly, native implementation of BitmapImage also maintains a cache of downloaded files and loads images from there. I read somewhere that this cache is same as the browser cache. So, I don't think we're doing anything differnet than that.
2. Size shouldn't be a problem. Doesn't matter if we download from Kodi server or the Internet. We get the exact same fanart. Only difference now is that insted of going to internet, we get image from local network. It's only faster. No difference in sizes.
3. No need to have a full fledged fancy cache. Generate some unique string by encoding the source URL. Check if file with that name exists. If it does, check if it's newer than 24 hours. If not, replace it with a new one. Otherwise, just return the original.
4. Doesn't current binding fire all the requests at once? If yes, it's working fine. If not, I don't see why our implementation should!
5. In my solution, image will always be loaded from the local storage. But we do not need to 'refresh' the URI. We bind to correct URI on the first go! Converter generates the local URI and passes it to the downloader. Downloader goes and stores the file in this local URI in background. Makes sense? If not, I'll try to make a flow chart.

[christianprescott]

Speed: I'm convinced Size: I understand the image size is the same, but now we're saving images to app storage and the user will see Kodi Assist's size grow and grow. In Storage Sense, my device indicates 237 KB of stored data. If we're saving all of the album art, movie posters, etc. it will grow very quickly into the 10s or 100s of megabytes, especially when some backgrounds are in the thousands of pixels wide. I'm just now reading about ApplicationData.TemporaryFolder which sounds like the solution. Cache: I suppose you're right, since we already have a path to the image it's easily turned into an encoded file name. Maybe necessary to actually create the folder structure if there is a file name length limit? Lazy: We wouldn't want all of our valueconverters to fire at once because we're not just converting a URI - we're doing all the network requests ourselves instead of letting BitmapImage do it. If, when you navigated to the All Music page for the first time, every album cover kicked off an HTTP request (followed by a write) the app would be swamped with async tasks. I think the converter is lazy loaded though, so it may not be an issue at all. Refresh: I understand, but when BitmapImage sees the local URI for the first time (while network is still in progress) it will find the file does not exist. After the download and storage write is complete, will BitmapImage see the file and load it at this time? If not, I'm sure we can work out a way to make BitmapImage attempt a second load. Obviously, this is only an issue for the first time in image is requested, but seeing no images the first time the app is launched is still bad experience.

Despite all the bullet points, I'm watering at the mouth for a slick BitmapImage solution and this sounds really good. :)

[christianprescott]

This is coming along and I like this solution better. There are still issues to resolve, but the big big big one is that you won't see any images the first time you open the app, when the cache is empty. The problem is exacerbated by the way Kodi Assist maintains the session even after backing out of the app.

[akshay2000]

I'm not gonna lie here. I was secretly hoping that image will somehow reload itself when the download is complete. It doesn't make sense, but I just thought it would. Here's another hacky solution I propose: Build image cache on the first handshake. When connection is selected on the MainPage, build the cache. If the images already exist, this will be over in no time. So no extra overhead.
Also, let's keep the behaviour for no authentication people as is. Let's use this solution only for the servers that require authentication.

[christianprescott]

Ageed, users who aren't using authentication shouldn't have to risk bugs in this bit of code.

Re building entire image cache: You have a better understanding of the Kodi remote abilities than me. Is it possible to download the whole image cache, or do we have to iterate every album, artist, movie, (and future items that could use proxied images) and check for their thumbnails. Is that practical?

[christianprescott]

More observations: when using ApplicationData.TemporaryFolder, the data stored there is reported by Storage Sense with other "Temporary Data" and not under Kodi Assist. I think that's a good thing, alleviates my concerns about reclaiming space and users skeptical of growing app storage. But the temp folder throws a wrench in the build image cache plan. If temp data is cleared, we'll have to build again, it won't be a one-time process. Building cache in app storage instead makes me nervous... it would be wise to manage it somehow because it's no longer a rudimentary cache, we're just collecting images now. Still thinking...

[akshay2000]

Kodi doesn't provide a straightforward way to download whole cache at once. There's a hacky solution, but it's neither reliable nor practical. Iterating over collections should be better. Since images are downloaded off the local network now, this will be fast enough. Using TemporaryFolder has some advantage here. I wasn't clear earlier. I meant that we check for the cache on every successful handshake. User taps connection > do ping-pong handshake > rebuild the cache > send user to CoverPage. Here rebuilding means downloading the images that aren't available and skipping those that are. Let the OS take care of clearing temporary folder. Skip on the checking age of the image. Unless I'm totally mistaken, this should be just fine for most of the use cases. In fact only scenario this might not work is when user somehow went out of way and changed (not added) album art of existing album - in this case older image might show up on remote until temp folder is cleared.

[christianprescott]

I've reached a point now where both the ImageSource and ValueConverter branches are far enough along that I can commit to one of them as the solution. The ThumbnailImage route has decent performance, but if you, say, scroll the all albums list fast enough you can outrun the image requests. The need to touch XAML all over the app is a bummer, but the lack of default BitmapImage behavior is what really kills it. I've recreated most of it, but BitmapImage is still faster and renders prettier than my ThumbnailImage. As for the image cache, performance right now is absolutely abysmal. Even if I could get a reference to the PlayerItem or the calling BitmapImage to "refresh" the source, the app is unusable when more than a couple images are being cached. The first time the all albums list is viewed it is totally unusable. BUT WAIT, THERE'S MORE. After images are cached, things improve, obviously. All albums list performs even better than with non-auth BitmapImage. For that reason I think it's worth implementing a full-blown image cache. I've created a few stubs for a CacheManager. christianprescott/XBMCRemoteRT@e755309f7243a5ec409d02d59ac849105270b696 I would appreciate your help on GetAllImagesAsync so I've added you as a contributor if you have time to chip in.

[akshay2000]

Let's make things more clear!

1. I think it can be agreed upon that reimplementing the BitmapImage with authentication is not a viable solution. Let's discard it.
2. You're right. Forcing 'refresh', if possible at all, would only solve the fraction of the problem. Not a good idea.
3. Form what you say, while building image cache app is unusable.By unusable, do you mean that images stay blank or the app is frozen altogether? Though, in any case, it's bad.
4. How's overall performance once cache is available. I expect everything is superfast and buttery smooth? If so, we're almost there!

Lastly, CacheManager is a very nice idea. It also keeps the converter clean. Let me come up with a rough design of solution so that it's easier for both of us to follow along.

[christianprescott]

1. Viable, but far inferior to cache in all the areas that matter. Discarded.
2. I disagree! Yes, this refresh effort is also discarded, but even now there are cases where an image could be displayed that hasn't been cached. The ability to trigger the binding would avoid the gray box left on a cache miss if for example media was added to the library. The feature is not going to happen though.
3. Images stay blank for up to several seconds and if there are enough images downloading it begins to slow down other tasks. Images from local app resources, like the music note on album tile, will lag behind. It may even starve invalidate and arrange and fill the screen with gray tiles before text and margins are layed out. Touch inputs lag by seconds. Very ugly. Similar behavior on Lumia 822 device and emulator.
4. With cache available, yes, very smooth. Minimal "popping in" of images as virtualized list changes their source. Because the smoothness is interrupted by cache writes, I am very interested in a full cache of images that appear in collections. We should do everything we can to shield the user from the cache write process. And to handle the case where a user needs an updated image right now I've added a cache refresh button to the settings page.

[akshay2000]

1. --
2. Will keep that in mind. Although, one or two images not being loaded shouldn't be a big problem.
3. Just checked out. App feels like IE6 trying to load heavy website on a dial-up connection.
4. That's what I have been thinking. On each successful handshake, update the cache and keep the app on 'connecting' progress ring.

[christianprescott]

1. Yes, one or two images not cached will show gray box, but only for a little while. (Maybe the user presses the "refresh" cache button or the the app is restarted) That is tolerable. All I'm saying is that we can't cache everything, so it may happen rarely. And in those rare cases it would be nice to update the image source binding after the image is added to cache.
2. Haha, and we definitely don't want the app compared to IE6!
3. Yes, we should block with progress indicator. I will implement IAsyncOperationWithProgress on CacheManager.InitCache so that we can show a progress bar rather than an indeterminate ring. I think that is really important especially for initial cache download.
4. So after initial cache is created, a. how do you envision cache updates working? GetAllImagesAsync will be called, yes, because that operation is fast relative to the actual downloading of images? And then the hashed image file path will be checked for existence? b. Would update happen on app resume or only when connection is selected from connection page?

[akshay2000]

a. Yes, GetAllImagesAsync will be called. That operation doesn't really take much time. Then yes, files will be checked for existance. b. For most cases, update on connection selected should be sufficient.

[christianprescott]

As of christianprescott/XBMCRemoteRT@417b59661ee20c437912e5b6dcb4f8f1dccda892 image cache is - for connections using auth only - implemented end-to-end.

I'm going to go ahead and put in a pull request. You can look it over, make changes, discuss, decide if it's worth doing at all... you know the drill. I have the feature on my device so I'm satisfied. Kodi Assist looks great with actual album art!

There is one last behavior I'd like to change: There are some cases where an image can't be cached and it has an impact on connection time. A request for an image at an external (proxied through Kodi) HTTP location responds 404. CacheManager attempts to download the image and fails. No file is saved. The next time Kodi Assist is closed and the app connects again, CacheManager makes another attempt to download the image. During a cache refresh the impact of this is next to nothing, but during an update the repeated request to a remote location is noticeable. I am considering a "blacklist" for locations that respond with certain results.

[akshay2000]

That's a valid point. If even one of the URLs has 404, it will significantly slow down the progress of cache update. However, for maintaining a blacklist you need some persistence and file based record keeping is flaky at best. On the other hand, how about this: I am pretty sure 404 can be detected at the CacheManager level. How about you save a dummy image like DefaultArt (or better yet, this tiniest PNG) whenever 404 occurs. Next time update is done, it will see that the image exists and won't attempt a download. And refresh will, of course, attempt the download like it should (in case the resource is 200 instead of 404)! Yes, we'll end up with duplicate copies of the same dummy image. But hey, this is lesser than 75 bytes we're talking about. Storage is cheap!

[christianprescott]

The solution implemented @ef2ed720b151b6c6da5f5dfcf2f03d1d88a25870 is to simply "touch" the file where the image would be saved. It does not move any image content into place.

[akshay2000]

Cool! But I'm afraid that there will be some client code somewhere in the app that will try to rented this null file. Have you checked for the possible scenarios? Does this work better than maintaining a blacklist?

[christianprescott]

I have tested it with a couple of images from my library that are unavailable over HTTP. The file is created and there are no further complaints from BitmapImage - it displays just like it does when an image is unavailable for other reasons. I imagine it handles an invalid image file the same way it would a poorly formed URI, unauthorized location, or no source at all.

[akshay2000]

Okay, sounds alright. I'll merge in the morning, then.

In other news, I think we can do a bit of refactoring on the XAML and have the image 'refresh' in case it's not cached earlier. I won't go into the details right now, but in essence, we use this kind of BitmapImage along with the DecodePixelHeight as an attached property on Image or converter parameter. Food for thought!

[christianprescott]

Hm, yes, you'll have to explain. Your link takes a similar strategy as the first attempt I made, but using a BitmapImage extension I think? If you're suggesting updating DecodePixelHeight when the download is complete, that might work. The tough part is getting a reference to the calling control into the ValueConverter, especially in a way that doens't update every bit of BitmapImage XAML. https://github.com/christianprescott/XBMCRemoteRT/blob/issue-27/XBMCRemoteRT/XBMCRemoteRT.Shared/Common/ThumbnailImage.cs

[akshay2000]

You're right @christianprescott - this solution is indeed similar to the ThumbnailImage solution. However, advantage is that we don't need to reimplement everything BitmapImage is doing. Moreover, we get advantage of optimizations done in the BitmapImage.

In terms of refactoring, we bind Image to string in model. Note that Fanart is a string here.

<Image Source="{Bining Fanart, Converter={StaticResource OurAwesomeConverter}, ConverterParameter=w250}"/>

Now, in OurAwesomeConverter we return a BitmapImage as follows.

//Check that image is not cached.
BitmapImage image = new BitmapImage();
image.DecodePixelWidth = 250;
image.SetSourceAsync(uriString, username, password);
return image;

Yes, this does require refactoring (a lot?) XAML. But we are also making it more concise. Having BitmapImage as Image.Source everywhere does feel verbose. As for the cache, this file still will be downloaded like it does now, difference being, they will be displayed after download. In essence, cache will trigger one download and BitmapImage another.

[akshay2000]

Check out commit 0f1b915 on bitmap-credentials. On CoverPage, I've implemented the proposed solution. Caching is currently disabled for CoverPage images (commented out). Performance seems to be at par with native BitmapImage. How about making combination of the two solutions? We maintain a cache of frequently used images and load others (like cast) on the fly using this method and add them to cache for later use? That way these images won't stay gray on the first load.

[christianprescott]

Haven't gone silent, just a busy couple of days... I'll try it out this weekend. I don't know about hybrid solution... if extension method works, it works, and I'm not upset about ditching cache for a single, simpler solution.

[akshay2000]

No problem! I'm still undecided with the direction. We need more testing. On one hand caching (advanced coupled with continuous) gives excellent performance. Image loading is fast. However, to enable this, we still need to check if file already exists in the cache. This process is resource intensive and results in lots of FileNotFoundExceptions and that doesn't sound good. But again, once these files are cached, performance jumps up! So, I'm undecided.

[akshay2000]

Congratulations! Cache just made it to master. I think hybrid solution works very well for most scenarios. Like you said, I've implemented cache for everyone (people with authentication as well as without). I'll soon launch a beta with these features bundled. Let's see how it performs. If that goes well, we can finally close this issue.

[christianprescott]

Right, so, reaction to image cache has been not so great. I think AuthBitmapConverter is a good solution too. Not as fast as storage cache, but plenty quick, and obviously has the big upside of not needing preparation or maintenance.

christianprescott/XBMCRemoteRT@93185c762bafb321b0c9f82a8cba922bcb22997f christianprescott/XBMCRemoteRT@419bd7f51a38aa5bed630d0e57ec812bfb1c2baa christianprescott/XBMCRemoteRT@4e9edca0ceb90691fd5b09d8c23e215ff3e30898 Cleaned up all the comments and ensured AuthBitmapConverter is used app-wide (including in the Windows project - not sure if this was the plan?) Removed cache functionality, ImagePathConverter, and MD5 entirely. Renamed CacheManager to ProxyManager and SetCustomSourceAsync to SetProxySourceAsync.

christianprescott/XBMCRemoteRT@f0b6d742d03337cf59d4f262167291599403fb2d Restored the old behavior of ImagePathConverter to only proxy "image://" URLs or default to DefaultArt.jpg. You may or may not want to include this functionality.

[akshay2000]

Yup. Authenticated bitmaps are giving nice results. Great work with the cleanup! It was plan to remove ImagePathConverter completely - even from Windows Project. I was about to assign that to a fresher - she's just getting started. I'll find something else for her, I suppose. It does make sense to return DefaultArt.jpg. If you're ready, go ahead an open up a pull request. Finally we can close the issue!