Add support for vips in the image analyzer #42005

brenogazzola · 2021-04-16T21:34:42Z

Summary

Fixes: #41940
Fixes: #41941

Adds support for using vips as an image analyzer in active storage.

Other Information

This is pretty much my first non trivial PR, so advice on how to improve this is welcome. Just trying to get the ball running on this one, since I'd like to use this too.

activestorage/lib/active_storage/analyzer/image_analyzer.rb

activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb

ghiculescu · 2021-06-02T20:52:12Z

activestorage/test/models/variant_test.rb

@@ -192,7 +192,7 @@ def process_variants_with(processor)
      previous_processor, ActiveStorage.variant_processor = ActiveStorage.variant_processor, processor
      yield
    rescue LoadError
-      skip "Variant processor #{processor.inspect} is not installed"
+      ENV["CI"] ? raise : skip("Variant processor #{processor.inspect} is not installed")


I'm probably missing it - what's the reasoning behind this change?

When I originally opened this PR all my vips tests were failing in the CI, even though they passed on the dev box. I got the help of @matthewd and we discovered that the CI environment did not have VIPS installed, and therefore the tests for vips were not running. He asked me to ensure that all vips tests raised errors in the CI, but were allowed to be skipped in contributor's machines to make development easier.

If the tests are passing now, I assume that means vips has been installed in CI?

Yes. rails/buildkite-config@2c92737

Just re-approved the CI to run, let us know if theres any other blockers! 🙇

brenogazzola · 2021-06-03T04:26:32Z

Changes made. Tests passed. There is an error in "Guides" but it seems it is unrelated, and Github says the build passed? There's 2 workflows awaiting approval. What should I do in this case?

zzak · 2021-06-03T05:34:21Z

@brenogazzola In the case where workflows are awaiting approval, one of us has to approve to run the CI 😢 Running again but I think based on your last comment we should be good!

ghiculescu · 2021-06-03T14:54:01Z

activestorage/lib/active_storage/engine.rb

@@ -24,7 +26,7 @@ class Engine < Rails::Engine # :nodoc:

    config.active_storage = ActiveSupport::OrderedOptions.new
    config.active_storage.previewers = [ ActiveStorage::Previewer::PopplerPDFPreviewer, ActiveStorage::Previewer::MuPDFPreviewer, ActiveStorage::Previewer::VideoPreviewer ]
-    config.active_storage.analyzers = [ ActiveStorage::Analyzer::ImageAnalyzer, ActiveStorage::Analyzer::VideoAnalyzer ]
+    config.active_storage.analyzers = [ ActiveStorage::Analyzer::ImageAnalyzer::Vips, ActiveStorage::Analyzer::ImageAnalyzer::ImageMagick, ActiveStorage::Analyzer::VideoAnalyzer ]


This looks like it could a breaking change. What happens on this branch if you try to use ActiveStorage::Analyzer::ImageAnalyzer directly?

Yes, it is. I didn't think about that. I can return my previous code, but it was looking pretty ugly:

def metadata if ActiveStorage.variant_processor == :vips Analyzer::ImageAnalyzer::Vips.new(@blob).metadata else Analyzer::ImageAnalyzer::ImageMagick.new(@blob).metadata end end

It also does not fit the pattern that was set with the PDF previewers, where each lib is a different previewer in the array, instead of having a generic PDFPreviewer that then chooses the correct class based on the libs available:

config.active_storage.previewers = [ ActiveStorage::Previewer::PopplerPDFPreviewer, ActiveStorage::Previewer::MuPDFPreviewer, ActiveStorage::Previewer::VideoPreviewer ]

I'm not sure where to go from here: breaking change, different patterns, or changing the previewers to have a generic PDFPreviewer that then decides between mu and poppler.

I've also just noticed I am not respecting the naming pattern and should probably have named the analyzers VipsImageAnalyzer and ImageMagickImageAnalyzer so they match the way the previewers are named.

Or I could return the original ImageAnalyzer and just add the VipsVersion ahead of it:

config.active_storage.analyzers = [ ActiveStorage::Analyzer::VipsImageAnalyzer, ActiveStorage::Analyzer::ImageAnalyzer, ActiveStorage::Analyzer::VideoAnalyzer ]

This would not be a breaking change and it would more or less follow the way the previewers are set. I'd just need another class to place the common metadata method so it stayed DRY.

I think the approach here is good, and if users have been doing things like config.active_storage.analyzers << MyAnalyzer then it shouldn't cause problems. The risk I can think of is code like config.active_storage.analyzers.insert(config.active_storage.analyzers.index(ActiveStorage::Analyzer::ImageAnalyzer) + 1, MyAnalyzer) breaking. Which seems like a bad pattern anyway.

Can you please update the docs: https://github.com/rails/rails/blob/main/guides/source/configuring.md#configuring-active-storage

Done. I've also updated the comments in the code. Unfortunately, the CI failed on the same place as last time (the bug report templates I think)

you might need @zzak to approve it again

@brenogazzola @ghiculescu Approved! 🙏

zzak · 2021-06-09T07:43:46Z

@brenogazzola Can you rebase and squash please? 🙇

brenogazzola · 2021-06-09T11:47:20Z

@zzak Done. Though it will conflict again with my #42392 since both updated the CHANGELOG and the same line in CONTRIBUTING. 😅

zzak · 2021-06-10T03:42:14Z

@brenogazzola Now that #42392 was merged hopefully this will be easier to rebase, sorry I missed that until now 🙇

brenogazzola · 2021-06-10T12:35:37Z

@zzak It's fine. It's my own fault for opening multiple PRs for the same feature in the Rails at the same time. I'll keep rebasing as needed.

brenogazzola · 2021-06-10T12:35:57Z

Also, done.

pixeltrix

@brenogazzola if can you rebase this, then I'll merge 👍🏻

brenogazzola · 2021-06-20T20:05:03Z

@pixeltrix Rebased. 👍

pixeltrix · 2021-06-21T09:27:10Z

@brenogazzola thanks! 👍🏻

wilg · 2021-06-21T17:51:56Z

Thanks for fixing my issues @brenogazzola!

sedubois · 2021-12-15T17:33:55Z

It appears that with this change away from ImageMagick, the analyzer does not store the image's width and height in the metadata any more, it only saves analyzed: true. Is that intended? Was that information used for something to begin with?

I had a test like this which used to pass before I started to migrate to Rails 7.0.0.rc3:

  test "poster is attached" do
    content = contents(:blog_post_1)

    assert content.poster.attached?

    blob = content.poster.blob.tap(&:analyze)

    assert_equal "cover.jpg", blob.filename.to_s
    assert_equal "image/jpeg", blob.content_type
    assert_equal({ identified: true, width: 1600, height: 900, analyzed: true }, # <-- Test fails here
                 blob.metadata.symbolize_keys)
    assert_equal 211486, blob.byte_size
    assert_equal "gDjkvuzYz1oUlbJ9hjbd5w==", blob.checksum
    assert_equal "public", blob.service_name
  end

brenogazzola · 2021-12-15T19:24:42Z

Active Storage has tests specifically to ensure that width and height are being extracted for both ImageMagick and Vips. You can see them in the files changed by this PR.

While upgrading, did you notice that Vips has become the new default in Rails 7.0? It's possible that you don't have Vips installed and this is causing the analyzer to fail.

sedubois · 2021-12-16T12:17:01Z

I am using Vips already in my 6.1.4 app. I will try to dig deeper to see why those width/height are disappearing in my test.

brenogazzola · 2021-12-16T13:26:09Z

Is it happening to all images or only some of them? There are 3 rescue points in the analyzer that would cause it to skip setting the dimensions:

1 - ruby-vips is not installed
2 - The image is not valid
3 - There was a generic error in vips.

All of them generate log when hit. Try going to test.rb and changing log_level to info before running the test again to see which rescue it's hitting.

sedubois · 2021-12-21T12:56:54Z

Thanks for your suggestions. After upgrading my local vips binary (brew upgrade vips), the test passes, so I suppose that was the issue!

rails-bot bot added the activestorage label Apr 16, 2021

brenogazzola commented Apr 19, 2021

View reviewed changes

activestorage/lib/active_storage/analyzer/image_analyzer.rb Outdated Show resolved Hide resolved

ghiculescu reviewed Jun 2, 2021

View reviewed changes

activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb Outdated Show resolved Hide resolved

ghiculescu reviewed Jun 2, 2021

View reviewed changes

ghiculescu reviewed Jun 3, 2021

View reviewed changes

rails-bot bot added the docs label Jun 3, 2021

brenogazzola force-pushed the activestorage/vips-analyzer branch from 87df9c4 to b4b7c6c Compare June 9, 2021 11:44

brenogazzola force-pushed the activestorage/vips-analyzer branch from b4b7c6c to 6dec647 Compare June 10, 2021 12:34

zzak approved these changes Jun 10, 2021

View reviewed changes

zzak added the ready PRs ready to merge label Jun 10, 2021

brenogazzola force-pushed the activestorage/vips-analyzer branch 2 times, most recently from a1ff72c to 6251ec2 Compare June 19, 2021 12:59

zzak requested a review from pixeltrix June 20, 2021 12:25

pixeltrix approved these changes Jun 20, 2021

View reviewed changes

Add support to libvips in the image analyzer

82fdf45

brenogazzola force-pushed the activestorage/vips-analyzer branch from 6251ec2 to 82fdf45 Compare June 20, 2021 19:52

pixeltrix merged commit 1f16768 into rails:main Jun 21, 2021

brenogazzola deleted the activestorage/vips-analyzer branch June 21, 2021 11:27

brenogazzola mentioned this pull request Jul 7, 2021

Better document how to migrate active storage from image magick to libvips #42700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for vips in the image analyzer #42005

Add support for vips in the image analyzer #42005

brenogazzola commented Apr 16, 2021 •

edited

ghiculescu Jun 2, 2021

brenogazzola Jun 2, 2021

ghiculescu Jun 2, 2021

brenogazzola Jun 2, 2021

zzak Jun 3, 2021

brenogazzola commented Jun 3, 2021

zzak commented Jun 3, 2021

ghiculescu Jun 3, 2021 •

edited

brenogazzola Jun 3, 2021

brenogazzola Jun 3, 2021

brenogazzola Jun 3, 2021

ghiculescu Jun 3, 2021

brenogazzola Jun 3, 2021

ghiculescu Jun 3, 2021

zzak Jun 4, 2021

zzak commented Jun 9, 2021

brenogazzola commented Jun 9, 2021

zzak commented Jun 10, 2021

brenogazzola commented Jun 10, 2021

brenogazzola commented Jun 10, 2021

pixeltrix left a comment

brenogazzola commented Jun 20, 2021

pixeltrix commented Jun 21, 2021

wilg commented Jun 21, 2021

sedubois commented Dec 15, 2021 •

edited

brenogazzola commented Dec 15, 2021 •

edited

sedubois commented Dec 16, 2021

brenogazzola commented Dec 16, 2021

sedubois commented Dec 21, 2021

Add support for vips in the image analyzer #42005

Add support for vips in the image analyzer #42005

Conversation

brenogazzola commented Apr 16, 2021 • edited

Summary

Other Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brenogazzola commented Jun 3, 2021

zzak commented Jun 3, 2021

ghiculescu Jun 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzak commented Jun 9, 2021

brenogazzola commented Jun 9, 2021

zzak commented Jun 10, 2021

brenogazzola commented Jun 10, 2021

brenogazzola commented Jun 10, 2021

pixeltrix left a comment

Choose a reason for hiding this comment

brenogazzola commented Jun 20, 2021

pixeltrix commented Jun 21, 2021

wilg commented Jun 21, 2021

sedubois commented Dec 15, 2021 • edited

brenogazzola commented Dec 15, 2021 • edited

sedubois commented Dec 16, 2021

brenogazzola commented Dec 16, 2021

sedubois commented Dec 21, 2021

brenogazzola commented Apr 16, 2021 •

edited

ghiculescu Jun 3, 2021 •

edited

sedubois commented Dec 15, 2021 •

edited

brenogazzola commented Dec 15, 2021 •

edited