Refactory the FirebaseUI-Storage binding of SDWebImage using the 5.0 modern Custom Image Loader API.

[dreampiggy]

Some of old public API is not compatible for the Custom Image Loader. Need upgrade or check later

Hey there! So you want to contribute to FirebaseUI? Before you file this pull request, follow these steps:

• Read the contribution guidelines.
• If this has been discussed in an issue, make sure to mention the issue number here. If not, go file an issue about this to make sure this is a desirable change.
• If this is a new feature please co-ordinate with someone on FirebaseUI-Android to make sure that we can implement this on both platforms and maintain feature parity.

Propose

Hi, Googlers. 😃

I'm the collaborator and maintainer of the iOS image loading framework, SDWebImage. I found that Firebase provide a UI-binding for some third-party framework. Like this FirebaseUI-Storage. Which binding the storage API with SDWebImage's API.

We'll recentlly release SDWebImage 5th major version update. The 5.0 version, it's aimed to provide a extensible and powerful architecture, to let SDWebImage be used for more posibility and work together with others.

In 5.0, we refactoried our framework, to make both Cache and Image Downloader become customizable. See the wiki page about this: Custom Loader. Which it aimed to emmbed third-party or Apple's system framework to become a downloader instead of the default SDWebImageDownloader.

Actually, it's not hard to write one custom loader. And it's a great replacement for some hack code before 5.0. We have already done one custom loader for Apple's PhotoKit, see SDWebImagePhotosPlugin

So, as a maintainer of SD, as well as open-source contributor. I'd want to contribute to use the new 5.0 API to update the binding of FirebaseUI-Storage component.

Why WIP

SDWebImage 5.0 is current in the last beta. It's aimed to be released in April. So I create this MR to show how to use the API. After the 5.0.0 released, maybe I can update the MR of Podspec dependency and remove the WIP.

Changes

After I checking the current FirebaseUI-Storage implementation, there are one UIImageView+FirebaseStorage category. All logic is here, including cache query and downloader.

With the custom loader API of SDWebImage. The user don't need to worry about the cache component, and don't need to carry about the decoding component. They are all built-in but replaceable.

During some effort, today I created one simple implementation using new API. I create one SDWebImageFirebaseLoader class for custom loader, and also the related component, for example, the context option for the maxImageSize arg, the NSURL category to build a URL representing FIRStorageReference.

But there are still some detail changes need to be polished, and there are also some incompatible API need to find a solution. And some usage make me a little confused and I want to get some reply about it.

Problems

Problem1. cache arg in UIImageView category

The sd_setImageWithStorageReference:maxImageSize:placeholderImage:cache:completion: have a cache arg. The description is below:

An image cache to check for images before downloading.

But I check the implementation files, it seems user can provide a nil, to totally disable the cache query. Is this is designed behavior ? If so, I can use the 5.0's new option SDWebImageFromLoaderOnly to totally avoid cache query for single image request.

Problem2. The cache key for FIRStorageReference *

Current version, the UIImageView category, using FIRStorageReference.fullPath as its cache key.

From the comment, the fullPath represent the Path, without the bucket. Like this:

path/to/object.jpg

But when using the new API constructed with +[NSURL sd_URLWithStorageReference:], the default cache key will become this one (By default, we using the absoluteString as cache key):

gs://bucket/path/to/object.jpg

Yes I can keep the old version's compatible using the custom cache key mapping. But I have one question: using only fullPath as cache key, how to solve cache conflict for these two Firebase path ?

gs://test_bucket/path/to/object.jpg
gs://deploy_bucket/path/to/object.jpg

I think using the full URL's string it's a better way. But note this will break old version compatible for caches already on the device.

Problem3. The return value FIRStorageDownloadTask * in UIImageView category

The sd_setImageWithStorageReference:placeholderImage: series method, have a return value, which type is FIRStorageDownloadTask *. The description is below

Returns a FIRStorageDownloadTask if a download was created (i.e. image could not be found in cache).

But when I want to using the new API. I found the problem. Why ? Because the key point of SDWebImage original loading logic, it's the following pipeline.

• Query Memory Cache (sync), if failed
• Query Disk Cache (async), if failed
• Download from network (async), if failed // <----Only here we have the download task
• Mark load failed

When I check the implementation, I surprisely found that current implementation, will query disk cache on the main queue, and even do a image decoding on the main queue (!). The code is here:
https://github.com/firebase/FirebaseUI-iOS/blob/v6.2.1/Storage/FirebaseStorageUI/UIImageView%2BFirebaseStorage.m#L104-L111

Decoding a compressed image data is really time-consuming (for example, you're using WebP and libwebp codec, a 100KB WebP may need 200ms to be decoded on iPhone 7). And this call will blocking the main queue for long time. It's not a good idea for UI rendering.

So, instead, currentlly I mark this issue, and suggest to change the return value from FIRStorageDownloadTask * to void.

I know some people, who use FIRStorageDownloadTask, it's to monitor prorgess changes of the download percent. Here I introduce a new arg progressBlock which represent the progress change. It's the same arg like SDWebImage original category.

But since this change it's an API-breaking, I wonder whether or not it's suitable to do here. So I need your guys' reply.

More enhancement in future

The Custom Image Loader, actually can support the progressive image loading and rendering. Like our built-in SDWebImageDownloader with SDWebImageProgressiveLoad option. It's also supported by the PhotosKit loader using the Apple's API PHImageRequestOptionsDeliveryModeOpportunistic.

It's not hard to implements since the decoding part it built-in. Custom Loader just need to supply a image data progressively.

Current I found there are some API to grab the current downloaded data. But it seems need some private API ?(Or am I wrong?). So I don't implements that now. Maybe todo in the future.

Final

Sorry for this long MR description. I want to express my idea about this changes, and also some consideration on behalf of SDWebImage's maintainer. If this MR it's not suitable or have something wrong on the implementation, I can check and change again. I'm glad to cooperate with FirebaseUI community and provide the help of SDWebImage.

[dreampiggy]

See: https://stackoverflow.com/questions/40480503/the-bundle-uitests-couldn-t-be-loaded-because-it-is-damaged-or-missing-necessary
and
CocoaPods/CocoaPods#8139

Current this Podfile seems cause the FirebaseStorageUITests run failed because it does not copy the Pod frameworks into the Test runner application.

[morganchen12]

Thanks for doing this @dreampiggy!

Agreed that the API should be made async and return void instead of returning a download task. This is something I wanted to do for a while, but haven't had the time to. Moving the image-constructing logic off of the main thread is a significant bonus as well.

We have a scheduled major breaking change coming up in May when Google I/O happens, since we need to bump our Firebase dependency by a major version anyway. It's ok to make breaking changes in this pull request.

[dreampiggy]

Thanks for your reply. So this means I can polish the API to match it match the current 5.0 version of SD.

Your reply is about the question 3. Which means I can change the FIRStorageDownloadTask * return value to avoid.

How about another 2 questions ?

1. cache arg in UIImageView category

If this arg is useless for most of people, I can suggest to remove this arg. Advanced user, who want to customize single image request level cache instance, can still using the context option:

// assign a new cache instance
SDWebImageManager *manager = [[SDWebImageManager alloc] initWithCache:SDImageCache.new loader:SDWebImageFirebaseLoader.sharedLoader];
// load with context option
[sd_setImageWithStorageReference:storageRef maxImageSize:size placeholderImage:nil options:0 context:@{SDWebImageContextCustomManager: manager}];

User who don't want to query cache, can using the SDWebImageFromLoaderOnly option.

// load with context option
[sd_setImageWithStorageReference:storageRef maxImageSize:size placeholderImage:nil options:SDWebImageFromLoaderOnly];

Both of these two use case are handled. Maybe we don't need this one cache arg. It's a little strange for user who is already famaliar with SDWebImage's API.

But anyway, it's my idea. If we need to keep the cache arg, I can still implement this in the implementation file, which do the same thing as users. But the full arg list of the method will become a little longer.

- (void)sd_setImageWithStorageReference:(FIRStorageReference *)storageRef
                           maxImageSize:(UInt64)size
                       placeholderImage:(nullable UIImage *)placeholder
                                  cache:(id<SDImageCache>)cache // In 5.0, image cache is protocol but not class
                                options:(SDWebImageOptions)options
                                context:(nullable SDWebImageContext *)context
                               progress:(void (^_Nullable)(NSInteger,
                                                           NSInteger,
                                                           FIRStorageReference *))progressBlock
                             completion:(void (^_Nullable)(UIImage *_Nullable,
                                                           NSError *_Nullable,
                                                           SDImageCacheType,
                                                           FIRStorageReference *))completionBlock;

2. The cache key for FIRStorageReference *

What about this one ? If we need to keep old version of cache compatible (using the fullpath of cache key), we need a custom cache key filter for each image request.

SDWebImageCacheKeyFilter *cacheKeyFilter = [SDWebImageCacheKeyFilter cacheKeyFilterWithBlock:^NSString * _Nullable(NSURL * _Nonnull url) {
    return url.sd_storageReference.fullPath;
}];

Cache Key: path/to/object.jpg.

If not, we can remove these code. Which use the URL representation of FIRStorageReference as cache key. I think it's more suitable because it can solve the potential conflict of cache.

Cache Key: gs://bucket/path/to/object.jpg.

[morganchen12]

1. The cache arg can be removed as long as there's still a way to avoid using the cache for users who want to skip the cache (looks like you've included this in your implementation).

2. We can change the cache keys since we don't guarantee that cache items will be preserved across major version bumps.

[dreampiggy]

OK. I've follow your reply for the 3 questions. And do a renaming of previous MR (SDWebImageFirebaseLoader -> SDWebImageFIRStorageLoader), since this naming match the Firebase Storage UI-Binding.

I'll provide the detail of API breaks after this change.

[dreampiggy]

Current break change (Need change code)

UIImageView+FirebaseStorage.h

• sd_defaultMaxImageSize Removed.
• sd_setImageWithStorageReference Removed.

Use SDWebImageFIRStorageLoader.defaultMaxImageSize for global level control
Use SDWebImageContextFIRStorageMaxImageSize context option for single imge request level control

• sd_setImageWithStorageReference:maxImageSize:placeholderImage:cache:completion: Removed. The cache arg can be replcaed with the new option and context arg.

To disble cache, use SDWebImageFromLoaderOnly option
To supply custom cache instance, use SDWebImageContextCustomManager with the manager of custom cache instance.
The new full args API is sd_setImageWithStorageReference:maxImageSize:placeholderImage:options:context:progress:completion:. You can supply control args and the progressBlock like SDWebImage's original API.

[dreampiggy]

Hi there !

SDWebImage 5.0.0 is official released today. See: SDWebImage 5.0.0 changelog

I've update the API changes to match the release version. And update the podspec. Maybe we can have a look at this and talke about some review ideas ?

[dreampiggy]

Another idea, I saw the Firebase-UI, have the custom Swift class name for better Swift support. Such as FIRStorageRef (Objective-C) -> StorageReference (Swift).

The current loader's naming is SDWebImageFIRStorageLoader, sounds a little verbose. Maybe this naming can be polished. And I don't know whether we need SD as prefix. If you don't care about SD prefix (Don't conflict with existing class of Firebase), maybe we can renaming to this ?

• FIRStorageImageLoader (Objective-C) -> ~StorageImageLoader (Swift)

[dreampiggy]

@morganchen12 Hi. I'm glad that you can take the time to review this PR.

And, do I need to change the naming of this class (see my above comment), which avoid the SDWebImage prefix and just use FIR instead (Swift name without that FIR)? If so, I can change it quickly.

[morganchen12]

Classes in this repository should use the FUI prefix, since FIR is used by the Firebase SDKs.

[morganchen12]

Fixing the unit tests requires adding the dynamic framework paths to the runpath search paths in the build file. I can add a commit that fixes those.

[dreampiggy]

I found the test case failed reason. It should be passed now. 😅

It seems that FirebaseUI, does not keep the CocoaPods installed project into git repo. (The Pods_FirebaseStorageUITests.framework and xcconfig etc).

Is this step listed in the contribution guideline? I didn't find somewhere mention this, only when I run the ./test.sh, I found there are strange pod deintegrate >/dev/null; shell for each test cases.

[morganchen12]

We've been deintegrating pods for mostly outdated reasons. It's definitely not necessary to continue doing so. But the breakages shouldn't be related to pod deintegrate.

[dreampiggy]

I'm confused about the CI test pipeline. I saw many ** TEST FAILED ** output even in a sucessfuly Pipeline, like this: https://travis-ci.org/firebase/FirebaseUI-iOS/builds/525471083

I can definitely run the local Test Scheme by Xcode. Any help for this ? Seems a little struggle...

[dreampiggy]

Seems still something about the test script? Or env issue?

[morganchen12]

The tests are failing upstream--I'm ok with merging this as long as Storage tests pass locally. Don't worry about the failures on Travis for now.

[dreampiggy]

@morganchen12 😢Any update about this PR ?

It seems this PR has already been opened for 1 and a half month...I remember the Google/IO 2019 is now begin. But this PR still sucking here.

Does FirebaseUI supports open source contribution outside Google ? Or is there anything I can do to make this PR to be take on ? Please reply me if you have idea about this.

[dreampiggy]

Or if this FirebaseUI have no plan to support the new Image Loader API, please at least update the dependency without no API breaking. To make this Pod compatible for user who already use SDWebImage 5.0.

Currently, even I change the old v6.x version of FirebaseUI's dependency to use SDWebImage 5.0, it still can not compile because a little incompatible issue (One header include issue, and one SDImageCache API naming issue). It just take a little time to fix. It does not effect the logic for current users.

[morganchen12]

@dreampiggy sorry about that, I got caught up in some I/O stuff. We can merge this and target it for a mid-year breaking change.

[dreampiggy]

Thanks for your support! Wish you and your team have a good Google I/O event. 👍

[dreampiggy]

And, during these days, I have some other idea about this. If you some free time, maybe you can try to consider my below description.

FirebaseUI seems combine many CocoaPods Subspecs for different components, and bind each of them with different dependency for third-party library. But actually, the main part of FirebaseUI which used by users, are the OAuth component for Facebook/Twitter.

What about try to seperate them into a standalone one, to make it easy to upgrade and to avoid version bump for other components which does not change at all ?

Or, if you find the maintain of this FirebaseUI/Storage component, which bind SDWebImage's API, is a little pain. Is there any plan, to make this component to be SDWebImage orgnization driven and maintained? Means we follow the Firebase Core Storage API update with SDWebImage's API changing, make it easy to move on.

Just a little curious, don't need to be seriously considering. If Firebase has its own milestone plan, don't be limited to this.