feat: add vips straighten #39

knarewski · 2024-11-29T15:37:49Z

Background

GdkPixbuf-based image processing can take over 1GB of memory for processing a single high-resolution photo. We've decided to introduce another processor, hoping that it'll improve Morandi's memory profile and performance.

Problems

Vips processor doesn't support straighten operation

Solution

Add support for straighten operation

Notes

Visually all the images appear to be slightly shifted. Since I didn't spot any issues with maths, I'm inclined to assume that it's due to rounding/trimming during computations
Because of the arbitrary angle rotation's nature, the post-rotation image is bigger; the "empty" areas in the corners are filled with colour; because of that, the rotated image is cropped and them zoomed to preserve original dimensions; the background colour, however, has a tendency to slightly bleed on the edges; that tendency is more visible for vips processor; in order to reduce that effect, I've changed the background colour in vips to gray; see screenshots below for example.

Pixbuf vs Vips comparison


Chunk of the test image rotated by arbitrary angle, before cropping and zooming (vips); note the gray colour which blends with the image edges


Close-up (4000%) of background bleed on the white image (left - pixbuf, right - vips)


straighten operation (-20 degrees) (left - pixbuf, right - vips), mean average error: `0.01176`

Other images can be compared by opening files with identical names in spec/fixtures/reference_images/ and spec/fixtures/reference_images/vips/

MrLukeSmith

This exceeds my mathematical capabilities. I'll defer to @Surge9 for the yay / nay approval. He's more fond of maths, IIRC. 😅

But I ran the class through GPT and it raised a few points that might be of value.

MrLukeSmith · 2024-12-05T13:55:01Z

lib/morandi/operation/vips_straighten.rb

+        rotation_value_rad = angle * (Math::PI / 180)
+        post_rotation_bounding_box_width = (img.height.to_f * Math.sin(rotation_value_rad).abs) +
+                                           (img.width.to_f * Math.cos(rotation_value_rad).abs)
+        post_rotation_bounding_box_height = (img.width.to_f * Math.sin(rotation_value_rad).abs) +


"Ensure that these formulas are rigorously tested for edge cases like angle = 45, where the diagonal lengths dominate." - GPT

See #39 (comment)

MrLukeSmith · 2024-12-05T13:57:02Z

lib/morandi/operation/vips_straighten.rb

+      def self.rotation_background_fill_colour(channels_count:, alpha:)
+        return [ROTATION_BACKGROUND_FILL_COLOUR] * channels_count unless alpha # Eg [127, 127, 127] for RGB
+
+        # Eg [127, 127, 127, 255] for RGBA
+        ([ROTATION_BACKGROUND_FILL_COLOUR] * (channels_count - 1)) + [ROTATION_BACKGROUND_FILL_ALPHA]
+      end


"The method self.class.rotation_background_fill_colour is a good abstraction, but ensure it handles edge cases where the alpha channel might complicate visual results. Test scenarios where partially transparent images are involved." - GPT

That's already in master ;-)

morandi-rb/spec/morandi_spec.rb

Lines 454 to 465 in 4328af3

context 'with straighten option' do

# Tested explicitly, because morandi happens to handle transparency differently when using straighten

let(:options) { super().merge('straighten' => 2) }

it 'applies transformations' do

process_image

expect(File).to exist(file_out)

expect(processed_image_type).to eq('jpeg')

expect(file_out).to match_reference_image('match-multiple-operations-and-straighten')

end

end

MrLukeSmith · 2024-12-05T13:58:48Z

lib/morandi/operation/vips_straighten.rb

+        crop_x = (width_diff / 2).round
+        crop_y = (height_diff / 2).round
+
+        img.crop(crop_x, crop_y, original_width, original_height)


"Cropping after rotation ensures no background fill color is visible, which is important. However, rounding (width_diff / 2) and (height_diff / 2) might cause slight misalignments if the bounding box size isn’t divisible by 2. If this is critical, consider using:

crop_x = (width_diff / 2.0).floor
crop_y = (height_diff / 2.0).floor
" - GPT

Rounding using "normal" mathematical rules yielded the most reliable results to me (the gray bleed was most subtle and even on both sides)

MrLukeSmith · 2024-12-05T14:01:57Z

spec/morandi_spec.rb

@@ -239,7 +239,7 @@
      end
    end

-    describe 'when given a straighten option', vips_wip: processor_name == 'vips' do
+    describe 'when given a straighten option' do


"Consider testing angles that are multiples of 45" - GPT

I have tested the typical scenarios for straightening (small positive and negative angle) and didn't want to make repo excessively heavy with reference images. Some interfaces don't even expose the ability to straighten by 45 degrees.

That said, I've tested -45 and +45 degrees manually for you and they look okay to me:

-45 degrees rotation (vips)

45 degrees rotation (vips)

They are also visually similar to pixbuf-based renders, which I didn't include for readability. Can be easily tested by adjusting angle in shared specs for straighten option.

Differences to pixbuf: - plasma-straighten-positive-5 - MAE (Mean Average Error): 0.0117613 - plasma-straighten-on-vertical-image - MAE: 0.0159686 - plasma-straighten-negative-20 - MAE: 0.01176 Visually all the images appear to be shifted by 1px. Since I didn't spot any issues with maths, I'm inclined to assume that it's due to rounding/trimming during computation.

MrLukeSmith

Thanks for entertaining the AI critique. I appreciate it's not your favourite approach, but it made it possible for me to better understand the specifics and the few points raised felt plausibly relevant. 😉

It was intended to be a knowledge filler for me as opposed to a wild goose chase for you. Apologies if it came across differently. 🙇

The comprehensive testing coupled with the context provided LGTM.

Feel free to wait for a second opinion if desired.

knarewski · 2024-12-05T22:13:27Z

It's all good, and if there are any gaps in your knowledge which you'd like to fill, I'm happy to help :-) .

Vips-based processor is a new, opt-in feature, so the risk of negative impact is minimal. Because of that I'll merge it. Of course I'm always happy to address post-merge suggestions if they arise.

knarewski force-pushed the feat-add-vips-straighten branch from 771801b to da1b983 Compare November 29, 2024 15:43

knarewski marked this pull request as ready for review November 29, 2024 15:47

knarewski requested a review from a team November 29, 2024 15:50

knarewski force-pushed the feat-add-vips-rotate branch from 865de1a to c38b981 Compare December 2, 2024 11:09

knarewski force-pushed the feat-add-vips-straighten branch from da1b983 to 1e1bba4 Compare December 2, 2024 11:10

Base automatically changed from feat-add-vips-rotate to master December 5, 2024 12:59

knarewski force-pushed the feat-add-vips-straighten branch from 1e1bba4 to 1bca566 Compare December 5, 2024 13:19

MrLukeSmith reviewed Dec 5, 2024

View reviewed changes

knarewski force-pushed the feat-add-vips-straighten branch from 1bca566 to 03c315c Compare December 5, 2024 16:07

MrLukeSmith approved these changes Dec 5, 2024

View reviewed changes

knarewski merged commit 3523a63 into master Dec 5, 2024
15 checks passed

knarewski deleted the feat-add-vips-straighten branch December 5, 2024 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add vips straighten #39

feat: add vips straighten #39

knarewski commented Nov 29, 2024

MrLukeSmith left a comment

MrLukeSmith Dec 5, 2024

knarewski Dec 5, 2024 •

edited

Loading

MrLukeSmith Dec 5, 2024

knarewski Dec 5, 2024

MrLukeSmith Dec 5, 2024

knarewski Dec 5, 2024

MrLukeSmith Dec 5, 2024

knarewski Dec 5, 2024

MrLukeSmith left a comment

knarewski commented Dec 5, 2024

	context 'with straighten option' do
	# Tested explicitly, because morandi happens to handle transparency differently when using straighten
	let(:options) { super().merge('straighten' => 2) }

	it 'applies transformations' do
	process_image

	expect(File).to exist(file_out)
	expect(processed_image_type).to eq('jpeg')
	expect(file_out).to match_reference_image('match-multiple-operations-and-straighten')
	end
	end

feat: add vips straighten #39

feat: add vips straighten #39

Conversation

knarewski commented Nov 29, 2024

Background

Problems

Solution

Notes

Pixbuf vs Vips comparison

MrLukeSmith left a comment

Choose a reason for hiding this comment

MrLukeSmith Dec 5, 2024

Choose a reason for hiding this comment

knarewski Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

MrLukeSmith Dec 5, 2024

Choose a reason for hiding this comment

knarewski Dec 5, 2024

Choose a reason for hiding this comment

MrLukeSmith Dec 5, 2024

Choose a reason for hiding this comment

knarewski Dec 5, 2024

Choose a reason for hiding this comment

MrLukeSmith Dec 5, 2024

Choose a reason for hiding this comment

knarewski Dec 5, 2024

Choose a reason for hiding this comment

MrLukeSmith left a comment

Choose a reason for hiding this comment

knarewski commented Dec 5, 2024

knarewski Dec 5, 2024 •

edited

Loading