[Bug] High resolution displays still render low resolution images?

[cprussin]

Bug Report

Describe the bug

It seems that in the srcset generated using this library, the 2x size uses the same actual image as the 1x size. I know very little about the code in this lib, but it appears this is caused by using widthResize to generate higher-resolution URLs in the loader, but then ignoring widthResize if it's bigger than the user-specified width.

Is this a regression?

I don't see any evidence that it is but I know little about the history of this library

Steps To Reproduce the error

1. Create an image with a width and height
2. Observe that in the srcset, the 2x src is the same as the 1x src
3. As a result, observe that on high-DPI displays, the image looks blurry, unless you manually set the image dimensions to double the required size and then downsize in css or something

Expected behaviour

Ideally I think we should leverage the newly added DRP support and specify a DPR that matches the multiplier calculated by the loader, instead of using resizes.

CodeSandbox or Live Example of Bug

My cloudinary instance is private so I'd need to spin up a public one to get a minimal repro together. It feels like this issue is straightforward enough that it's not worth the effort, but let me know if you think it would be valuable in deciding what to do here and I'm happy to put the effort in.

Your environment

$ uname -a
Darwin Connor-Prussin-C4P793YFRV.local 22.6.0 Darwin Kernel Version 22.6.0: Fri Sep 15 13:41:28 PDT 2023; root:xnu-8796.141.3.700.8~1/RELEASE_ARM64_T6020 arm64 arm Darwin
$ node --version
v18.17.0
$ npm --version
9.6.7
$ pnpm --version
8.6.11
$ /Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome --version
Google Chrome 118.0.5993.88 

[cprussin]

Incidentally it also seems that this lib is setting heightResize in the loader which it passes down to @cloudinary-util/url-loader, but I don't see any evidence that it's actually handled anywhere in @cloudinary-util. I didn't trace far enough to see if heightResize is ever passed along somehow to the underlying js-url-gen library, but even if it is it doesn't seem like it's handled anywhere there either, but this is of course a very naiive search and it's very possible I'm missing something.

[cprussin]

I just found the note in the docs about using the original image size instead of the rendered size. That does resolve the issue with the quality settings, but IMO it's not really documented well (it's kind of buried in the docs in a side note that's easy to miss and I actually read that line a few times thinking that when it said "the actual image width and height", that "actual" meant "rendered", not "original"). Perhaps can I suggest improving the docs / making the note about using the original dimensions really obvious / front and center?

Additionally, using the original dimensions conflicts with the underlying next API for the same properties which explicitly says to use rendered dimensions. It might make more sense to replace or augment with new properties originalWidth and originalHeight or something -- or even better, is it even necessary to specify the original dimensions for cloudinary, since we could probably use the limit option instead to downscale without upscaling (or alternatively I think the DPR setting is actually really ideal here as in my original comment)

[colbyfayock]

thanks for the report @cprussin - great suggestions. is this something you'd like to help out with? if not i can definitely update and clarify the docs myself

it seems like there are also some improvements / fixes that can be made on the loader side of things

good pointout for the next docs, i dont remember it specifically asking for the rendered dimensions

we're fortunately currently using c_limit in order to avoid the upscaling

both heightResize and widthResize were options added by me to Cloudinary Util to help with the responsive sizing while maintaining the i guess we can call them rendered dimensions as both of those numbers can be important throughout the transformation chain logic. its definitely possible heightResize ended up not being used anymore, but im having a hard time remembering the history on that one

it sounds like the dpr setting is a great option here

going to try to sit on this for a minute and think it through to make sure i fully undersatnd what's going on and think about how to tackle it, but let me know if you have any additional thoguhts since posting this. i appreciate all the digging you did on this

[cprussin]

hey @colbyfayock yeah I'm definitely willing to contribute a fix, but I'm no expert on any of these things so I could use advice on what the right fix is. I don't know if there's any reason to use the dpr if we just use c_limit, if we're using c_limit can't we just remove the if guard here since presumably the limit would prevent upscaling anyways?

[cprussin]

I also don't necessarily believe that the dpr setting is correct given that the nextjs srcsets are explicitly scaling the image dimensions so the browser can compensate for the higher dpr and whatever other scenarios it needs to, so it seems like converting from the dimensions as set by next.js into the original dimensions and a dpr scaling factor is not necessarily accurate to the underlying, but just passing through the dimensions -- so long as they don't upscale -- is exactly the intent. But again, I'm no expert, so advice would be appreciated.

[colbyfayock]

hey @cprussin appreciate the patience here, finally getting back to it

after exploring, i think your absolutely right in your assessment

it seems that the fix is that i should be applying the widthResize value no matter what and letting the Cloudinary API handle not upscaling it

hard to remember where my mind was at there but i think the intent was to avoid generating additional, unnecessary transformation URLs for people, so for instance:

• .../w_100/
• .../w_200/
• .../w_300/

would all be unique URLs, and if i knew we wouldnt need larger sizes, why not avoid generating new unique URLs?

but i think that ended up causing more harm than good and if there's any value there, needs to be rethought

would you still be interested in contributing the ifx? i believe it would be in Cloudinary Util, exactly where you were showing

here are the tests that you can use to work on it:

https://github.com/colbyfayock/cloudinary-util/blob/4df69214e9fed7a1a2b3d71c5817729083cedf56/packages/url-loader/tests/plugins/cropping.spec.js#L73-L97

if i dont hear back from you in a few days ill go ahead and move forward with fixing it!

[colbyfayock]

note: if you end up getting rid of it, want to clean out the heightResize property that sbeing set in one of the tests just to clean it out since its not used?

https://github.com/colbyfayock/cloudinary-util/blob/3237f299ca16172c9562fb2d0bbffb0b96d5bcb2/packages/url-loader/tests/lib/cloudinary.spec.js#L540

[cprussin]

hey @colbyfayock yeah I'm more than happy to work on a fix, but I won't be able to get to it for a couple of days at the soonest. If you manage to get to it before I do just tag me here and I can be a guinea pig.

[colbyfayock]

since i wont want to deploy this fix on a friday, i can wait :) if i dont hear back by tuesday i'll throw it in myself. thanks

[colbyfayock]

i started a PR here: https://github.com/colbyfayock/cloudinary-util/pull/90/files

but im thinking that this doesn't quite resolve the issue as expected

as you know, there are two different width indicators: width and widthResize

the way these impact the URL are as such:

image/upload/c_limit,w_${width}/...transformations/c_limit,w_${widthResize}

the way that this corresponds to usage in Next.js, if im not mistaken, when using something like:

<CldImage width="400"

the URL will be generated as:

image/upload/c_limit,w_400/...transformations/c_limit,w_${widthResize}

Where widthResize would be based on the responsive sizing:

image/upload/c_limit,w_400/...transformations/c_limit,w_400
image/upload/c_limit,w_400/...transformations/c_limit,w_500
image/upload/c_limit,w_400/...transformations/c_limit,w_600

the above being the result of the fix that i linked to above

however as you might notice, the c_limit being applied at the end is being limited based on the width in the front

if the original image is 1200 in the above, it will always be limited to 400

the reason it's set up this way is to maintain a consistent "canvas" for transformations like text overlays, as you can imagine, having a specific font size and positioning can get very tricky while trying to cover the same transformation on multiple image sizes, so by having that first width resize, all the transformations relative to that, then the resize after, we're able to provide consistent transformations and relative sizing, while still providing responsive sizing

with all that said, my thinking is that it may make senes to additionally set @2x as a default

in an example such as:

c_limit,w_600,dpr_2/c_limit,w_800

w_600 would first give a 1200 image as a result. when moving to the 2nd limit, the DPR would still be at 2, so technically the w_800 would result in 1600, meaning with limit, it would not resize, resulting in the same 1200 image

i'm unsure if this would be considered "expected" though, as the sizes wouldn't match the value passed in by Next.js

im runnign this by a colleague to get their thoughts on this as well who is more familiar with responsive sizing than myself, but curious if you have an opinion on the above as well

[cprussin]

hey @colbyfayock sorry for the slow responses, I've been absolutely swamped recently.

In my opinion defaulting to 2x doesn't make a lot of sense as it would result in significantly more bandwidth consumption on devices that can't use the 2x size anyways.

I'd also argue that the use case for text overlays on web, where you can trivially layer with the dom and CSS, is quite limited. My opinion here is that if you had to prioritize making resolutions work correctly always or making text overlays work correctly, the former is vastly more important on web because it has real perf and output impacts and no good workaround. If you could also make text overlays work that would be great but it feels a bit silly to sacrifice the actual image resolution part of things for it.

I'm no cloudinary expert, however, so take my opinion here with a grain of salt!

[colbyfayock]

All good. I agree, I was thinking about it from a different angle and doesn't make sense in hindsight.

The current thought after brainstorming with a colleague is to remove the two stage resize and allow any relative sizing to be based off of the original image size. Creates some challenges but as you said isn't the priority and is workable

That will have to be a breaking change as it would impact people's current sizing. It'll be a bit before I get there as I want to plan out some other major rework to go out in that release. Likely wouldn't want to do that before the holidays as well to avoid big changes right during peak shopping season