Skip to content
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

Use SDWebImageAvoidDecodeImage to allow user to control force decode feature for individual image request #2283

Merged

Conversation

dreampiggy
Copy link
Contributor

@dreampiggy dreampiggy commented Apr 15, 2018

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

This merge request fixes / reffers to the following issues: #1927

Pull Request Description

Reason

This is about that decompressImages feature. We all know that what decompressImages will do. It actually call sd_decodedImageWithImage: in the background, to draw the image from Image/IO on the bitmap context, this can help to force decode the image. Then when we set this UIImage on the UIImageView, it does not cause any lag or blocking on the main queue to wait for Core Animation firstly decode it.

This options is good for performance, and it seems that most of application use this well. However, due to the force decode process, this will consume more memory, especially for huge images. It's a trade-off between Time & Space.

In previous SDWebImage, we have a option to enable/disable this feature. We have these:

  • SDImageCache: SDImageCache.config.shouldDecompressImages
  • SDWebImageDownloader: SDWebImageDownloader.shouldDecompressImages
  • SDWebImageDownloadOperation: SDWebImageDownloadOperation.shouldDecompressImages

But however, they are both central control. Which means if I disable on in shared downloader, or shared cache, this will effect all the image request for this. In most cases, we do not want this. Because if user want to set this option for individual image request, they have no way to do this, except create another SDImageCache and SDWebImageDownloader instance, which is really heavy on usage and cause many problem when syncing for shared cache/downloader.

From the real world application experience by our company(Such as Tik Tok), most images are small enough (less than 1000x1000), the force decode process can help to increase nearly 4-5 frame rates on common UITableView or UIScrollView. It's quite rare we need turn it off, unless some huge images which consume much memory.

So, we'd better move that option, into SDWebImageOptions level, to allow we control it for each image request from sd_setImageWithURL:

Design

From the description above, we should treat force decode ON as the default value. And this options can be used both in manager, cache, downloader. So we just put it in the SDWebImageOptions. Naming it SDWebImageAvoidDecodeImage to specify that you can avoid this force decode feature.

There are also the correspond options for SDImageCacheOptions and SDWebImageDownloaderOptions, keep it the same name as usual.

Implementation

/**
 * By default, we will decode the image in the background during cache query and download from the network. This can help to improve performance because when rendering image on the screen, it need to be firstly decoded. But this happen on the main queue by Core Animation.
 * However, this process may increase the memory usage as well. If you are experiencing a issue due to excessive memory consumption, This flag can prevent decode the image.
 */
SDWebImageAvoidDecodeImage = 1 << 17

…e feature for individual image request. Replace all the central control for `decompressImages`
@dreampiggy
Copy link
Contributor Author

dreampiggy commented Apr 15, 2018

Current break change (Need change code)

SDImageCacheConfig.h

  • shouldDecompressImages REMOVED. Use SDImageCacheAvoidDecodeImage in cache options instead.

SDWebImageDownloader.h

  • shouldDecompressImages REMOVED. Use SDWebImageDownloaderAvoidDecodeImage in downloader options instead.

SDWebImageDownloadOperation.h

  • shouldDecompressImages REMOVED. Use SDWebImageDownloaderAvoidDecodeImage in downloader options instead.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (5.x@1367b18). Click here to learn what that means.
The diff coverage is 25%.

Impacted file tree graph

@@          Coverage Diff           @@
##             5.x    #2283   +/-   ##
======================================
  Coverage       ?   42.75%           
======================================
  Files          ?       51           
  Lines          ?     6063           
  Branches       ?        0           
======================================
  Hits           ?     2592           
  Misses         ?     3471           
  Partials       ?        0
Impacted Files Coverage Δ
SDWebImage/SDImageCacheConfig.m 100% <ø> (ø)
SDWebImage/SDWebImageDownloaderConfig.m 88.88% <ø> (ø)
SDWebImage/SDWebImageDownloader.m 79.77% <ø> (ø)
SDWebImage/SDImageCache.m 53.2% <0%> (ø)
SDWebImage/SDWebImageDownloaderOperation.m 73.26% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1367b18...781c079. Read the comment docs.

Copy link
Contributor

@mythodeia mythodeia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dreampiggy dreampiggy changed the title Use SDWebImageAvoidDecodeImage to allow user to control force decod… Use SDWebImageAvoidDecodeImage to allow user to control force decode feature for individual image request Apr 16, 2018
@dreampiggy dreampiggy merged commit c5647c8 into SDWebImage:5.x Apr 16, 2018
@bpoplauschi bpoplauschi deleted the feature_force_decode_individual_request branch April 16, 2018 09:06
@ripperhe
Copy link

ripperhe commented May 8, 2019

I need central control. Can I set the default Option to SDWebImageAvoidDecodeImage?
Is there any code equivalent to the following two sentences?

[[SDImageCache sharedImageCache] setShouldDecompressImages:NO];
[[SDWebImageDownloader sharedDownloader] setShouldDecompressImages:NO];

@ripperhe
Copy link

ripperhe commented May 8, 2019

@dreampiggy 呃呃呃,帮忙看看

@dreampiggy
Copy link
Contributor Author

dreampiggy commented May 8, 2019

@ripperhe You and @welsonxue have the exact opposite request for this option. 😅

To solve this problem, there are 3 solutions, maybe...

Change your code

You can try build a wrapper category method, which always pass the default option SDWebImageAvoidDecodeImage to the original sd_setImageWithURL: API. Then change all your code to call your wrapper one instead.

@implementation UIImageView (MyWebCache)
- (void)my_setImageWithURL:(NSURL *)url options:(SDWebImageOption)options {
    SDWebImageOption newOptions = SDWebImageAvoidDecodeImage | options;
    [self sd_setImageWithURL:url options:newOptions];
}
@end

See comment here: #2679. Similar problem.

Allows global control to override

We can use a global control, which override your SDWebImageOption. However, this is a problem: How can we override ?

This enum can only represent two meaning: YES (use force decoding) or NO (do not use). It can not represent this meanning: default (use the global control instead). Consider this code:

SDWebImageManager.sharedManager.avoidDecodeImage = NO;
[imageView sd_setImageWithURL:url option:SDWebImageAvoidDecodeImage];
// What's the final result for this option ?

One way for this, it to use the 5.0 new context option, which use a 3-case enum, or can use @(YES) for YES, @(NO) for NO, and nil for default.

This may cause API breaking, in my opinion. Or we need to deprecate the old option SDWebImageAvoidDecodeImage to use new context option like SDWebImageContextForceDecodeImage instead.

Add a default options feature

We can introduce a feature called default options, which will append the default option to final SDWebImageOptions.

This can also fix other related option where user want to take both global control && per-image-request control like SDWebImageDecodeFirstFrameOnly.

But actually, this feature have one disadvantage, how to solve the problem when some image request don't want the default option, but others want ?

SDWebImageManager.sharedManager.defaultOptions = SDWebImageAvoidDecodeImage;
[imageView sd_setImageWithURL:url option:0]; // How to do if I really want this image request, to trun the force decode on?
// Maybe something like this ?
[imageView sd_setImageWithURL:url option:SDWebImageIgnoreDefaultOptions]; // Ignore default options appending, final options result to 0

Which solution to use, need some discussion. You can create a new feature request for this.

@ripperhe
Copy link

ripperhe commented May 8, 2019

Thank you very much for your reply. I get a rough idea of what you mean.

I finally decided to use version 4.4.6 at the moment and update it later when default options are available.

There are too many places to use SDWebImage, and I'm not sure what risks there are.I'm looking forward to the next version.

@dreampiggy
Copy link
Contributor Author

Moved the discussion and feature request to #2719

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants