Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect ratio when thumbnail_image called after autorot #226

Closed
malomalo opened this issue Apr 1, 2020 · 13 comments
Closed

Incorrect ratio when thumbnail_image called after autorot #226

malomalo opened this issue Apr 1, 2020 · 13 comments

Comments

@malomalo
Copy link

malomalo commented Apr 1, 2020

thumbnail_image uses the old dimensions/ratio when called after an autorot.

Here is an example image and test case, I'm not sure on the best path forward to fix this issue.

Anyone else run into this issue?

require 'ruby-vips'
require "minitest/autorun"

IMAGE_PATH = '....'

class VipsTest < Minitest::Test
  
  def test_resize_after_autorot_on_image_with_orientation_6
    vips = ::Vips::Image.new_from_file(IMAGE_PATH, select_valid_loader_options(IMAGE_PATH, {}))
    assert_equal([vips.width, vips.height], [3264, 2448])

    vips = vips.autorot
    assert_equal([vips.width, vips.height], [2448, 3264])

    vips = vips.thumbnail_image(128, height: 128, size: :both)
    assert_equal([vips.width, vips.height], [128, 171])
  end

  def select_valid_loader_options(source_path, options)
    loader = ::Vips.vips_foreign_find_load(source_path)
    loader ? select_valid_options(loader, options) : options
  end

  def select_valid_options(operation_name, options)
    operation = ::Vips::Operation.new(operation_name)
    introspect = ::Vips::Introspect.get(operation_name)
    operation_options = introspect.required_input.map{ |arg| arg[:arg_name] }.map(&:to_sym)

    options.select { |name, value| operation_options.include?(name) }
  end

end

Output:

  1) Failure:
VipsTest#test_resize_after_autorot_on_image_with_orientation_6 [Downloads/error.rb:16]:
Expected: [128, 96]
  Actual: [128, 171]
@malomalo
Copy link
Author

malomalo commented Apr 1, 2020

Sorry forgot to include versions:

ruby-vips (2.0.17)
vips: stable 8.9.1 (bottled)

@jcupitt
Copy link
Member

jcupitt commented Apr 2, 2020

Hi @malomalo, thanks for the report, I'll have a look.

@malomalo
Copy link
Author

Thanks @jcupitt.

While your looking at it is there a way to have ruby-vips rescan/reload the image to get the right dimensions? Currently I'm trying to find a way to dump a buffer and reload, but it seems I always need define a file type.

@jcupitt
Copy link
Member

jcupitt commented Apr 14, 2020

Ooops, this dropped off my radar, sorry, I'll try to check tomorrow.

jcupitt added a commit to libvips/libvips that referenced this issue Apr 16, 2020
We were writing the wrong image to the output.

See libvips/ruby-vips#226
jcupitt added a commit to libvips/libvips that referenced this issue Apr 18, 2020
we were writing the wrong image to the output, thanks malomalo

see libvips/ruby-vips#226
@jcupitt
Copy link
Member

jcupitt commented Apr 18, 2020

It seems to be working for me now -- there was a regression in autorot. This fix will be in 8.9.2 (head of 8.9 branch).

Thanks for reporting this dumbness!

@jcupitt
Copy link
Member

jcupitt commented Apr 18, 2020

On your code snippet, thumbnail_image is very inefficient, since it can't exploit shrink on load for formats like JPG, and it can't set the rendering resolution correctly for things like SVG and PDF. If you possibly can, it's much better to use thumbnail.

You are using autorot before thumbnail_image, which is also very slow (it'll decode and rotate the entire image). It's much better to rotate after thumbnail. There's an auto_rotate flag to thumbnail to do this for you.

    vips = vips.thumbnail_image(128, height: 128, size: :both)

These are the default settings for height and size, so you don't need to change them.

(I realize this is just test code and you probably know all this, but I thought I'd say just in case)

@jcupitt
Copy link
Member

jcupitt commented Apr 18, 2020

While your looking at it is there a way to have ruby-vips rescan/reload the image to get the right dimensions? Currently I'm trying to find a way to dump a buffer and reload, but it seems I always need define a file type.

I'm not sure I understand -- how might it not have the correct dimensions? Is this after the file has been modified?

@jcupitt
Copy link
Member

jcupitt commented Apr 18, 2020

Here's a benchmark for thumbnail to show the difference:

john@kiwi:~/pics$ time vips thumbnail wtc.jpg x.jpg 128

real	0m0.397s
user	0m0.348s
sys	0m0.040s
john@kiwi:~/pics$ time vips thumbnail_image wtc.jpg x.jpg 128

real	0m2.211s
user	0m1.954s
sys	0m0.454s

So thumbnail is more than 5x faster for this image.

@malomalo
Copy link
Author

Awesome! Thanks @jcupitt, it's resolved all my issues.

I'll take a look at thumbnail, might help me in some cases.

@jcupitt
Copy link
Member

jcupitt commented Apr 24, 2020

OK, glad it's working. 8.9.2 is out now.

@jcupitt jcupitt closed this as completed Apr 24, 2020
@malomalo
Copy link
Author

Sorry to bring this up again, I just deployed this into production and still seeing this on some images, thumbnail_image still believes the images dimensions are not rotated and bases the cropping off of the old dimensions

@jcupitt
Copy link
Member

jcupitt commented May 10, 2020

Hi @malomalo, sorry to hear it's not working.

Could you post a sample image and the code that fails, please.

@jcupitt jcupitt reopened this May 10, 2020
@malomalo
Copy link
Author

Sorry, found the issue after some more debugging, it was an issue with what I was passing that gave me the same error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants