envygeeks / jekyll-assets

:art: Asset pipelines for Jekyll.
ISC License
1.12k stars 170 forks source link

Optim and Magick processing order is not optimal or configurable. #566

Open brianhartsock opened 5 years ago

brianhartsock commented 5 years ago

Description

Optim and Magick processing order is not optimal or configurable. If you use both Magick and Optim on an image, results are sub-par with Optim running first then magick.

Steps

Try the following asset options on the same image:

Output

Respectively:

Expected

Obviously this shows that magick isn't optimizing the image after resize which is expected. In an ideal world, optim would always be run after magick IMO but there may be scenarios I haven't thought about.

The root of the problem appears that in proxy.rb, the Proxy.proxies_for method returns the classes in whatever order Proxy.inherited returns.

I was able to monkey patch with the following code to get a 204k file size:

module Jekyll
  module Assets
    module Plugins
      class ImageOptim
        def self.priority
          2
        end
      end
      class MiniMagick
        def self.priority
          1
        end
      end
    end
    class Proxy
      def self.priority
        @priority || 1
      end

      def self.proxies_for(asset:, args:)
        proxies = Proxy.inherited.select do |o|
          o.for?({
            type: asset.content_type,
            args: args,
          })
        end

        proxies.sort_by { |o| o.priority }
      end
    end
  end
end

I imagine there might be better ways to do this as I'm not a Jekyll plugin expert.

envygeeks commented 5 years ago

Have you tried.... just flipping the arguments? {% asset image.png magick:half @optim %}?

brianhartsock commented 5 years ago

Yup, didn't have any impact on the order.

envygeeks commented 5 years ago

I don't know that I want literal priorities, but a quick look at the source shows it doesn't necessarily respect the order of arguments even though I explicitly state arguments are positional and respected in the parser (but not necessarily in Jekyll-Assets.)

I'll have a quick thought about whether I'd rather move to priorities, or implement a pop, and push algorithm to sort based on the positional order of arguments, more than likely I'll land on the latter as I already feel like the way we handle plugins is a bit complicated and needs a bit of rework, and I tend to fall on the latter side of "unless it's necessary, convention first, configuration on request"