apache / incubator-pagespeed-ngx

Automatic PageSpeed optimization module for Nginx
http://ngxpagespeed.com/
Apache License 2.0
4.37k stars 362 forks source link

Add way to crop image thumbnails #418

Open joevandyk opened 11 years ago

joevandyk commented 11 years ago

It's great that ngx_pagespeed allows you to resize thumbnails automatically. But it's useful to crop thumbnails into certain dimensions (i.e. square). It would be fantastic if something like this worked:

<img src="img.jpg" width=50 height=50 crop /> (or whatever the syntax is)

And the image would be a 50x50 thumbnail, cropped to fit.

jeffkaufman commented 11 years ago

How would you want it to be cropped? Top left corner? Center?

In general a feature like this would only work if someone were writing a site specifically to go through PageSpeed. Which means it's not going to benefit most users of PageSpeed, who either have an existing site or don't want to be "locked-in" to the product.

joevandyk commented 11 years ago

Center, by default. Perhaps there could be options for cropping methods.

I'd assume that most users of pagespeed assume they are going to use it? Especially for image manipulations -- if pagespeed wasn't going to resize/format the images correctly, another tool would need to.

As is now, I have to use a separate image resizing service to handle the crop needs. If pagespeed could crop images, that would simplify many sites significantly (source images gets uploaded to s3, the source html references the s3 image and the size/crop options).

chaizhenhua commented 10 years ago

@jeffkaufman can we optimize following CSS and html?

.crop {
    width: 200px;
    height: 150px;
    overflow: hidden;
}
<div class="crop">
    <img src="....jpg" alt="..." />
</div>
igrigorik commented 10 years ago

@chaizhenhua same image can be reused later with a different crop (ala sprites). So it's exactly a safe operation..

chaizhenhua commented 10 years ago

@igrigorik can pagespeed only send the croped image and ignore the hidden part?

joevandyk commented 10 years ago

@jeffkaufman I'd want it cropped to center.

Some other systems allow cropping on the face.

jeffkaufman commented 10 years ago

@chaizhenhua

At first I thought it might be easier to recognize with the class on the image:

.crop {
    width: 200px;
    height: 150px;
    overflow: hidden;
}
<img class="crop" src="....jpg" alt="..." />

but then I realized that this would squish the image instead of cropping it.

Your code snippet crops the image to the top left; is that something people do often? @joevandyk wants it center-cropped, which makes more sense to me. To center-crop it with standard css I think you'd need to do something like http://stackoverflow.com/questions/11552380/how-to-automatically-crop-and-center-an-image which would be pretty hard for pagespeed to pick out and optimize.

(This would also break pages where they used javascript to change the cropping in response to an event.)

tomasdev commented 7 years ago

What if the crop is in percentage?

I mean... this would be unfixable if the image has percent-based dimensions :|