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

Use cache! call to Carrierwave so that base image process functions run in addition to version process calls. #321

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

astapinski
Copy link

Hi there, I'm opening a PR for an issue we saw recently with an upgrade we made to carrierwave v3 and carrierwave_backgrounder. I opened an issue for it but it has been sitting for a little while (#320 ), so I'll open a PR for the changes. Most of the comments are going to be copied to here.

Before our upgrade:

carrierwave 1.3.2
carrierwave_backgrounder 0.4.3

After:

carrierwave 3.0.5
carrierwave_backgrounder 1.0.2

Amongst many other things that were an issue with carrierwave v3, we noticed that base image processing in a background worker was not working after updating. For example, using an uploader class similar to this:

class AvatarUploader < BaseImageUploader
  include ::CarrierWave::Backgrounder::Delay
  include CarrierWave::MiniMagick
  include ApplicationUploader::StripExif

*** Snip lots of other code ***

  # Process files as they are uploaded:
  process resize_to_limit: [2048, 1024]
  process :auto_rotate_base

  # Create different versions of your uploaded files:
  version :profile do
    process resize_to_limit: [200, 200]
    process :auto_rotate_profile
  end
  version :thumb, from_version: :profile do
    process resize_to_limit: [60, 45]
  end

  private

  def auto_rotate_base
    warn 'AUTO ROTATE BASE'
    manipulate! do |img|
      img.auto_orient
      img = yield(img) if block_given?
      img
    end
  end

  def auto_rotate_profile
    warn 'AUTO ROTATE PROFILE'
    manipulate! do |img|
      img.auto_orient
      img = yield(img) if block_given?
      img
    end
  end

Excuse the different process functions here for auto rotate. In our code they are the same but it helped with debug logging here. There is a StripExif one as well in the included module but it's basically just another process function. BaseImageUploader is just a class that inherits from ApplicationUploader::Base while specifying a content_type_allowlist. When Carrierwave is used by itself without backgrounder or prior to version 3/backgrounder 1.0.2, the base processing of the resize to 2048x1024 and auto_rotate ran fine. After upgrading, these base image processors no longer run.

I think this is a change in recent Carrierwave versions that recreate_versions! does not apply processing to the base image. We've been using a fork for us of carrierwave_backgrounder where we have switched lib/backgrounder/workers/process_asset_mixin.rb to use cache! instead which seems to be both processing the base image as well as versions.

It may be a bit clearer to see with some logging. I enable MiniMagick debug logging with an initializer that has MiniMagick.logger.level = Logger::DEBUG if Rails.env.development? in it. With the current carrierwave_backgrounder 1.0.2, the logs look like this:

web_1  | 15:35:04 sidekiq2.1 | 2024-03-19T19:35:04.902Z pid=103 tid=17wf class=QuieterProcessAssetWorker jid=a89e16f0cf649e2c805374f5 INFO: start
web_1  | 15:35:05 sidekiq2.1 | D, [2024-03-19T15:35:05.539152 #103] DEBUG -- : [0.03s] convert /var/www/tmp/1710876905-328246157739531-0001-7183/profile/e1b53454cde660655264d506564891a2.jpg -auto-orient -resize 200x200> /tmp/image_processing20240319-103-2sjo4a.jpg
web_1  | 15:35:05 sidekiq2.1 | AUTO ROTATE PROFILE
web_1  | 15:35:05 sidekiq2.1 | D, [2024-03-19T15:35:05.556633 #103] DEBUG -- : [0.02s] identify /tmp/mini_magick20240319-103-jwavae.jpg
web_1  | 15:35:05 sidekiq2.1 | D, [2024-03-19T15:35:05.573774 #103] DEBUG -- : [0.02s] mogrify -auto-orient /tmp/mini_magick20240319-103-jwavae.jpg
web_1  | 15:35:05 sidekiq2.1 | D, [2024-03-19T15:35:05.592770 #103] DEBUG -- : [0.02s] identify /var/www/tmp/1710876905-328246157739531-0001-7183/profile/e1b53454cde660655264d506564891a2.jpg
web_1  | 15:35:05 sidekiq2.1 | D, [2024-03-19T15:35:05.615829 #103] DEBUG -- : [0.02s] convert /var/www/tmp/1710876905-328246157739531-0001-7183/thumb/e1b53454cde660655264d506564891a2.jpg -auto-orient -resize 60x45> /tmp/image_processing20240319-103-pcmqq4.jpg
web_1  | 15:35:06 sidekiq2.1 | 2024-03-19T19:35:06.222Z pid=103 tid=17wf class=QuieterProcessAssetWorker jid=a89e16f0cf649e2c805374f5 elapsed=1.32 INFO: done

QuieterProcessAssetWorker by the way is a worker class of ours that inherits from CarrierWave::Workers::ProcessAsset and has some error catching, no real changes beyond that. So in this log, note that "AUTO ROTATE BASE" is not called/shown and the resize to 2048,1024 does not happen.

Here is a log with our fork/branch where the base resizing/auto rotate occurs as it should:

web_1  | 15:24:11 sidekiq2.1 | 2024-03-19T19:24:11.056Z pid=62 tid=17wy class=QuieterProcessAssetWorker jid=145b2c58768b527e08e3b3b4 INFO: start
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.539807 #62] DEBUG -- : [0.04s] identify /tmp/mini_magick20240319-62-upzijg.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.564045 #62] DEBUG -- : [0.02s] mogrify -auto-orient -strip /tmp/mini_magick20240319-62-upzijg.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.584408 #62] DEBUG -- : [0.02s] identify /var/www/tmp/1710876252-128886268082131-0001-3691/d7dfab0c2bf1774e50f2510c08ff0f73.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.636944 #62] DEBUG -- : [0.02s] convert /var/www/tmp/1710876252-128886268082131-0001-3691/d7dfab0c2bf1774e50f2510c08ff0f73.jpg -auto-orient -resize 2048x1024> /tmp/image_processing20240319-62-1qslaq.jpg
web_1  | 15:24:12 sidekiq2.1 | AUTO ROTATE BASE
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.655672 #62] DEBUG -- : [0.02s] identify /tmp/mini_magick20240319-62-su2yco.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.677259 #62] DEBUG -- : [0.02s] mogrify -auto-orient /tmp/mini_magick20240319-62-su2yco.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.697086 #62] DEBUG -- : [0.02s] identify /var/www/tmp/1710876252-128886268082131-0001-3691/d7dfab0c2bf1774e50f2510c08ff0f73.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.723686 #62] DEBUG -- : [0.02s] convert /var/www/tmp/1710876252-128886268082131-0001-3691/profile/d7dfab0c2bf1774e50f2510c08ff0f73.jpg -auto-orient -resize 200x200> /tmp/image_processing20240319-62-i1bh7h.jpg
web_1  | 15:24:12 sidekiq2.1 | AUTO ROTATE PROFILE
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.743014 #62] DEBUG -- : [0.02s] identify /tmp/mini_magick20240319-62-ch9p24.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.761581 #62] DEBUG -- : [0.02s] mogrify -auto-orient /tmp/mini_magick20240319-62-ch9p24.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.777593 #62] DEBUG -- : [0.02s] identify /var/www/tmp/1710876252-128886268082131-0001-3691/profile/d7dfab0c2bf1774e50f2510c08ff0f73.jpg
web_1  | 15:24:12 sidekiq2.1 | D, [2024-03-19T15:24:12.796091 #62] DEBUG -- : [0.02s] convert /var/www/tmp/1710876252-128886268082131-0001-3691/thumb/d7dfab0c2bf1774e50f2510c08ff0f73.jpg -auto-orient -resize 60x45> /tmp/image_processing20240319-62-yhhljh.jpg
web_1  | 15:24:13 sidekiq2.1 | 2024-03-19T19:24:13.697Z pid=62 tid=17wy class=QuieterProcessAssetWorker jid=145b2c58768b527e08e3b3b4 elapsed=2.641 INFO: done

Note that "AUTO ROTATE BASE" is in the log and the base image resizing is done as expected.

I should say that we primarily use remote_X_url to start the process here. We don't use direct assignment via assigning a file to the mounted uploader, but I just tested it and it is the same situation/issue/fix as above. When I was debugging carrierwave 3.0.5, the call stack that finally arrived at the image processor functions came through download! and then cache! in carrierwave, so even though it looks a bit odd to me to call cache! to process everything, it was the only way I saw to have both the base image processing done as well as the versions.

@astapinski
Copy link
Author

Bump for this please.

zzak added a commit to 88labs/carrierwave_backgrounder that referenced this pull request Jun 26, 2024
@zzak
Copy link

zzak commented Jun 27, 2024

I've tested this branch against my app and it works, but I needed to make this small change:

commit cb31d3757f07743713154f5afd748b69d412129b (HEAD -> upstream-patched-321, origin/upstream-patched-321)
Author: zzak <zzakscott@gmail.com>
Date:   Thu Jun 27 13:50:57 2024 +0900

    Guard against undefined method

diff --git a/lib/backgrounder/workers/process_asset_mixin.rb b/lib/backgrounder/workers/process_asset_mixin.rb
index 91d3132..e41f6f5 100644
--- a/lib/backgrounder/workers/process_asset_mixin.rb
+++ b/lib/backgrounder/workers/process_asset_mixin.rb
@@ -9,11 +9,11 @@ module CarrierWave

       def perform(*args)
         record = super(*args)
-        record.send(:"process_#{column}_upload=", true)
+        return unless record.respond_to?(:"#{column}")
         asset = record.send(:"#{column}")
-
         return unless record && asset_present?(asset)

+        record.send(:"process_#{column}_upload=", true)
         process_asset_by_cache!(asset)

         return unless record.respond_to?(:"#{column}_processing")

Does that make sense?

Sorry I don't have much context here, just mashing keys together to get our tests green atm. Thanks!

@astapinski
Copy link
Author

astapinski commented Jun 27, 2024

I've tested this branch against my app and it works, but I needed to make this small change:

So I'm not the maintainer and I was just trying to get this PR up for the issue re: version processing. From what I remember of this change (it was a while ago) and looking at the diff, the change didn't touch the logic around process_COLUMN_upload. I'd say you probably want to try your tests with the original repo/branch to make sure that they are working there. I think process_COLUMN_upload is setup by using the process_in_background on a model so you may want to check that the model actually has process_in_background setup for that column.

If I'm not getting the issue just let me know. I'm not an expert in this gem, we just saw the original issue with base version calls not running and were trying to get it fixed.

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