ProgressiveJPEG enhancement

[jyounus]

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

• [x] I have read the wiki page and cheat sheet, but there is no information I need.
• [x] I have searched in existing issues, but did not find a same one.
• [ ] I want to report a problem instead of asking a question. It'd better to use kingfisher tag in Stack Overflow to ask a question.

Issue Description

It looks like the only way to use Kingfisher and progressiveJPEG downloads is using code like this:

cell.imageView.kf.setImage(with: url, options: [.progressiveJPEG(.default), .cacheMemoryOnly, .transition(.fade(0.2))]) { receivedSize, totalSize in
                    print("progress: \(CGFloat(receivedSize) / CGFloat(totalSize))")
                } completionHandler: { result in
                    print("complete: \(result)")
                }

I have to use it with an imageView directly. However my application requirements are a bit complex, and I need more flexibility. I want to be able to use progressiveJPEG downloads without having to use the kf extension on UIImageViews.

I tried doing something like this:

KingfisherManager.shared.retrieveImage(with: url, options: [.progressiveJPEG(.default), .cacheMemoryOnly, .transition(.fade(0.2))]) { receivedSize, totalSize in
                    print("progress: \(CGFloat(receivedSize) / CGFloat(totalSize))")
                } downloadTaskUpdated: { newTask in
                    print("download task updated")
                } completionHandler: { result in
                    print("complete: \(result)")

                    switch result {
                    case .success(let imageResult):
                        cell.imageView.image = imageResult.image
                    case .failure:
                        break
                    }
                }

But the problem is the progressiveJPEG is pretty much ignored. Maybe I'm missing something but I think having a callback function where the progressiveJPEG downloaded so far can be manually set would be super useful. So something like this, by modifying the progress callback to return an additional RetrieveImageResult parameter:

KingfisherManager.shared.retrieveImage(with: url, options: [.progressiveJPEG(.default), .cacheMemoryOnly, .transition(.fade(0.2))]) { receivedSize, totalSize, imageResult in
                    print("progress: \(CGFloat(receivedSize) / CGFloat(totalSize))")
                    cell.imageView.image = imageResult?.image // <-- being able to set the progressive image here

                } downloadTaskUpdated: { newTask in
                    print("download task updated")
                } completionHandler: { result in
                    print("complete: \(result)")
                }

Maybe something similar like this is already possible, and I just missed it. But if not, would it be possible to add such functionality?

EDIT I've had a look at some other third-party libraries and Nuke seems to handle this by returning an additional parameter in the progress closure. Here's a link to their docs. Would it be possible to do something similar for Kingfisher?

[onevcat]

It is now a pure ImageView thing in Kingfisher. I will see if there is a good way to move it up to the manager.

[onevcat]

@jyounus

With changes in feature/progressive-update-callback and #1961, now it should be possible to get the event of preparing another progressive updating image. You can do something like this now:

let progressive = ImageProgressive(
  isBlur: true,
  isFastestScan: true,
  scanInterval: 0
)
progressive.onImageUpdated.delegate(on: self) { (self, image) in
  print("Update image: \(image)")
  return .default
}

KingfisherManager.shared.retrieveImage(
  with: ImageLoader.progressiveImageURL,
  options: [.progressiveJPEG(progressive)],
  completionHandler: { result in print("All Done!") }
)

Furthermore, although I am not quite sure about what do you want to do with these intermediate images, it is in fact quite easy for Kingfisher to implement an UpdatingStrategy to change the default behavior when using inside an image view: You can just instead of returning .default, return a .replace(yourImage) to let Kingisher to use that image:

progressive.onImageUpdated.delegate(on: self) { (self, image) in
  return .replace(UIImage(named: "hello"))
}

KF.url(ImageLoader.progressiveImageURL)
    .progressiveJPEG(progressive)
    .set(to: imageView)

It is not merged yet, and you would not mind to let me know more about your context and purpose, maybe we can together make your life even eaiser.

[jyounus]

Amazing!

I did find a workaround for my use case. I'm building an infinite scrollable feed, where I want the photos to load as fast as possible. I also use a combination of prefetching pictures before the next set of UICollectionViewCells are displayed. My primary workaround was to check the progress. If it's at least 50%, I would assume a decent quality progressive photo has been set, and I can continue with my other app-specific logic.

However, with the changes in your new branch, I don't need to do that anymore. I can now use onImageUpdated.delegate(on: self) and run my app-specific code as soon as the first frame has been downloaded and displayed to the user!

I'm doing something like this now:

let progressive = ImageProgressive.default
progressive.onImageUpdated.delegate(on: self) { (self, image) in
  imageView.image = image // Is this correct? Without this, the progressive images are not automatically set.

  if !hasCalledCallback {
    hasCalledCallback = true
    onProgressivePhotoSet?()
  }

  return .default
}

KF.url(url)
  .cacheMemoryOnly()
  .keepCurrentImageWhileLoading()
  .fade(duration: 0.5)
  .forceTransition()
  .retry(DelayRetryStrategy(maxRetryCount: 3))
  .downloadPriority(priority.value)
  .progressiveJPEG(progressive)
  .set(to: imageView)

I want to confirm one thing. Do I need to manually set the progressive image in the delegate, or am I doing something wrong? Without that line of code, it doesn't set the progressive photos but only the final one.

[onevcat]

That is weird. It should be set for you! Let me check.

[onevcat]

I checked with the demo scene in the project and it did set automatically.

One thing I can say is wrong was the ImageProgressive.default you are using. It is a shared one and was originally designed as a configuration setter. You should not set onImageUpdated since it does not make a copy of the progressive value and this delegate will be shared and used by all your instances. When you have multiple image views to set, it causes the problem.

Instead, create a new one every time and use that as my snippet above:

let progressive = ImageProgressive(
  isBlur: true,
  isFastestScan: true,
  scanInterval: 0
)
progressive.onImageUpdated.delegate(on: self) { (self, image) in
  if !hasCalledCallback {
    hasCalledCallback = true
    onProgressivePhotoSet?()
  }
  return .default
}

I will see if there is a better way to design the API to avoid misusing it like this.

[onevcat]

Updated the PR and now the ImageProgressive.default is marked as deprecated due to the conflicting semantics.

@jyounus Can you check if replacing your ImageProgressive.default to ImageProgressive() can solve your issue?

[jyounus]

That works much better! Another question, do I need to keep a strong reference to the ImageProgressive instance for each download task?

By the way, I decided to try my existing code with your custom branch (so using imageView.kf.setImage() and have progressiveJPEG enabled with that. It's not working as the release version of Kingfisher. It's not setting the progressive photos. It only sets the final image at the end of the download.

To fix this, I had to instantiate an ImageProgressive object and assign the delegate. This means it will be a breaking change if you merge the PR (as the functionality is different).

I think it would be better if Kingfisher could handle this in the background without having to assign this delegate for each progressive image download explicitly. For example:

imageView.kf.setImage(
  ...
  options: [.progressiveJPEG(.init())]
  ...
)

This should be all that's required if I'm not interested in tracking the progressive images via the new delegate option you have added. This isn't just for the helper extension, it's also for KingfisherManager and the KF builder.

Currently, I have to do this with your branch's code in order to have the same behaviour as the release version of Kingfisher:

let progressive = ImageProgressive()
progressive.onImageUpdated.delegate(on: self) { _, _ in
  return .default
}

...
options: [.progressiveJPEG(progressive)]
...

[onevcat]

do I need to keep a strong reference to the ImageProgressive instance for each download task?

You do not need that! But if anything goes wrong please let me know.

By the way, I decided to try my existing code with your custom branch (so using imageView.kf.setImage() and have progressiveJPEG enabled with that. It's not working as the release version of Kingfisher. It's not setting the progressive photos. It only sets the final image at the end of the download.

Good catch. I guess I was dizzy when doing this. I will update and push another commit for this.

[onevcat]

@jyounus Here it is done! https://github.com/onevcat/Kingfisher/pull/1961/commits/acf79278410cef2371adf11e96ac1f4707f79cad

Could you help to confirm and check how's going on?

[jyounus]

I can confirm it's working as expected now. Thank you for making all these updates! :D

[onevcat]

Fantastic! I am going to merge it and prepare a release soon.