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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Idea] Supporting WebP via CocoaPod's subspec #53

Closed

Conversation

liamnichols
Copy link
Contributor

Hey! First off, I only recently discovered this library and wow it's impressive! As our project has evolved we've outgrown our existing tool used for managing remotely served images and when I came across TIP it seemed to check all of the boxes for what I was looking for!

We currently also depend on WebP support and I understand why you didn't include it built into TIP however I was wondering if there is any reason not to provide the Codec via an opt-in subspec using CocaPods?

I see that there is a bit of trickery around getting libwebp building for Mac Catalyst however I stubbed across SDWebImage/libwebp-Xcode and it appears to handle all platforms correctly (although I need to double check that). In the sample PR here, I've added a simple subspec that includes the TIPXWebPCodec as well as a dependency on the libwebp pod provided by the linked repo and can confirm that this works for my iOS project... Is this approach something that interests you?

I guess that there are still a few unresolved questions though but I wanted to get opinions before I dig deeper:

  1. Does this work for everything (macCatalyst and so on)?
  2. How much work does it take to get TIPXWebPCodec up to scratch to release alongside (I noticed a couple of TODO's)
  3. Should we somehow auto-register the codec with TIPImageCodecCatalogue or leave that up to the user?

No worries if this isn't something that you want to support in TIP, I just thought that it's better to check here as I'd much rather help give back a little than just integrate something directly into our project 馃檹

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2020

CLA assistant check
All committers have signed the CLA.

Comment on lines -18 to +19
#if TARGET_OS_MACCATALYST
#import <webp/webp.h>
#else
#import <WebP/decode.h>
#import <WebP/encode.h>
#endif
#import <libwebp/decode.h>
#import <libwebp/encode.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NSProgrammer
Copy link
Collaborator

Thank you for taking active interest! We are committed to having Twitter Image Pipeline be a zero dependency project but have taken great pains to provide abstractions so that consumers can plug in their own customizations. It is important that the WebP extended code be committed to the repo but also unnecessary for using TIP, only Apple provided decoders/encoders will be automatically supported. This will avoid any binary bloating for consumers that do not need codecs that are not included by Apple. iOS 14 adds WebP support and we have a large set of revisions coming down the pipeline that would include that WebP decoder for iOS 14+, among other things.

I think the libwebp CocoaPod dependency is great and, as an alternative, would encourage yourself and others to create a WebP codec for you application layer to plug into Twitter Image Pipeline. You can simply copy the the TIPXWebPCodec code and replace any implementation parts you want to customize to fit your needs (being that you want to use a different dependency for webp but still want webp, the delta ought to be quite trivial - but you could add other codecs too). Then you merely need to register your own codec with the TIPImageCodecCatalogue.

I can empathize with the in-between-ness of where does such a "glue" layer live in open source. We have a similar problem between Twitter Image Pipeline and Twitter Network Layer. Using TIP & TNL together requires a small amount of "glue" and we provide that as a gist: https://gist.github.com/NSProgrammer/6e4c93ca9b9518178c9cbc7d950efd9c --- perhaps we need to think more about "glue" being open sourced but that is what we have for now. Were you to construct your own "glue" gist, we would happily link to it.

Again, thank you for your PR and great thoughts on advancing Twitter Image Pipeline. Although this PR doesn't align with the philosophy of this project, we hope that you would continue to maximize TIP for your use cases and provide PRs of improvement in the future too! Feel free to reach out for questions or comments on this or any other Twitter iOS open source projects on Twitter: @NolanOBrien

@NSProgrammer
Copy link
Collaborator

Also, I should note, that in the upcoming release of TIP, the codec protocol interface will be revised slightly. Adapting to the changes will be very straightforward, but I feel it only fair to prepare you so that you won't be caught off guard that any custom codecs will stop compiling after updating to the latest TIP in the future.

@liamnichols
Copy link
Contributor Author

Thanks @NSProgrammer for such a detailed explanation, I imagined that something like this might be the case so no worries at all! We're more than happy to bring the custom codec under our control so will do just that given your reasoning here

I agree with you regarding the glue logic and that was my main reasoning for thinking about how it could be included since the codec itself was already checked into this repo, but it makes sense not to do that because of the libwebp dependency like you mentioned.

The idea of using a subspec though was inspired by libraries such as Firebase where their frameworks integrate with one another using subspecs so in theory something similar might be possible here where there is a 'TNL' subspec in this repo that contains the glue code you linked in the gist ... That kind of approach might help consumers who look to use both dependencies 馃檪

Thanks again for all the work that went into this, I'll close this PR off now 馃憤

@liamnichols liamnichols closed this Jul 5, 2020
@liamnichols liamnichols deleted the ln/support-webp-via-subspecs branch August 6, 2020 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants