.stats() implementation #915

rn4391 · 2017-08-18T06:16:06Z

Please review the code. Especially the C++ part as I am not very confident with the language. Let me know if something else or some other helper value like isOpaque needs to be exposed in the output.

Pull from sharp / master

coveralls · 2017-08-18T06:27:59Z

Coverage decreased (-10.009%) to 89.309% when pulling cd97d38 on rnanwani:feature_stats_implementation into 791fd35 on lovell:master.

lovell · 2017-08-19T17:28:41Z

Looks great, and with lots of tests too, thank you very much Rahul!

In terms of the sporadically failing values, perhaps there should be a margin of error for these? For example the difference in the sum of pixel values appears to be <0.1% across all platforms.

I realise the min and max coords will vary across test runs. Perhaps we could verify their values are positive integers that fall within the bounds of the image?

lovell · 2017-08-19T17:30:02Z

test/unit/stats.js

+    readable.pipe(pipeline);
+  });
+
+  it('Stream in, Promise out', function (done) {


This test should be able to return the Promise instance, which will then remove the need to use done and catch.

.stats() does return a promise in this test. I am not sure if I got your point. A similar test exists for metadata() as well.

mocha deals with tests that return (and reject) a Promise, e.g.:

it('Stream in, Promise out', function () { const pipeline = sharp(); fs .createReadStream(fixtures.inputJpg) .pipe(pipeline); return pipeline .stats() .then(function (stats) { ... }); });

lovell · 2017-08-19T17:32:11Z

src/stats.cc

+      (baton->err).append(err.what());
+    }
+    if (imageType != sharp::ImageType::UNKNOWN) {
+      stats = image.stats();


Can stats() throw? This might need to live inside the try/catch block.

I don't think stats() throws
https://github.com/jcupitt/libvips/blob/master/libvips/arithmetic/stats.c

libvips' C++ wrapper passes through call_option_string that can throw.

Rebase from master

…mentation

coveralls · 2017-09-21T04:40:35Z

Coverage decreased (-1.0%) to 98.355% when pulling 8850096 on rnanwani:feature_stats_implementation into 3511723 on lovell:master.

lovell · 2017-09-21T08:19:56Z

test/unit/stats.js

+    readable.pipe(pipeline);
+  });
+
+  it('Stream in, Promise out', function (done) {


mocha deals with tests that return (and reject) a Promise, e.g.:

it('Stream in, Promise out', function () { const pipeline = sharp(); fs .createReadStream(fixtures.inputJpg) .pipe(pipeline); return pipeline .stats() .then(function (stats) { ... }); });

lovell · 2017-09-21T08:23:01Z

src/stats.cc

+      (baton->err).append(err.what());
+    }
+    if (imageType != sharp::ImageType::UNKNOWN) {
+      stats = image.stats();


libvips' C++ wrapper passes through call_option_string that can throw.

Merge from lovell/sharp

…mentation

coveralls · 2017-11-08T15:49:11Z

Coverage decreased (-0.1%) to 99.186% when pulling 30540a0 on rnanwani:feature_stats_implementation into aad16ac on lovell:master.

rn4391 · 2017-11-09T10:25:33Z

Is there anything I can do to fix this build?

coveralls · 2017-11-09T11:41:33Z

Coverage decreased (-0.1%) to 99.186% when pulling 7ef26b1 on rnanwani:feature_stats_implementation into aad16ac on lovell:master.

lovell

Hello, thanks for all the updates to this PR, I've added a few comments inline that should hopefully solve that final test failure.

lovell · 2017-11-10T10:41:12Z

src/stats.cc

+    sharp::ImageType imageType = sharp::ImageType::UNKNOWN;
+
+    try {
+      std::tie(image, imageType) = OpenInput(baton->input, VIPS_ACCESS_SEQUENTIAL);


Probably best to use the configured baton->accessMode instead of VIPS_ACCESS_SEQUENTIAL as this defaults to random, which avoids over-computation from the source decoder (and should fix those libtiff test errors).

lovell · 2017-11-10T10:42:30Z

src/stats.cc

+        int bands = image.bands();
+        double const max = MaximumImageAlpha(image.interpretation());
+        for (int b = 1; b <= bands; b++) {
+          ChannelStats cStats(stats.getpoint(STAT_MIN_INDEX, b).front(), stats.getpoint(STAT_MAX_INDEX, b).front(),


I think the values returned by getpoint are double precision so some of these will require static_cast<int> conversion.

lovell · 2017-11-10T10:45:11Z

test/unit/stats.js

+
+const fs = require('fs');
+const assert = require('assert');
+// const exifReader = require('exif-reader');


These unused imports can be removed.

lovell · 2017-11-10T10:46:40Z

test/unit/stats.js

+      assert.strictEqual(true, isInAcceptableRange(stats.channels[0]['squaresSum'], 83061892917));
+      assert.strictEqual(true, isInAcceptableRange(stats.channels[0]['mean'], 101.44954540768993));
+      assert.strictEqual(true, isInAcceptableRange(stats.channels[0]['stdev'], 58.373870588815414));
+      assert.strictEqual(true, isInteger(stats.channels[0]['minX']) && isInRange(stats.channels[0]['minX'], 0, 2725));


Perhaps split these assert(true, x && y) statements into two separate assert statements to aid debugging and remove conditional logic from tests.

lovell · 2017-11-10T10:49:57Z

docs/api-input.md

@@ -4,6 +4,7 @@

 -   [clone](#clone)
 -   [metadata](#metadata)
+-   [stats](#stats)


The docs/api-*.md files are autogenerated so this content needs to live in the jsdocs above the new stats function.

coveralls · 2017-11-14T13:35:31Z

Coverage decreased (-0.1%) to 99.186% when pulling 2aea940 on rnanwani:feature_stats_implementation into aad16ac on lovell:master.

coveralls · 2017-11-14T14:08:58Z

Coverage increased (+0.02%) to 99.349% when pulling 2aea940 on rnanwani:feature_stats_implementation into aad16ac on lovell:master.

coveralls · 2017-11-14T14:53:19Z

Coverage increased (+0.02%) to 99.349% when pulling c8d01fe on rnanwani:feature_stats_implementation into aad16ac on lovell:master.

lovell · 2017-11-14T19:50:45Z

This all looks great, thank you for the updates Rahul. As soon as the suit WIP branch is merged into master we can look at getting this merged in too for the v0.19.0 release.

lovell · 2017-12-09T11:18:09Z

Merged to master, thanks Rahul, this will be in v0.19.0.

sebastienbarre · 2020-04-11T16:38:39Z

Hi. I'm having issues getting the stats from a PNG image converted to LCH space.

  sharp(file)
    .toColorspace('lch')
    .toBuffer()
    .then(data => sharp(data).stats())
    .then(stats => console.log(stats))
    .catch(error => signale.error('Failed getting stats. ' + error.message));

This returns the same stats, whether I convert to 'srgb', or 'lch', or 'lab'. Am I not understanding the stats function correctly? I thought it would give me the stats for the L, C, H channels, which would be different than the stats for R, G, B channels.

Thanks

RNanwani and others added 2 commits August 8, 2017 19:00

Merge pull request #1 from lovell/master

b58b156

Pull from sharp / master

stats API

cd97d38

lovell reviewed Aug 19, 2017

View reviewed changes

RNanwani and others added 3 commits September 20, 2017 20:17

Merge pull request #2 from lovell/master

d713c67

Rebase from master

Merge remote-tracking branch 'origin/master' into feature_stats_imple…

b8faea8

…mentation

stats() unit tests update

8850096

lovell requested changes Sep 21, 2017

View reviewed changes

RNanwani and others added 4 commits October 31, 2017 13:16

Merge pull request #3 from lovell/master

58aaa14

Merge from lovell/sharp

Merge remote-tracking branch 'origin/master' into feature_stats_imple…

f9f26d8

…mentation

try..catch for stats

de24edb

more tests

30540a0

stats fix for fully transparent image

8e9aeeb

stats test fix

7ef26b1

lovell requested changes Nov 10, 2017

View reviewed changes

rn4391 added 2 commits November 14, 2017 18:26

comment fixes

e279970

api docs fix

2aea940

increase unit coverage

c8d01fe

lovell added the ready-to-ship label Nov 14, 2017

lovell added this to the v0.19.0 milestone Nov 14, 2017

lovell approved these changes Nov 15, 2017

View reviewed changes

lovell merged commit d6aee8e into lovell:master Dec 9, 2017

lovell added a commit that referenced this pull request Dec 9, 2017

Changelog and doc refresh for #915

1d7a0ea

rn4391 mentioned this pull request Dec 18, 2017

Enhancement: expose image stats (e.g. min, max, avg, sum, isOpaque) #723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.stats() implementation #915

.stats() implementation #915

rn4391 commented Aug 18, 2017

coveralls commented Aug 18, 2017 •

edited

Loading

lovell commented Aug 19, 2017

lovell Aug 19, 2017

rn4391 Sep 21, 2017

lovell Sep 21, 2017

lovell Aug 19, 2017 •

edited

Loading

rn4391 Sep 21, 2017

lovell Sep 21, 2017

coveralls commented Sep 21, 2017 •

edited

Loading

lovell Sep 21, 2017

lovell Sep 21, 2017

coveralls commented Nov 8, 2017 •

edited

Loading

rn4391 commented Nov 9, 2017

coveralls commented Nov 9, 2017 •

edited

Loading

lovell left a comment

lovell Nov 10, 2017

lovell Nov 10, 2017

lovell Nov 10, 2017

lovell Nov 10, 2017

lovell Nov 10, 2017

coveralls commented Nov 14, 2017 •

edited

Loading

coveralls commented Nov 14, 2017 •

edited

Loading

coveralls commented Nov 14, 2017 •

edited

Loading

lovell commented Nov 14, 2017

lovell commented Dec 9, 2017

sebastienbarre commented Apr 11, 2020

.stats() implementation #915

.stats() implementation #915

Conversation

rn4391 commented Aug 18, 2017

coveralls commented Aug 18, 2017 • edited Loading

lovell commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovell Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 8, 2017 • edited Loading

rn4391 commented Nov 9, 2017

coveralls commented Nov 9, 2017 • edited Loading

lovell left a 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

coveralls commented Nov 14, 2017 • edited Loading

coveralls commented Nov 14, 2017 • edited Loading

coveralls commented Nov 14, 2017 • edited Loading

lovell commented Nov 14, 2017

lovell commented Dec 9, 2017

sebastienbarre commented Apr 11, 2020

coveralls commented Aug 18, 2017 •

edited

Loading

lovell Aug 19, 2017 •

edited

Loading

coveralls commented Sep 21, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 9, 2017 •

edited

Loading

coveralls commented Nov 14, 2017 •

edited

Loading

coveralls commented Nov 14, 2017 •

edited

Loading

coveralls commented Nov 14, 2017 •

edited

Loading