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

Modification to resolution handling in TIFF reader / writer #4185

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ruven
Copy link
Contributor

@ruven ruven commented Oct 6, 2024

Vips currently sets a default X and Y resolution of 1.0 if no resolution information is found within an image file. Rather than write "fake" resolution information to a TIFF, this pull request modifies things so that the TIFF X/Y resolution is only written if this value is really known.

Also when writing a multi-resolution pyramid TIFF, identical resolution information is currently set for each layer in the pyramid. This information is now scaled by the sub-sampling factor so that the resolution now correctly represents what the resolution really is for each layer.

A similar change has also be made for PNG writing.

…ithin TIFF file.

TIFF writer now only sets resolution if this is really known, rather than setting a default value.
The resolution of sub-resolution layers within a pyramid TIFF are now scaled depending on the sub-sampling of that layer.
@jcupitt
Copy link
Member

jcupitt commented Oct 7, 2024

Hi Ruven,

Sorry, I don't think this is going to work. You can't use res == 0 to mean no resolution information is available. It'll cause divide by zero errors everywhere :( 1.0 is a bit dumb, but it's at least safe.

I think the correct fix would be to move xres/yres out of the vips image header and store it as generic metadata. Those values can be unset, so you could represent "no data available". It would break a lot of stuff though, so ... something for libvips 9.0.

@ruven
Copy link
Contributor Author

ruven commented Oct 7, 2024

Yeah, I was a bit hesitant to make a pull request for this for as I was unsure of the wider impact. That's also why the I put the setting of the default to zero in a separate commit.

However, having said that, it works perfectly for loading and saving to TIFF. And I'm not sure it's as big a problem as you think because:

  1. It is already possible to have an xres/yres of zero and this is explicitly allowed in several places in the code. For example in foreign/exif.c and iofuncs/header.c you have image->Xres = VIPS_MAX(0, xres);. Similarly in iofuncs/vips.c there's im->Xres = VIPS_MAX(0, im->Xres_float);. And several other similar lines where you clip to a +ve number, but allow zero. This does not seem to have posed any problems so far. In other words, vips currently loads TIFF/JPEG and other formats which have Xres/Yres specifically set to zero in their metadata and will keep the zero and will handle them just fine.

  2. Apart from in iofuncs vips.c and header.c, the only code that I can see that uses xres/yres are the foreign format reader/writers. The resample functions, for example, explicitly state that This operation does not change xres or yres.

  3. The only potential divide by zero case I can find is in foreign/niftisave.c which should be fine if enclosed in an if-block:
    nifti->nim->dx = 1.0 / image->Xres;

Of course there may be things I've missed!

@jcupitt
Copy link
Member

jcupitt commented Oct 8, 2024

I think those tests are trying to allow very small but still positive resolutions. I remember we used to have zero res in output file sometimes, and it caused a lot of problems downstream, including crashes in some apps. That was a good while ago, mind you.

@jcupitt
Copy link
Member

jcupitt commented Oct 8, 2024

nifti->nim->dx = 1.0 / image->Xres;

That's a common thing in downstream code working with eg. slide output, where resolution is usually expressed as microns per pixel.

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