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

Drop our GIF support and integrate a 3rd party solution #945

Closed
bpoplauschi opened this issue Nov 3, 2014 · 26 comments
Closed

Drop our GIF support and integrate a 3rd party solution #945

bpoplauschi opened this issue Nov 3, 2014 · 26 comments
Assignees
Milestone

Comments

@bpoplauschi
Copy link
Member

Hi @rs, @mythodeia ,

I think you both know our GIF support is pretty sucky (consumes a lot of memory #548 #538 and also blocks the main thread #933, doesn't work on disk retrieval #916). I've tried a few approaches, but none seems to fix it.

In #733 someone suggested we look at FLAnimatedImage a library developed by Flipboard. I also found YLGIFImage.

Now given those 2 libraries know their way with GIFs (low CPU and memory footprint), since they are already focused on GIF, making patches and maintaining their code, I strongly suggest we remove our GIF code and create a subspec that supports integration with one of them. Basically via CocoaPods, we will be referencing one of those 2 libraries.

Let me know if you agree with me so far.

Now deciding which one:

  • both work very well on the simulator, I still need to test on a few devices
  • FLAnimatedImage has the advantage of being maintained by a team which handles working with images a lot (Flipboard), releases often (yesterday was their last release). The big disadvantage is FLAnimatedImage is not an UIImage subclass, but a NSObject subclass, which could make it harder for us to integrate.
  • YLGIFImage is built by Yong Li, developer at Facebook, so he must also have a good experience with images. The lib was not updated in the past 6 months. The advantage is their YLGIFImage is a UIImage subclass.

If you agree with this, I can play with an integration. I think it's our best option.

@bpoplauschi bpoplauschi added this to the 4.0.0 milestone Nov 3, 2014
@mythodeia
Copy link
Contributor

@bpoplauschi sounds good to me.
plus there are these if you want to check them out:

@bpoplauschi
Copy link
Member Author

@mythodeia many thanks. I didn't know Mattt had such a library. Looks very good because it uses UIImage directly. Need to see how it performs.

@mythodeia
Copy link
Contributor

@bpoplauschi
if you want you can do a benchmark like you did with this. https://github.com/bpoplauschi/ImageCachingBenchmark

@bpoplauschi
Copy link
Member Author

I would if I had the time :) I played with AnimatedGifImageSerialization and it has the same performance issue as our own code. They both work with very small GIF's, but that's it. :( I was hoping we could use Mattt's lib

@fanyj
Copy link

fanyj commented Nov 4, 2014

I've integerated YLGIFImage and it works well for me. But it seems this lib is no longer supported. FLAnimatedImage sounds like a good idea and they are considering change FLAnimatedImage to an UIImage subclass. See #11.
So I vote FLAnimatedImage.

@rs
Copy link
Member

rs commented Nov 5, 2014

+1 for the general idea of replacing the GIF support by an external library. Based on your comment I would also prefer FLAnimatedImage.

@bpoplauschi
Copy link
Member Author

Note to self: look at https://github.com/kaiinui/KIChameleonView, it integrates SDWebImage with FLAnimatedImage.

@bpoplauschi
Copy link
Member Author

Guys, I've tried very hard to integrate FLAnimatedImage with our library, but because it's not a UIImage subclass, it involves too many changes and it breaks all the decoupling we have now. I have started integrating YLGIFImage instead. It works fine and it is easier to hook in. If FLAnimatedImage changes their implementation soon and subclass UIImage, we might turn this around, but from where I sit right now, I don't see a good way to integrate them with their current architecture.

@bpoplauschi
Copy link
Member Author

Seems like one contributor to FLAnimatedImage is willing to work on making FLAnimatedImage a subclass of UIImage this weekend, I'll keep in touch with him and see how that goes.

@chenium
Copy link

chenium commented Nov 18, 2014

The author of YLGIFImage has a project that combines SDWebImage and YLGIFImage. The author states that he plans to maintain this fork.

https://github.com/liyong03/SDWebImage-YLGIFImage

@bpoplauschi
Copy link
Member Author

Thanks @chenium

@zhw511006
Copy link

When we will integrate a 3rd party solution for Gif? Thx!

@wanghengheng
Copy link

I modified the methods in file "UIImage+GIF" to return a "YLGIFImage", and modified the method in file"SDWebImageCompat" to do nothing with 'gif' data.

It looks work well.

@mythodeia
Copy link
Contributor

@bpoplauschi FLAnimatedImage has an active UIImage subclass branch...
check it out

@bpoplauschi
Copy link
Member Author

Thanks @mythodeia, I will look at this again.

@bpoplauschi
Copy link
Member Author

No solution yet, I will look at the new FLAnimatedImage variant and take it from there.

@peterpaulis
Copy link

Any progress on this?

@peterpaulis
Copy link

the solution for https://github.com/liyong03/SDWebImage-YLGIFImage no longer works for current sdwebimage

@mythodeia mythodeia mentioned this issue May 27, 2015
@neil-wu
Copy link

neil-wu commented Jul 7, 2015

I modify some SDWebImage's source code. And work with FLAnimatedImage very well.
Here is a demo project

@yuvalDeri
Copy link

I don't know if this is known but Brewer Herb gives a nice solution of using FLAnimatedImage with SDWebImage using swizzlling
https://gist.github.com/exherb/1257263e350831d6c205

@hjanuschka
Copy link

@yuvalDeri thx for the hint?! - but it actually does not animate the gif, it is loaded but does not run.
any idea how to start the animation?

using the following original sd method to load the image from web

sd_setImageWithURL:placeholderImage:options:SDWebImageCacheMemoryOnly completed:

@yuvalDeri
Copy link

@hjanuschka
I simply added those 2 files to my project and added a load class method under the implementation part like this
@implementation SDImageCache (Swizzle)

"+ (void)load {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
[self swizzle];
});
}"
.....
@EnD

make sure you are calling
sd_setImageWithURL:placeholderImage:options: completed:
on a FLAnimatedImageView object and not an UIImageView.

It works for me, even the completion block holds a FLAnimatedImage instead of an UIImage when GIFs are involved.
good luck

@hjanuschka
Copy link

@yuvalDeri - MANY THX - changing the type from uiimageview to flanimatedimageview did the trick, also nice with the +load() thx - someone should do a POD of this

@hilen
Copy link

hilen commented Sep 1, 2015

Take a look at this -> https://gist.github.com/hilen/8ef4a810406051e8f0c1

@bpoplauschi
Copy link
Member Author

By merging #1575 into the 4.x branch, I think we can close this thicket. Feel free to test it and let us know if it works.

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

No branches or pull requests

13 participants