eltiare / carrierwave-vips

CarrierWave image processing powered by VIPS
MIT License
92 stars 39 forks source link

[IMP] Updated ruby-vips dependency to the 1.0 API #25

Closed rescribet closed 7 years ago

rescribet commented 7 years ago

I recently reinstalled vips on osx, which bumped the version to 8, and since version upgrades in homebrew are a hassle, I decided to fix up this project to support the latest ruby-vips dependency which has support for vips 8+, but which comes at the expense of backwards incompatible changes.

It has a lot of changes which make the API a lot simpler, reducing the amount of code needed to implement the same functionality

The specs are all green on my system

eltiare commented 7 years ago

Awesome! I'll review this by Monday.

eltiare commented 7 years ago

I'm reviewing this now. I didn't say which Monday. ;)

eltiare commented 7 years ago

Gah, the testing stuff is soooo out of date. Going to change over to ActiveRecord since DataMapper seems to have been abandoned and upgrade to RSpec 3. The "good news" is that my health is pretty good and client demand has died down quite a bit for the moment so I'll be able to better verify that this all works properly.

eltiare commented 7 years ago

The stripping tests appear to be improperly written. Seems that the exif method no longer exists which is what was throwing the error. Investigating what needs to be done instead.

eltiare commented 7 years ago

@jcupitt Has the method of accessing EXIF data changed? It's late and I may be missing something obvious. I used to be able to use this code to look at EXIF data: Vips::Image.new(instance.current_path).exif

rescribet commented 7 years ago

Yeah, I noticed rspec 2 as well, but since C-V works fine in my project, I decided to keep this PR small and to the point.

For what I could tell, there is no way to gather all the metadata anymore, but instead one must use get_value with prior knowledge about the key names, when a non-existent key is requested, the library raises (couldn't find a method which lists the keys, so I hand checked with the vips-headers executable). Stripping is now done by simply passing a param to write_to_file.

I seem to have overlooked the second exif check, which should be equal to the first a few lines above.

jcupitt commented 7 years ago

Yes, it's .get_value now with a field name, for example:

irb(main):008:0> x = Vips::Image.new_from_file "img_0254.jpg"
=> #<Vips::Image:0x2843760 ptr=0x26f31b0>
irb(main):002:0> x.get_value "exif-data"
=> "Exif\x00\x00II*\x00\b\x00\x00\x00\t\x00\x0F\ .....
irb(main):003:0> x.get_value "icc-profile-data"
=> "\x00\x00\fHLino\x02\x10\x00\x00mntrRGB XYZ \a\xCE\x00\....

You're right, there should be something like a #get_field_names method to return a list of names. I added an issue to ruby-vips for this.

eltiare commented 7 years ago

Thanks guys. I should have this up and going today, barring any emergencies.

eltiare commented 7 years ago

I've merged this into master. After some manual testing today to make sure it works I'll push a new gem.

eltiare commented 7 years ago

Looks like the change format and quality features aren't functional anymore. Yay, we get to update tests. @jcupitt is there a way to tell the new library to convert the image to a new format?

jcupitt commented 7 years ago

Yes, that's right, #cast.

irb(main):004:0> require 'vips'
=> true
irb(main):005:0> x = Vips::Image.new_from_file "k2.jpg"
=> #<Vips::Image:0x1a8b4d8 ptr=0x1a46020>
irb(main):006:0> x = x.cast :float
=> #<Vips::Image:0x1b024c0 ptr=0x1a461b0>
eltiare commented 7 years ago

I thought so, but then I deleted my comment when I realized that it wasn't from something like jpeg to png.

eltiare commented 7 years ago

The old version used the Writer class to save the image as a different image format. What's the corollary for the new one? We also need the quality option for JPEGs and I'm not seeing it in the YARD documentation.

jcupitt commented 7 years ago

Oh sorry, I thought you meant numeric format.

You can just save as a .jpg file and it'll use that format, so:

x.write_to_file "myfile.jpg"

You can put options after the filename, eg.:

x.write_to_file "myfile.jpg", :Q => 90

You can see all the options for jpeg save on the jpegsave operation:

http://www.vips.ecs.soton.ac.uk/supported/current/doc/html/libvips/VipsForeignSave.html#vips-jpegsave

You can also call jpegsave directly if you wish, but I'd use write_to_file I think.

x.jpegsave "myfile.jpg", :Q => 90
eltiare commented 7 years ago

Awesome. Thank you. I'm realizing just how little I know about the mechanics of image files with these discussions.

jcupitt commented 7 years ago

The image save operations have had a lot of work done on them by the people on the sharp project:

http://sharp.dimens.io/en/stable/

They are really quite hairy now.

They have some nice benchmarks on that site:

http://sharp.dimens.io/en/stable/performance/

I meant to ask, are you using the new resize operation? It's quite a bit faster and better-quality than the old one. You just do:

image = image.resize 0.5

For a x2 reduction. Internally it does a vectorized block shirnk, then a vectorized lanczos3 down to the final size.

eltiare commented 7 years ago

I'm not sure. It's been so long since I looked at this I feel like I'm learning it all over again.

eltiare commented 7 years ago

We are using the new operation. We have been since near the beginning, I think.

eltiare commented 7 years ago

Well, I've found out that the tests are horribly borked and I may have found a bug in ruby-vips. If the bug does exist then I'll file a report over on on jcupitt's repo. Stay tuned...

eltiare commented 7 years ago

@jcupitt Is there some sort of caching thing going on with Vips::Image.new_from_path? Seems that I had to use Vips::Image.new_from_buffer(File.open(instance.current_path, 'rb').read, '') to make my tests pass, which is weird.

jcupitt commented 7 years ago

Yes, it caches everything very aggressively, and it expects files to be immutable, so if you change what's in /some/path/thing.x, it'll give you the previous version. This behaviour is necessary because of how much vips caches. For example, if you try:

irb(main):014:0> x = Vips::Image.new_from_file "wtc.jpg"
=> #<Vips::Image:0x1a37040 ptr=0x1a46980>

It'll be almost instant, even for very large files, since it just reads the image header. Now try:

irb(main):015:0> x.avg
=> 117.85399474082952

And there will be a short pause while vips decompresses the image and calculates the average. If you try avg again:

irb(main):016:0> x.avg
=> 117.85399474082952

You'll find it's instant. vips caches the arguments to the most recent 1,000 operations and if you repeat them, it gives you the previous result. It has to assume files are immutable, otherwise for every operation cache lookup, it would have to trace back through the graph to all the source images and check their modification times, which would be far too slow.

The caching is very helpful for programs which work intensively on a small number of fixed files, but for something like carrierwave, where you are doing simple operations to a large number of files, it's not much use.

You can turn off the operation cache with Vips::cache_set_max 0, meaning "cache at most 0 operations". There are some other operation cache control functions:

http://www.vips.ecs.soton.ac.uk/supported/current/doc/html/libvips/VipsOperation.html#vips-cache-set-max

tldr: I'd put Vips::cache_set_max 0 somewhere in your startup code.

jcupitt commented 7 years ago

Oh dear, I remember seeing something about the rounding behaviour of resize, but I can't find it now.

Yes, resize in current stable does strict round down, so if you resize with a factor that generates an image 199.9999 pixels across, you'll get a 199 pixel image, not 200. This can happen very easily with float maths. A simple fix is to add a small offset to the target size, so:

original_size = 24857
target_size = 200
scale = (target_size + 0.1) / original_size

The 0.1 will be enough to stop any round-down crossing a pixel boundary.

After a lot of complaining, git master vips does round-to-nearest instead, so you don't need this trick. The 0.1 will still work fine even with this change, so I'd just do that.