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

Allow images to upscale #193

Merged
merged 1 commit into from
Oct 13, 2018
Merged

Allow images to upscale #193

merged 1 commit into from
Oct 13, 2018

Conversation

drkibitz
Copy link
Contributor

Hello, so I ran into a problem. I have a required design, and minimal control over my content pipeline. So I rely heavily on client side optimizations. I am only recently trying Nuke, and love it so far, but ran into a little snag, line 157 of ImageProcessing.swift. This CGFloat(min(scale, 1)) in particular.

What this means is you wouldn't expect anyone to need to upscale an image. Though I use client-side image processing for 2 main reasons (among others).

  1. to decode off the main thread, which Nuke does beautifully
  2. to ensure there is no scaling using offscreen rendering

The problem is, the images I get back are just slightly smaller than the views they are displayed in, so with this, and the the maximum of scale 1, means the only option is to use UIImageView scaling again. Basically, that makes the Nuke targetSize transformation a bit pointless for this case.

So when trying to figure out the best approach, I couldn't really decide, but chose one to make a pull request. Here are some thoughts:

  1. Remove the CGFloat(min(scale, 1)) and just use the scale as is. Allowing upscaling by default.
  2. Make 2 new ContentModes, for aspectFillWithUpscaling and aspectFitWithUpscaling
  3. Make the current ContentModes take a bool aspectFill(true) or aspectFit(false)
  4. Finally, add a third boolean to accompany targetSize, and contentMode through the process.

For this PR as you can see I went with 4, and it defaults to false because I am no stranger to open source, and I understand how important backward compatibility can be.

So let me know your thoughts. In the mean time, I am using a fork with my changes in number 4 applied.

@drkibitz drkibitz force-pushed the dev/allows-upscaling branch from f8064f7 to 15f48a4 Compare October 12, 2018 10:57
@drkibitz drkibitz force-pushed the dev/allows-upscaling branch from 15f48a4 to c2a5cd8 Compare October 12, 2018 11:01
@drkibitz
Copy link
Contributor Author

I should correct myself, this doesn't cause offscreen rendering. It causes subpixel anti-aliasing.

@kean
Copy link
Owner

kean commented Oct 13, 2018

Hey, @drkibitz. I'm not sure what the original motivation behind preventing upscaling was. It's clear why you would want to downscale the images, especially if the source images are much bigger then the target size and you have no control over the source. It's not clear to me what the upsides or downsides of upscaling are. Would you say that upscaling should be enabled by default? If there is a valid argument why, the default can be changed in the next major version.

@kean kean merged commit 599f92a into kean:master Oct 13, 2018
@drkibitz
Copy link
Contributor Author

drkibitz commented Oct 14, 2018

@kean
The upside is avoiding sub-pixel anti-aliasing, which is my goal. (another term is misaligned images in the profiler). This isn't the worst perf offender, but is one of them, especially in image heavy applications.

The downside is that if the transform is done before saving to a disk cache, we would save a larger decompressed image to the cache than the source image. Though in Nuke's case, that is not a downside, because as far as I can tell, the decompression is done off the main thread, but before rendering, not before saving to cache.

@drkibitz
Copy link
Contributor Author

@kean I guess to answer your question, whether or not it is the default might not be important for many people, but I think it is important to provide the option. So as with most projects, we should see how often we all actually use it to opt-in to true.

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.

2 participants