Fix quantization and dithering. #1114
Conversation
| public interface IDither | ||
| { | ||
| /// <summary> | ||
| /// Gets the <see cref="Dithering.DitherType"/> which determines whether the | ||
| /// transformed color should be calculated and supplied to the algorithm. | ||
| /// </summary> | ||
| public DitherType DitherType { get; } | ||
|
|
||
| /// <summary> | ||
| /// Transforms the image applying a dither matrix. | ||
| /// When <see cref="DitherType"/> is <see cref="DitherType.ErrorDiffusion"/> this | ||
| /// this method is destructive and will alter the input pixels. | ||
| /// </summary> | ||
| /// <param name="image">The image.</param> | ||
| /// <param name="bounds">The region of interest bounds.</param> | ||
| /// <param name="source">The source pixel</param> | ||
| /// <param name="transformed">The transformed pixel</param> | ||
| /// <param name="x">The column index.</param> | ||
| /// <param name="y">The row index.</param> | ||
| /// <param name="bitDepth">The bit depth of the target palette.</param> | ||
| /// <param name="scale">The dithering scale used to adjust the amount of dither. Range 0..1.</param> | ||
| /// <typeparam name="TPixel">The pixel format.</typeparam> | ||
| /// <returns>The dithered result for the source pixel.</returns> | ||
| TPixel Dither<TPixel>( | ||
| ImageFrame<TPixel> image, | ||
| Rectangle bounds, | ||
| TPixel source, | ||
| TPixel transformed, | ||
| int x, | ||
| int y, | ||
| int bitDepth, | ||
| float scale) | ||
| where TPixel : struct, IPixel<TPixel>; | ||
| } |
There was a problem hiding this comment.
Nice job with the unification, but I'm still wondering if we can find a better abstraction/representation here. The worst thing is that is hard to introduce any optimization with the current design, because of the per-pixel virtual Dither<T> method. Having a 2-element enum defining the two possible algorithms in parallel with the two matching subclasses is a warning sign for me.
A little bit unorthodox in the world of OO, but I'm wondering about your thoughts on the following idea: Instead of pretending that we are doing something abstract and pluggable in this method, we could simply represent the ditherers by the data (and only the data!) that defines them:
public struct ErrorDiffusionParameters
{
public int Offset { get; }
public DenseMatrix<float> Matrix { get; }
}
public struct OrderedDitherParameters
{
public DenseMatrix<float> ThresholdMatrix { get; }
}
public sealed class Dither
{
// Determines which algorithm to use
public DitherType DitherType { get; }
// Parameters in case of ErrorDiffusion
public ErrorDitherParameters? ErrorDiffusionParameters { get; }
// Parameters in case of OrderedDither
public OrderedDitherParameters? OrderedDitherParameters { get; }
public static Dither CreateErrorDiffusion(int offset, DenseMatrix<float> matrix) { ... }
public static Dither CreateOrderedDither(DenseMatrix<float> thresholdMatrix) { .... }
}
public static class KnownDitherings
{
public static Dither BayerDither2x2 { get; } = CreateBayerDither2x2();
private static Dither CreateBayerDither2x2()
{
DenseMatrix<float> thresholdMatrix;
// calculate the Bayer thresholdMatrix
return Dither.CreateOrderedDither(thresholdMatrix);
}
}
// Implementation could be moved to utility methods,
// since we are branching on `DitherType` anyways.
// Use the per-pixel variants for now, but it would be easy to switch to bulk variants later,
// without getting lost in API design
internal static class DitherExtensions
{
internal static TPixel ApplyOrderedDither<TPixel>(
this Dither dither,
ImageFrame<TPixel> image,
Rectangle bounds,
TPixel source,
TPixel transformed,
int x,
int y,
int bitDepth,
float scale)
where TPixel : struct, IPixel<TPixel>
{
// ....
}
internal static TPixel ApplyErrorDither<TPixel>(
this Dither dither,
ImageFrame<TPixel> image,
Rectangle bounds,
TPixel source,
TPixel transformed,
int x,
int y,
int bitDepth,
float scale)
where TPixel : struct, IPixel<TPixel>
{
// ....
}
}There was a problem hiding this comment.
This is very interesting! I'll have a play around later tonight.
There was a problem hiding this comment.
One issue is the sheer amount of dithering approaches.
- Random,
- BlueNoise
- Halftone
- etc
Each one means an extra nullable options property which seems a bit icky.
There was a problem hiding this comment.
I didn't know that, but it changes the picture a lot, because it means we need real extensibility here. The DitherType enum makes it hard to add a new dithering, since it needs touching processor/quantizer code as well.
I think we should keep thinking and find a reasonable abstraction. I have an idea for a double-dispatch trick, I will gist it up later today.
There was a problem hiding this comment.
Yeah, good point. I'd really like to investigate the possibility of IDither implementations being a struct passed via a generic argument also. (IResampler in the future also in a different PR).
Been looking at the JIT ASM to see what the difference is between the two approaches and it's chalk and cheese.
Regarding utility methods or extensions. We'll have to build ones that handle the different behaviors in the FrameQuantizer<TPixel> and PaletteDitherProcessor<TPixel>.
PaletteDitherProcessor
FrameQuantizer
There was a problem hiding this comment.
Good idea, I'll keep that in mind!
There was a problem hiding this comment.
Ok, I mostly figured out PaletteQuantizer, but I have no idea how to handle PaletteDitherProcessor:
https://gist.github.com/antonfirsov/cbad26bffc39fdd9803b7978a2e87abb
This is getting extremely complex and time-consuming if we want to do it right. Wouldn't it be wiser to hide the API for now, and open a post-RC1 issue?
There was a problem hiding this comment.
Looks like we're both heading in the same direction. Bear with me, I think I can crack it today.
| /// <summary> | ||
| /// Gets the default dithering algorithm to use. | ||
| /// </summary> | ||
| public static IDither DefaultDither { get; } = KnownDitherings.FloydSteinberg; |
There was a problem hiding this comment.
Isn't really a constant. Woldn't it better to define it in KnownDitherings instead? (KnownDitherings.Default)
There was a problem hiding this comment.
We'd have to do that across the board for all our Known*** types.
src/ImageSharp/Processing/Processors/Quantization/IFrameQuantizer{TPixel}.cs
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1114 +/- ##
==========================================
+ Coverage 82.02% 82.06% +0.04%
==========================================
Files 701 680 -21
Lines 28797 28655 -142
Branches 3281 3244 -37
==========================================
- Hits 23621 23516 -105
+ Misses 4494 4462 -32
+ Partials 682 677 -5
Continue to review full report at Codecov.
|
| public QuantizedFrame<TPixel> QuantizeFrame(ImageFrame<TPixel> source, Rectangle bounds) | ||
| => FrameQuantizerExtensions.QuantizeFrame(ref this, source, bounds); |
There was a problem hiding this comment.
Nice trick! Did not get it first.
| /// An error diffusion dithering implementation. | ||
| /// <see href="http://www.efg2.com/Lab/Library/ImageProcessing/DHALF.TXT"/> | ||
| /// </summary> | ||
| public readonly partial struct ErrorDither : IDither, IEquatable<ErrorDither> |
There was a problem hiding this comment.
I wonder what are the use-cases for equality comparison. Should we also overload ==? Do we also need to implement IEquatable<IDither>? Don't we need a few tests to make sure it works as intended?
While analyzing this I found #1116.
There was a problem hiding this comment.
Yeah we should implement IEquatable<IDither> plus operators. I'll do that now.
There was a problem hiding this comment.
Done. Also fixed the DenseMatrix<T> hashcode generation and added tests.
Prerequisites
Description
A deep investigation into our quantization and dithering algorithms yielded issues we needed to fix.
Color.Transparentdid not match W3C specification, rather copied incorrect System.Drawing value.This PR:
IErrorDiffusionandIOrderedDitherAPIs into newIDitherapi.QuantizerOptionsfor easier bundling of options.Color.Transparentvalue.Examples
Here's an example of the new dithering output vs the old using
BayerDither8x8ordered dithering. Note the much improved handling of gradients.Old
New
With the new
DitherScaleoptions built intoQuantizerOptionsit's possible to incrementally dither output.Here's incremental quantization and dithering of a sample image. I've used the websafe palette to accentuate differences.
Original
0%
25%
50%
75%
100%
Benchmarks
Note: Previous benchmarks did not have Core 3.1 versions. Memory increase is due to changing of color lookup caching strategy.
Before
After