Skip to content

Commit

Permalink
fix!: remove support for dominant border colour (broken)
Browse files Browse the repository at this point in the history
When running specs on ubuntu 24.04, the "dominant" border rendered using completely different
and clearly wrong colour. It turns out that this feature is thoroughly broken.

Problems:
1. The generated histogram information is plain wrong, suggesting the colours which clearly aren't dominant.
   These are mostly blueish, with some brownish and two flashy greens. I don't know why that happens, possibly
   the image preprocessing somehow conflicts with the histogram option.
2. In the ubuntu 22.04's imagemagick version (used to find dominant colours), the pixel count is not padded, leading
   to incorrect sorting. That happened to have picked the brown-ish value for border colour in the spec example,
   which didn't look perfect but also wasn't so clearly wrong.
   However, ubuntu 24.04's imagemagick adds padding to pixel count, so the sorting is correct, but as per point 1.
   the entire histogram is wrong. This leads to selecting bright green colour for the border.

Solution:
The colorscore gem seems unmaintained, so one alternative is to reimplement dominant handling ourselves. What made the output
sane to me was substituting colorscore's command:

convert spec/fixtures/reference_images/plasma-no-op-output.jpg -resize 400x400 -format %c -dither None -quantize YIQ -colors 16 -depth 8 histogram:info:-

with the following one:

convert spec/fixtures/reference_images/plasma-no-op-output.jpg -resize 400x400 -format %c -dither None -quantize YIQ -colors 16 -depth 8 gif:- \
  | convert - -define histogram:unique-colors=true -format %c histogram:info:- \
  | sort

Ideally the natural sorting should be used to handle unpadded output.

That said, I don't expect anyone to miss that feature, so I'm just removing it, leaving the hints to fix it here :) .

References:
- original code from colorscore: https://github.com/quadule/colorscore/blob/32dad8ee5d7500f01208bc9262e8d4d4f3248642/lib/colorscore/histogram.rb#L6
  • Loading branch information
knarewski committed Nov 22, 2024
1 parent eabb10e commit a79d930
Show file tree
Hide file tree
Showing 6 changed files with 9 additions and 26 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## UNRELEASED
### Removed
- [BREAKING] dropped support for a broken 'dominant' border colour

## [0.99.4] 22.11.2024
### Added
- Better test coverage for straighten operation
Expand Down
2 changes: 1 addition & 1 deletion lib/morandi.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ module Morandi
# @option options [Array[Integer,Integer,Integer,Integer]] 'crop' Crop image (x, y, width, height)
# @option options [String] 'fx' Apply colour filters ('greyscale', 'sepia', 'bluetone')
# @option options [String] 'border-style' Set border style ('square', 'retro')
# @option options [String] 'background-style' Set border colour ('retro', 'black', 'white', 'dominant')
# @option options [String] 'background-style' Set border colour ('retro', 'black', 'white')
# @option options [Integer] 'quality' (97) Set JPG compression value (1 to 100)
# @option options [Integer] 'output.max' Downscales the image to fit within the square of given size before
# processing to limit the required resources
Expand Down
12 changes: 3 additions & 9 deletions lib/morandi/operation/image_border.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ module Morandi
module Operation
# Image Border operation
# Supports retro (rounded) and square borders
# Background colour (ie. border colour) can be white, black, dominant (ie. from image)
# Background colour (ie. border colour) can be white, black
# @!visibility private
class ImageBorder < ImageOperation
attr_accessor :style, :colour, :crop, :size, :print_size, :shrink, :border_size
Expand All @@ -26,7 +26,7 @@ def call(pixbuf)

@border_scale = [img_width, img_height].max.to_f / print_size.max.to_i

draw_background(cr, img_height, img_width, pixbuf)
draw_background(cr, img_height, img_width)

x = border_width
y = border_width
Expand Down Expand Up @@ -75,7 +75,7 @@ def draw_pixbuf(pixbuf, cr, img_height, img_width, pb_scale, x, y)
cr.paint(1.0)
end

def draw_background(cr, img_height, img_width, pixbuf)
def draw_background(cr, img_height, img_width)
cr.save do
cr.translate(-@crop[0], -@crop[1]) if @crop && ((@crop[0]).negative? || (@crop[1]).negative?)

Expand All @@ -86,12 +86,6 @@ def draw_background(cr, img_height, img_width, pixbuf)

cr.rectangle(0, 0, img_width, img_height)
case colour
when 'dominant'
pixbuf.scale_max(400).save(fn = "/tmp/hist-#{$PROCESS_ID}.#{Time.now.to_i}", 'jpeg')
histogram = Colorscore::Histogram.new(fn)
FileUtils.rm_f(fn)
col = histogram.scores.first[1]
cr.set_source_rgb col.red / 256.0, col.green / 256.0, col.blue / 256.0
when 'retro'
cr.set_source_rgb 1, 1, 0.8
when 'black'
Expand Down
Binary file not shown.
Binary file modified spec/fixtures/reference_images/plasma-bordered-retro-style.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 1 addition & 16 deletions spec/morandi_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -633,21 +633,6 @@
}
end

context 'dominant colour background' do
let(:background_style) { 'dominant' }

it 'should maintain the target size' do
process_image

expect(File).to exist(file_out)
expect(processed_image_type).to eq('jpeg')
expect(processed_image_width).to eq(original_image_width)
expect(processed_image_height).to eq(original_image_height)

expect(file_out).to match_reference_image('plasma-bordered-dominant')
end
end

context 'black colour background' do
let(:background_style) { 'black' }

Expand Down Expand Up @@ -698,7 +683,7 @@
let(:options) do
{
'border-style' => 'retro',
'background-style' => 'dominant',
'background-style' => 'retro',
'border-size-mm' => 5,
'output.width' => original_image_width,
'output.height' => original_image_height
Expand Down

0 comments on commit a79d930

Please sign in to comment.