Feature proposal: Dithering #457
Conversation
|
Very cool, thanks for working on this! I'll need to take a closer look through the code later once it's ready for review A couple initial thoughts:
|
| int deltaB = color1.B - color2.B; | ||
|
|
||
| // Euclidean distance | ||
| return Math.Sqrt (deltaR * deltaR + deltaG * deltaG + deltaB * deltaB); |
There was a problem hiding this comment.
I think working in terms of squared distance would be fine here and let you avoid the square root and keep things in integers?
| byte newR = Utility.ClampToByte (color.R + (int) (factor * errorRed)); | ||
| byte newG = Utility.ClampToByte (color.G + (int) (factor * errorGreen)); | ||
| byte newB = Utility.ClampToByte (color.B + (int) (factor * errorBlue)); | ||
| return ColorBgra.FromBgra (newB, newG, newR, 255); |
There was a problem hiding this comment.
Should this preserve the original alpha?
| public override void Render (ImageSurface src, ImageSurface dest, ReadOnlySpan<RectangleI> rois) | ||
| { | ||
| var src_data = src.GetReadOnlyPixelData (); | ||
| var src_copy = new ColorBgra[src_data.Length]; |
There was a problem hiding this comment.
The article you linked to mentioned that you only need a couple rows to store the errors accumulated for the current + next row (varies based the matrix of course), so that could be a future optimization.
Or, at least avoid the extra copy by copying into the destination image and then working in-place there
| var src_copy = new ColorBgra[src_data.Length]; | ||
| src_data.CopyTo (src_copy); | ||
| var dst_data = dest.GetPixelData (); | ||
| foreach (var rect in rois) { |
There was a problem hiding this comment.
Because this algorithm is sequential, we definitely need to do some testing of rendering it live in Pint with the live preview dialog. I think it will try to run the effect separately on many small tiles which won't behave correctly?
| var dst_data = dest.GetPixelData (); | ||
| foreach (var rect in rois) { | ||
| for (int y = 0; y < rect.Height - Data.DiffusionMatrix.RowsBelow; y++) { | ||
| for (int x = Data.DiffusionMatrix.ColumnsToLeft; x < rect.Width - Data.DiffusionMatrix.ColumnsToRight; x++) { |
There was a problem hiding this comment.
I'm not really sure about the indexing here, this seems to ignore the left and top coordinates of the rectangle?
Also I think the dithering should likely still be applied to the first pixel(s) of the row?
|
Thank you for the feedback. I took a look at the 'Quantize' effect in Paint.NET. It's kinda sorta similar but it has a few key differences. Among those that I could notice:
|
…hould be implemented
…at the standard effect dialog can be used
|
Also, points taken. I will address the issues you've raised, little by little, when I have the time. The first thing I've done is changing the |
|
Sounds good! Yeah I agree that being able to use a specific palette seems like a useful option to still have |
… from even being changed. Now I must figure out why the effect doesn't work as intended...
|
Just to let you know, I'll be on vacation the next couple weeks so I won't be responsive on here for a bit :) |
…nation data itself, So that effect doesn't 'spill over' to pixels outside ROIs
…e color is being quantized but the dithering is not being done. I wonder why
|
I finally made the dithering work in the application itself instead of the fake Pinta setup I used at the beginning. Also, no pixels are being ignored (addressing one of the points you made) One thing I'm noticing is that the ROIs are being passed one by one, by which I mean the |
|
Yeah, this is happening because the live preview manager controls the multithreading, so it decides on the tiles to render and then calls Render() many times in parallel with the different rectangles. (And, if the effect is slow to render it will update the canvas to show partial progress) |
|
Looking at the latest diff more, what might be happening is:
|
…ays within the bounds of ROI
|
Thank you, you are right about the clamping, and I just added checks so that the application of the matrix stays within the rectangle. I updated the images being used for testing, too, and they are indeed different after making this change. |
|
I added a few new palettes. Not necessarily the most important ones, but rather a selection of those that are easy to create programmatically (usually those that can be represented exactly in the color space that most modern computers use). |
`RgbColorCube` instead of `ColorCube`, as there may be an `RgbaColorCube`
|
|
||
| static Predefined () | ||
| { | ||
| BlackWhite = ImmutableArray.CreateRange (new[] { ColorBgra.FromBgr (0, 0, 0), ColorBgra.FromBgr (255, 255, 255) }); |
There was a problem hiding this comment.
ColorBgra.White and Black can be used here
|
|
||
| public sealed class DitheringData : EffectData | ||
| { | ||
| [Caption ("Diffusion Matrix")] |
There was a problem hiding this comment.
Diffusion Matrix might not be the most helpful for non-technical users. Maybe just something like Dithering Method or Diffusion Method?
There was a problem hiding this comment.
Sure. I changed it to "Error Diffusion Method"
|
|
||
| public enum PredefinedDiffusionMatrices | ||
| { | ||
| Sierra, |
There was a problem hiding this comment.
These should probably get Caption attributes added, e.g. Floyd-Steinberg should have a hyphen in it from what I see online, along with explanations for translators
It would also be good to see if there are any more descriptive names for users, e.g. "Fake" Floyd-Steinberg might not help a user understand what it does
There was a problem hiding this comment.
Sure. I added a CaptionAttribute to those values that have hyphens or spaces; and I labeled the matrix you mentioned as "Floyd-Steinberg Lite"
There was a problem hiding this comment.
Thanks - since these captions will also show up as translatable strings, also adding some Translators: ... comments would be good to explain to translators that these are algorithms named after people
There was a problem hiding this comment.
Sure. I just added comments for translators making it clear that the matrices were named after people
| FakeFloydSteinberg, | ||
| } | ||
|
|
||
| internal sealed class ErrorDiffusionMatrix |
There was a problem hiding this comment.
Please also add some doc comments for this class
There was a problem hiding this comment.
I just added a doc comment with basic explanation
| if (thisItem.Y < roi.Top || thisItem.Y >= roi.Bottom) | ||
| continue; | ||
|
|
||
| if (thisItem.X < 0 || thisItem.X >= settings.sourceWidth) |
There was a problem hiding this comment.
This second set of checks should be unnecessary since the rectangle should be inside the image bounds
There was a problem hiding this comment.
At first I added it because I wasn't sure if the ROIs were guaranteed to be inside the image bounds, but in the light of this I removed it.
Explanation: Sometimes we want to render an image using a different (usually more limited) palette, but also have the result look decent. Some of the most common algorithms for this 'diffuse the error' to neighboring pixels. In other words, instead of, say, simply applying a threshold to each pixel, they rely on neighboring pixels to help approximate the overall original look of the area.
This is the output in my development setup after dithering the image that is used to test the effects, with the default settings of this proposed effect, which consist of a fixed 16-color palette and the Floyd-Steinberg diffusion matrix:
The output looks sane, but we still need to verify that it is actually correct.
I will freely admit the code that I'm proposing needs to be improved. I wrote it quickly in order to have something working. Opening a pull request opens the door for discussion, though.
The implementation was inspired by this article:
https://tannerhelland.com/2012/12/28/dithering-eleven-algorithms-source-code.html