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

Impute TIFF xres/yres from withMetadata({density}) #2952

Closed
wants to merge 1 commit into from

Conversation

mbklein
Copy link
Contributor

@mbklein mbklein commented Oct 28, 2021

Closes #2941

This PR uses the density value passed to withMetadata() as a fallback if the output format is TIFF and netter xres nor yres is provided in the TIFF options. I've also updated the API documentation with to include a short explanation of this behavior.

@mbklein
Copy link
Contributor Author

mbklein commented Oct 28, 2021

@lovell While creating two new tests for this functionality, I discovered that I couldn't get them to fail at all when written as promises. I believe this is due to the fact that the it() function completes (and passes) before the promise resolves, as described in this guide. I tried using the done() callback, but found I could achieve better and more consistent results by using asynchronous test functions with async/await. I'm pretty sure this same issue might cause some false passes in other parts of the test suite. I would be happy to refactor the rest of asynchronous code tests to use async/await as well, but that would obviously be the subject of a different pull request.

@lovell
Copy link
Owner

lovell commented Nov 7, 2021

Hi Michael, thank you very much for this PR, and always a pleasure to see unit tests too.

I wonder if it might be a little simpler to have this logic in the C++ layer, perhaps something like the following:

--- a/src/pipeline.cc
+++ b/src/pipeline.cc
@@ -1496,6 +1496,9 @@ Napi::Value pipeline(const Napi::CallbackInfo& info) {
   baton->tiffTileHeight = sharp::AttrAsUint32(options, "tiffTileHeight");
   baton->tiffXres = sharp::AttrAsDouble(options, "tiffXres");
   baton->tiffYres = sharp::AttrAsDouble(options, "tiffYres");
+  if (baton->tiffXres == 1.0 && baton->tiffYres == 1.0 && baton->withMetadataDensity > 0) {
+    baton->tiffXres = baton->tiffYres = baton->withMetadataDensity / 25.4;
+  }
   // tiff compression options
   baton->tiffCompression = static_cast<VipsForeignTiffCompression>(
   vips_enum_from_nick(nullptr, VIPS_TYPE_FOREIGN_TIFF_COMPRESSION,

I think this should still satisfy the test cases you added.

@mbklein
Copy link
Contributor Author

mbklein commented Nov 8, 2021

I skimmed pipeline.cc for an appropriate spot to add it, but came up short. This is much more concise. I'll make the change and force-push my branch.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 33b5da3 on mbklein:tiff-density into b33231d on lovell:master.

lovell added a commit that referenced this pull request Nov 8, 2021
@lovell
Copy link
Owner

lovell commented Nov 8, 2021

Brilliant, thank you, landed via commit 342de36

@lovell lovell closed this Nov 8, 2021
@lovell lovell added this to the v0.29.3 milestone Nov 8, 2021
martinj pushed a commit to aptoma/sharp that referenced this pull request Mar 31, 2022
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.

Enhancement: use density provided to withMetadata as fallback value for TIFF xres/yres
3 participants