Added tiff compression option #738

kristojorg · 2017-03-21T03:00:29Z

I didn't see a work-in-progress branch so I am sending this to master for now. It doesn't introduce any breaking change in api in any case.

It is actually not totally working yet, and I'm wondering if you can help me identify why. My guess is that I'm setting the wrong option here: this.options.tiffCompression = options.compression;, as I just followed the pattern of similar functions. Where can I find the correct property name to set, and in what format does the input option need to be? I followed what I had used through the vips CLI: 'lzw', 'deflate', and 'jpeg'.

Additionally, I'm not exactly sure what to assert for in a test to determine if this is working? I would assume the compression info must be encoded somewhere, but don't see that as accessible through vips.

If you can help me out with what I am doing wrong, and a testing strategy I will update the PR and add documentation for the feature too. Thanks!!

kristojorg · 2017-03-21T13:59:09Z

My guess is that I need to change this line to support the options, but am not super sure about the proper syntax:

sharp/src/pipeline.cc

Line 845 in 72f69dd

->set("compression", VIPS_FOREIGN_TIFF_COMPRESSION_JPEG));

kristojorg · 2017-03-21T15:04:27Z

Okay I mucked around a bit and it looks like this is working now. Still not exactly sure how to test the compression is actually compressing...

Also not entirely sure how to tiff predictor works, but it does seem to help significantly in reducing file size. Multiple .TIF files around 27MB are reduced to ~20MB without predictor and ~10MB with horizontal predictor.

coveralls · 2017-03-21T15:38:54Z

Coverage increased (+0.008%) to 99.371% when pulling 9173913 on kristojorg:tiff-compression into 9707f8c on lovell:master.

lovell · 2017-03-21T18:02:18Z

Hello, thanks for this PR, a quick glance suggests it's a very good start. I'll make some time in the next day or so to take a more detailed look.

lovell · 2017-03-22T23:55:07Z

lib/output.js

@@ -211,6 +211,8 @@ const webp = function webp (options) {
 * @param {Object} [options] - output options
 * @param {Number} [options.quality=80] - quality, integer 1-100
 * @param {Boolean} [options.force=true] - force TIFF output, otherwise attempt to use input format
+ * @param {Boolean} [options.compression='jpeg'] - compression options: lzw, deflate, jpeg
+ * @param {Boolean} [options.compression='jpeg'] - compression predictor options: none, horizontal, float


Didi you mean options.predictor here?

lovell · 2017-03-22T23:59:53Z

src/pipeline.cc

@@ -1199,6 +1200,25 @@ NAN_METHOD(pipeline) {
  baton->webpLossless = AttrTo<bool>(options, "webpLossless");
  baton->webpNearLossless = AttrTo<bool>(options, "webpNearLossless");
  baton->tiffQuality = AttrTo<uint32_t>(options, "tiffQuality");
+  // tiff compression options
+  std::string tiffCompression = AttrAsStr(options, "tiffCompression");


Given we're going for a string-based input here, can we use vips_enum_from_nick for compression and predictor? (like the kernel property of the resize operation.)

Okay yea this looks like a better idea. Could you show me where I define the map from nickname -> type though? I can't seem to find it for the example you gave, so I'm wondering how the function takes the string input and decides which type to use?

lovell · 2017-03-23T00:01:38Z

test/unit/io.js

@@ -861,6 +861,54 @@ describe('Input/output', function () {
    });
  });

+  it('TIFF lzw compression does not throw error', function () {


Thanks for adding some failure scenarios to the tests. Looking forward to seeing some more positive scenarios/results too.

Could you suggest a strategy for the positive tests? I don't want to simply test that the file got smaller, since occasionally that doesn't work (16 bit tiff files with zip lzw have issues sometimes). Is there a way to read from the metadata that a tiff is compressed?

I don't want to simply test that the file got smaller, since occasionally that doesn't work

Ah, that's along the lines of what I was thinking. Feel free to add a test fixture that can be relied upon to do this. I guess we can at least verify that the TIFF file(s) can be re-opened.

I guess we can test that the file got smaller for the testing image, I'll add that.

kristojorg · 2017-03-23T14:53:37Z

I added a positive test, but not sure it works at the moment. Need to fix the vips_enum_from_nick thing in order to get this working again. Let me know how to use that function and I'll get it set up. Thanks!

lovell · 2017-03-23T17:29:59Z

src/pipeline.cc

@@ -1199,6 +1200,27 @@ NAN_METHOD(pipeline) {
  baton->webpLossless = AttrTo<bool>(options, "webpLossless");
  baton->webpNearLossless = AttrTo<bool>(options, "webpNearLossless");
  baton->tiffQuality = AttrTo<uint32_t>(options, "tiffQuality");
+  // tiff compression options
+  VipsForeignTiffCompression tiffCompression = static_cast<VipsForeignTiffCompression>(


The following might work:

baton->tiffCompression = static_cast<VipsForeignTiffCompression>( vips_enum_from_nick(nullptr, VIPS_TYPE_FOREIGN_TIFF_COMPRESSION, AttrAsStr(options, "tiffCompression")));

kristojorg · 2017-03-24T02:27:20Z

Okay I added the positive test and started using vips_enum_from_nick for both tiffCompression and tiffPredictor. Let me know if this works

lovell · 2017-03-24T13:58:09Z

I think you'll need to add the default values for tiffCompression and tiffPredictor to the options near constructor.js#L171 to prevent "undefined" making its way through as a value.

lovell · 2017-03-24T13:58:55Z

yarn.lock

@@ -0,0 +1,4467 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Was the inclusion of this file intentional?

Ah, no it wasn't. I'll remove that.

lovell · 2017-03-24T13:59:56Z

test/fixtures/index.js

@@ -84,6 +84,7 @@ module.exports = {
  inputWebPWithTransparency: getPath('5_webp_a.webp'), // http://www.gstatic.com/webp/gallery3/5_webp_a.webp
  inputTiff: getPath('G31D.TIF'), // http://www.fileformat.info/format/tiff/sample/e6c9a6e5253348f4aef6d17b534360ab/index.htm
  inputTiffCielab: getPath('cielab-dagams.tiff'), // https://github.com/lovell/sharp/issues/646
+  inputTiffUncompressed: getPath('uncompressed_tiff.tiff'),


What are the origins/rights etc. of this image?

Just a photo I took and have been using for testing myself. Would you prefer a different?

As long as you're happy to licence it, perhaps using CC0, that's OK.

If possible, please could you use a smaller file, or perhaps only a portion of this file, as this is 15MB!

Not sure exactly what that would mean. Do you have a place you suggest I look for a licensed tiff?

https://code.google.com/archive/p/imagetestsuite/wikis/TIFFTestSuite.wiki includes 20 images tagged as "Compression Scheme: None" so might provide something suitable. Your own image is fine as long as you grant us permission to distribute it :)

lovell · 2017-03-24T16:54:55Z

test/unit/io.js

+  it('TIFF lzw compression shrinks test file', function (done) {
+    let startSize;
+    sharp(fixtures.inputUncompressedTiff)
+          .toBuffer((err, buffer, info) => {


Only one toBuffer is supported but there are two here. Perhaps use fs.statSync to read startSize.

lovell · 2017-03-25T17:05:51Z

test/unit/io.js

@@ -863,11 +863,11 @@ describe('Input/output', function () {

  it('TIFF lzw compression shrinks test file', function (done) {
    let startSize;
+    fs.statSync(fixtures.inputUncompressedTiff, (err, stats) => {


Did you mean:

const startSize = fs.statSync(fixtures.inputUncompressedTiff).size;

çç

lovell · 2017-03-25T23:08:51Z

test/unit/io.js

@@ -861,6 +861,70 @@ describe('Input/output', function () {
    });
  });

+  it('TIFF lzw compression shrinks test file', function (done) {
+    const startSize = fs.statSync(fixtures.inputUncompressedTiff).size;


The variable that you created is called inputTiffUncompressed not inputUncompressedTiff.

Please, please run the tests locally and ensure they are passing before pushing further code changes to Github.

I have it working on my end, so I was only getting up on a Saturday to make the tests work to enrich your library. I apologize if it annoyed you.

Feel free to fix it if you want it.

Thank you. I hope you understand that PRs where tests are failing cannot be merged and code that is untested cannot be safely maintained, refactored or otherwise improved.

I've just re-read my original comment and it makes me sound so much more annoyed than the slight level of frustration I was experiencing at that point. I'm sorry for any ill feelings it caused to you. Perhaps some form of pre-push hook to run the tests might help here?

That's alright, sorry I got frustrated as well. It's my second ever PR and I just wasn't thinking about the courtesy and notifications you were probably getting. I'll try to get the tests working and then push it. Thanks for the great lib.

kristojorg · 2017-03-28T21:14:44Z

Alright I have run tests on my machine 😏 and added positive tests for other compression options. All tests are passing at this point. I also switched out the uncompressed tiff file for one from the google library you recommended, as I just didn't want to deal with licensing mine.

The one strange thing I am seeing is that compressing a tiff with the VipsForeignTiffPredictor set to VIPS_FOREIGN_TIFF_PREDICTOR_FLOAT is outputting an all-black image for me. You can see the input and output files here for a deflate compressed tiff. I don't think this has to do with sharp but rather vips. It might not even be an error, I'm unsure.

coveralls · 2017-03-28T21:25:05Z

Coverage increased (+0.01%) to 99.373% when pulling e728e79 on kristojorg:tiff-compression into 9707f8c on lovell:master.

coveralls · 2017-03-28T21:25:05Z

Coverage increased (+0.01%) to 99.373% when pulling e728e79 on kristojorg:tiff-compression into 9707f8c on lovell:master.

lovell · 2017-03-29T11:12:10Z

This is fantastic Kristo, thank you for persisting with this PR, it will make a great addition to sharp.

I was able to reproduce the "float" predictor problem using the vips command line tools so let's report this upstream.

kristojorg · 2017-03-29T13:02:51Z

Thanks for putting up with me : )

lovell · 2017-03-29T16:30:04Z

Running man tiffcp reveals:

A value 3 is for floating point predictor which you can use if the encoded data are in floating point format.

Running the following at the command line works so libvips is OK.

vips cast uncompressed_tiff.tiff out.tiff[compression=deflate,predictor=float] float

So we'll need to modify sharp to cast pixel values to float when using predictor=float. I'll fix this :)

lovell · 2017-04-01T09:19:11Z

TIFF float predictor support added in 088d36b, thanks again!

kristojorg added 2 commits March 20, 2017 22:39

added tiff compression option

5df20e9

added a few tests

95dbaac

kristojorg added 10 commits March 21, 2017 10:20

added vips foreign compression to pipeline.cc

147039d

changed tiff compression options to baton declaration

4ef5a72

added VipsForeignTiffCompression to baton.h

3410367

Initialized foreign compression with jpeg

0f09423

fixed a typo, appears to be working now

87a7445

added tiffpredictor option

2652670

fixed predictor option, now seems to be reducing file size by 2/3! yay

9f4dc72

added tests for predictor options

e18dcc1

fixed style errors

2bb7a8e

removed invalid 'zip' test

9173913

kristojorg mentioned this pull request Mar 21, 2017

Support non-JPEG TIFF output by exposing VipsForeignTiffCompression #594

Closed

lovell added the enhancement label Mar 21, 2017

lovell requested changes Mar 23, 2017

View reviewed changes

kristojorg added 2 commits March 23, 2017 09:57

attempted to use vips_enum_from_nick

a6583b2

added tests

8229db5

added uncompressed tiff fixture for test

8e47c7e

lovell reviewed Mar 23, 2017

View reviewed changes

kristojorg added 3 commits March 23, 2017 22:18

fixed enum_from_nick and test

6657b35

reverted accidental "prettier" formatting of output.js

5f2f588

fixed typo

b331df8

lovell reviewed Mar 24, 2017

View reviewed changes

kristojorg added 2 commits March 24, 2017 10:12

removed yarn.lock

c41b0b1

added default constructor values

b7bf556

lovell reviewed Mar 24, 2017

View reviewed changes

used statSync in tests

c23e9b0

lovell reviewed Mar 25, 2017

View reviewed changes

fixed test error)

9399482

çç

lovell reviewed Mar 25, 2017

View reviewed changes

kristojorg added 2 commits March 28, 2017 16:45

added new image from google source and fixed tests

d272fef

added tests

e728e79

lovell merged commit f8e72f4 into lovell:master Mar 29, 2017

lovell added this to the v0.17.3 milestone Mar 29, 2017

lovell added a commit that referenced this pull request Mar 31, 2017

Add changelog and credit for #738

4001c4a

		@@ -0,0 +1,4467 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

Added tiff compression option #738

Added tiff compression option #738

Conversation

kristojorg commented Mar 21, 2017

kristojorg commented Mar 21, 2017

kristojorg commented Mar 21, 2017

coveralls commented Mar 21, 2017

lovell commented Mar 21, 2017

lovell Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristojorg Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristojorg commented Mar 23, 2017

Choose a reason for hiding this comment

kristojorg commented Mar 24, 2017

lovell commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristojorg commented Mar 28, 2017

coveralls commented Mar 28, 2017 • edited Loading

coveralls commented Mar 28, 2017

lovell commented Mar 29, 2017

kristojorg commented Mar 29, 2017

lovell commented Mar 29, 2017

lovell commented Apr 1, 2017

lovell Mar 22, 2017 •

edited

Loading

kristojorg Mar 23, 2017 •

edited

Loading

coveralls commented Mar 28, 2017 •

edited

Loading