Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New option SDWebImageRefreshCached #326

Merged
merged 1 commit into from
Mar 12, 2013

Conversation

nioq
Copy link

@nioq nioq commented Mar 12, 2013

Even if the image is cached, fetch the URL again anyway. When set,
NSURLCache is enabled in the downloader via the new option
SDWebImageDownloaderEnableNSURLCache.

NSURLCache will handle the protocol caching while SDWebImage remains
useful for offline images.

This option helps deal with images changing behind the same request URL,
e.g. Facebook graph api profile pics where the request URL
https://graph.facebook.com/[userid]/picture returns a redirect to the
actual profile image.

If a cached image exists, the completion block is called once with the
cached image and again with the final image.

Even if the image is cached, fetch the URL again anyway. When set,
NSURLCache is enabled in the downloader via the new option
SDWebImageDownloaderEnableNSURLCache.

NSURLCache will handle the protocol caching while SDWebImage remains
useful for offline images.

This option helps deal with images changing behind the same request URL,
e.g. Facebook graph api profile pics where the request URL
https://graph.facebook.com/[userid]/picture returns a redirect to the
actual profile image.

If a cached image exists, the completion block is called once with the
cached image and again with the final image.
@rs
Copy link
Member

rs commented Mar 12, 2013

For some reason, requests are performed regardless of the response cacheability when this option is passed. Any idea?

@rs
Copy link
Member

rs commented Mar 12, 2013

Ok got it, the problem is located in SDWebImageDownloaderOperation's connection:willCacheResponse:. You should test your modifications before requesting for pulling ;)

@rs
Copy link
Member

rs commented Mar 12, 2013

Ok so I've got a working commit. But this option is real performance killer:

  1. You'll get images twice in disk & memory caches in two representationa (image and HTTP cacheable response)
  2. On cache-hit, the image will be loaded and displayed from the SDWebImage cache first and then re-loaded from the source NSData returned by NSURLCache, and finally re-extracted, re-decoded and re-cached in the SDWebImage cache.

The 2. is a big issue. A workaround to prevent re-decoding and re-caching of a full cache-hit (in both SDWebImage cache and NSURLCache) must be found in order for me to consider merging.

rs pushed a commit that referenced this pull request Mar 12, 2013
rs pushed a commit that referenced this pull request Mar 12, 2013
@rs
Copy link
Member

rs commented Mar 12, 2013

I've pushed fixes for this pull in a separate branch for you to test it. If you try the example app with the flag on each image, you will notice a very choppy scrolling, even in the simulator.

rs pushed a commit that referenced this pull request Mar 12, 2013
rs pushed a commit that referenced this pull request Mar 12, 2013
@rs rs merged commit bbad1bc into SDWebImage:master Mar 12, 2013
@rs
Copy link
Member

rs commented Mar 12, 2013

I found some workarounds and thus merge this pull request.

@nioq
Copy link
Author

nioq commented Mar 12, 2013

Thanks for this, and sorry about the extra work you had to do to get this into shape!

Any chance you would you be open to keeping double disk caching as an option? With the SDWebImage cache we get to associate the cached image with the request URL, which is useful for offline access in cases where the request URL responds with a 302 redirect to the actual image and no-cache is specified in the response headers.

NSURLCache without the SDWebImage cache will associate the image with the actual image URL but since it won't cache the original 302 response we don't know the actual image URL when offline. You may recognise this as the situtation we face with Facebook graph API profile pictures:

a) https://graph.facebook.com/[id]/picture?height=200&width=200 returns a 302 with no-cache specified
b) 302 points to a static image on the Akamai CDN with max-age=1209600

If both NSURLCache and SDWebImage's disk cache are enabled, SDWebImage caches (a) so we still have profile pics while offline, and NSURLCache caches (b) so even if we keep requesting (a) again, we do not re-download the static image.

It's still wasteful though so I understand if you'd rather this situation be dealt with some other way. Either way, thanks for the great library!

@rs
Copy link
Member

rs commented Mar 12, 2013

I'll think about it, but in case of an uncacheable 302, the network will be hit on each load, regardless of the cache status. That's a pretty bad situation.

@rs
Copy link
Member

rs commented Mar 12, 2013

I removed the disk cache dup protection waiting for a better idea.

hawflakes pushed a commit to hawflakes/SDWebImage that referenced this pull request Mar 13, 2013
hawflakes pushed a commit to hawflakes/SDWebImage that referenced this pull request Mar 13, 2013
hawflakes pushed a commit to hawflakes/SDWebImage that referenced this pull request Mar 13, 2013
hawflakes pushed a commit to hawflakes/SDWebImage that referenced this pull request Mar 13, 2013
hawflakes pushed a commit to hawflakes/SDWebImage that referenced this pull request Mar 13, 2013
@nioq
Copy link
Author

nioq commented Mar 13, 2013

Thanks!

SebastianZus pushed a commit to raumfeld/SDWebImage that referenced this pull request Nov 5, 2013
@mohamedadly
Copy link

Hey,

this works fine if you are using the Facebook username in the graph URL.

But if you are using the Facebook ID for example "100007160439164", this will not work, and will not refresh the image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants