New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Progressive decoding for large images run too frequently and continue even when download finished #2477

Open
dreampiggy opened this Issue Sep 8, 2018 · 14 comments

Comments

Projects
None yet
3 participants
@dreampiggy
Contributor

dreampiggy commented Sep 8, 2018

New Issue Checklist

Issue Info

Info Value
Platform Name ios
Platform Version 11.4
SDWebImage Version 4.4.3 && 5.0.0-beta3
Integration Method cocoapods
Xcode Version Xcode 9
Repro rate all the time (100%)
Repro with our demo prj e.g. does it happen with our demo project? YES
Demo project link e.g. link to a demo project that highlights the issue

Issue Description and Steps

What's the issue

Progressive decoding in SDWebImage 4.x && 5.x, seems consume too much of CPU for large images (when image resolution 2000 * 2000 or above). And this cause high rate of OOM && battery usage. The most important, the progressive decoding will continue, even when all image data download finished. For example, this image url: http://ims.haiziguo.cn/qa/parent/6/201808/0bcc1bc2b2f4a246f1528c38479daa43.jpg. Contains 3024*4032 pixels, the image data (3.7MB) is downloaded in nearlly 5 seconds, but the progressive decoding time take more than 10 seconds and finally callback the sd_setImage(with:placeholderImage:options:completed:) completion block. It's not as expected.

What you expected

To say, in my opinion. The progressive decoding, should not continue when all download finished, since we have all pixels, it's time to callback the compltion block and render the full pixel images. And the cost during progressive decoding, should be limited. In 5.x there is one config called minimumProgressInterval. But that does not solve all of this problem. The progressive decoding still block full pixel decoding until previous one was finished.

Demo code to reproduce this issue:

override func viewDidLoad() {
    super.viewDidLoad()
    
    let imageView = UIImageView()
    imageView.contentMode = .scaleAspectFit;
    imageView.backgroundColor = .red
    self.view.addSubview(imageView)
    imageView.frame = self.view.bounds
    let url = URL(string: "http://ims.haiziguo.cn/qa/parent/6/201808/0bcc1bc2b2f4a246f1528c38479daa43.jpg")
    imageView.sd_setImage(with: url, placeholderImage: nil, options: [.progressiveLoad], completed: nil)
}

Run the demo without patch from #2478 will crash because of OOM.
Run the demo with patch, you will see the images rendering really slow. Even when network finished, the progerssive loading still happended. And the CPU consumption is really high until the progressive loading finished.

@bpoplauschi

This comment has been minimized.

Member

bpoplauschi commented Sep 8, 2018

  • I don't think doing work on the NSURLSession delegate queue is ok. That must remain available for network ops
  • I think letting the user control on which queue we decode is a bad idea, they should not need to know these details
  • I did like the idea of decoding on a dedicated queue. I do not like the idea of each download operation having another queue.
  • I think we should just use a single queue for all decoding, which is concurrent - multiple downloads at the same time could decode concurrently
  • about progressive decodes, I think we need a mechanism to cancel an existing progressive decode if we already have another batch of data. For this, NSOperation would work better.
  • I would try this:
    • create a NSOperationQueue and use it from all the download operations. We could just create it in our SDWebImageDownloader and pass it to the operations.
    • leaving maxConcurrentOperationCount to its default value means based on the system conditions, multiple operations can run at the same time
    • when adding a progressive decoding operation (can simply be NSBlockOperation blockOperationWithBlock), we need to store it (as a download operation property) and when trying to add another progressive operation, first cancel the existing one. This way we can have only one progressive download decode at a time for a certain download.

Ideas?

@bpoplauschi bpoplauschi referenced this issue Sep 9, 2018

Open

SDWebImage 5.0.0 release checklist #2285

26 of 27 tasks complete

bpoplauschi added a commit to bpoplauschi/SDWebImage that referenced this issue Sep 9, 2018

For now, just a demo of an idea regarding SDWebImage#2477. Using `NSO…
…perationQueue` to run the decoding. Always have just one progressive decode op per download.
@bpoplauschi

This comment has been minimized.

Member

bpoplauschi commented Sep 9, 2018

See #2480 per my comment

@zhongwuzw

This comment has been minimized.

Member

zhongwuzw commented Sep 10, 2018

My main opinion is the same as @bpoplauschi .

  1. Don't do progressive job on session delegate queue. It would block the progress notification and any other delegate callbacks.
  2. One queue for progressive decoding.

I did a PR #2481 to demonstrate my opinion.What I added:

  • Maintain a dependency list, one operation, one running progressive decoding.
  • User can set max decoding count of images.
  • Remove imageData copy for call of each didRecieveData delegate . imageData may occupy big size, if we maintain each copy, spikes would be produce.
  • Added decode image cancellation when download task cancelled.
@zhongwuzw

This comment has been minimized.

Member

zhongwuzw commented Sep 10, 2018

@bpoplauschi

when adding a progressive decoding operation (can simply be NSBlockOperation blockOperationWithBlock), we need to store it (as a download operation property) and when trying to add another progressive operation, first cancel the existing one. This way we can have only one progressive download decode at a time for a certain download.

Emm, this cancellation may not work if last operation is decoding. And may leads to issues because progressCoder is shared.

@dreampiggy

This comment has been minimized.

Contributor

dreampiggy commented Sep 10, 2018

I think maybe we consider too much. We can simplify the problems.

The previous behavior before 4.2.0, we do progressive decoding synchronously in URLSession:dataTask:didReceiveData: method. So the progressBlock callback speed is slower than the normal decoding even for the same image URL. But they share the same completionBlock callback time duration.

In 4.2.0 we change it, and now the progressBlock callback speed is the same as normal decoding even for the same image url. But the progressive decoding completionBlock callback time duration is larger than the normal decoding (When the image resolution is large).

People who use 4.2.0 and above does not complain about why when using SDWebImageProgressDownloader the progressBlock callback speed is lower than normal. So we can just fix it with #2478.

If you two and more our user, agree that for progressive decoding && normal decoding for the same url, the progressBlock callback times should be equal. Then we just need to fix this behavior.

To fix it, I think we don't need to hold a global shared operation queue or something. We just need to use NSOperationQueue instead of the dispatch queue. Then ensure you can only do progressive decoding if current operationCount is 1. No any header files should be modified.

@dreampiggy

This comment has been minimized.

Contributor

dreampiggy commented Sep 10, 2018

See the 3 different solutions' behavior:

Solutioin(PR) progress callback ratio for same url total completion callback duration for same url progressBlock times equals to image refreshing times multiple urls effect total callback duration
previous (4.2.0) progressive < normal progressive == normal YES YES
current progressive == normal progressive > normal (buggy) NO NO
#2478 progressive < normal progressive == normal YES YES for progressive, NO for normal
#2479 the same as #2478 when avoidConcurrentDecoding; keep current when no options - - -
#2483 progressive == normal progressive == normal NO NO
@zhongwuzw

This comment has been minimized.

Member

zhongwuzw commented Sep 12, 2018

@dreampiggy The reason why we can't use one NSOperationQueue or dispatch_queue_t for one operation is:
GCD has its limitation, we can't create much queue or put many tasks on global queue, it would leads much threads overhead, and more severe, thread explosion.

You can check the sessions about GCD from WWDC. And if you already see before, I recommend you look Image and Graphics Best Practices WWDC 2018. Your implementation is what Apple said not to do.

@dreampiggy

This comment has been minimized.

Contributor

dreampiggy commented Sep 13, 2018

@zhongwuzw I guess maybe we think over than the current context of problems. Make it too complicated.

The SDWebImageDownloadOperation is running on the URLSession, for each URLSesson, they can only run 4 concurrent network downlaod task (See HTTPMaximumConnectionsPerHost).

So I don't think add an nother operation queue management component is useful. We just need to find one most suitable behavior (maybe from ourself as well as some other user) for this. Don't need to change too much of architecture.

@zhongwuzw

This comment has been minimized.

Member

zhongwuzw commented Sep 13, 2018

@dreampiggy Emm, maybe I and you cannot reach a consensus 🤔 . Maybe needs more people to get in. But firstly, I can list some reasons why we should change the architecture:

From usage of GCD

  • From Apple、other famous open source framework、or my experience, one queue to one subsystem or module is the correct one.
  • Thread explosion.
  • Thread context switch.
  • More queue not equal to more performance.

From a view of a third framework

  • SD is just a image loading framework, which means, it's just a small part of an App, so this is the reason we should be more careful when uses CPUmemorynetwork or other system resources.
  • HTTPMaximumConnectionsPerHost has limits, but we can't assume user would use one host to load images, I think your company also would not use only one host, because we should consider server load balance, CDN or Domain Hijacking and so on. And if you trust HTTPMaximumConnectionsPerHost, maybe we can remove downloadQueue, it has no meaning any more 😢 , we can't right?
  • Just like the previous PR about adding autorelease pool to dispatch_queue, why I propose refactor these things is I see many people complain the crash, I think maybe many users may not see the crash log. We should change it.
  • By adding a NSOperationQueue, we can expose it to user like downloadQueue, provide user a more robust, modular, and configurable API. I think API breaking is needed, actually, it didn't change too much, easy to adopt for users.
  • I think change some API is adoptable, framework needs to optimize, just like 5.x, I don't mean my solution is the best, but we should try to do the right things, other than just do some small changes, and think maybe it satisfy the needs. We never can expect users do anything.

So, except you said the API breaking , I just can't get any reason that we should insist to keep one queue to one `operation.

@dreampiggy

This comment has been minimized.

Contributor

dreampiggy commented Sep 13, 2018

? I mean it's no use to manually add any operation queue manager by yourself. What main propose of that change ?

This issue is about not oh I want to profile and get the best performance on the progressive decoding. I just mean we should keep a balance and most acceptable behavior of it. If people want detailed control of download && decoding. They can use Custom Download Operation.

The decoding is part of download post-process, not a standalone process should be handled globally. If we decide to change everything related to decoding into the global management. We have to break many APIs, including the cache (contains decoding && encoding), the transformer (contains image process code), the image rendering (contains JIT decoding), and so on. I don't think it's good or suitable for solving this problem.

Maybe I think we shoud collect more idea from our user. Totally revert the code is another backup way, but I just want to ensure whether previous behavior(4.2.0 before) is suitable for most of people.

@zhongwuzw

This comment has been minimized.

Member

zhongwuzw commented Sep 13, 2018

@dreampiggy Please see my PR #2481 , it express what I want to do currently. Or #2480 make by @bpoplauschi .

@stale

This comment has been minimized.

stale bot commented Oct 26, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions.

@stale stale bot added the stale label Oct 26, 2018

@zhongwuzw zhongwuzw removed the stale label Oct 27, 2018

@zhongwuzw zhongwuzw referenced this issue Oct 27, 2018

Open

Memory Issues Problem #2513

0 of 3 tasks complete

@zhongwuzw zhongwuzw referenced this issue Nov 9, 2018

Open

Refactor progressive decode image #2481

3 of 8 tasks complete
@stale

This comment has been minimized.

stale bot commented Nov 26, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions.

@stale stale bot added the stale label Nov 26, 2018

@stale stale bot closed this Dec 3, 2018

@bpoplauschi bpoplauschi reopened this Dec 9, 2018

@stale stale bot removed the stale label Dec 9, 2018

@bpoplauschi

This comment has been minimized.

Member

bpoplauschi commented Dec 9, 2018

Reopen until we apply one of the fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment