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

Added support for the "crop" method. #2721

Merged
merged 11 commits into from
Feb 12, 2024

Conversation

clickworkorange
Copy link
Contributor

I had a need for this functionality, and while I know I could use the vips! method in my uploader it felt like an omission - so I put together a PR with a (passing) spec. The test is a little basic, but I wasn't sure if it would be ok to add new image fixtures, which I would need to do in order to test that the cropping produces the correct output (my test only checks the resulting dimensions). I'd be happy to look at implementing further vips methods if desired!

@clickworkorange
Copy link
Contributor Author

clickworkorange commented Jan 25, 2024

Two tests are failing, but I'm pretty sure neither has anything to do with me!

1. Test / RSpec and Cucumber (ruby-head, gemfiles/rails-7-1.gemfile, false) (pull_request)

LoadError:
     cannot load such file -- RMagick (You may need to install the rmagick gem)
     ...
     # ./vendor/bundle/ruby/3.4.0+0/gems/webmock-3.19.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # LoadError:
     #   cannot load such file -- observer
     #   ./vendor/bundle/ruby/3.4.0+0/gems/rmagick-5.3.0/lib/rmagick_internal.rb:24:in `<top (required)>'

2. Test / RSpec and Cucumber (jruby-head, gemfiles/rails-7-0.gemfile, true) (pull_request)

CarrierWave::ActiveRecord#mount_uploader #image should return valid XML when to_xml is called when image is nil
     Failure/Error: hash = Hash.from_xml(@event.to_xml)["event"]

     REXML::ParseException:
       #<RuntimeError: Illegal character "\u0000" in raw string "true\"/>\u0000\u0000\u0000">
       /home/runner/work/carrierwave/carrierwave/vendor/bundle/jruby/3.1.0/gems/rexml-3.2.6/lib/rexml/text.rb:140:in `block in check'
       org/jruby/RubyArray.java:1989:in `each'

mshibuya added a commit that referenced this pull request Jan 29, 2024
@clickworkorange
Copy link
Contributor Author

Only one test failing now, on JRuby-Head. Been looking at it for a while and it seems to have to do with the serialisation of ActiveRecord object; it's failing on @event.to_xml every time, with @event being an ActiveRecord object:

# activerecord_spec.rb

describe CarrierWave::ActiveRecord do
  def create_table(name)
    ActiveRecord::Base.connection.create_table(name, force: true) do |t|
      t.column :image, :string
      t.column :images, :json
      t.column :textfile, :string
      t.column :textfiles, :json
      t.column :foo, :string
    end
  end
 
 ...

  def reset_class(class_name)
    Object.send(:remove_const, class_name) rescue nil
    Object.const_set(class_name, Class.new(ActiveRecord::Base))
  end

  before(:all) { create_table("events") }
  after(:all) { drop_table("events") }

  before do
    @uploader = Class.new(CarrierWave::Uploader::Base)
    reset_class("Event")
    @event = Event.new
  end

  ...

      it "should return valid XML when to_xml is called when image is nil" do
        #### THIS TEST FAILS ON @event.to_xml ####
        hash = Hash.from_xml(@event.to_xml)["event"]

        expect(@event[:image]).to be_nil
        expect(hash.keys).to include("image")
        expect(hash["image"].keys).to include("url")
        expect(hash["image"]["url"]).to be_nil
      end

      it "should return valid XML when to_xml is called when image is present" do
        #### THIS TEST ALSO FAILS ON @event.to_xml ####
        @event[:image] = 'test.jpeg'
        @event.save!
        @event.reload

        expect(Hash.from_xml(@event.to_xml)["event"]["image"]).to eq({"url" => "/uploads/test.jpeg"})
      end
      
      ...

It might be that a change in rexml and/or activemodel-serializers-xml which pulls it in is where the "illegal" null characters are coming from (\u0000) - but whatever that change might be it only appears to affect JRuby.

@clickworkorange
Copy link
Contributor Author

clickworkorange commented Feb 2, 2024

Some obeservations:

  • activemodel-serializers-xml hasn't seen any changes for the last four years, so that's not it.
  • Rexml had a new release on July 23 - but since it's rexml complaining about those null characters my feeling is that the problem lies elsewhere, namely:
  • JRuby had a major release in November last year, which added Ruby 3.1 compatibility. I'm really not a Java guy >shudder<, but this looks like it may be relevant: jruby/jruby@830c0dc Specifically, they changed from java.util.HashMap to java.util.LinkedHashMap in the VariableAccessor getVariableAccessorWithBuilder.

clickworkorange referenced this pull request in jruby/jruby Feb 2, 2024
I could not find anywhere this is specified, except for:

* A very recent test in CRuby's test/ruby/test_shape.rb that
  confirms a degraded "complex" shape using CRuby's "st table"
  hashtable will continue to be returned in insertion order. This
  implies that insertion order is expected.
* A rubyspec added in 2021 in instance_variable_spec.rb that
  confirms instance variables are returned in insertion order. No
  links to issues or discussions is provided.

Since this is a simple enough change, we'll go ahead with it, but
I don't know that it has ever been formally specified (and indeed
I looked through other tests of other types of variables and see
that there are commits to .sort them before inspection in tests.)

Fixes #7988
@clickworkorange
Copy link
Contributor Author

I've added a question to the JRuby commit mentioned in my previous comment:

jruby/jruby@830c0dc

@mshibuya
Copy link
Member

mshibuya commented Feb 4, 2024

Thank you so much for taking a deep look on the JRuby failure!
It's not a blocker for this PR anyway, as it's unrelated.

But is it possible to add #crop to CarrierWave::MiniMagick and CarrierWave::RMagick as well? They should be considered as a sort of adapter, so they'd better to have the same interface unless doing so is infeasible.

@clickworkorange
Copy link
Contributor Author

I'll take a look!

Updated "crop" method of Vips processor to retain image bottom/right edge if cropping box falls outside the image boundaries.
Added tests for cropping within and beyond image boundaries for all three processors.
@clickworkorange
Copy link
Contributor Author

I have added support for image cropping to the RMagic and MiniMagic processors as well. In doing so I found a difference in behaviour between Vips and the other two: while both RMagic and MiniMagic silently accept cropping beyond the image boundaries (retaining the original image bottom/right edge), Vips would throw an Vips::Error: extract_area: bad extract area exception when asked to do so. I thought it made most sense to make Vips match the behaviour of the other two processors, and did so by recalculating the cropping area if either edge of it falls outside the image:

def crop(left, top, width, height, combine_options: {})
  width, height = resolve_dimensions(width, height)
  width = vips_image.width - left if width + left > vips_image.width
  height = vips_image.height - top if height + top > vips_image.height

  vips! do |builder|
    builder.crop(left, top, width, height)
      .apply(combine_options)
  end
end

I added specs to all three processors to test for this case.

@clickworkorange clickworkorange changed the title Added support for the "crop" method to the vips processor. Added support for the "crop" method. Feb 5, 2024
RMagic should `crop!` with a bang!
MiniMagic `crop` method should `.apply(combine_options)`.
Fixed copy/paste error in MiniMagic `crop` method description.
Removed redundant blank line in MiniMagic & RMagic `crop` method descriptions.
@clickworkorange
Copy link
Contributor Author

I'm working on updating the README to include information about Vips support and the crop method - would you prefer that as a separate PR, or should I include those changes here?

@mshibuya
Copy link
Member

mshibuya commented Feb 6, 2024

You can include it into this PR 👍

@clickworkorange
Copy link
Contributor Author

clickworkorange commented Feb 6, 2024

Ok, here's what I had in mind:


Manipulating images

[no changes]

Using MiniMagick

[no changes]

Using RMagick

[no changes]

Using Vips

CarrierWave version 2.2.0 added support for the libvips image processing library, through ImageProcessing::Vips. Its functionality matches that of the RMagic and MiniMagic processors, but it uses less memory and offers faster processing. To use the Vips processing module you must first install libvips, for example:

$ sudo apt install libvips

You also need to tell your uploader to use Vips:

class ImageFileUploader < CarrierWave::Uploader::Base
  include CarrierWave::Vips
  
  ...

end

List of available processing methods:

Note

While the intention is to provide uniform interfaces to all three processing libraries the availability and implementation of processing methods can vary slightly between them.

  • convert - Changes the image encoding format to the given format (eg. jpg). This operation is treated specially to trigger the change of the file extension, so it matches with the format of the resulting file.
  • resize_to_limit - Resize the image to fit within the specified dimensions while retaining the original aspect ratio. Will only resize the image if it is larger than the specified dimensions. The resulting image may be shorter or narrower than specified in the smaller dimension but will not be larger than the specified values.
  • resize_to_fit - Resize the image to fit within the specified dimensions while retaining the original aspect ratio. The image may be shorter or narrower than specified in the smaller dimension but will not be larger than the specified values.
  • resize_to_fill - Resize the image to fit within the specified dimensions while retaining the aspect ratio of the original image. If necessary, crop the image in the larger dimension. Optionally, a "gravity" may be specified, for example "Center", or "NorthEast".
  • resize_and_pad - Resize the image to fit within the specified dimensions while retaining the original aspect ratio. If necessary, will pad the remaining area with the given color, which defaults to transparent (for gif and png, white for jpeg). Optionally, a "gravity" may be specified, as above.
  • crop - Crop the image to the contents of a box with the specified height and width, positioned a given number of pixels from the top and left. The original image edge will be retained should the bottom and/or right edge of the box fall outside the image bounds.

Supported processing methods

The following table shows which processing methods are supported by each processing library, and which parameters they accept:

Method RMagick MiniMagick Vips
convert format format, page1 format, page1
resize_to_limit width, height width, height width, height
resize_to_fit width, height width, height width, height
resize_to_fill width, height, gravity2 width, height, gravity2 width, height
resize_and_pad width, height, background, gravity2 width, height, background, gravity2 width, height, background, gravity2
resize_to_geometry_string geometry_string3 not implemented not implemented
crop left, top, width, height left, top, width, height left, top, width, height

1page refers to the page number when converting from PDF, frame number when converting from GIF, and layer number when converting from PSD.

2gravity refers to an image position given as one of Center, North, NorthWest, West, SouthWest, South, SouthEast, East, or NorthEast.

3geometry_string is an ImageMagick geometry string.

README.md Outdated Show resolved Hide resolved

<sup>2</sup>`gravity` refers to an image position given as one of `Center`, `North`, `NorthWest`, `West`, `SouthWest`, `South`, `SouthEast`, `East`, or `NorthEast`.

<sup>3</sup>`geometry_string` is an [ImageMagick geometry string](https://rmagick.github.io/imusage.html#geometry).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice comparison table 👍

Seppling...

Co-authored-by: Mitsuhiro Shibuya <[email protected]>
@mshibuya mshibuya merged commit ed87991 into carrierwaveuploader:master Feb 12, 2024
12 checks passed
@mshibuya
Copy link
Member

Great work, thank you so much!

@clickworkorange
Copy link
Contributor Author

My pleasure!

mshibuya added a commit that referenced this pull request Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants