-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ASImageNode / PINRemoteImage] Support WebP. #117
Comments
From @appleguy on December 18, 2015 23:42 @smyrgl are you setting a decoded image on the image node? If so, ASDK has no way to release memory without completely losing the reference to the image. It is absolutely essential that you set coded / compressed images (e.g. created from a UIImage created from JPEG NSData off the network, or a UIImage created from a PNG on disk, etc). |
From @smyrgl on December 19, 2015 2:3 @appleguy I'm using WebP encoded images so I don't really have a choice but to use decoded images since ImageIO doesn't support WebP. What's your suggestion for working around this? I suppose I could convert them into JPEG before sticking them into my image cache (I'm using a local image cache instead of NSURLCache for image caching) but if there's another way it would be good to avoid this step. I'd like to continue to use WebP on the wire for the size improvements. |
From @appleguy on December 19, 2015 4:9 @smyrgl ah, very cool. It should work quite well if you want to implement a subclass to ASImageNode or ASNetworkImageNode. Check out this method in the ASDisplayNode+Subclasses.h header, and look at the implementation of ASImageNode:
But yeah, it's definitely going to be a sad day if you are providing fully decoded images to ASDK. You certainly could locally re-encode them, but that's going to be phenomenally expensive. I'd be interested in merging in support for WebP even if it required adding a weak_framework that folks could optionally link to have support for that. I believe this is how PINRemoteImage supports a few extra things like FLAnimatedImage and I think it supports WebP too. In the next couple months we are going to do a deep integration between PINRemoteImage and AsyncDisplayKit, thus supporting this, but it's a ways out still... |
From @smyrgl on December 19, 2015 21:6 @appleguy I'm familiar with the implementation details of ASImageNode so I could do what you are suggesting but just to verify how this might work let me list out my understanding of what the problem is:
Do I have that right? I'd like to suggest that this is not just a problem with WebP--although I realize that this is ultimately a problem with the way UIImage abstracts ImageIO but it just seems very dangerous to me for unsuspecting users. I don't see a lot of good options for solving this universally without drastic changes but I suspect a lot of users are seeing this issue without knowing it and that it can be a really crippling issue in the context of a Table/Collection view. |
From @appleguy on December 20, 2015 4:44 @smyrgl Yes, you have a detailed understanding of the issue. Especially bullet point #2 - by design, ASDataController holds onto a large number of nodes, as they serve as the vessel that holds cached layout information (specifically calculatedLayout). It is intended that after calling /both/ -clearContents and -clearFetchedData, that the memory footprint of nodes is very small. This is true for most normal ASNetworkImageNodes and ASMultiplexImageNodes as they can even go re-fetch the data from cache or the downloader...but disastrously non-true if users are directly setting large decoded buffers themselves. Because I have not actually imagined the utility / reasoning behind setting decoded buffers on ASImageNode, it escaped me entirely until recently that this is a clear and serious pitfall for public users. Trivial to fix for most cases, but also just as easy to commit the mistake without even realizing it. I realized this about 1 month ago and haven't yet had time to think in detail about a solution. One mitigation that I have thought about, which is not satisfying, is to detect if a large ratio of the images set on ASImageNodes are in fact decoded. This would be an excellent indicator to throw a loud warning / possibly even assert (in DEBUG of course) that the framework is in an unhealthy state of usage. Regarding your third bullet point, there I'm not as confident you are correct, but I may not completely understand what you mean. The upshot is that it is essential that the range-driven calls for fetchData, display (which does trigger the async draw parameters call), clearContents, and clearFetchedData properly handle image memory. The fetch data calls obviously are intended to load /and free/ in-memory compressed image data; the display / clearContents calls are intended to do the same for uncompressed data, managed internally by ASDK. In this golden state, users have a great deal of control over the memory footprint and runtime characteristics of the framework via the rangeTuningParameters. @smyrgl I'm really glad to have an engineer as senior as yourself considering this challenge. If you have any ideas / suggestions for how safeguards can be put in place, even if you aren't able to implement them yourself, please do share and I will work to prioritize them over the holidays. Diffs will be eagerly collaborated on if you do want to take a pass at it :). |
From @smyrgl on December 21, 2015 21:49 @appleguy Thanks for the vote of confidence, I want to finish some investigation before I comment further--I have a few ideas I'm looking at for how to solve this and I need to dig deeper into them before I can say definitively one way or the other what the best course is going to be. And you can pretty much ignore my point 3...that was purely an attempt to suggest a way to resolve the specific problem OUTSIDE of ADK...I'm pretty convinced that this needs to be resolved as a patch to ADK for this to work now. This is my top priority right now so I should have an update in the next couple of days. |
From @appleguy on December 22, 2015 5:13 @smyrgl update on my side. I dove into ImageIO and have been pretty desperately trying to figure out the best way to determine if an image is compressed or not. The sad news is that I am pretty sure it can't be done in a practical way without access to the original data - you can go down a weird path with CGDataProvider but that doesn't seem to always work either, but better than that is to inspect the actual NSData using CGImageSource. I'm thinking of changing the Downloader and Cache protocols to return NSData in their completion blocks rather than CGImageRef. This could be done as a breaking change for the 2.0 API for anyone who is not using ASBasicImageDownloader, which would certainly be one way to ensure that everyone is using the framework as intended. We could inspect the NSDatas in DEBUG mode to ensure they are compressed, and if almost all are decompressed, we know something funny is going on (like a user getting the data out of an image after another library decompresses it). Really though, I want to get the PINRemoteImage integration accomplished. PIRI supports WebP, but also Progressive JPEG, GIF, etc...and ASDK absolutely needs a "1st party"-quality image downloader and decoding suite. @Adlai-Holler has done a lot of work on ASMultiplexImageNode recently and might have some further thoughts here. One last thing - the implementation of ASImageNode's async draw method can be significantly improved for the case where a decoded image IS provided. It was much more efficient for the version used by Paper. The current version is using UIGraphics contexts regardless of the incoming image, since we can't check if it is decoded. If it is decoded, we can use the image directly as the contents (potentially setting contentsCenter / contentsRect if necessary) to reduce by approximately half the memory consumed (depends on how much upscaling / downscaling is currently being done by allocating the new buffer of exactly the right size). There's a great deal more that can be done with image handling; for example the Paper version would decode images at native size if they were smaller than the area to be shown in, rather than drawing them at the final size; if they were larger than the ideal size, it would redraw them into the ideal size to save memory. Very simple and clearly just the beginning compared to using progressive JPEG to its fullest potential, supporting awesome fading transitions (the current placeholderFadesOut option is pretty shady), etc. |
From @smyrgl on December 28, 2015 21:15 @appleguy The holidays kind of threw this off for me but my findings were the same--there just doesn't seem to be a great way to tell if an image is decoded (that seems to be the whole point actually...). It is possible to resolve this by using a custom image buffer abstraction that is different than UIImage but that doesn't seem to be the most elegant solution. I do think that working on this from the ASImageNode side seems to be the best approach but it isn't bulletproof since we still need a way to cache the images so they don't need to be kept in memory. I am still working on a few workarounds but I don't have anything solid yet. |
From @smyrgl on January 4, 2016 5:58 @appleguy So one thing I've been playing with--what about implementing our own buffer in the setter on an class ImageBuffer: NSObject {
private let fileURL: NSURL
private var imageBufferData: NSData
init(imageData: NSData) {
let filename = NSProcessInfo.processInfo().globallyUniqueString
self.fileURL = NSURL(fileURLWithPath: (NSTemporaryDirectory() as NSString).stringByAppendingPathComponent(filename))
imageData.writeToURL(fileURL, atomically: true)
imageBufferData = try! NSData(contentsOfFile: fileURL.path!, options: [.DataReadingMappedAlways])
super.init()
}
init(contentsOfURL: NSURL) {
self.fileURL = contentsOfURL
imageBufferData = try! NSData(contentsOfFile: fileURL.path!, options: [.DataReadingMappedAlways])
super.init()
}
func imageFromBuffer() -> UIImage? {
return UIImage(data: imageBufferData)
}
func purge() {
try! NSFileManager.defaultManager().removeItemAtURL(fileURL)
}
} This could then be purged during EDIT: The idea here is to maintain compatibility with UIImage...if we are willing to discard that then the options increase dramatically...also if there is some magical way to deduce if a UIImage is decoded or not that also helps. The problem with this solution I see is the duplication which is tough to work around since I'm not aware of an easy way to keep hashes of image data. Typically the hash value of an image is the same as the image bytes themselves... |
From @smyrgl on January 4, 2016 6:4 Of course the other solution is to implement the equivalent of UITableViewCell's |
From @appleguy on March 19, 2016 1:7 @smyrgl we now support PINRemoteImage and so this issue is resolved by default. I will keep this issue open to add support for webP, and we should separately revisit whether there is a way to detect that a developer's custom downloader set up is erroneously providing decoded images to the framework. At the moment, I am not aware of the way to reliably detect whether an image reference is decoded or not. Please let me know if you can think of a way to do this. cc @garrettmoon, @maicki |
I think you can just add the PINRemoteImage/webp pod and it will 'just work'. |
I didn't read the entirety of the thread. There isn't currently a way to get out encoded webp, but a change in working on internally may help support this. |
From @holgersindbaek on March 29, 2016 19:36 +1 for WebP support. |
From @dssheng on July 13, 2016 17:27 Pods PINRemoteImage/webp not works well for me.
|
From @appleguy on July 13, 2016 17:53 Interesting. What does that WebP method actually do? If it runs the decode at that time and results in a decoded image, this would have some memory problems in practice.
|
The changes I mentioned have been landed in the beta version of PINRemoteImage 3 (included by default in ASDK, specifically look for alternativeRepresentation). You should be able to get the data directly now for a image download (that's how animated GIFs work) and delay decoding until the image node is drawn. You'd likely need to add a category adding support for setting webp data on ASImageNode. |
From @euwars on July 16, 2016 21:33 Not sure if it changes anything, but iOS 10 beta supports WebP. |
+1 |
+1 hey @garrettmoon @appleguy -- is there any work arounds/plans or tricks to enable animated webp? we are working on a heavy Gif dependent project and this will make life 10000% easier. |
Hi guys, Any way to support webp images on Texture ? |
@anirudhramanan it's already supported if you're using PINRemoteImage (Texture is using it by default). Animated webp is coming soonish |
From @smyrgl on December 18, 2015 22:2
I have created a sample project to demonstrate this behavior here: https://github.com/smyrgl/adktest
So here's what I'm noticing: it seems that in ASCollectionView the memory isn't properly being freed up. This is very easy to see in the sample project: just play around with clicking on items in the collection view (each will push to another collection view) and scroll and watch the memory usage continue to increase. Firing memory warnings will not change this behavior, the memory usage just keeps climbing.
However if in the ASCellNode I manually implement clearContents like this:
Then memory usage stays under control. Also if I define a custom subclass of
ASImageNode
that implementsclearContents
I also get the correct memory behavior:I'd be glad to create a diff which adds this to ASImageNode but I am uncertain if there is a gotcha to this change.
Copied from original issue: facebookarchive/AsyncDisplayKit#955
The text was updated successfully, but these errors were encountered: