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

ImageAnalyzer incorrectly assumes that it can handle all images #36065

Closed
ConfusedVorlon opened this issue Apr 23, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@ConfusedVorlon
Copy link
Contributor

commented Apr 23, 2019

Steps to reproduce

  1. Attach a .heic image to a model using active storage

Expected behavior

no crashes

Actual behavior

the analyze job crashes

E, [2019-04-23T15:03:07.688988 #27434] ERROR -- : [ActiveJob] [ActiveStorage::AnalyzeJob] [ac0cc544-ca92-435a-b489-a216eee92931] Error performing ActiveStorage::AnalyzeJob (Job ID: ac0cc544-ca92-435a-b489-a216eee92931) from Sidekiq(default) in 247.25ms: MiniMagick::Error (`identify -format %[orientation] /tmp/ActiveStorage20190423-27434-xfqooz.heic[0]` failed with error:
identify-im6.q16: no decode delegate for this image format `HEIC' @ error/constitute.c/ReadImage/504.
):


System configuration

Rails version:
5.2.2

Ruby version:
ruby-2.4.0

This happens because the ImageAnalyzer assumes that ImageMagic can handle all image types. Unfortunately it can't.

The blob correctly identifies a .heic file as an image based on the content_type (image/heic)

The ImageAnalyzer incorrectly assumes that it can handle all images and sends things off to ImageMagic

ImageMagic (as of 6.9.7-4) can't handle .heic files, so it throws an error which means that sidekiq re-enqueues the job a bunch of times.


For the specific case of .heic, at some point my Ubuntu repo will end up with a more up-to-date version of ImageMagik which can handle the file, however there is always going to be a new image file (or an old one) that it can't handle.

It would seem like a good idea to provide some kind of config option for the ImageAnalyzer allowing users to override specific cases (e.g. don't try to analyze image/xxx, and possibly do try to analyze picture/yyy)

itsWill added a commit to itsWill/rails that referenced this issue Apr 23, 2019

Don't fail ImageAnalyzer on unsupported types
Fix: rails#36065

The IamgeAnalyzer passes a image to ImageMagick without checking if the
image is supported by ImageMagick. This patch checks that image is
supported and if not logs an error and returns an empty hash instead of
raising an error. This is the same error handling we do when we
encounter a LoadError when mini_magick is not installed.
@itsWill

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

I saw that we handle the LoadError if the minimagick gem is not installed by logging and returning an empty config. I figured this approach made sense for other operations that are garanteed to fail like the image not being valid.

I put up a quick PR for this to see what the general feeling is on that approach.

As for a workaround you can rescue the MiniMagick::Error in the Sidekiq job or check with MiniMagick::Image.new(file).valid?, but this feels like internal implementation of the framework and should probably live there.

I'm not sure how valuable it is to make this behaviour configurable.

@ConfusedVorlon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

My solution for now is to create a custom HeicAnalyzer that accepts image/heic and returns {} in metadata

I'm not sure what the 'right' approach is - but it seems 'wrong' to have an image analyzer which is kicked off automatically, and repeatedly crashes for some kinds of images

Given that the Image Analyzer does explicitly depend on ImageMagick - and ImageMagick can't handle all image files, it seems like it should handle the failure case more gracefully.

You're probably right that configuration makes little sense - perhaps the Analyzer should just return different metadata {"identified":true,"analyzed":true,"supportedImageType":false} or something along those lines

edit: I belatedly spotted at your pull request; That looks like a much better approach.

itsWill added a commit to itsWill/rails that referenced this issue Apr 24, 2019

Don't fail ImageAnalyzer on unsupported types
Fix: rails#36065

The IamgeAnalyzer passes a image to ImageMagick without checking if the
image is supported by ImageMagick. This patch checks that image is
supported and if not logs an error and returns an empty hash instead of
raising an error. This is the same error handling we do when we
encounter a LoadError when mini_magick is not installed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.