Feature custom image loader - Supports loader protocol #2256

dreampiggy · 2018-03-21T17:46:53Z

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: #1970 #906 #1702 #697

Pull Request Description

Reason

Though our project SDWebImage's fundamental function is to load the image from network, cache and render in the views. Things become more and more complicated and many user may have different use case

For example, some user may use third-party cloud service such as FireBase, which provide their SDKs to retrive the image or data, so they can not be communicated with SDWebImage's total function easily without hacking. Many user also wants to load the file URL, Photos library as well, which may be a really helpful feature for beginning user. Since we can change things in 5.x, maybe we can make a more abstract layer to allow user to provide their own implementation of image loading system. But also keep the current downloader function with minimal change.

Design

For common image loading system, we can seperate them into two type.

The first one, a image loader will handler all the stuff, including image data retrieving (From network, or local files, whatever) and also the image processing. They can produce all the informationwe need for the final completion block we need for SDWebImageManager. Which means they should provide NSData + UIImage. This is useful for Photos library loader which may use requestImageForAsset, which the data may be nil but the UIImage instance is valid (This can work well with our cache system).

The second one, a image loader may just concentrate on the image data retrieving, and don't care about image processing stuff. We can apply the common image decoding logic from SDWebImage itself to allow same behavior acrossing the coder. Which means they should provide NSData only. This is useful for local file loader, which just need to get the image data.

So, we can design a protocol which allow these two types of image loading process. This apply a abstract layer to let user provide their own image loader. And we can build a image load manager like SDWebImageCodersManager to allow multiple loaders at the same time. For our current SDWebImageDownloader, it should now also conform this protocol.

Implementation

SDWebImageLoader Protocol

typedef void(^SDWebImageLoaderProgressBlock)(NSInteger receivedSize, NSInteger expectedSize, NSURL * _Nullable targetURL);
typedef void(^SDWebImageLoaderCompletedBlock)(UIImage * _Nullable image, NSData * _Nullable data, NSError * _Nullable error, BOOL finished);

@protocol SDWebImageLoader <NSObject>

/**
 Return whether current image loader can load the image with the given URL.
 */
- (BOOL)canLoadWithURL:(nullable NSURL *)url;

@optional
/**
 Load the image and image data with the given URL and return the image data. You're responsible for producing the image instance.
 */
- (nullable id<SDWebImageOperation>)loadImageWithURL:(nullable NSURL *)url
                                             options:(SDWebImageOptions)options
                                            progress:(nullable SDWebImageLoaderProgressBlock)progressBlock
                                           completed:(nullable SDWebImageLoaderCompletedBlock)completedBlock
                                             context:(nullable SDWebImageContext *)context;

@end

Loders Manager

@interface SDWebImageLoadersManager : NSObject <SDWebImageLoader>

@property (nonatomic, strong, readwrite, nullable) NSArray<id<SDWebImageLoader>>* loaders;
- (void)addLoader:(nonnull id<SDWebImageLoader>)loader;
- (void)removeLoader:(nonnull id<SDWebImageLoader>)loader;

@end

dreampiggy · 2018-03-21T17:55:20Z

SDWebImage/SDWebImageDefine.h

@@ -12,6 +12,113 @@ typedef void(^SDWebImageNoParamsBlock)(void);
 typedef NSString * SDWebImageContextOption NS_STRING_ENUM;
 typedef NSDictionary<SDWebImageContextOption, id> SDWebImageContext;

+typedef NS_OPTIONS(NSUInteger, SDWebImageOptions) {


This is just code moving :)

Because that SDWebImageLoader protocol should not import SDWebImageDownloader, or it will cause a cycle import issue.

I can seperate this into another PR after some 5.x feature merged to avoid future merge conflict. Not a big change.

This is already been done in current 5.x branch.

dreampiggy · 2018-03-21T17:57:29Z

SDWebImage/SDWebImageDownloader.m

@@ -192,6 +192,37 @@ - (void)setOperationClass:(nullable Class)operationClass {
    }
 }

+- (id<SDWebImageOperation>)loadImageWithURL:(NSURL *)url options:(SDWebImageOptions)options progress:(SDWebImageLoaderProgressBlock)progressBlock completed:(SDWebImageLoaderCompletedBlock)completedBlock context:(SDWebImageContext *)context {


Code moving as well. Actually the SDWebImageDownloader's basic logic does not change at all.

dreampiggy · 2018-03-21T18:00:03Z

Current break change (Need change code)

SDWebImageManager.h

-[SDWebImageManager initWithCache:downloader:] REPLACED with -[SDWebImageManager initWithCache:loader:]
imageDownloader property REPLCAED with imageLoader.

dreampiggy · 2018-03-21T18:04:03Z

SDWebImage/SDWebImagePhotosLoader.m

+        return nil;
+    }
+
+    if (![[self class] isPhotosStatusAuthorized]) {


Maybe when Photos Library no authorized, we should through a exception ? Or does this authorization check impact the performance ? I will check later. (Currently will check each time you request Photos asset)

dreampiggy · 2018-03-21T18:07:49Z

SDWebImage/SDWebImageManager.m

+                        }
+                        if (!error) {
+                            dispatch_async(strongSubOperation.coderQueue, ^{
+                                UIImage *loadedImage = [[SDWebImageCodersManager sharedManager] decodedImageWithData:data];


Acutally, here is not finished because we supports progressive image loading.

One solution, it's to leave this for other developer to use the loadImageWithURL: one and build their own. Or I can continue to implement it to allow loadImageDataWithURL: progressive loading. (Actually, copy the code in SDWebImageDownloadOperation. Or maybe a better way to share the code ?).

skyline75489 · 2018-04-15T04:09:39Z

These are a lot of changes. I think the priority of the Photos support is not high. Maybe we should hold this PR until other PRs are merged.

dreampiggy · 2018-04-15T04:38:36Z

@skyline75489 :) Most changes is because of some unnessary changes from the early days of 5.x. I can rebase it to the 5.x after that #2278 merged. There should be only less than 10 files get effected.

And because of that id<SDWebImageLoader> changes, this will be API break. Though we can currently not consider Photos Library. The architecture for image loader protocol should be kept, to let 5.x future get function of this, instead of another major version release.

dreampiggy · 2018-04-15T08:44:52Z

@skyline75489 Done...I just remove all the unnecessary changes, only keep the basic design for custom loader protocol.

The Photos Library loader implementation can be create into another PR. I just keep in my temp branch to wait for this been merged.

bpoplauschi · 2018-04-16T08:03:10Z

Some tests are failing

dreampiggy · 2018-04-16T08:38:40Z

This because I change that our complicated decoding logic, from SDWebImageDownloadOperation, into a global method called SDWebImageLoaderDecodeImageData() & SDWebImageLoaderDecodeProgressiveImageData(), this is useful for user who write custom loader to just care about download, but not image processsing. Or we have to copy & paste this code anywhere into the manager. (This is what this PR about)

However, I can't done this to pass the test, because I need to wait for #2283 . That global method does not pass a extra shouldDecompressImages BOOL value, but should grab the information from SDWebImageOptions.

And also, this can be polished to wait for #2280, which can be used to generate the correct cacheKey from NSURL(Currently, I just use URL.absoluteString), with the context arg SDWebImageCacheKeyFilter.

So, currently just ignore this PR. I'll polish this after some dependent PR merged 😅

dreampiggy · 2018-04-16T09:34:55Z

A question. Am I considered too much to introduce two protocol methods for custom image loader ?

typedef void(^SDWebImageLoaderCompletedBlock)(UIImage * _Nullable image, NSData * _Nullable data, NSError * _Nullable error, BOOL finished);
typedef void(^SDWebImageLoaderDataCompletedBlock)(NSData * _Nullable data, NSError * _Nullable error, BOOL finished);

loadImageWithURL:options:context:progress:completed:
loadImageDataWithURL:options:context:progress:completed:

Previously I think the second one is useful to integrate for some common network downloader which does not need to concentrate about image decoding, processing or something to callback the image arg. Just give us the correct NSData. So you can directlly using something like AFNetworking or Alamofire.

However, after some consideration, since we can abstract the common logic for image decoding like that SDWebImageLoaderDecodeImageData() or SDWebImageLoaderDecodeProgressiveImageData(). If user use the second API, they can also easilly get the same result for the first API. Just start a global queue, call that helper function, then you can get the image arg. So easy......

Maybe just leave the first one is enough ? (Actually the first one which need UIImage arg can be helpful for some SDKs which already give UIImage but not NSData for you. Like Photos framework's requestImageForAsset:targetSize:contentMode:options:resultHandler:)

dreampiggy · 2018-04-16T10:26:52Z

SDWebImage/SDWebImageManager.m

@@ -253,12 +245,6 @@ - (SDWebImageCombinedOperation *)loadImageWithURL:(nullable NSURL *)url
                    }

                    BOOL cacheOnDisk = !(options & SDWebImageCacheMemoryOnly);
-


This will be refactored using the custom cache key filter feature. So we pass the cache key filter in context for downloader, downloader use that to calculate cache key (or string from URL). We do not need this hack logic.

mythodeia · 2018-04-16T10:38:18Z

@dreampiggy i like the idea of loadImageWithURL:options:context:progress:completed: which returns a UIImage. it would be helpful

dreampiggy · 2018-04-16T18:59:26Z

@mythodeia @skyline75489 @bpoplauschi For that Photos framework loader, I create a demo CocoaPods and move the code there. You can see that about the way to write a custom image loader.

Project:

SDWebImage-Photos

Just pod install and run, this actually using the latest branch of this PR. It's recommend to run it on your real device because you can check large Photos library images from your iOS device.

PS:

Actually...This is just a test demo and I'm not satisfied with the naming (naming a CocoaPods with -, will cause the headers become like this #import <SDWebImage_Photos/SDWebImagePhotosLoader.h>...
Maybe I can create another project called just SDWebImagePhotosLoader or something. Anyway, it just small thing 😅

dreampiggy · 2018-04-18T12:24:30Z

Done

Solving conflict and will rebase to small commits...Wait :)

…ce the `SDWebImageLoadersManager` to manage multiple loaders with priority

…ess.

… image downloader description to loader

…loader function

bpoplauschi · 2018-04-19T07:07:02Z

Looking good @dreampiggy. I will go ahead and merge it.

bpoplauschi · 2018-04-19T07:18:04Z

@dreampiggy regarding SDWebImage-Photos, I would recommend we move it under github.com/SDWebImage/ so all those extension projects are there. I would also rename it to SDWebImagePhotosFramework or even SDWebImage+PhotosLoader. I saw this naming convention for adding extensions and using +.

dreampiggy · 2018-04-19T07:28:10Z

@bpoplauschi Correct. I create it just for a demo and show for you. And I find that naming is wired if you add - or + (+ is fobbiend in CocoaPods).

Maybe we'd better naming it like SDWebImagePhotosFramework or just SDWebImagePhotos. And I can transfer that if we finish it. Don't be hurry :)

And also, it seems we should also create a SDWebImageFLAnimatedImageBridge or something. I'm not quite good at naming. Maybe it's up to you 😅

bpoplauschi · 2018-04-19T07:42:21Z

I think SDWebImagePhotos is just confusing :) Few people can think it's related to the Photos framework. What do you mean by + is fobbiend?

dreampiggy · 2018-04-19T07:46:17Z

@bpoplauschi

➜  GitHub git:(master) ✗ pod lib create SDWebImage+PhotosFramework
[!] The Pod name cannot contain plusses.

Usage:

    $ pod lib create NAME

      Creates a scaffold for the development of a new Pod named `NAME` according to
      the CocoaPods best practices. If a `TEMPLATE_URL`, pointing to a git repo
      containing a compatible template, is specified, it will be used in place of the
      default one.

Options:

    --template-url=URL   The URL of the git repo containing a compatible template
    --silent             Show nothing
    --verbose            Show more debugging information
    --no-ansi            Show output without ANSI codes
    --help               Show help banner of specified command

And here is the code showing the reason: CocoaPods/CocoaPods

The reason may belong to the issue that the build script will parse the path wrong if you have + in Pod name...So why I try that - instead. However, this will also cause your Pod headers files become _(underscore). Maybe the best naming is using CamelCase...

bpoplauschi · 2018-04-19T08:35:48Z

I see. Thanks for the details. CamelCase is ok, but not that readable. Anyway, we can go with CamelCase.

bpoplauschi · 2018-07-25T09:17:59Z

@dreampiggy I'm updating the diagrams for the 5.0.0 release and I can't figure out what the SDImageLoadersManager is doing. It's not being used from other classes. SDWebImageManager directly uses its instance of SDImageLoader. Can you give me a hint?

dreampiggy · 2018-07-25T10:52:02Z

@bpoplauschi The SDImageLoadersManager and the correspond SDImageCachesManager, are the multiple loader/cache system useful for people, who want multiple cache source, or multiple loader. See the wiki loaders-manager.

For example, you want to support both Network image URL, file path URL, as well as Photos URL for UIImageView loading. You can bind multiple loaders into the loaders manager, and set as SDWebImageManager.defaultLoader. So that it can be really useful for wide usage.

As compatible reason (See that SDWebImageManager.m comment), the default cache & manager are just SDImageCache.sharedImageCache && SDWebImageDownloader.sharedDownloader, to keep the most compatible behavior from 4.x to 5.x. So this is why diagrams dependency not hanlde these class (But there are test cases cover these classes' function).

bpoplauschi · 2018-07-25T11:08:44Z

@dreampiggy I see, they can be used via defaultImageLoader or similar.

bpoplauschi · 2018-07-25T11:09:52Z

I must say this is not too intuitive :)

dreampiggy added feature refactoring labels Mar 21, 2018

dreampiggy added this to the 5.0.0 milestone Mar 21, 2018

dreampiggy requested review from bpoplauschi, mythodeia and skyline75489 March 21, 2018 17:46

dreampiggy commented Mar 21, 2018

View reviewed changes

dreampiggy force-pushed the feature_custom_loader branch from 0f58946 to fa45269 Compare March 22, 2018 03:23

This was referenced Apr 2, 2018

Refactor cache - Support custom memory cache and disk cache #2195

Closed

Added initial support for ALAsset and PHAsset #1702

Closed

SDWebImageManager uses DISPATCH_QUEUE_PRIORITY_HIGH and downloadOperationQueue is redundant #2268

Closed

dreampiggy mentioned this pull request Apr 13, 2018

Refactor cache - Support custom web cache #2278

Merged

8 tasks

dreampiggy force-pushed the feature_custom_loader branch from fa45269 to 51fa2fb Compare April 15, 2018 08:42

dreampiggy changed the title ~~Feature custom loader (Photos Library / File URL supports)~~ Feature custom image loader - Supports loader protocol Apr 15, 2018

dreampiggy mentioned this pull request Apr 16, 2018

Refactor downloader config #2263

Merged

8 tasks

dreampiggy added the DO_NOT_MERGE label Apr 16, 2018

dreampiggy force-pushed the feature_custom_loader branch 2 times, most recently from 94e6f27 to 6832284 Compare April 16, 2018 09:15

dreampiggy force-pushed the feature_custom_loader branch from 84bca88 to 5baeae7 Compare April 16, 2018 10:17

dreampiggy commented Apr 16, 2018

View reviewed changes

dreampiggy force-pushed the 5.x branch from ed8b657 to 7cba215 Compare April 17, 2018 06:21

dreampiggy mentioned this pull request Apr 17, 2018

SDWebImage 5.0.0 release checklist #2285

Closed

52 tasks

dreampiggy force-pushed the feature_custom_loader branch 2 times, most recently from db526ab to dc98660 Compare April 17, 2018 12:13

dreampiggy removed the DO_NOT_MERGE label Apr 17, 2018

dreampiggy force-pushed the feature_custom_loader branch 2 times, most recently from 294f841 to 5159f8c Compare April 18, 2018 07:58

dreampiggy added the DO_NOT_MERGE label Apr 18, 2018

dreampiggy added 2 commits April 18, 2018 20:26

Add SDWebImageLoader protocol for custom image loader. Also introdu…

57db312

…ce the `SDWebImageLoadersManager` to manage multiple loaders with priority

Update the loader protocol to refactor the common image decoding proc…

8292c0c

…ess.

dreampiggy force-pushed the feature_custom_loader branch from 5159f8c to 23dcaf6 Compare April 18, 2018 12:43

dreampiggy added 2 commits April 18, 2018 20:46

Refactor to only use one publish protocol for custom loader. Renaming…

60759f8

… image downloader description to loader

Add tests for custom loader protocol, using a test loader to specify …

d507442

…loader function

dreampiggy force-pushed the feature_custom_loader branch from 23dcaf6 to d507442 Compare April 18, 2018 12:46

dreampiggy removed the DO_NOT_MERGE label Apr 19, 2018

bpoplauschi merged commit 09cb3ec into SDWebImage:5.x Apr 19, 2018

bpoplauschi deleted the feature_custom_loader branch April 19, 2018 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature custom image loader - Supports loader protocol #2256

Feature custom image loader - Supports loader protocol #2256

dreampiggy commented Mar 21, 2018 •

edited

dreampiggy Mar 21, 2018

dreampiggy Apr 15, 2018

dreampiggy Mar 21, 2018

dreampiggy commented Mar 21, 2018

dreampiggy Mar 21, 2018 •

edited

dreampiggy Mar 21, 2018 •

edited

skyline75489 commented Apr 15, 2018

dreampiggy commented Apr 15, 2018 •

edited

dreampiggy commented Apr 15, 2018 •

edited

bpoplauschi commented Apr 16, 2018

dreampiggy commented Apr 16, 2018 •

edited

dreampiggy commented Apr 16, 2018 •

edited

dreampiggy Apr 16, 2018 •

edited

mythodeia commented Apr 16, 2018

dreampiggy commented Apr 16, 2018 •

edited

dreampiggy commented Apr 18, 2018 •

edited

bpoplauschi commented Apr 19, 2018

bpoplauschi commented Apr 19, 2018

dreampiggy commented Apr 19, 2018 •

edited

bpoplauschi commented Apr 19, 2018

dreampiggy commented Apr 19, 2018 •

edited

bpoplauschi commented Apr 19, 2018

bpoplauschi commented Jul 25, 2018

dreampiggy commented Jul 25, 2018 •

edited

bpoplauschi commented Jul 25, 2018

bpoplauschi commented Jul 25, 2018

		@@ -253,12 +245,6 @@ - (SDWebImageCombinedOperation )loadImageWithURL:(nullable NSURL )url
		}

		BOOL cacheOnDisk = !(options & SDWebImageCacheMemoryOnly);

Feature custom image loader - Supports loader protocol #2256

Feature custom image loader - Supports loader protocol #2256

Conversation

dreampiggy commented Mar 21, 2018 • edited

New Pull Request Checklist

Pull Request Description

Reason

Design

Implementation

SDWebImageLoader Protocol

Loders Manager

dreampiggy Mar 21, 2018

Choose a reason for hiding this comment

dreampiggy Apr 15, 2018

Choose a reason for hiding this comment

dreampiggy Mar 21, 2018

Choose a reason for hiding this comment

dreampiggy commented Mar 21, 2018

Current break change (Need change code)

SDWebImageManager.h

dreampiggy Mar 21, 2018 • edited

Choose a reason for hiding this comment

dreampiggy Mar 21, 2018 • edited

Choose a reason for hiding this comment

skyline75489 commented Apr 15, 2018

dreampiggy commented Apr 15, 2018 • edited

dreampiggy commented Apr 15, 2018 • edited

bpoplauschi commented Apr 16, 2018

dreampiggy commented Apr 16, 2018 • edited

dreampiggy commented Apr 16, 2018 • edited

dreampiggy Apr 16, 2018 • edited

Choose a reason for hiding this comment

mythodeia commented Apr 16, 2018

dreampiggy commented Apr 16, 2018 • edited

dreampiggy commented Apr 18, 2018 • edited

bpoplauschi commented Apr 19, 2018

bpoplauschi commented Apr 19, 2018

dreampiggy commented Apr 19, 2018 • edited

bpoplauschi commented Apr 19, 2018

dreampiggy commented Apr 19, 2018 • edited

bpoplauschi commented Apr 19, 2018

bpoplauschi commented Jul 25, 2018

dreampiggy commented Jul 25, 2018 • edited

bpoplauschi commented Jul 25, 2018

bpoplauschi commented Jul 25, 2018

dreampiggy commented Mar 21, 2018 •

edited

dreampiggy Mar 21, 2018 •

edited

dreampiggy Mar 21, 2018 •

edited

dreampiggy commented Apr 15, 2018 •

edited

dreampiggy commented Apr 15, 2018 •

edited

dreampiggy commented Apr 16, 2018 •

edited

dreampiggy commented Apr 16, 2018 •

edited

dreampiggy Apr 16, 2018 •

edited

dreampiggy commented Apr 16, 2018 •

edited

dreampiggy commented Apr 18, 2018 •

edited

dreampiggy commented Apr 19, 2018 •

edited

dreampiggy commented Apr 19, 2018 •

edited

dreampiggy commented Jul 25, 2018 •

edited