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

Image scaling bug #791

Closed
prabhuamol opened this issue Jun 13, 2024 · 11 comments
Closed

Image scaling bug #791

prabhuamol opened this issue Jun 13, 2024 · 11 comments
Labels

Comments

@prabhuamol
Copy link

prabhuamol commented Jun 13, 2024

We are sometimes noticing when images are scaled using .thumbnailKey they appear blurry. I wonder if its cause in func makeThumbnail(data: Data, options: ImageRequest.ThumbnailOptions) -> PlatformImage? scale is not taken into account when returning back the UIImage. Which is causing the dimensions of the image to be reported incorrectly.

We do some post processing via prepareThumbnail if the returned thumbnail size does not match the view size exactly and the scaled down image returned using https://developer.apple.com/documentation/uikit/uiimage/3750845-preparethumbnailofsize is too blurry.

Heres a sample request and response to show the issue

▿ ImageRequest(resource: "some url (hidden for privacy)", priority: normal, processors: [], options: Options(rawValue: 0), userInfo: [Nuke.ImageRequest.UserInfoKey(rawValue: "github.com/kean/nuke/thumbnail"): Nuke.ImageRequest.ThumbnailOptions(targetSize: Nuke.ImageRequest.ThumbnailOptions.TargetSize.fixed(474.0), createThumbnailFromImageIfAbsent: true, createThumbnailFromImageAlways: true, createThumbnailWithTransform: true, shouldCacheImmediately: true), Nuke.ImageRequest.UserInfoKey(rawValue: "imageSize"): (110.0, 158.0)])
▿ ref : <Container: 0x600001819c40>

(lldb) po response.image
<UIImage:0x6000030307e0 anonymous {338, 474} renderingMode=automatic(original)>

(lldb) po response.image.size
▿ (338.0, 474.0)

  • width : 338.0
  • height : 474.0

As you can see image size is reported in pixel size and not points. The fix might be that when we return PlatformImage in Nuke.makeThumbnail use scale initializer for UIImage.

return PlatformImage(cgImage: image)

return PlatformImage(cgImage: image, scale: scale, orientaion: orientation) instead of return PlatformImage(cgImage: image) I had to do that in an app i was working on as well

@prabhuamol
Copy link
Author

You can also verify this by just chaning any of the ImageProcessorsResizeTests to use points and all of them fail

Screenshot 2024-06-14 at 2 50 28 PM

I also ran the testThatImageIsResized and verified that image size and sizeInPixels is the same. Where one should be in points and smaller

Screenshot 2024-06-14 at 2 53 56 PM

@prabhuamol
Copy link
Author

@kean any luck investigating this? Would be amazing to get this fixed

@prabhuamol
Copy link
Author

Submitted a PR here #793

@kean
Copy link
Owner

kean commented Jul 7, 2024

Hey, @prabhuamol!

I've looked into it, and I might need some clarification as I'm not sure I follow this entirely.

By default, all images fetched by Nuke are returned with a scale of 1, so it's expected that image.size will return size in pixels. The framework doesn't know and can't know what is the logical size of the images it fetches.

For example, let's say you download an icon for a button from a server. The server returns an image with a resolution of 88×88 pixels but a logical size of 44×44 pixels. In the iOS terminology, it will be an image with a scale of 2. If you fetch it on a device with a UIScreen scale of 3, the image should still have a scale of 2.

If you need to change the scale of the feched image, you can use ImageRequest.UserInfoKey.scaleKey. But the way I typically approach it is by never relying on the logical scale of the images downloaded from the internet and instead restricting the point size of the views in which they are displayed.

You can also verify this by just chaning any of the ImageProcessorsResizeTests to use points and all of them fail

It works as expected. When you change the unit to .points, it simply means "resize the image to a width 400 × UIScreen.main.scale pixels". On a 3x device, it will be 1200 pixels, but ImageProcessors.Resize never upscales images by deafult, so you end up with an image that is 640 pixels wide. The unit has nothing to do with the scale of the output images or their logical size.

I think there may be some confusion in terms of what to expect from this API. I've been actually already considering removing the unit parameter in the next major version and keeping the target size in pixels.

@kean kean added the question label Jul 7, 2024
@prabhuamol
Copy link
Author

prabhuamol commented Jul 7, 2024

Hey @kean thanks for the response. So the issue is that the returned UIImage from resize process does not take device scale into account. So lets say i wanted to resize an image to CGSize of 44 on a device with scale of 2. The returned image has pixel size of 8888 which makes sense but associated UIImage.size is also 8888 (which is not correct) The size of the UIImage should take the device scale into account and should not be a 1:1 mapping to the CGImage pixel size. The PR i submitted here fixes that #793 and also adds related tests. Also the scaleKey doc says that by default it matches the scale of the display which seems to be not true based on your comment above .

By default, all images fetched by Nuke are returned with a scale of 1

Currently you can also see the tests you have in the app will fail as i have shown in the screenshot above. Hope this helps clear things up

@kean
Copy link
Owner

kean commented Jul 7, 2024

Also the scaleKey doc says that by default it matches the scale of the display which seems to be not true based on your comment above .

Good catch. I updated it to the following:

 /// The image scale to be used. By default, the scale is `1`.

@kean
Copy link
Owner

kean commented Jul 7, 2024

I'd also suggest checking the Apple documentation about UIImage scale. I also used to think it should generally always match the scale of the screen, but that's not the case. The default scale for unknown images should be 1.

@prabhuamol
Copy link
Author

prabhuamol commented Jul 8, 2024

e the tests you have in the app will fail as i have show

I read the doc but I think that is very specific to images inside a bundle. So can you point us to how do we make sure that the resized image we get back is returned with the right point size information? i.e the image is getting resized correctly but its just not returning the point size information accurately (i feel like this is a bug). It should follow the following logic as far as i can tell UIImage.size (points) = CGImage.size (pixels) * UIScreen.main.scale. I feel like the scale information is just lost when we resize images. So from consumer side the image returned is "larger" than it should be because the size info is not correct and is always matching 1:1 to pixel size irrespective of the device.

On a related note i dont see the scale key being used anywhere in the thumbnail creation func makeThumbnail(data: Data, options: ImageRequest.ThumbnailOptions) -> PlatformImage? Is there a way for us to specify the scale and get the right size back? Thanks!

@kean
Copy link
Owner

kean commented Jul 8, 2024

On a related note i dont see the scale key being uses anywhere in the thumbnail creation in func makeThumbnail(data: Data, options: ImageRequest.ThumbnailOptions) -> PlatformImage? Is that a way for us to specify the scale and get the right size back? Thanks!

This is a bug, and PRs with a fix would be more than welcome.

In the future major version, I have it on my list to see if the scale support could be moved to the later stages of the pipeline. Applying it early has implications for caching and request coalescing.

For example ,AsyncImage lets you change it during the display phase – very later.

@prabhuamol
Copy link
Author

On a related note i dont see the scale key being uses anywhere in the thumbnail creation in func makeThumbnail(data: Data, options: ImageRequest.ThumbnailOptions) -> PlatformImage? Is that a way for us to specify the scale and get the right size back? Thanks!

This is a bug, and PRs with a fix would be more than welcome.

In the future major version, I have it on my list to see if the scale support could be moved to the later stages of the pipeline. Applying it early has implications for caching and request coalescing.

For example ,AsyncImage lets you change it during the display phase – very later.

PR is here #793

@kean kean added bug and removed question labels Jul 13, 2024
@kean
Copy link
Owner

kean commented Jul 13, 2024

Fixed in v12.8.0. Thanks @prabhuamol for sticking with in and getting a PR out.

@kean kean closed this as completed Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants