SDWebImage / SDWebImageSwiftUI

SwiftUI Image loading and Animation framework powered by SDWebImage
https://sdwebimage.github.io/SDWebImageSwiftUI
MIT License
2.18k stars 225 forks source link

CPU spikes when calling async method #95

Closed sidster-io closed 4 years ago

sidster-io commented 4 years ago

Using UIImageColor to get the dominant color on the photo to set background accordingly but when I do try the following

WebImage(url: URL("https://img-9gag-fun.9cache.com/photo/aQ1GWV8_700b.jpg"))
                 .onSuccess { image, cacheType in
                            image.getColors(quality: .lowest) {colors in
                                guard let colors = colors else { return }
                                if (colors.background.toHexString() != "#000000") {
                                    self.cardBackground = colors.background
                                } else {
                                    self.cardBackground = UIColor(red: 0.130, green: 0.130, blue: 0.130, alpha: 1.0)
                                }
                                self.cardHeading = colors.primary
                                self.cardDesc = colors.secondary
                            }
                    }

Before I using WebImage I had

Image.init(uiImage: imageData)
           .onAppear(perform: {
                            image.getColors(quality: .lowest) { colors in
                                guard let colors = colors else { return }
                                if (colors.background.toHexString() != "#000000") {
                                    self.cardBackground = colors.background
                                } else {
                                    self.cardBackground = UIColor(red: 0.130, green: 0.130, blue: 0.130, alpha: 1.0)
                                }
                                self.cardHeading = colors.primary
                                self.cardDesc = colors.secondary
                            }
                        }) 
dreampiggy commented 4 years ago

Can you describe your issue more clearly ?

For example, does this getColors method call a system API or your custom code, which take huge CPU to calculate ?

One more thing, if you modify the View Status inside the onSuccess, you should take care to use DispatchQueue.main.async, because when Memory Cache Hit, the onSuccess is called back sync (just in the same timing when SwiftUI call .body). So this may leak to recursive call ?

dreampiggy commented 4 years ago

@Sidrock28 A little demo which can reproduce this issue is more welcomed. They I can use Instruments to debug the issue cause this CPU peak.

sidster-io commented 4 years ago

Hey @dreampiggy, Thank you for taking a look, here is the library that has the getColors implementation https://github.com/jathu/UIImageColors. I did try DispatchQueue.main.async but I do also believe onSuccess is getting called recursively as I can see it on profiler in Instrumentation. But not sure how to handle it, any help is appreciated.

sidster-io commented 4 years ago

I'm about to try the sync function on UIImageColor inside DispatchQueue.main.async to see if that fixes it.

dreampiggy commented 4 years ago

https://github.com/jathu/UIImageColors

This API seems looks the same as SDWebImage's UIImage.sd_colorsWith(rect:) method, which loop through all the pixels (consume CPU and RAM) and do math sorting, which is not designed to call in the main queue, it will drop down your frames.

The previous example you use Image, and your own imageData, does that called each time your view loaded ? Which means, does previous code do the same logic (including a memory disk cache && URLSession fetching) as current code ? I assume this is because of memory cache (previous you don't have so it always use async). To approve that behavior, you can pass .fromLoaderOnly to disable memory cache fetching, then run again to check:

WebImage(url: url, options: [.fromLoaderOnly])
dreampiggy commented 4 years ago

recursively as I can see it on profiler in Instrumentation

You can use Instrument's Time Profile template, which tell your which of method call is the most expensive, and who block the main queue.

or, just write a simple demo for me to help for this

sidster-io commented 4 years ago

Will try what you mentioned, if it doesn't help I will upload a simple demo. Thank you!

dreampiggy commented 4 years ago

Performance tip:

  1. That UIImage.getColors consume CPU && RAM, you should not call it on main queue, even your previous code. To do the correct thing, you should place it with global queue (for example, start a global queue in onAppear).
  2. More radical, since for single image, the dominant color is the only fixed one, right ? (never change). So you can cache it. SDWebImage provide the extendedObject which can be stored into cache, so maybe this is the better code. (See SDWebImage's extended object: https://github.com/SDWebImage/SDWebImage/pull/2898, or I can update the wiki)
// Make the ImageColors to allows for archive, or use a wrapper NSObject (you can even use a Dictionary to store color hexStrings) class to decode, both OK.
// See: https://medium.com/if-let-swift-programming/migrating-to-codable-from-nscoding-ddc2585f28a4

extension ImageColors : NSCoding {
}
let dominantCacheSerilizer = SDWebImageCacheSerializer { image, data, imageUrl in
    // This cache serilizer is always on global queue, safe to calculate huge thing
    guard let colors = image.getColors else { 
        return data
    }
    image.sd_extendedObject = colors
    return data
}
WebImage(url: URL("https://img-9gag-fun.9cache.com/photo/aQ1GWV8_700b.jpg"), context: [.cacheSerilizer: dominantCacheSerilizer])
                 .onSuccess { image, cacheType in
            guard let colors = image.sd_extendedObject as? ImageColors else {return}
            if (colors.background.toHexString() != "#000000") {
                self.cardBackground = colors.background
            } else {
                self.cardBackground = UIColor(red: 0.130, green: 0.130, blue: 0.130, alpha: 1.0)
            }
            self.cardHeading = colors.primary
            self.cardDesc = colors.secondary
                    }
sidster-io commented 4 years ago

Trying what you suggested. Thank you so much!

sidster-io commented 4 years ago

Hey @dreampiggy, Thank you for suggesting a performant solution. I'm currently stuck with this one error Type 'SDWebImageContextOption' has no member 'cacheSerilizer'

dreampiggy commented 4 years ago

Ops. The cacheSerializer...

sidster-io commented 4 years ago

Thank you!

dreampiggy commented 4 years ago

Seems UIColor does not conforms to NSCoding or Codable It support. Nor that third party library ImageColors. You can use the hexString for serialization. Like you create a dummy class, for example

class ImageColorsObject : NSObject, NSCoding {
    func encode(with coder: NSCoder) {
        let backgroundColorString = background.hexString
        coder.encode(backgroundColorString, forKey: "backgroundColorString")
    }

    required convenience init?(coder: NSCoder) {
        guard let backgroundColorString = coder.decodeObject(forKey: "backgroundColorString") as? String else {
            return nil
        }
        let background = UIColor(hexString: backgroundColorString)
        self.init(background: background)
    }

    public init(background: UIColor) {
        self.background = background
    }

    public var background: UIColor
}

extension ImageColors {
    public func toObject() -> ImageColorsObject {

    }
}

Update:

class ImageColorsObject : NSObject, NSCoding {
    func encode(with coder: NSCoder) {
        coder.encode(background, forKey: "background")
    }

    required convenience init?(coder: NSCoder) {
        guard let background = coder.decodeObject(of: UIColor.self, forKey: "background") else {
            return nil
        }
        self.init(background: background)
    }

    public init(background: UIColor) {
        self.background = background
    }

    public var background: UIColor
}

Then use ImageColorObject to do serialization and de-serialization. The extendedObject will be associated to image, save to disk when store cache. Which means it works for both memory cache and disk cache.

sidster-io commented 4 years ago

I was doing the above part but I guess following the medium post. This is the first time I'm introduced to NSCoding so I was doing it in the most elaborated way possible, I guess :D. I will try yours approach, which is much cleaner. Thank you for all the help :)

dreampiggy commented 4 years ago

That mdium post seems not related to this. I previously assume the ImageColors conforms to Codable, but actually not. So you need to implements it yourself.

The NSCoding and Codable can be bridged using a internel NSObject class, but actually for this cases (SDWebImage need NSCoding), you can just do what I show above.

In future:

SDWebImage 6 will provide a Swift Overlay framework, which rewrite the Swift API instead of the automatically generated one, the Codable is what we will done. I'll fire a proposal about this as well.

SDWebImageSwiftUI will bump to another major version after that...So at that time the API may be more Swifty.

sidster-io commented 4 years ago

I currently have this and work on serialization part

class ImageColors : NSObject, NSCoding {

    var dominant: UIColor
    var primary: UIColor
    var secondary: UIColor
    enum Key:String {
      case dominant = "dominant"
      case primary = "primary"
      case secondary = "secondary"
    }

    init(dominant:UIColor, primary:UIColor, secondary:UIColor) {
      self.dominant = dominant
      self.primary = primary
      self.secondary = secondary
    }

    func encode(with coder: NSCoder) {
        coder.encode(dominant, forKey: Key.dominant.rawValue)
        coder.encode(primary, forKey: Key.primary.rawValue)
        coder.encode(secondary, forKey: Key.secondary.rawValue)
    }

    required convenience init?(coder aDecoder: NSCoder) {
        guard let dominant = aDecoder.decodeObject(forKey: Key.dominant.rawValue) as? UIColor else { return nil}
        guard let primary = aDecoder.decodeObject(forKey: Key.primary.rawValue) as? UIColor else { return nil}
        guard let secondary = aDecoder.decodeObject(forKey: Key.secondary.rawValue) as? UIColor else { return nil}
        self.init(dominant: dominant, primary: primary, secondary: secondary)
    }
}

let dominantCacheSerilizer = SDWebImageCacheSerializer { image, data, imageUrl in
    // This cache serilizer is always on global queue, safe to calculate huge thing
    let background: UIColor
    let heading: UIColor
    let title: UIColor

    guard let colors = image.getColors(quality: .lowest) else {return data}
    if (colors.background.toHexString() != "#000000") {
        background = colors.background
    } else {
        background = UIColor(red: 0.130, green: 0.130, blue: 0.130, alpha: 1.0)
    }
    heading = colors.primary
    title = colors.secondary

    image.sd_extendedObject = (ImageColors(dominant: colors.background, primary: colors.primary, secondary: colors.secondary) as! NSCoding & NSObjectProtocol)
    return data
}
dreampiggy commented 4 years ago

The class ImageColors should inherited from NSObject, not just conforms to NSObjectProtocol...The NSCoding actually, can not be implements by root object or struct from Swift...Because it powered by ObjectiveC Runtime some optimization :)

sidster-io commented 4 years ago

I'm not sure if I got this part right

let dominantCacheSerilizer = SDWebImageCacheSerializer { image, data, imageUrl in
    // This cache serilizer is always on global queue, safe to calculate huge thing
    let background: UIColor
    let heading: UIColor
    let title: UIColor

    guard let colors = image.getColors(quality: .lowest) else {return data}
    if (colors.background.toHexString() != "#000000") {
        background = colors.background
    } else {
        background = UIColor(red: 0.130, green: 0.130, blue: 0.130, alpha: 1.0)
    }
    heading = colors.primary
    title = colors.secondary

    image.sd_extendedObject = ImageColors(dominant: colors.background, primary: colors.primary, secondary: colors.secondary)
    return data
}
dreampiggy commented 4 years ago

You can just have a try now to see the result. I saw you calculate the title etc during the ImageColors object, so the onSuccess method can be simplified into just 3 lines...

sidster-io commented 4 years ago

onSuccess looks like this. I updated to follow ur naming of ImageColorsObject

WebImage(url: trendingNews.image, context: [.cacheSerializer: dominantCacheSerilizer])
                        .onSuccess(perform: { (image, _) in
                            image.getColors(quality: .lowest) {colors in
                                guard let colors = image.sd_extendedObject as? ImageColorsObject else {return}
                                self.cardBackground = colors.dominant
                                self.cardHeading = colors.primary
                                self.cardDesc = colors.secondary
                            }
                        })
                        .resizable()

but no luck. Everything defaulting to white. :(

dreampiggy commented 4 years ago

...You already cache the ImageColors, don't call that API image.getColors to regenerate...

Do so:

WebImage(url: trendingNews.image, context: [.cacheSerializer: dominantCacheSerilizer])
                        .onSuccess(perform: { (image, _) in
                                guard let colors = image.sd_extendedObject as? ImageColorsObject else {return}
                                self.cardBackground = colors.dominant
                                self.cardHeading = colors.primary
                                self.cardDesc = colors.secondary
                        })
                        .resizable()

or maybe , need another options ? .waitForCache. By default, the written cache part is async...the onSuccess may execte before cache serializer process that image...

WebImage(url: trendingNews.image, options: [.waitStoreCache], context: [.cacheSerializer: dominantCacheSerilizer])
                        .onSuccess(perform: { (image, _) in
                                guard let colors = image.sd_extendedObject as? ImageColorsObject else {return}
                                self.cardBackground = colors.dominant
                                self.cardHeading = colors.primary
                                self.cardDesc = colors.secondary
                        })
                        .resizable()
sidster-io commented 4 years ago

Sorry, that was stupid I was copy pasting code and I accidentally pasted getColor call again. However, I don't see [.waitForCache] option, I used .waitStoreCache but did not help. My code looks like the bottom one u posted.

dreampiggy commented 4 years ago

It's .waitStoreCache...I just write that demo code without Xcode... Still not works ? Sounds strange. Is that the dominantCacheSerilizer method get called ?

Can you provide a little example contains this ? For example, you can just use a placeholder URL, the cardDesc can just use a @State and showing a rectangle of colors in your View. Let me debug the issue.


Or I write a demo by myself...

sidster-io commented 4 years ago

I added guard let colors = image.sd_extendedObject as? ImageColorsObject else {return print("returning")} and I see it

2020-04-09 01:04:52.753040-0400 LearningSwift[17269:666694] [Agent] Create remote injection Mach transport: 6000016f6060
2020-04-09 01:04:52.753296-0400 LearningSwift[17269:666668] [Agent] No global connection handler, using shared user agent
2020-04-09 01:04:52.753426-0400 LearningSwift[17269:666668] [Agent] Received connection, creating agent
2020-04-09 01:04:53.548159-0400 LearningSwift[17269:666668] [Agent] Received message: < DTXMessage 0x6000019f8540 : i2.0e c0 object:(__NSDictionaryI*) {
    "updates" : <NSArray 0x7fff8062cc40 | 0 objects>
    "id" : [0]
    "scaleFactorHint" : [3]
    "providerName" : "12LearningSwift 21TrendingCard_PreviewsV"
    "products" : <NSArray 0x600003fe0480 | 1 objects>
} > {
    "serviceCommand" : "forwardMessage"
    "type" : "display"
}
returning
returning
returning
returning
returning
returning
returning
returning
returning
returning
sidster-io commented 4 years ago

I will send a demo. I appreciate your help. I don't want you to offer more help without an editor. Thank you so much ❤️

dreampiggy commented 4 years ago

My demo works without any issue ?

See: image

Code (Full):

/*
 * This file is part of the SDWebImage package.
 * (c) DreamPiggy <lizhuoli1126@126.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

import SwiftUI
import SDWebImage
import SDWebImageSwiftUI
import UIImageColors

extension UIColor {
    public func toHexString() -> String {
        let components = cgColor.components
        let r: CGFloat = components?[0] ?? 0.0
        let g: CGFloat = components?[1] ?? 0.0
        let b: CGFloat = components?[2] ?? 0.0

        let hexString = String(format: "#%02lX%02lX%02lX", lroundf(Float(r * 255)), lroundf(Float(g * 255)),
                               lroundf(Float(b * 255)))

        return hexString
    }
}

class ImageColors : NSObject, NSCoding {

    var dominant: UIColor
    var primary: UIColor
    var secondary: UIColor
    enum Key:String {
      case dominant = "dominant"
      case primary = "primary"
      case secondary = "secondary"
    }

    init(dominant:UIColor, primary:UIColor, secondary:UIColor) {
      self.dominant = dominant
      self.primary = primary
      self.secondary = secondary
    }

    func encode(with coder: NSCoder) {
        coder.encode(dominant, forKey: Key.dominant.rawValue)
        coder.encode(primary, forKey: Key.primary.rawValue)
        coder.encode(secondary, forKey: Key.secondary.rawValue)
    }

    required convenience init?(coder aDecoder: NSCoder) {
        guard let dominant = aDecoder.decodeObject(forKey: Key.dominant.rawValue) as? UIColor else { return nil}
        guard let primary = aDecoder.decodeObject(forKey: Key.primary.rawValue) as? UIColor else { return nil}
        guard let secondary = aDecoder.decodeObject(forKey: Key.secondary.rawValue) as? UIColor else { return nil}
        self.init(dominant: dominant, primary: primary, secondary: secondary)
    }
}

struct ContentView : View {
    static let dominantCacheSerilizer = SDWebImageCacheSerializer { image, data, imageUrl in
        // This cache serilizer is always on global queue, safe to calculate huge thing
        let background: UIColor
        let heading: UIColor
        let title: UIColor

        guard let colors = image.getColors(quality: .lowest) else {return data}
        if (colors.background.toHexString() != "#000000") {
            background = colors.background
        } else {
            background = UIColor(red: 0.130, green: 0.130, blue: 0.130, alpha: 1.0)
        }
        heading = colors.primary
        title = colors.secondary

        image.sd_extendedObject = ImageColors(dominant: colors.background, primary: colors.primary, secondary: colors.secondary)
        return data
    }

    let url = URL(string: "http://apng.onevcat.com/assets/elephant.png")!
    @State var cardBackground: UIColor?
    @State var cardHeading: UIColor?
    @State var cardDesc: UIColor?

    var body: some View {
        VStack {
            Rectangle().foregroundColor(Color(self.cardHeading ?? .clear))
            WebImage(url: url, options: [.waitStoreCache], context: [.cacheSerializer: ContentView.dominantCacheSerilizer])
            .onSuccess { (image, _) in
                guard let colors = image.sd_extendedObject as? ImageColors else {
                    return
                }
                DispatchQueue.main.async {
                    self.cardBackground = colors.dominant
                    self.cardHeading = colors.primary
                    self.cardDesc = colors.secondary
                }
            }
            .resizable()
            .border(Color(self.cardBackground ?? .clear), width: 10)
            Circle().foregroundColor(Color(self.cardDesc ?? .clear))
        }
    }
}

#if DEBUG
struct ContentView_Previews: PreviewProvider {
    static var previews: some View {
        ContentView()
    }
}
#endif
dreampiggy commented 4 years ago

Note, you can not update the @State value in sync when SwiftUI call the body method. The onSuccess will be callback in sync when they are in memory cache. (I dislike this, but some user want this behavior, see #76 )...You should use async. This is why here have dispatch queue. And Xcode will point this out.

image

sidster-io commented 4 years ago

I made a demo and that works for me too. Idk now why my main doesn't. Will work my way through. I honestly appreciate your prompt replies and help all the way. Thank you!

dreampiggy commented 4 years ago

If you found the reason why your real project not works, please reply. I assume the issue may related to some cache config (Like you disable the memory cache/disk cache path wrong, etc)...

sidster-io commented 4 years ago

You are right, I was about to update you and I see you posted the answer. The images were already cached 🤦‍♂. I spun up a new preview device and it worked.

dreampiggy commented 4 years ago

You can call SDImageCache.shared.clearDisk(with:) method to clear all disk cache.


It's useful during debugging :) The disk cache is permanent (actually have a expire date, but by default is 7 days)...So If you do something debug code wrong, you need to clear them all.

sidster-io commented 4 years ago

Thank @dreampiggy! Will do that 👍🏽