-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
vips_resize for resizing #977
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, thank you Jarda. I've added a couple of questions/comments inline.
yshrink = std::max(1, static_cast<int>(floor(yfactor))); | ||
xresidual = static_cast<double>(xshrink) / xfactor; | ||
yresidual = static_cast<double>(yshrink) / yfactor; | ||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly worried that removing the swap
here will break something but the tests that were added for the original bug #387 continue to pass. There's probably an edge case that, if ever found, we can add the fix and another test for later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats removed because xresidual is not used after that point. Its used only before for the shrink_on_load edge case.
@@ -279,23 +279,6 @@ class PipelineWorker : public Nan::AsyncWorker { | |||
} | |||
xfactor = static_cast<double>(shrunkOnLoadWidth) / static_cast<double>(targetResizeWidth); | |||
yfactor = static_cast<double>(shrunkOnLoadHeight) / static_cast<double>(targetResizeHeight); | |||
xshrink = std::max(1, static_cast<int>(floor(xfactor))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the xshrink
, yshrink
, xresidual
and yresidual
variables entirely now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats what I wanted to do. But they are still used outside of the resize logic for the shrink_on_load logic.
Particularly xresidual and yresidual is used here
Line 245 in a3457a0
if (shrink_on_load > 1 && (xresidual == 1.0 || yresidual == 1.0)) { |
and xshrink is also used in context of shrink_on_load here
Line 226 in a3457a0
xshrink == yshrink && xshrink >= 2 && |
@@ -98,6 +98,7 @@ describe('Partial image extraction', function () { | |||
sharp(fixtures.inputPngWithGreyAlpha) | |||
.extract({ left: 20, top: 10, width: 380, height: 280 }) | |||
.rotate(90) | |||
.jpeg() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this is a much better test now.
test/unit/gamma.js
Outdated
.toBuffer(function (err, data, info) { | ||
if (err) throw err; | ||
assert.strictEqual('png', info.format); | ||
assert.strictEqual('jpeg', info.format); | ||
assert.strictEqual(320, info.width); | ||
fixtures.assertSimilar(fixtures.expected('gamma-alpha.jpg'), data, { threshold: 20 }, done); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can remove the { threshold: 20 }
bit now that you've updated this to (correctly) use JPEG output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try it out tomorrow.
Oh, I had an idea for the shrink-on-load optional name too: |
Sounds good. And if its fastShrinkOnLoad: false it would just use that lower shrink_on_load factor that I stole from vipsthumbnail, right? That should be easy.. just creating test will be tricky :-). |
Yes, that's correct, thank you. In terms of testing variable shrink-on-load, you could try extracting a 9x8 "problem" region that you know will be different based on the |
cb9c598
to
f642da9
Compare
@lovell So here is update. I removed successfully the treshold for And added the fastShrinkOnLoad logic so you can review that in one go. |
Thanks for the updates Jarda. As predicted that one test failure is annoying. Perhaps resize to just under half (or quarter) the original size and |
@lovell Interesting.. Will double check that later on.. it was not failing on my computer.. maybe some oversight.. |
@lovell It's really passing locally. Only difference I could think of is somewhat different vips version. Locally I compiled from vips-8.6.0-4 package (OS X). If I change expect to the other file (fastShrinkOnLoad: false vs fastShrinkOnLoad: true) similarity goes from 0 to 14, which seems that it can recognize the difference between desired and undesired result good enough. Using extract I don't think it would be make much of the difference, because the issue I am capturing is around 1px offset, which is always around 1px regardless if I resize and extract or just resize. Any idea why we are getting different result on travis? |
Are you using libjpeg or libjpeg-turbo? The shrink-on-load feature of libjpeg-turbo is SIMD-enabled and has slightly different rounding logic. The Linux and Windows builds both use the libjpeg-turbo version. If so, perhaps just increase the threshold to 7 and add a separate test that verifies the result of |
A rebase is probably required too. My fault, sorry. I had to rebase the suit branch against master to pick up recently merged PRs. |
@lovell On my system I have only libjpeg. And your libvips build you are providing for sharp - |
I should also do my 'image shifting' test with jpeg-turbo to see how bad it gets there. |
@lovell Just recompiled vips with jpeg-turbo and you are right, thats really that makes the difference. So maybe compiling with turbo on os x would also make sense for consistent result for these nuanced situations :-). Will double check how much difference it makes to my shifting issue. |
@lovell ok.. so shifting is very much the same. There were just subtle differences for some pixels shades. Let me know if in this context makes more sense to you make the test happy with both jpeg libraries (with higher treshold) or just update expected image with turbo |
We should probably support both libjpeg variants for the tests if it's as simple as increasing the threshold a little bit :) The prebuilt libvips provided for OS X is based on the homebrew default, currently libjpeg. We should probably switch to the turbo version as that will make at least a 2x difference for Mac users - leave that bit with me. |
f642da9
to
9e2677d
Compare
@lovell ok.. updated, anything else? |
9e2677d
to
d3f9780
Compare
Podivuhodný, děkuji! |
This is follow-up for #954.
Bit struggle with failing tests. So intentionally posted tests adjustments in second commit so its easy to go back and forward to double check these adjustments.
I got
If you compare new expected image with original - I think it could be considered as improvement, or at least not worse result. It might have something to do with that vips_resize combine somewhat differently shrink and reduce. So in this case I just updated the expected result.
Here it gets confusing for me, because test is comparing jpeg with png which already makes difference and not sure what the intention if it has something to do with alpha handle testing. I could not tell any difference visually and by comparing jpeg with jpeg I got good similarity. But not sure if its good fix.
Similar situation as in 2) as its comparing png with jpeg and expected result is already in pretty poor quality. I tried png and jpeg outputs with and without this PR and could not tell difference apart from some slightly different jpeg compression.
I am happy to make any adjustments based on your feedback, but also feel free to make the adjustments yourself.. I am ok either way :-).
I would make the PR for 'lessShrinkOnLoad' thing once this is sorted out.
Thanks!