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 support for trimming (#491) #492

Merged
merged 1 commit into from
Jul 8, 2016
Merged

Conversation

kleisauke
Copy link
Contributor

@kleisauke kleisauke commented Jul 5, 2016

Usage: trim() or defining a threshold percentaged tolerance between 0-255 0-100: trim(20)

2 test cases included (a test with a 16-bit PNG and a normal PNG, both with an alpha channel). Trimming is always performed before any resize operation.

I was working on an optional argument defining the border(s) that should be trimmed away. By default the trimming is performed on all borders. Let me know if you want such additional argument.


// find the value of the pixel at (0, 0) ... we will search for all pixels
// significantly different from this
std::vector<double> background = image(0, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should let the users define the the point from where the trimming color is picked. Don't know how we should handle this (by a color or a position). For now it's always the top-left color.

@jcupitt
Copy link
Contributor

jcupitt commented Jul 5, 2016

Trim() will scan the whole image once to find the crop area, so it won't work with sequential mode images.

I guess you'd need to scan to find the crop area, then close and reopen to crop and shrink. This would halve speed for things like PNG.

@lovell
Copy link
Owner

lovell commented Jul 5, 2016

Wow, that was fast, thanks Kleis!

I don't think this needs to be overly-configurable for now, so let's have it (attempt to) trim all edges and always start with the top-left pixel.

A test with a 16-bit input image is probably a must given lossless PNG will likely be the format of choice here.

Making the trim operation mutually-exclusive with sequentialRead should alleviate the problem John mentions.

VImage profileBottomV;
VImage profileBottomH = rows.flipver().profile(&profileBottomV);

int left = (int) profileLeftV.min();
Copy link
Owner

Choose a reason for hiding this comment

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

Please can these use static_cast<int>(floor( ... )).

@jcupitt
Copy link
Contributor

jcupitt commented Jul 5, 2016

I suppose the threshold could be a percentage. That would avoid having to change it for 8 and 16-bit images.

@@ -406,7 +406,9 @@ namespace sharp {

// we need to smooth the image, subtract the background from every pixel, take
// the absolute value of the difference, then threshold
VImage mask = (image.median(3) - background).abs() > threshold;
// TODO: Not sure if this calculation supports tolerance. Or should we do this?:
// VImage mask = (image.median(3) - background).abs() > (MaximumImageAlpha(image.interpretation()) / 100 * tolerance);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd have this as max * tolerance / 100. I guess this should be uncommented too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Committed by mistake.

it('16-bit PNG with alpha channel', function(done) {
sharp(fixtures.inputPngWithTransparency16bit)
.resize(32, 32)
.trim()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I change this to trim(100) then it will fail:

Uncaught Error: extract_area: parameter width not set
extract_area: parameter height not set

Debug:

(sharp:6296): GLib-GObject-WARNING **: value "-32" of type 'gint' is invalid or out of range for property 'width' of type 'gint'

(sharp:6296): GLib-GObject-WARNING **: value "-32" of type 'gint' is invalid or out of range for property 'height' of type 'gint'
[Error: extract_area: parameter width not set
extract_area: parameter height not set
]

It looks like only values from 1-99 are permitted.

@kleisauke
Copy link
Contributor Author

Squashed, rebased, added logic to NAN_METHOD(pipeline) and added some inline notes.

int bottom = rows.height() - static_cast<int>(floor(profileBottomH.min()));

// and now crop the original image
return image.extract_area(left, top, right - left, bottom - top);
Copy link
Owner

Choose a reason for hiding this comment

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

I guess even with a 99% tolerance we could see rounding errors that lead to a 100%-like failure. How about a non-zero check for the width and height passed to extract_area?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we throw a VError if width or height is less than or equal to 0? Or should we return the 'untouched' input image?

Copy link
Owner

Choose a reason for hiding this comment

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

Good question. Not failing here could produce unexpected results so I think throwing a VError is the right thing to do.

@lovell
Copy link
Owner

lovell commented Jul 8, 2016

I "fixed" the Appveyor CI builds via 8dd554b so after a rebase against master this PR should start passing again :)

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage decreased (-0.6%) to 97.791% when pulling 7314578 on kleisauke:feature-trim into 673d827 on lovell:master.

@kleisauke
Copy link
Contributor Author

All builds are passing, thanks!

@lovell lovell added this to the v0.15.1 milestone Jul 8, 2016
@lovell lovell merged commit b696278 into lovell:master Jul 8, 2016
@lovell
Copy link
Owner

lovell commented Jul 8, 2016

Dank je!

@kleisauke kleisauke deleted the feature-trim branch July 9, 2016 08:34
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

4 participants