kean / Nuke

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

Progress handler values are incorrect(?) on resumed downloads #187

Closed j531 closed 6 years ago

j531 commented 6 years ago

I use the values from the progress handler to set the progress in a loading bar. This works great for the first download of an image but then produces what feel like semantically incorrect values on resumed downloads of that same image.

To reproduce:

Simple example:

class PrototypeViewController: UIViewController {

    private let imageView = UIImageView()
    private var imageTask: ImageTask?

    override func viewDidLoad() {
        super.viewDidLoad()

        view.addSubview(imageView)

        let startButton = UIButton()
        view.addSubview(startButton)
        startButton.setTitle("Start", for: .normal)
        startButton.addTarget(self, action: #selector(startDownload), for: .touchUpInside)
        let buttonHeight: CGFloat = 40
        startButton.frame = CGRect(x: 0, y: buttonHeight, width: view.bounds.width, height: buttonHeight)

        let cancelButton = UIButton()
        view.addSubview(cancelButton)
        cancelButton.setTitle("Cancel", for: .normal)
        cancelButton.addTarget(self, action: #selector(cancelDownload), for: .touchUpInside)
        cancelButton.frame = CGRect(x: 0, y: buttonHeight * 2, width: view.bounds.width, height: buttonHeight)
    }

    override func viewDidLayoutSubviews() {
        super.viewDidLayoutSubviews()

        imageView.frame = view.bounds
    }

    @objc private func startDownload() {
        let url = URL(string: "https://apod.nasa.gov/apod/image/1809/Broom_Pickering_milne_APODw1200.jpg")!

        imageTask = Nuke.loadImage(
            with: url,
            into: imageView,
            progress: { _, completed, total in
                let progress = (CGFloat(completed) / CGFloat(total)) * 100
                print("Progress:", progress, "Completed:", completed, "Total:", total)
            },
            completion: { (response, error) in
                print("Completed")
        })
    }

    @objc private func cancelDownload() {
        imageTask?.cancel()
    }
}

Is there a setting I can change to alter this behaviour?

Cheers

kean commented 6 years ago

Note that the total data remaining passed to the progress handler is now the value remaining of the download, not the total value of the image.

Yep, that's seems to be how it works right not, but it's not by design.

I think there are two reasonable options that can be implemented instead:

  1. Report the progress of the remaining chunk.
  2. Report the progress of the entire download.

I'm not sure which would to make a default one though. I'd say I would expect a second one. What are your thoughts?

j531 commented 6 years ago

Reporting the progress of the entire download makes the most sense to me.

Implementation-wise I think it would be easy enough to store the expectedContentLength on the first chunk received for that task and just return that as the total from there on.

Right now, I'm front-running the image load task with a HEAD request to get the content-length up front and using that for my calculation. Happy to leave this issue open.

Cheers

kean commented 6 years ago

Nuke total progress value uses expectedContentLength of the response which is derived from the HTTP Content-Length header of the response.

I've performed some tests just to make sure that the Content-Length of the Partial (206) responses report not the total length of the file, but rather the length of the remaining chunk that is being requested:

~> curl https://cloud.githubusercontent.com/assets/1567433/9781817/ecb16e82-57a0-11e5-9b43-6b4f52659997.jpg

HTTP/1.1 200 OK
Last-Modified: Thu, 10 Sep 2015 06:47:19 GMT
ETag: "ee2fa107a9618d19df8d37dd0e40d2fa"
Cache-Control: max-age=31557600
Content-Type: image/jpeg
Content-Length: 31038

~> curl -i -H "Range: bytes=15000-" https://cloud.githubusercontent.com/assets/1567433/9781817/ecb16e82-57a0-11e5-9b43-6b4f52659997.jpg

HTTP/1.1 206 Partial Content
Last-Modified: Thu, 10 Sep 2015 06:47:19 GMT
ETag: "ee2fa107a9618d19df8d37dd0e40d2fa"
Cache-Control: max-age=31557600
Content-Type: image/jpeg
Accept-Ranges: bytes
Age: 478813
Content-Range: bytes 15000-31037/31038
Content-Length: 16038

So it seems that what I need to do is just add the length of the data that has already been downloaded to the expectedContentLength of the partial response and that's it.

kean commented 6 years ago

Fixed in https://github.com/kean/Nuke/commit/91cde7666f7d61585357d27880da7aba74e71072