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

Support enlargement when using embed option (was: Possible error with embed()) #243

Open
ernestopye opened this issue Jul 23, 2015 · 4 comments

Comments

@ernestopye
Copy link

Unsure if this is on sharp's end, or possibly within libvips. The issue is that when attempting to resize(w,h).embed() an image which has either a width or height lower than w or h respectively, the embed() call simply never gets hit, and you get the full image returned.

For example, with an image that is 200x200, if I do:

resize(100, 150).embed() - works fine, you get a 100x150 image, with vertical padding.
resize(150, 100).embed() - works fine, you get a 150x100 image, with horizontal padding.
resize(200, 150).embed() - works fine, you get a 200x150 image, with horizontal padding (2 param = image width)

However:
resize(201, 150).embed() - returns original image.
resize(150, 201).embed() - returns original image.
resize(220, 220).embed() - returns original image.

This is also noticeable if you have images with much larger widths than heights, say, a logo. If I want to ensure they come back as 200x200, then this returns the original image on most scenarios (h is very small, almost always less than 200, even if the width is generally greater than 200).

Is this expected behavior? I understand we never want to attempt to enlarge an image for obvious reasons. However, since we're padding the images with some background, the issue of quality loss becomes less likely.

In some of the scenarios I'm running into, there's enough width to the images I'm attempting to resize for them to simply receive vertical padding, but whatever's checking the dimensions seems to cut through and just return the original instead. Even if both the width/height passed to resize are larger than the image's dimensions, could both vertical and horizontal paddings be added?

@lovell
Copy link
Owner

lovell commented Jul 23, 2015

Hi Ernesto, the embed test cases verify reduction but not enlargement so I suspect it's probably never been supported.

Supporting enlargement would make a useful addition, thank you for the suggestion.

I'd expect padding to apply to only one of either the vertical or horizontal edges. Padding on all sides sounds more like #128.

"I understand we never want to attempt to enlarge an image for obvious reasons."

Only when using the withoutEnlargement option, I think. We might need to test the interaction of an attempted enlargement+embed with this option as well as maxand min.

I'd be very happy to accept a contribution implementing some or all of this feature if you're in a position to help. It looks like you've already got plenty of test cases!

@lovell lovell changed the title Possible error with embed() Support enlargement when using embed option (was: Possible error with embed()) Jul 23, 2015
@ernestopye
Copy link
Author

Absolutely. I've forked the repo and will try to take a closer look this week.

Edit: Or maybe right now. :)

I set up some test cases, and withoutEnlargement does appear to be the culprit. The issue is:

pipeline.cc - line 355

// Do not enlarge the output if the input width *or* height are already less than the required dimensions
if (baton->withoutEnlargement) {
  if (inputWidth < baton->width || inputHeight < baton->height) {
    xfactor = 1;
    yfactor = 1;
    xshrink = 1;
    yshrink = 1;
    xresidual = 0;
    yresidual = 0;
    baton->width = inputWidth;
    baton->height = inputHeight;
  }
}

The thing is that when embedding an image, then there really is never a need for any enlargement of the image itself, even if the result might have either a width or height with a larger value. This would involve adding padding both vertically and horizontally (similar to #128 like you said). So if you have a 50x50 image, and wanted a 200x200 version, you'd have the 50x50 original in the middle (or somewhere else specified via gravity) with a border around the whole thing of some color via background.

The behavior for withoutEnlargement = false is already what I would expect. It upscales the image as expected.

Of course, both of these behaviors would be exclusive to when baton->canvas == Canvas::EMBED.

ImageMagick has a feature that would come in handy for this, as of 6.2.5 - the "flatten" option:

http://imagemagick.org/Usage/thumbnails/#pad

Not sure if libvips supports this or not.

For my case, I think I will just avoid calling withoutEnlargement for padded images for now, and allow some poor quality images to be generated. Let me know what you think.

@lovell
Copy link
Owner

lovell commented Aug 25, 2015

Hi @ernestopye, sorry for the delay in responding. Do you think the addition of a pad feature, as described by the comments in #128, might provide enough API to solve the problem you're facing?

@ernestopye
Copy link
Author

Hey @lovell, no worries. Yes, I think it would as long as withoutEnlargement = true was handled in a way that avoids the scenario I'm describing above. It would basically never prevent resizing/padding of an image, even if both the width/height of the original image are lower than the target width/height, as padding could be added around the whole thing.

ernestopye referenced this issue in tripviss/image-resizer Aug 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants