Make Vips the recommended/default variant processor for Active Storage

rubyonrails-core
1

Currently, the ActiveStorage guide opening section contains the following phrase: “Using Active Storage, an application can transform image uploads with ImageMagick”.

A developer looking to start using ActiveStorage will assume that this is Rails recommendation, even if they later see in the Transforming Images section that using vips is possible.

It is my experience, running Active Storage in production since version 5.2, that this is not a good enough default and Rails should steer users away from it, and incentivize them to use vips instead.

The original PR by janko gave the technical reasons why supporting vips was a good idea, so I’ll stick to explaining why vips is better from a user experience point of view.

Vips is faster, so it improves queue times

Let’s say you have a feature in your app that allows your user to upload multiple photos at the same time, and after the upload is done you direct them to their photo gallery. Let’s also say that your user just uploaded 5 photos.

When that user reaches the photo gallery, his browser will make 5 requests to the representations_controller, which will start processing the 5 variants, keeping 5 puma workers occupied. For smaller apps, that might be as many workers as they have available, which means that every other HTTP request must wait until the representations_controller is done processing the variant. This wait time is the “queue time”. With its faster processing vips can reduce the queue time and provide a faster user experience.

This is specially bad on Heroku. From my experience in the CGRP Slack group and from talking to other devs, I’ve noticed that many are using Heroku as a host and using Standard-1x or Standard-2x dynos with a puma concurrency of 1-2. Since Heroku’s load balancer assigns requests at random, this means that even if there are puma workers free, users nagivigation requests might get stuck in the queue of a server that is busy processing an image.

Back when we were in Heroku this got so bad that we could tell when a user had uploaded a large amount of photos at the same time by the fact that there was a spike in queue times and the auto scaler had kicked in.

Vips consumes less memory, so it improves response times

ImageMagick is very memory intensive when processing variants. Benchmarks by libvips author demonstrate it using 3GB memory versus 200MB by vips. With cameras in mobile phones getting better and better, the photos that must be processed are becoming larger. This inefficiency causes problems for developers running their apps in servers with tight memory constraints, like 512MB or 1GB (eg: Heroku). With Rails apps already being memory intensive, they should be always close to the memory ceiling.

Processing a single photo with ImageMagick might be enough to force the server to start swapping, and this kills ActiveRecord’s performance. I’ve seen Scout reporting that a simple find had increased from 1-2ms to 10-30ms sometimes as much as 50ms when our Heroku dyno reached its memory limit. With most pages requiring a few queries, we are talking about an additional few hundred ms of response time for at least as long as ImageMagick is processing.

Conclusion

Recommending Vips is a form of conceptual compression. It eliminates the need for developers to understand how Active Storage’s chosen implementation for variant generation, when combined with ImageMagick slow processing and high memory requirements, is impacting their user’s experience.

Vips creates a better user experience. Let’s make it the recommended option. If possible, let’s make it the default in Rails 7 since it’s a major version and allowed to introduce breaking changes.

The PR: https://github.com/rails/rails/pull/42744

8 Likes
2

I’ve prepared a gist with a step by step guide on how to make the migration, based on my experience doing that:

5 Likes
3

I agree about making vips the default image processor and I think there is another argument to be made: security. We are not running ActiveStorage due to the high security we need for our storage. Running transformations on uploaded files is often a security risk as the tools have lots of vulnerabilities. Most developers seem to run the transformation binaries on the same webserver as the rest of their application, making it even more insecure.

The list of known CVE’s in vips is significantly smaller than the list of known CVE’s for ImageMagick. I’m not familiar with the internals of both tools, so vips’ lower number of CVE’s can mean a better security architecture or less CVE’s found due to being a less popular tool. For me it seems to be a more secure option if you don’t take other security measures for transformations.

3 Likes
4

Thx for sharing with us!

2 Likes
5

I’m happy to say this one has been merged :heart_eyes:

2 Likes
6

Sounds good but my only issue is related to watermark (composition), which seems to be impossible with Vips and ActiveStorage.

7

It seems from the linked issue that this is not a fixable problem :thinking:

Fortunately, ImageMagick has not been deprecated, so if watermarking is required devs can still use it instead.

1 Like
8

This is really a huge issue. ImageMagick has had so many problems that it’s verging on malpractice to allow it to be used on the backend with untrusted files. I’d be wary of vips or any other solution implemented in a non memory safe language.

1 Like
9

I’ve tried switching from imagemagick to vips last week, here are some issues I’ve noticed, I’ve since switched back to imagemagick, but YMMV, we sure can figure things out gradually.

First thing I’ve noticed has been brought up already, the “composite” option with Active Storage, which is useful for adding a watermark or annotation to an image. I can get it to work in Active Storage variant with image.variant(composite: [overlay.to_s, :over]), but I can’t send more args to it, such as image.variant(composite: [overlay.to_s, :over, {gravity: "south-east"}]), it seems only 2 arguments are accepted, more args are ignored. And using a list , is kind of weird.

Second thing I’ve noticed is about Heroku. Heroku buildpack of Ruby supports imagemagick out of box, but not Vips. Libvips is required to be specified manually in a buildpack, including an Apt file to support different image formats. I haven’t figured this out yet but this sure can be worked out. I’m not Heroku expert, but I guess if I add Vips to my buildpack, then this buildpack should also be updated manually by myself, which used to be handled by Heroku? Say when we upgrade from Ubuntu18 to 20? Maintaining this buildpack for Vips is also kind of annoying for a one-man team, though this should be the Heroku team’s work not Rails team’s.

If I’m wrong or other options are available, please tell me, I’d love to know how other people are saving ram by switching to Vips.

10

I’ve never used composition, but one of the requirements for the switch to vips is to use the new macros defined by the image_processing gem, which move many of the transformations to a saver hash. You can see this on the upgrade guide, item 2.6.7 (formatting is a bit broken, sorry, there’s a PR open to fix). That might help solve your problem.

On Heroku you don’t need a buildpack. You can use the apt buildpack and add vips using the Apt file which installs ubuntu packages during build. Frankly, now that vips is the default I’m hoping they add it to their active-storage buildpack.

1 Like
11

Hello, I’m a libvips maintainer and I just stumbled upon this interesting discussion.

Watermarking

ruby-vips supports watermarking and you can drop down to ruby-vips within image_processing, so you can add a watermarked variant with a couple of lines of code. There’s a simple watermark example in the ruby-vips repo:

https://github.com/libvips/ruby-vips/blob/master/example/watermark.rb

Security

You’re right, this is a big issue.

libvips has been part of OSS-Fuzz for a year or two now, and (I think?) only one serious bug has been found in libvips itself, and that was right at the start. You can see the configuration that’s being fuzzed here:

https://github.com/google/oss-fuzz/blob/master/projects/libvips/build.sh

In other words, libvips itself, plus libexif, lcms2, aom, libheif, libjpeg-turbo, libpng, libspng, libwebp, libtiff, libjxl, libimagequant and libcgif. In file format terms, that’s AVIF, HEIC, JPG, PNG, WEBP, TIFF, JXL and GIF. libvips includes built-in loaders for PPM, CSV, Analyze and HDR. All these formats should be pretty safe.

However, libvips as usually distributed by Debian etc. includes many, many other loaders for more obscure (and much less well tested) formats like NIfTI and FITS, plus a complete copy of imagemagick as a fallback processor for unsupported formats, like BMP and ICO.

If people use the Debian binary directly, they are unlikely to see a significant improvement in security. I think I would include a note in the docs recommending the use of a stripped-down libvips which only included support for the popular and well-tested web formats.

Ideally I suppose Heroku would make a libvips-web package with just the fuzzed loaders and include it in their buildpack. The libvips team would be delighted to help in any way we can with setting this up.

12

Hey @jcupitt. It’s awesome to see you here, and thank you for always being so prompt at responding to issues/questions in vips/ruby-vips.

Heroku unfortunately includes imagemagick as part of the base image for all their stacks and when I asked about vips they said it was a frequent request, and they had considered it, but adding extra packages would make deploys slower. So we can deduce that at the time they had no intention to remove magick

I’m hoping that with libvips as the new Rails default we can get Heroku to reconsider. I’m thinking the first step (or a fallback option) would be a “buildpack” that removed imagemagick and depencies and installed vips in it’s place.

Since this process would have to run on every deploy we’d need to use precompiled packages. I’m not sure if Heroku allows adding repos during the build step (I think not, because their official chrome buildpack downloads it from an url) we’d probably need someplace with the static build or something similar.

And since heroku limits images to 500MB it would probably be better to have a pared down version with just the most commom loaders built in.

13

While ruby-vips can handle composition it would be more convinient if we could go through Active Storage since it would transparently handle downloading the original file and uploading the result.

Currently that process is failing because AS employs delayed/lazy generation, and therefore has to Marshal.dump all parameters that will be passed to vips. And according to Janko, Vips::Image is failing marshalling because it employs C Structs that ruby does not know how to Marshal.

I think there are some alternatives to solve this from AS side (maybe marshal the uri/path to the file) but how feasible do you think it is to solve from Vips side for comparison’s sake?

14

Hi Bruno,

I’m not sure I really understand the problem with composition and marshalling. It sounds like AS assumes that the series of image processing operations can be described as a set of strings, and I suppose this comes from mini_magick, where everything ultimately turns into a list of command-line options that IM interprets.

ruby-vips deals with images directly and moves the laziness into the library itself, so it’s true you can’t easily represent an image processing pipeline as a sequence of str options. Does that sounds right?

Why is marshalling into a set of strings important? Do these things need to be passed between machines? I can imagine the machine that does page generation passing an image spec to another machine for image generation. Is that what happens? I’m probably misunderstanding.

I suppose in that case we’d need to implement a tiny DSL that could execute libvips operations. It sounds a bit hacky, but you could always use ruby itself — AS could generate some ruby source code which would later be evaled by the image generator.

Alternatively, libvips supports introspection (the ruby-vips binding is mostly generated at run-time), so a tiny ruby-vips shell would be pretty simple. This method:

https://github.com/libvips/ruby-vips/blob/master/lib/vips/operation.rb#L277

Introspects the libvips binary and lets you call any C func in a typesafe way. You’d just need something to call that several times, driven by a string of text options.

15

Correct. Active Storage assumes that all transformations can be encoded as strings and that one machine will generate said string, and another machine will take said string and use to the process the image

Let’s say we have an app where users can upload their avatar. When he sends us the file, Active Storage (AS) will upload it to storage (S3), and we later can request an optimized version by doing this:

<%= image_tag @user.avatar.variant(
  resize_to_limit: [128,128], saver: { strip: true, quality: 85 }
) %>

It would be reasonable to assume that the first, time this variant of the original avatar file is requested, AS would download the original file, apply the transformations, upload it back storage and the url for the resized image. That is not actually happens however. This is what you get:

<img src="rails/active_storage/representations/redirect/[signed_id]/[variation_key]/girl.jpg" %>

This is the URL of a special AS controller that is responsible for serving variants of images it has stored. signed_id is the encrypted ID that AS uses to locate the original file, and variation_key is the result of this:

ActiveStorage.verifier.generate(
  { resize_to_limit: [128,128], saver: { strip: true, quality: 85 } }
  purpose: :variation
)

This verifier performs Marshal.dump on that hash and then signs it to prevent tampering.

When the user’s browser makes a request to this URL, the AS controller will decode the variation_key and pass it back to image_processing. So the entire problem here:

<%= image_tag @user.avatar.variant(
  composite: Vips::Image.jpegload('avatar_flair_1.jpg')
).processed.url %>

The processed method tells AS to process the image immediately in the current machine (since the other machine will not have access to the image used for composition) and it would be reasonable to assume this would solve the marshaling problem…

Unfortunately, because of the architecture I described above, AS expects to the able to Marshal.dump the transformations into a key that will be part of the upload location of the processed file. And that’s where it breaking. It cannot Marshal.dump the Vips::Image instance.

2 Likes
16

Here is the reason it was built like that:

class User < ApplicationRecord
  has_one_attached :avatar
end

I only have to tell Rails about the original image. Then I can do this:

user.avatar.variant(resize_to_limit: [32, 32])
user.avatar.variant(resize_to_limit: [256, 256])
user.avatar.variant(colourspace: "b-w")

And Active Storage will handle downloading, processing, uploading and tracking each of those variants without forcing me to add extra columns to my models to store the urls. But to do this, it needs to be able to take a set of transformations and turn it into a unique key that will always be the same for a given set of transformations:

# Returns a signed key for all the +transformations+ that this variation was instantiated with.
def key
  self.class.encode(transformations)
end

# Returns a signed key for the +transformations+, which can be used to refer to a specific
# variation in a URL or combined key (like <tt>ActiveStorage::Variant#key</tt>).
def self.encode(transformations)
  ActiveStorage.verifier.generate(transformations, purpose: :variation)
end

So when I ask about the difficulty of solving this on ruby-vios side I’m asking if it is possible for vips:image to be serializable in a way that would always give the same result for an image, and a different result for another”?

(I’m sorry, I don’t know the correct terminology for this, as I’m basically reading through the code and figuring out as I go)

17

Ah gotcha (I think). Thanks for the clear explanation.

So I would suggest creating a tiny ruby-vips DSL (just to have a name, let’s call it RVL, for ruby-vips language) to describe a pipeline (really a tree) of ruby-vips operations. It only needs expressions, there’s no need for variables or a stack, since libvips already does common sub-expression elimination during pipeline evaluation.

Perhaps with JSON for the text form it might look something like:

"jpegsave": {
    "filename": "/path/to/output.jpg",
    "in": {
        "rotate": {
            "angle": 127.6,
            "in": {
                "pngload": {
                    "filename": "/path/to/input.png"
                }
            }
        }
    }
}

The names of operations and the number and types of the arguments are all found via introspection of the libvips binary, just like ruby-vips.

image_processing would then need a new backend that generated RVL instead of calling ruby-vips directly, plus a thing to serialise into string, and a method to execute a stored RVL program. #method_missing in image_processing would simply add a node to the RVL being generated.

There would need to be a few predefined RVL things to implement image_processing methods that can’t be easily described in this form.

I think (I think!) this could all be done inside image_processing, and AS wouldn’t need to know about it. The user would just select RVL as the backend and would then be able to do watermarking. Does that sound plausible? I could implement RVL (it should be very simple, just a few 100 lines), perhaps someone else could do the image_processing backend.

Or I could be talking nonsense! Any thoughts?

18

I’ve pinged Janko on the original bug report to ask him if he could give us his input. I’ve never looked at image_processing source’s code, so I have no idea how feasible this would be.

19

I’m definitely open to any changes that would make compositing images easier. @brenogazzola could you open an issue on the ImageProcessing repo, summarizing the issue? I’m mainly curious about why the existing API, which allows passing paths instead of Vips::Image objects, isn’t sufficient.

20

@jankomarohnic :man_facepalming:I read the linked issue and assumed using Vips::Image was the only way. This makes things simpler, but there’s still a problem to solve: if the ‘watermark’ image is actually an active storage attachment. I think this is mostly an AS problem, but since you actually have knowledge of both you’d be able to point to the best solution to make this work.

photo.file.variant(composite: company.logo)

Both file and logo are has_one_attached. If I use .processed.url I have the problem that company.logo.open does not generate a stable path for AS to use as key and I’d need some extra coding to solve that. If I just use url and let representations_controller, it currently has no way of knowing it’s supposed to download the file and pass it to image_processing.

So my question is if it would be better to make active storage aware of how to handle both cases, or should it just hand a blob to image_processing and let it do the work of doing .open on the file?

(I’ll wait an issue in the image processing repo if you think this is something that would be better to solve on that side, instead of AS side)

@jcupitt Sorry, it seems we don’t actually need to Marshal.dump Vips::Image. Unless Janko thinks there’s something to the suggestion what he’d like to use?

@Mario_Chavez is using paths instead of Vips::Image acceptable for your use case?