API for progressive JPG and other recommended web thumb best practices

jrochkind commented 6 years ago

Is making sure the JPG output is a progressive JPG part of the ImageProcessing API?

It makes so much sense to make sure your thumbnails are progressive JPGs. They are often (for mathematical reasons I don't understand) smaller in file size, but more importantly they load in a way better for UX under a slow connection.

Another, although less important, thing that makes a lot of sense for web thumb JPGs, is stripping EXIF metadata, and making sure they are translated to color-profile sRGB.

These are things that all the guides for creating JPG thumbs on the web recommend (including Google's), but I find often left out or not highlighted in documentation in ruby easy-thumbnail-gen libraries.

janko commented 6 years ago

Is making sure the JPG output is a progressive JPG part of the ImageProcessing API?

It's not there by default, but you can pass options to the underlying processor:

ImageProcessing::Vips
  .source(image)
  .resize_to_limit(400, 400)
  .saver(interlace: true) # passed to #jpegsave and #pngsave
  .call

ImageProcessing::MiniMagick
  .source(image)
  .resize_to_limit(400, 400)
  .saver(interlace: "...") # adds -interlace (I'm not sure whether it should be "Line", "Plane", "JPEG", "PNG", "GIF" or something else)
  .call

It makes so much sense to make sure your thumbnails are progressive JPGs. They are often (for mathematical reasons I don't understand) smaller in file size, but more importantly they load in a way better for UX under a slow connections.

Yeah, sounds like for generating JPEG thumbnails interlacing should be the default (ImageOptim thinks so too). For PNGs we'd probably want to leave it off by default, because if I understood https://github.com/jcupitt/libvips/issues/140#issuecomment-47515390 correctly, generating progressive PNGs might negatively impact performance.

Another, although less important, thing that makes a lot of sense for web thumb JPGs, is stripping EXIF metadata,

Yes, that was something that I had in mind. For ImageMagick stripping EXIF and other metadata is as simple as using -thumbnail instead of -resize, though ImageProcessing uses -resize for #resize_* methods and it's not currently possible to switch to -thumbnail (PRs welcome). For libvips there isn't a built-in solution, though I've been playing with EXIF stripping locally and I might have a working solution (though would love to cover TIFF data as well).

Both ImageMagick and libvips have a "strip" option, but for ImageMagick that also strips color profiles, for libvips probably as well.

At the moment I'm reluctant to add it as default behaviour, as some people might not be using it only for generating thumbnails, e.g. they might resize the original image and not keep the original, so they might be surprised to find out that EXIF and other data is stripped.

and making sure they are translated to color-profile sRGB.

I'm not sure how to do that yet, but if that's a reasonable default we might add it to ImageProcessing.

For now we can just highlight how to do that in the README. Maybe we could add a :thumbnail saver option that will apply all these defaults for generating thumbnails.

hmistry commented 6 years ago

It's not a default setting as such things are best left up to the dev's using the library. The ImageProcessing API is a common generic wrapper to provide a uniform interface to underlying image processors like ImageMagick and Libvps. Different apps have different needs (e.g. image archiving, photographers, etc where preservation of quality and metadata is key).

I think linking such recommendations is a good idea. We would welcome a PR for consideration to such links that you feel will benefit dev's.

jrochkind commented 6 years ago

Right, whether default or not, I think a common generic API to exersize these options (whether in IM or vips), without having to provide back-end-specific directives, might be a good idea? Is my suggestion.

jrochkind commented 6 years ago

(PS: I actually work in digital preservation/archiving, and while we don't want to touch the originals, there is no reason I know of not to make the derivative/version thumbs optimized for the web).

renchap commented 6 years ago

I'm not sure whether it should be "Line", "Plane", "JPEG", "PNG", "GIF" or something else

It does not matter for JPEG files according to this answer: https://stackoverflow.com/a/36436945/1377358

jrochkind commented 6 years ago

For libvips there isn't a built-in solution (for stripping EXIF)

There is from the vips command-line, which makes it seem like there might be from libvips? See --strip and --delete options to vipsthumbnail. http://libvips.blogspot.com/2013/11/tips-and-tricks-for-vipsthumbnail.html

This kind of having to figure stuff out, though, is part of why I suggest that stripping metadata and progressive JPG should be part of the ImageProcessing API, figure it out once and put it in ImageProcessing, and then every consumer doens't have to figure it out -- and can have it keep working if they switch from IM to vips backends.

hmistry commented 6 years ago

@janko-m if you decide to add strip as default, please consider the additional time cost if any and provide option to turn it off.

When I implemented this a year ago using ImageProcessing with MiniMagick, I discovered stripping added another 200-300 ms to process time. My optimum recipe for version generation was:

1. fetch original
2. generate a large copy using resize and strip (have the 200-300 ms computation penalty once)
3. generate medium copy from large, small from medium, etc (no strip for metadata because there should be none to begin with)

janko commented 6 years ago

For stripping metadata both processors coincidentally already have the same API:

ImageProcessing::MiniMagick
  .saver(strip: true) # appends the `-strip` option
  # ...

ImageProcessing::Vips
  .saver(strip: true) # passes the `:strip` saver option
  # ...

However, as discussed in https://github.com/janko-m/image_processing/issues/28, it might not be desirable to strip by default, as people might be relying on EXIF tags existing, as was the case with earlier versions of ImageProcessing. Stripping also removes color profiles, which if I understood correctly might make the image display with different colors in the browser? From the post that @jrochkind linked to it seems that it should be a good as long as images are converted to the sRGB colorspace (if they aren't already).

Progressive JPEGs/PNGs are also easy to do, as mentioned previously. I agree with @jrochkind that we should at least document these common tips & tricks. I'm thinking we should add it as a wiki page, then link to it from the README.

I discovered stripping added another 200-300 ms to process time.

@hmistry Wow, that's good to know. I'm not getting any performance difference locally, but it may be visible with images that have more data.

It does not matter for JPEG files according to this answer

@renchap That's good to know, thanks!

janko commented 6 years ago

If anyone would like to take a stab at documenting these tips, or think that some should be applied by default, please feel free to edit the wiki and/or send a PR.

jrochkind commented 6 years ago

So I believe --strip doesn't actually remove color profile information, so is unrelated to converting to sRGB colorspace. If you read the blog post about vips, I believe this is what it says. --delete removes color profile information, so should only be done in some cases.

Yes, this stuff is confusing. That it is confusing to figure out is one reason I still suggest image_processing should provide operations that do it, not just docs on how minimagick/image_magick/vips do it.

I'm not sure why the discussion has become about what should be default. The issue as I opened it does not suggest anything about defaults. Nowhere I have suggested any of this stuff should be default. I have no real opinion on what should be default. It may indeed be reasonable for none of it to be default.

What I am still suggesting is that these operations should be part of the _imageprocessing api, that image_processing should provide these operations. Because they are often best practices, recommended by many sources.

Of course you can pass the specific back-end-specific options to the specific back-end. But if we were just going to do that, why would we need image_processing at all? We could just use MiniMagick or ruby-vips or whatever. The reason this gem is valuable is because it encapsulates common operations so we don't have to go figure out how to do them. (And, as of recently, in a backend-independent way, which is awesome). The same reason we want a resize_to_limit method in image_processing is the reason we want operations in image_processing that allow you to easily do the things that many sources recommend as best practices when making JPG thumbnails for the web: progressive JPGs, stripping EXIF, possibly sRGB colorspace, possibly stripping color profile info.

Of course, this is just my opinion, you can disagree, but please let's not talk about what should be default anymore. And I hope you at least understand my suggestion now, I'm sorry I've been having trouble making it clear.

janko commented 6 years ago

I'm not sure why the discussion has become about what should be default.

Yeah, sorry, that was something that I unnecessarily added to the discussion. Since all of us are ok with this being opt-in, let's talk about the API.

Ok, these are some of my comments on what we've mentioned so far:

Progressive JPGs
- libvips has :interlace saver option, ImageMagick has -interlace option
Stripping EXIF
- I've been searching how to do it with ImageMagick but couldn't find it, -strip strips away more than just EXIF data
- with libvips you can #remove EXIF metadata before saving (:strip saver option might also be an option?)
Converting to sRGB colorspace
- I haven't researched this yet; in the vipsthumbnail blog post it shows vipsthumbnail shark.jpg --eprofile /usr/share/color/icc/sRGB.icc --delete, but --delete is not exposed in ruby-vips, and I don't know where /usr/share/color/icc/sRGB.icc came from
Stripping color profile info
- vipsthumbnail has --delete, but it's not available from the Ruby API; I would prefer something that's not tied to generating thumbnails
- would .remove("icc-profile-data") be enough for libvips?

We could add #progressive and #strip_exif methods to the API, and maybe more. We could definitely take a look at Sharp to see what solutions it has.

jrochkind commented 6 years ago

You gotta supply your own sRGB.icc color profile in vips for whatever reason. There are several open source ones under licenses that would allow you to package them with whatever you want, not sure why libvips chooses not to package one, maybe it doesn't want to decide for you which one to use. (I think under normal/default/non-expert use, any of them are just fine).

https://github.com/jcupitt/libvips/issues/776

This stuff is totally crazy confusing to figure out! Which is why figuring it out in image_processing for everyone else would be so useful heh.

renchap commented 6 years ago

I think this is a good idea to provide built-in "profiles" (the first one being web thumbnail) in this gem, as image processing (the field, not the gem 🙂 ) is complex and not easy to grasp.

Something to keep in mind is that the options will depend on the output image format. You dont want a progressive PNG for perf reasons, quality does not have the same meaning, …

janko commented 6 years ago

You gotta supply your own sRGB.icc color profile in vips for whatever reason. There are several open source ones under licenses that would allow you to package them with whatever you want, not sure why libvips chooses not to package one, maybe it doesn't want to decide for you which one to use. (I think under normal/default/non-expert use, any of them are just fine).

It seems that we should probably use this profile for sRGB conversion.

This stuff is totally crazy confusing to figure out! Which is why figuring it out in image_processing for everyone else would be so useful heh.

You can say that again. I've never even heard of "image profiles" until I started reading other libvips wrappers.

Something to keep in mind is that the options will depend on the output image format. You dont want a progressive PNG for perf reasons, quality does not have the same meaning, …

Interesting! Yeah, we should definitely try to do the right thing.

jrochkind commented 6 years ago

yep, that's the profile I ended up using myself too, since it it is after all direct from the International Color Consortium, and is licensed for unrestricted use and redistribution. It's not the one jcupitt used/recommended for whatever reasons. I think really just about any will do (if someone is enough of a digital image wizard to know enough to have a preference, they can do it manually :) ).

jrochkind commented 6 years ago

I'm thinking about this again -- in your original directions for ["It's not there by default, but you can pass options to the underlying processor"] (https://github.com/janko-m/image_processing/issues/34#issuecomment-380170221) -- would there be any way to use those commands with ActiveStorage?

If so, an example would be awesome.

If not, that seems a lot of motivation for figuring out a way to bake it in as explicit built-in API to image_processing, so it can be used with ActiveStorage.

(If there's no way to use ActiveStorage to get standard web best practices like interlaced JPG, dropped profiles, standardized color profiles... that would seem to be a rather bad mark against ActiveStorage, which in a sense is ActiveStorage's problem not yours.... but AS is likely to kick it back as "sure, you can do that, as soon as image_processing supports it" heh).

janko commented 6 years ago

To my knowledge, the current ImageProcessing API should already support specifying options necessary for generating images according to web best practices. ImageProcessing can do anything that MiniMagick/ruby-vips can do, and if there is something they can't do, that should be fixed in those gems.

ActiveStorage master branch exposes all kinds of ImageProcessing settings, including loader and saver options, so it should be possible to do all those things with ActiveStorage as well, e.g:

photo.image.variant(saver: { interlace: true }, ...)

I don't think there is anything that needs to be done on the ImageProcessing side, in other words: "innocent until proven guilty" 😉

I also don't have the bandwidth and the knowledge to determine what are the important best practices for web images, and then how to exactly translate them into MiniMagick/ruby-vips settings. So I would be grateful to anyone who figures all of that out 😃

jcupitt commented 5 years ago

Hello, I just stumbled across this interesting issue.

That libvips blogpost about thumbnailing has been updated and worked up into a chapter in the main docs:

https://libvips.github.io/libvips/API/current/Using-vipsthumbnail.md.html

tldr: it recommends:

$ vipsthumbnail fred.jpg \
    --size 128 \
    -o tn_%s.jpg[optimize_coding,strip] \
    --eprofile /usr/share/color/icc/sRGB.icc \
    --rotate

Or in Ruby:

thumb = Vips::Image.thumbnail "filename-of-image", 128, eprofile: "filename-of-srgb-profile", :rotate true
thumb.jpegsave "filename-of-thumbnail", :optimize_coding: true, strip: true

You can use any sRGB profile, that's just an example. They do vary more than you'd expect, so test the one you pick.

libvips 8.8 (due RSN) will bundle this one (free, high-quality):

https://github.com/libvips/libvips/blob/master/libvips/colour/profiles/sRGB.icm

To use the bundled profiles, just set the filename to the profile name, so:

thumb = Vips::Image.thumbnail "filename-of-image", 128, eprofile: "srgb", :rotate true

You could add progressive: true to the jpegsave, though I don't like it much myself.

Please let me know if there's anything I can do to help!

jcupitt commented 3 years ago

Hello again, a quick update:.

First (trivially), rotate is now the default, so there's no need to give that as an arg to thumbnail (the option is still there, it just does nothing now).

Secondly, P3 colourspace is becoming very widely used. This is significantly bigger than sRGB, and P3 images converted to sRGB will either be drastically clipped or desaturated. Either way, your users are likely to complain, especially the photographers.

Rather than forcing everything to srgb, I think best practice now is to strip all metadata except the icc profile.

There's a new mutate feature in ruby-vips 2.1+ which lets you modify image metadata safely. I would write:

image = image.mutate do |mutable_image|
    mutable_image.get_fields.each { |field| mutable_image.remove! field unless field == "icc-profile-data"}
end

To remove all metadata except the icc profile.

A few users like to keep EXIF on thumbnails, so metadata stripping should probably be optional.

jrochkind commented 3 years ago

@jcupitt so useful to have your advice! Earlier you linked to https://jcupitt.github.io/libvips/API/current/Using-vipsthumbnail.md.html, which it looks like is no longer there. Is there an updated docs page with your new advice about (not) sRGB?

jcupitt commented 3 years ago

Hi @jrochkind, it's here now: https://www.libvips.org/API/current/Using-vipsthumbnail.md.html

Though I've not got around to updating the colourspace advice, unfortunately.

jrochkind commented 3 years ago

OK thanks @jcupitt . Not relevant to this PR, but I'm trying to figure out if there's a way to "strip all metadata except the icc profile" using vips CLI. I think there may not be? If this is the current recommended best practice, perhaps there should be a way to do it with CLI too? But I'll stop talking about it here, not relevant to image_processing which does not use CLI!

dezman commented 2 years ago

interlace: true results in an error for me, but this works, based on the imagemagick docs.

Unclear if I should use { interlace: :plane } or { interlace: :JPEG }.

has_one_attached :image, do |attachable|
  attachable.variant :progressive_jpeg, format: :jpg, saver: { interlace: :plane }
end

janko commented 3 months ago

I'm going to convert this into a discussion. I'd welcome any guides for progressive JPEGs, the wiki is publicly editable 😉

janko / image_processing

API for progressive JPG and other recommended web thumb best practices #34