kean / Nuke

Image loading system
https://kean.blog/nuke
MIT License
8.12k stars 528 forks source link

UITableViewCell image mix-up #174

Closed michaelnisi closed 6 years ago

michaelnisi commented 6 years ago

Looks like I’ve found an interesting edge case, provoking Nuke to load the wrong image into a table view cell.

img_1237

The loading code, called from within the table view cell, does nothing special really:

  private static func load(
    url: URL,
    into view: UIImageView?,
    sized size: CGSize,
    cb: @escaping ImageTask.Completion
  ) {
    let req = ImageRequest(
      url: url,
      targetSize: size,
      contentMode: .aspectFill
    ).processed(with: ScaledWithRoundedCorners(size: size))

    guard let v = view else { return }

    os_log("loading image: %{public}@ %{public}@", log: log, type: .debug,
           url as CVarArg, size as CVarArg)

    guard let currentImage = v.image else {
      Nuke.loadImage(with: req, into: v, completion: cb)
      return
    }

    os_log("placeholding with current image", log: log, type: .debug)

    let opts = ImageLoadingOptions(
      placeholder: currentImage,
      transition: nil,
      failureImage: currentImage,
      failureImageTransition: nil,
      contentModes: nil
    )

    Nuke.loadImage(with: req, options: opts, into: v, completion: cb)
  }

It’s solid for all use cases, except for one. Erratically, after state restoration in landscape mode, my table view appears with mixed up images. Scrolling, of course, fixes the problem immediately.

My pipeline mainly uses defaults:

  init() {
    let pipeline = ImagePipeline {
      $0.imageCache = ImageCache.shared

      let config = URLSessionConfiguration.default
      config.urlCache = nil
      $0.dataLoader = DataLoader(configuration: config)

      $0.dataCache = try! DataCache(name: "ink.codes.podest.images") { name in
        return String(djb2Hash32(string: name))
      }

      $0.dataLoadingQueue.maxConcurrentOperationCount = 6
      $0.imageDecodingQueue.maxConcurrentOperationCount = 1
      $0.imageProcessingQueue.maxConcurrentOperationCount = 2

      $0.isDeduplicationEnabled = true

      $0.isProgressiveDecodingEnabled = false
    }

    ImagePipeline.shared = pipeline
  }

This isn’t a show stopper really, because of its rare appearance, but I find it fascinating and would love to dig in. I don’t have much time, but I can reproduce it quite reliably at the moment. Where would you look first?

kean commented 6 years ago

String(djb2Hash32(string: name))

This seems to be the problem. The FilenameGenerator function should provide a valid filename for a given URL. Naturally each URL should have a unique file name. A cryptographic hash functions like MD5 or SHA1 are usually a good choice and are in fact what most caches use. djb2 is not a cryptographic hash function. With djb2 some URLs will end up having the same filename generated for them which would result in a mix-up like that.

Nuke 7.3 ships with a default FilenameGenerator implementation which uses SHA1 but it's only available in Swift 4.2.

kean commented 6 years ago
guard let currentImage = v.image else {
      Nuke.loadImage(with: req, into: v, completion: cb)
      return
}

Please take a look at isPrepareForReuseEnabled option, I think you could replace this code with it.

michaelnisi commented 6 years ago

Thanks, Alex. I will try isPrepareForReuseEnabled.

The FilenameGenerator can’t be the reason, the issue was already there before I changed the pipeline to use one. Good guess though, I wanted to believe that as well. Besides, I haven’t seen a single collision, hashing URLs with djb2, been using it quite a bit.

To me, it looks more like a timing issue with the first few images loaded and/or UIKit could do something unexpected during state restoration.

michaelnisi commented 6 years ago

The thing is, this same code path is used throughout a well tested app, but the issue only occurs sometimes after state restoration in landscape mode. I will check if scrolling to the selected row has something to do with it.

kean commented 6 years ago

That's just weird. ImageView.loadImage should be solid. If you could build a short demo, I'd be happy to dig into it.

michaelnisi commented 6 years ago

I hope I can separate it, but look what I’ve found in the meantime 🔥

After populating the table view, the view controller updates the selection. Pass anything other than UITableViewScrollPosition.none and the UITableViewCell image mix-up occurs.

 tableView.selectRow(at: ip, animated: false, scrollPosition: .none) // is fine
michaelnisi commented 6 years ago

I can isolate it to this call, selecting the table view row, scrolling to its position; even delaying it, waiting for one second, doesn’t change anything, images get mixed-up.

tableView.selectRow(at: ip, animated: false, scrollPosition: .middle) // mixed-up
// ...before first full page scroll

I will try to produce a demo of this, but not today—onwards to other mysteries. 🙂

kean commented 6 years ago

Might be something related to the way UITableViewDelegate is implemented?

michaelnisi commented 6 years ago

Well yeah, but it’s a UITableViewController, so...

But here’s another detail I’ve found, the issue starts at having three rows, below is fine. 😏

michaelnisi commented 6 years ago

Still an issue, but as I haven’t got around and this is kind of esoteric, I’m closing this for now. Will reopen when I get to resolving this, annoying but rare, problem. Please reopen if you find it relevant or related—I’ve just skimmed through another issue regarding the first few table view cells.

kean commented 6 years ago

I'm going to investigate this soon, seems really weird 😔

DanielStormApps commented 6 years ago

@michaelnisi can you provide your UITableViewDelegate implementation. This sounds like a cell reuse issue. Mainly, where is load(...) called? On orientation change do you reload your UITableView?

michaelnisi commented 6 years ago

Getting ready for release, I fixed an indirectly/maybe related 🤷‍♂️ Dispatch issue in the app, haven’t seen this issue since.