[ASImageNode / PINRemoteImage] Support WebP.

[smyrgl]

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:

    override func clearContents() {
        super.clearContents()
        imageNode.image = nil
    }

Then memory usage stays under control. Also if I define a custom subclass of ASImageNode that implements clearContents I also get the correct memory behavior:

class ClearImageNode: ASImageNode {
    override func clearContents() {
        super.clearContents()
        image = nil
    }
}

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.

[appleguy]

@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).

[smyrgl]

@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.

[appleguy]

@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:

• (UIImage *)displayWithParameters:(id)parameters
  isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock;

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...

[smyrgl]

@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:

• ASImageNode works by holding a strong reference to the UIImage and then drawing it in displayWithParameters: by creating a draw context and drawing it using drawInRect:. This works because encoded images have a CGImageSource backing which doesn't keep the actual image data in memory.
• However if the UIImage is already decoded then its underlying data isn't lazily fetched from disk but rather is kept in memory and will not free the memory until the ASImageNode itself is destroyed. In a Table/Collection view this is pretty much never since the ASDataController holds onto its nodes even if they aren't in the display range.
• This could be resolved a number of ways but the simplest seems to be to create a subclass of ASImageNode that overrides drawParametersForAsyncLayer: and then returns the _ASImageNodeDrawParameters which will include the decoded image only at that point which could be built from a new param that is added (such as a cacheKey or fileURL). Since the node itself doesn't keep a reference to the decoded image this should be fine and the decoded image memory should be freed as soon as displayWithParameters: is finished.

Do I have that right?

I'd like to suggest that this is not just a problem with WebP--although ASImageNode has the imageModificationBlock property I'm sure a lot of people might be doing things like tinting and corner rounding before setting the image property of a node which is going to trigger this issue. The ASButtonNode class for example is practically begging for this to be the case since the biggest use case for different images for different control states is image tints and that will trigger this issue right there. If you image an ASButtonNode being used in a Table/Collection view...well you get the idea. This is a memory nightmare waiting to happen.

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.

[appleguy]

@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 :).

[smyrgl]

@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.

[appleguy]

@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.

[smyrgl]

@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.

[smyrgl]

@appleguy So one thing I've been playing with--what about implementing our own buffer in the setter on an ASImageNode that discards the UIImage and just keeps a reference to the buffer object? Something like this (simplistic example):

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 clearContents on the node in question which would cleanup the temp file. Not an ideal solution but it works.

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...

[smyrgl]

Of course the other solution is to implement the equivalent of UITableViewCell's prepareForReuse. That seems to cut against the fundamental design philosophy of ADK though since we would be making the nodes stateful...

[appleguy]

@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

[garrettmoon]

I think you can just add the PINRemoteImage/webp pod and it will 'just work'.

[garrettmoon]

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.

[holgersindbaek]

+1 for WebP support.

[dssheng]

Pods PINRemoteImage/webp not works well for me.
Here is a workaround with SDWebImage and JRSwizzle.

@implementation PINImage (SD_JRSwizzle)

+ (void)load
{
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{        
        [self jr_swizzleClassMethod:@selector(pin_decodedImageWithData:skipDecodeIfPossible:) withClassMethod:@selector(ACK_decodedImageWithData:skipDecodeIfPossible:) error:nil];
    });
}

+ (UIImage *)ACK_decodedImageWithData:(NSData *)data skipDecodeIfPossible:(BOOL)skipDecodeIfPossible
{
    if (!data) {
        return nil;
    }

    UIImage *image;
    NSString *imageContentType = [NSData sd_contentTypeForImageData:data];

    if ([imageContentType isEqualToString:@"image/webp"]) {
        image = [UIImage sd_imageWithWebPData:data];
    } else {
        return [self ACK_decodedImageWithData:data skipDecodeIfPossible:skipDecodeIfPossible];
    }

    return image;
}

@end

[appleguy]

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.

On Jul 13, 2016, at 10:27 AM, dssheng notifications@github.com wrote:

Here is a workaround with SDWebImage

@implementation PINImage (ACK_JRSwizzle)

• (void)load
  {
  static dispatch_once_t onceToken;
  dispatch_once(&onceToken, ^{
  [self jr_swizzleClassMethod:@selector(pin_decodedImageWithData:skipDecodeIfPossible:) withClassMethod:@selector(ACK_decodedImageWithData:skipDecodeIfPossible:) error:nil];
  });
  }

• (UIImage *)ACK_decodedImageWithData:(NSData *)data skipDecodeIfPossible:(BOOL)skipDecodeIfPossible
  {
  if (!data) {
  return nil;
  }

  UIImage *image;
  NSString *imageContentType = [NSData sd_contentTypeForImageData:data];

  if ([imageContentType isEqualToString:@"image/webp"]) {
  image = [UIImage sd_imageWithWebPData:data];
  } else {
  return [self ACK_decodedImageWithData:data skipDecodeIfPossible:skipDecodeIfPossible];
  }

  return image;
  }

@EnD

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #955 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAigA5UaWhEf5_boDxp_6NzLfyBFT9Phks5qVSAYgaJpZM4G4dlz.

[garrettmoon]

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.

[euwars]

Not sure if it changes anything, but iOS 10 beta supports WebP.
And just using PINRemoteImage is enough which is faster and has less usage than PINRemoteImage/WebP, So when you figure out the workaround here you might wanna add version check for iOS before 10.

[garrettmoon]

This issue was moved to TextureGroup/Texture#117