LibGfx+image+animation+PixelPaint: Minor dithering tweaks #26432
Conversation
github-actions
bot
added
the
👀 pr-needs-review
=>
(Keep https://nico.github.io/hack/web/gamma-upsample.html in mind – if you're on a retina screen, you have to look at the image at 50% zoom to get an accurate impression.) |
We have to divide by n * n, not n * n - 1, to make sure that the result is strictly less than 1.0f. Otherwise, a perfectly white image will have a black block in the output in each tile. (To test this, I locally uncommented the `input_is_bilevel` early-out in BilevelImage::create_from_bitmap().) Also test the property that the bayer dither result of half gray is exactly a checkerboard pattern, and make that test pass by adding a missing round_to() call.
Also add a few comments to DitheringAlgorithm to classify the entries. For now, just hardcode the dithering matrix for this. I created this matrix with a python program that sorted the matrix indices first by distance from the center, and then by the angle around the center (`mod(atan2(dy, dx), 2pi)`) to break ties.
This allows overriding the frame duration, which is especially useful when making an animation out of several single-frame images.
| for (int y = 0, i = 0; y < bitmap.height(); ++y) { | ||
| for (int x = 0; x < bitmap.width(); ++x, ++i) { | ||
| u8 threshold = (bayer_matrix[(y & mask) * n + (x & mask)] * 255) / ((n * n) - 1); | ||
| u8 threshold = round_to<u8>((bayer_matrix[(y & mask) * n + (x & mask)] * 255.0f) / (n * n)); |
There was a problem hiding this comment.
I have the same patch locally 😅, just with a comment for clarity.
diff --git a/Userland/Libraries/LibGfx/ImageFormats/BilevelImage.cpp b/Userland/Libraries/LibGfx/ImageFormats/BilevelImage.cpp
index 4884c34c1f..4e7f3e884b 100644
--- a/Userland/Libraries/LibGfx/ImageFormats/BilevelImage.cpp
+++ b/Userland/Libraries/LibGfx/ImageFormats/BilevelImage.cpp
@@ -246,7 +246,7 @@ static constexpr auto bayer_matrix_8x8 = make_bayer_matrix<3>();
ErrorOr<NonnullRefPtr<BilevelImage>> BilevelImage::create_from_bitmap(Gfx::Bitmap const& bitmap, DitheringAlgorithm dithering_algorithm)
{
bool input_is_bilevel = true;
for (auto pixel : bitmap) {
if (pixel != 0xFF000000 && pixel != 0xFFFFFFFF) {
input_is_bilevel = false;
@@ -312,10 +312,15 @@ ErrorOr<NonnullRefPtr<BilevelImage>> BilevelImage::create_from_bitmap(Gfx::Bitma
VERIFY(is_power_of_two(n));
auto mask = n - 1;
+ // A bayer matrix of dimension N has N x N +1 different states. First one
+ // is an all black matrix, and then one more for each element turning white.
+ u32 number_of_states = n * n + 1;
+
for (int y = 0, i = 0; y < bitmap.height(); ++y) {
for (int x = 0; x < bitmap.width(); ++x, ++i) {
- u8 threshold = (bayer_matrix[(y & mask) * n + (x & mask)] * 255) / ((n * n) - 1);
+ u8 threshold = round_to<u8>(bayer_matrix[(y & mask) * n + (x & mask)] * 255.f / (number_of_states - 1));
bilevel_image->set_bit(x, y, gray_bitmap[i] > threshold ? 0 : 1);
}
}
break;There was a problem hiding this comment.
We still have an issue though, as the range of gray values is not the same for each matrix step. For a Bayer2x2 there are 5 states and we map:
| Gray value range | Pattern # |
| 0 | 0 |
| 1 - 64 | 1 |
| 65 - 127 | 2 |
| 128 - 190 | 3 |
| 190 - 255 | 4 |
I also fixed this in #26433.
There was a problem hiding this comment.
That's expected though and just an effect of gamma, right? My mental model is that dithering works in linear space: A black pixel next to a white pixel looks roughly as if both were a half-gray pixel, but 128 isn't half-gray due to gamma. (…or do you mean something else?)
There was a problem hiding this comment.
and just an effect of gamma
The value on the left are the thresholds in the Bayer matrix. And as do the transformation to linear sRGB before that, it's compared to linear values.
Let's consider uniform bitmaps of the size of the Bayer matrix for simplicity, colors are in linear space. The issue is that an input bitmap with a value of 1 / 255 (= 0.004) is associated to 25% gray (the average of the ditehring) and a value of 128 / 255 (0.502) is associated to 75% gray. These errors are way bigger than necessary and could be reduced by using these mappings:
| Gray value range | Pattern # |
| 0 - 51 | 0 |
| 52 - 101 | 1 |
| 102 - 152 | 2 |
| 153-203 | 3 |
| 204-255 | 4 |
| for (auto const& decoder : decoders) | ||
| if (decoder->size() != output_size) | ||
| return Error::from_string_literal("All input images must have the same dimensions"); |
There was a problem hiding this comment.
CodingStyle.md wants you to add brackets for the for.
| { | ||
| Options options = TRY(parse_options(arguments)); | ||
|
|
||
| // FIXME: Allow multiple single frames as input too, and allow manually setting their duration. |
There was a problem hiding this comment.
You can remove this fixme or update it and remove the one you added below about frame duration.
| TRY(animation_writer->add_frame_relative_to_last_frame(*frame.image, frame.duration, last_frame, options.allow_inter_frame_compression)); | ||
| last_frame = frame.image; | ||
| for (auto const& decoder : decoders) { | ||
| for (size_t i = 0; i < decoder->frame_count(); ++i) { |
There was a problem hiding this comment.
We might want to throw away the decoder once we're done with it to free some memory.
| } | ||
| } | ||
| break; | ||
| case DitheringAlgorithm::Clustered4x4: |
There was a problem hiding this comment.
Do we want even-sized matrices for this?
I would expect better results with a matrix with a centered zero.
2 participants
animationto make it possible to create animations from single images