Turistforeningen / node-s3-uploader

Flexible and efficient resize, rename, and upload images to Amazon S3 disk storage. Uses the official AWS Node SDK for transfer, and ImageMagick for image processing. Support for multiple image versions targets.
https://www.npmjs.com/package/s3-uploader
MIT License
241 stars 54 forks source link

Not scale up if the image has small size than configuration #23

Open vothanhkiet opened 9 years ago

vothanhkiet commented 9 years ago

Like the title said, do we have option like that.

Starefossen commented 9 years ago

This is currently not supported. s3-uploader is using gm under the hood, aheckmann/gm#226 discusses this functionality.

vothanhkiet commented 9 years ago

I think your library can support that with some checking. I don't know coffee script, so I give some pseudo code in node as example.

var resizeWith = version.maxWidth;
var resizeHeight = version.maxHeight;
if (!version.force && self.meta && self.meta.imageSize) {
    if (version.maxWidth > self.meta.imageSize.width) {
        resizeWith = self.meta.imageSize.width;
    }
    if (version.maxHeight > self.meta.imageSize.height) {
        resizeHeight = self.meta.imageSize.height;
    }
}

var img = mpc.resize(resizeWith, resizeHeight, '^')
    .gravity('Center')
    .crop(resizeWith, resizeHeight)
    .quality(version.quality || self.config.opts.resizeQuality);

The version.force = true will indicate that user want to scale up if the image size was smaller than the configuration size. The '^' will make the image is resized while maintaining the aspect ratio of the image, but the resulting width or height are treated as minimum values rather than maximum values.

oblador commented 9 years ago

Not sure what you mean that GraphicsMagick doesn't support this. You'd only append a > at the end of the -resize argument, for example 400x400>.

Do you want a PR for this? It's a breaking change, but IMHO with the current API I would not expect it scaling up when passing maxWidth/Height arguments hence it's a bug that needs fixing rather than breaking current behavior.

Starefossen commented 9 years ago

The short answer is yes, I would want a PR for this.

The long answer is that this is outside of what I need from s3-uploader so I will not have the time to do this myself.

Since this issue first was opened we have refactored away from gm so most of the functionality would need to go in im-resize package. Also aspectratio would need a corresponding change to calculate correct image size based on this new logic. It currently assumes that the image will be scaled up if it is smaller than the max width / height option.

oblador commented 9 years ago

@Starefossen: no worries, already did those changes locally for im-resize can do it for aspectratio too. Question is if you're willing to merge a breaking change. Replied here since there was no issue and very low activity in the im-resize repo.

oblador commented 9 years ago

By the way, wouldn't it be better to rely on identify or similar than to get actual dimensions rather than precalculate them?

Starefossen commented 9 years ago

Yes, I am willing to merge this breaking change as long as the existing behavior can be enabled with an option.

It would be easier to use ImageMagics's identify command, but it will also be much much slower since it requires reading each image version from disk in order to return the size of it.

oblador commented 9 years ago

identify would be more accurate since rounding and resizing algorithms might differ between im-resize and Image/GraphicsMagick. Slower, sure, but in the context of manipulating images and sending them to S3 it's probably negligible unless you're at Facebook scale.

If you think aspectratio is the way to go I'll do it :-)

Starefossen commented 9 years ago

Yes, for the time being I think aspectratio is the right way to go. I welcome any PR for that :thumbsup: