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

add skipBlanks support for dz and zoomify layout #1687

Merged
merged 1 commit into from Jul 12, 2019
Merged

add skipBlanks support for dz and zoomify layout #1687

merged 1 commit into from Jul 12, 2019

Conversation

RaboliotLeGris
Copy link
Contributor

Hi!

Here it is !

  • I didn't upgrade the libvips in the package.json version
  • It seems that upgrade libvips to 8.8.0 broke 2 tests (Image metadata -> JPEG with IPTC/XMP and Attention strategy -> JPEG) It also appears with master when libvips 8.8.0 is activated. I didn't fix them because i don't underestand their purposes ...
  • I had to force the the skipBlanks in output.js because AttrTo<int32_t>(options, "skipBlanks") return 0 if it's NULL/undefined

Let me know if it needs some modifications.

@lovell
Copy link
Owner

lovell commented May 3, 2019

Bonjour Jordan, merci beaucoup for this PR, which from a quick scan looks good - thank you for adding tests!

I notice there's some logic in libvips that defaults skip_blanks to a value of 5 when using the "google" layout, which this change should probably mirror.

https://github.com/libvips/libvips/blob/bcdaeca57877ba1d8c3cc7659708e4ef6f396a0b/libvips/foreign/dzsave.c#L1832-L1836

This PR will have to wait until after libvips v8.8.0 is released, which sharp will probably pick up in v0.23.0. I'll fix up the tests at that point to reflect the new/changed features that are making them fail.

@RaboliotLeGris
Copy link
Contributor Author

Hi, I've fixes this part and added a test to the google layout.

let me know if you see any improvement :)

@lovell lovell changed the base branch from master to vision July 6, 2019 12:50
@lovell
Copy link
Owner

lovell commented Jul 6, 2019

@RaboliotTheGrey I've changed the destination branch for this PR to the vision branch, which will become v0.23.0. Are you able to rebase against the vision branch and push to allow the CI tests to pass?

@RaboliotLeGris
Copy link
Contributor Author

RaboliotLeGris commented Jul 6, 2019 via email

@RaboliotLeGris
Copy link
Contributor Author

Hi,

I've rebase the branch on the lastest version of vision and test agains the version 8.8.1 of libvips.
All the tests are green on local.

@coveralls
Copy link

coveralls commented Jul 9, 2019

Coverage Status

Coverage increased (+0.02%) to 97.503% when pulling 18a8a89 on RaboliotTheGrey:feature/add-skipBlanks-tile-option into b737d46 on lovell:vision.

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.

Fantastic, thanks for updating this, there's one small comment about the jsdocs but otherwise this is ready to merge.

lib/output.js Outdated
@@ -539,6 +539,7 @@ function toFormat (format, options) {
* @param {Number} [tile.overlap=0] tile overlap in pixels, a value between 0 and 8192.
* @param {Number} [tile.angle=0] tile angle of rotation, must be a multiple of 90.
* @param {String} [tile.depth] how deep to make the pyramid, possible values are `onepixel`, `onetile` or `one`, default based on layout.
* @param {Number} [tile.skipBlank=-1] threshold to skip tile generation, a value 0 - 255 for 8-bit images or 0 - 65535 for 16-bit images
Copy link
Owner

Choose a reason for hiding this comment

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

I think tile.skipBlank should be the plural form tile.skipBlanks.

Please can you run npm run docs after fixing this, which should automagically update docs/api-output.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch :)
Sorry I took a bit long to fix this.

@lovell lovell merged commit 6c02949 into lovell:vision Jul 12, 2019
@lovell
Copy link
Owner

lovell commented Jul 12, 2019

Merveilleux, merci beaucoup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants