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

Support page selection for multi-page TIFF #1204

Merged
merged 1 commit into from
Apr 24, 2018
Merged

Support page selection for multi-page TIFF #1204

merged 1 commit into from
Apr 24, 2018

Conversation

woolite64
Copy link
Contributor

This PR is based off of #654 with changes requested by @lovell. It adds support for zero-indexed page selection from multi-page TIFF's.

The included test image was generated from the existing G31D.TIF. The second page is a 50% scaled copy of the first page.

The error message for an invalid page index could be more informative. If there is an easy way to determine the TIFF page count with libvips, please let me know. Feedback appreciated. :)

@coveralls
Copy link

coveralls commented Apr 24, 2018

Coverage Status

Coverage increased (+0.004%) to 99.007% when pulling f9df4f1 on woolite64:master into 8ef1532 on lovell:master.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you Nathan. There's one small comment inline but otherwise LGTM.

I think the error message is OK as it is as that's an underlying libvips error, the likes of which we already expose in many places.

I've created #1205 to track the possible future enhancement to expose page counts in the metadata feature.

Please feel free to add your name to the list of contributors too.

src/common.cc Outdated
@@ -229,6 +233,9 @@ namespace sharp {
if (imageType == ImageType::MAGICK) {
option->set("density", std::to_string(descriptor->density).data());
}
if (imageType == ImageType::TIFF) {
option->set("page", static_cast<int>(descriptor->page));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the static_cast required here (and in the similar code below) given this is already an int?

@woolite64
Copy link
Contributor Author

Removed the static_cast and updated contributors.

Thanks for the review, and I'll keep an eye on #1205.

@lovell lovell merged commit b154cd0 into lovell:master Apr 24, 2018
@lovell lovell added this to the v0.20.2 milestone Apr 24, 2018
lovell added a commit that referenced this pull request Apr 25, 2018
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.

3 participants