Re: [Pixman] [Patch] Gradient dithering into pixman (Try 2)
Hi, Can I get any input if my second patch follows "the right approach"? Are there any astylerc or clang_format to check that my code complies with code style ? Thanks On 04/19/2018 08:41 PM, Marc Jeanmougin wrote: Hi, Could anyone help me from here? If there is still a fundamental problem with the patch, I'll be happy to rework it (or if there is anything more needed in it) From what I gathered, pixman is without maintainer, so I'm not sure either how to proceed or who to contact... -- Marc On 04/08/2018 10:46 PM, Marc Jeanmougin wrote: Hi, I'm back :) I tried to implement the proposed changes : * the gradient walker now deals with argb_t (floats) and not uint32_t * all gradients are WIDE because of the above change * WIDE formats (using argb_t) can be dithered on write_back call depending on the "dither" property of the image (set with pixman_image_set_dither) * The dithering is still random. I could try to implement other ones if it's really needed for the patch to be accepted Thanks for any feedback, please do tell if there are any more changes needed for the patch to be accepted. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [Patch] Gradient dithering into pixman (Try 2)
Hi, Could anyone help me from here? If there is still a fundamental problem with the patch, I'll be happy to rework it (or if there is anything more needed in it) From what I gathered, pixman is without maintainer, so I'm not sure either how to proceed or who to contact... -- Marc On 04/08/2018 10:46 PM, Marc Jeanmougin wrote: > Hi, I'm back :) > > > I tried to implement the proposed changes : > > * the gradient walker now deals with argb_t (floats) and not uint32_t > * all gradients are WIDE because of the above change > * WIDE formats (using argb_t) can be dithered on write_back call > depending on the "dither" property of the image (set with > pixman_image_set_dither) > * The dithering is still random. I could try to implement other ones if > it's really needed for the patch to be accepted > > > Thanks for any feedback, please do tell if there are any more changes > needed for the patch to be accepted. > > > > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman signature.asc Description: OpenPGP digital signature ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [Patch] Gradient dithering into pixman (Try 2)
Hi, I'm back :)
I tried to implement the proposed changes :
* the gradient walker now deals with argb_t (floats) and not uint32_t
* all gradients are WIDE because of the above change
* WIDE formats (using argb_t) can be dithered on write_back call
depending on the "dither" property of the image (set with
pixman_image_set_dither)
* The dithering is still random. I could try to implement other ones if
it's really needed for the patch to be accepted
Thanks for any feedback, please do tell if there are any more changes
needed for the patch to be accepted.
--
Marc
From 2cd0d0a12c9a68bdedde10ae4b10c335f8961501 Mon Sep 17 00:00:00 2001
From: Marc Jeanmougin <m...@jeanmougin.fr>
Date: Sun, 8 Apr 2018 22:25:27 +0200
Subject: [PATCH] Adds a gradient dithering function to pixman.
This dithering is random, resulting is no artifacts and very smooth result,
and uses a very fast prng (xorshift algorithm).
Also adds a pixman_image_set_dithering to toggle the feature,
and a pixman_dither_t type for possible future other implementations.
Signed-off-by: Marc Jeanmougin <m...@jeanmougin.fr>
---
.gitignore | 2 ++
pixman/pixman-bits-image.c | 47
pixman/pixman-conical-gradient.c | 7 ++
pixman/pixman-general.c | 3 ++-
pixman/pixman-gradient-walker.c | 24 +---
pixman/pixman-image.c| 12 ++
pixman/pixman-linear-gradient.c | 11 --
pixman/pixman-private.h | 7 +-
pixman/pixman-radial-gradient.c | 18 +++
pixman/pixman-utils.c| 10 +
pixman/pixman.h | 9
11 files changed, 108 insertions(+), 42 deletions(-)
diff --git a/.gitignore b/.gitignore
index a245b69..02ad685 100644
--- a/.gitignore
+++ b/.gitignore
@@ -53,3 +53,5 @@ test/trap-crasher
*.ilk
*.obj
*.exe
+*.log
+*.trs
diff --git a/pixman/pixman-bits-image.c b/pixman/pixman-bits-image.c
index dcdcc69..bdedb99 100644
--- a/pixman/pixman-bits-image.c
+++ b/pixman/pixman-bits-image.c
@@ -752,6 +752,51 @@ _pixman_bits_image_src_iter_init (pixman_image_t *image, pixman_iter_t *iter)
iter->get_scanline = _pixman_iter_get_scanline_noop;
}
+
+static float
+dither_channel (float f, int n_bits, uint32_t *state)
+{
+uint32_t u, i_rem;
+float f_rem;
+if (f>1.0) f = 1.0;
+if (f<0) f = 0.;
+
+u = f * (1 << n_bits);
+u -= (u >> n_bits);
+f_rem = ( (float)(1<<n_bits) * f - (float)u);
+i_rem = f_rem * 4294967296.;
+
+*state = pixman_prng_get(*state);
+if (*state < i_rem)
+return MIN (1.0, f + (1.0 / ((float)(1<<n_bits;
+return f;
+}
+
+
+void
+dither(bits_image_t *image, argb_t *buf, int width)
+{
+int i;
+int a_size, r_size, g_size, b_size;
+uint32_t *state = &(image->common.state);
+pixman_format_code_t format = image->format;
+
+if(image->common.dither == PIXMAN_DITHER_NONE)
+return;
+
+a_size = PIXMAN_FORMAT_A (format);
+r_size = PIXMAN_FORMAT_R (format);
+g_size = PIXMAN_FORMAT_G (format);
+b_size = PIXMAN_FORMAT_B (format);
+
+for (i = 0; i < width; ++i) {
+buf[i].a = dither_channel(buf[i].a, a_size, state);
+buf[i].r = dither_channel(buf[i].r, r_size, state);
+buf[i].g = dither_channel(buf[i].g, g_size, state);
+buf[i].b = dither_channel(buf[i].b, b_size, state);
+}
+}
+
static uint32_t *
dest_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
{
@@ -856,6 +901,8 @@ dest_write_back_wide (pixman_iter_t *iter)
int width = iter->width;
const uint32_t *buffer = iter->buffer;
+dither (image, (argb_t*)buffer, width);
+
image->store_scanline_float (image, x, y, width, buffer);
if (image->common.alpha_map)
diff --git a/pixman/pixman-conical-gradient.c b/pixman/pixman-conical-gradient.c
index 8bb46ae..35a64f4 100644
--- a/pixman/pixman-conical-gradient.c
+++ b/pixman/pixman-conical-gradient.c
@@ -57,11 +57,11 @@ conical_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
int x = iter->x;
int y = iter->y;
int width = iter->width;
-uint32_t *buffer = iter->buffer;
+argb_t *buffer = (argb_t*)(iter->buffer);
gradient_t *gradient = (gradient_t *)image;
conical_gradient_t *conical = (conical_gradient_t *)image;
-uint32_t *end = buffer + width;
+argb_t *end = buffer + width;
pixman_gradient_walker_t walker;
pixman_bool_t affine = TRUE;
double cx = 1.;
@@ -165,9 +165,6 @@ conical_get_scanline_wide (pixman_iter_t *iter, const uint32_t *mask)
{
uint32_t *buffer = conical_get_scanline_narrow (iter, NULL);
-pixman_expand_to_float (
- (argb_t *)buffer, buffer, PIXMAN_a8r8g8b8, iter->width);
-
return buffer;
}
diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c
in
Re: [Pixman] [patch] Gradient dithering into pixman
On 03/27/2018 07:45 PM, Bill Spitzak wrote:
> Quick look at the patch and it seems like this will work, though it is a
> random dither. You might get much better compression of .png with a
> patterned dither. I've also had good luck with pseudo-error-diffusion.
> You keep in static memory an accumulated error (per color, but not
> really depending on the location of the pixel) and that is the threshold
> for the random number. This produces a bit more patterning than a
> straight random generator. You might also try it with no random number
> generator at all, but you have to preserve the accumulated error so that
> solid areas that are an integer don't reset it so each adjacent line
> gets the same pattern.
I implemented the accumulated error dithering (attached file, can be
applied over the previous 0001 patch). It greatly improves PNG
compression ratio for purely horizontal gradients (approximately +15%
instead of +1%, in filesize), but produces small artifacts (it's
still way better than no dithering of course, they are almost invisible)
without a random accumulated error at the start of lines (which
increases a lot the filesize).
> May want to call the "on" setting PIXMAN_DITHERING_GOOD. On the
> assumption that anybody who wants it on is happy with "good" dithering,
> and that they may not want to pay for the slowness of "best" dithering.
Considering the prng used, the slowness is almost negligible ("good
except for the small stripes" dithering may in fact be slower)
--
Marc
From 85aa2a84e74f3079796ec4686c4fa2acf6c2e93f Mon Sep 17 00:00:00 2001
From: Marc Jeanmougin <m...@jeanmougin.fr>
Date: Tue, 27 Mar 2018 21:36:10 +0200
Subject: [PATCH 2/2] Adds PIXMAN_DITHERING_GOOD with diffusion of accumulated
error
Signed-off-by: Marc Jeanmougin <m...@jeanmougin.fr>
---
pixman/pixman-gradient-walker.c | 21 -
pixman/pixman-image.c | 2 +-
pixman/pixman.h | 1 +
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/pixman/pixman-gradient-walker.c b/pixman/pixman-gradient-walker.c
index dbd7a92..1311fa4 100644
--- a/pixman/pixman-gradient-walker.c
+++ b/pixman/pixman-gradient-walker.c
@@ -52,7 +52,7 @@ _pixman_gradient_walker_init (pixman_gradient_walker_t *walker,
walker->need_reset = TRUE;
walker->dithering = gradient->dithering;
-walker->prng_state = rand();
+walker->prng_state = (gradient->dithering == PIXMAN_DITHERING_BEST) ? rand() : 0;
}
static void
@@ -179,6 +179,7 @@ _pixman_dither (pixman_gradient_walker_t *walker, float *in)
{
float f_rem = (*in-(float)((int)(*in)));
uint32_t i_rem = f_rem * 4294967296.;
+uint32_t color_state;
switch (walker->dithering)
{
@@ -190,6 +191,24 @@ _pixman_dither (pixman_gradient_walker_t *walker, float *in)
return *in +1;
else
return *in;
+
+case PIXMAN_DITHERING_GOOD:
+/* we use prng_state to keep four 8-bit values of errors to propagate */
+color_state = walker->prng_state & 0xff;
+i_rem >>= 24; /*keep 8 most significant bits*/
+color_state+=i_rem;
+if (color_state >= 256){
+color_state -= 256;
+/*rotate*/
+walker->prng_state = (walker->prng_state >> 8) | (color_state<<24);
+return *in+1;
+}
+else {
+/*rotate*/
+walker->prng_state = (walker->prng_state >> 8) | (color_state<<24);
+return *in;
+}
+
case PIXMAN_DITHERING_NONE:
default:
/* round */
diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
index 75f168e..61e6988 100644
--- a/pixman/pixman-image.c
+++ b/pixman/pixman-image.c
@@ -102,7 +102,7 @@ _pixman_init_gradient (gradient_t * gradient,
gradient->n_stops = n_stops;
gradient->common.property_changed = gradient_property_changed;
-gradient->dithering = PIXMAN_DITHERING_NONE;
+gradient->dithering = PIXMAN_DITHERING_GOOD;
return TRUE;
}
diff --git a/pixman/pixman.h b/pixman/pixman.h
index 5ac0db6..2422eb8 100644
--- a/pixman/pixman.h
+++ b/pixman/pixman.h
@@ -319,6 +319,7 @@ typedef enum
typedef enum
{
PIXMAN_DITHERING_NONE,
+PIXMAN_DITHERING_GOOD,
PIXMAN_DITHERING_BEST
} pixman_dithering_t;
--
2.16.2
signature.asc
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [patch] Gradient dithering into pixman
Hi, Le 27/03/2018 à 02:04, Søren Sandmann a écrit : > A long time ago I wrote this: > > https://lists.freedesktop.org/archives/pixman/2012-July/002175.html > > about how dithering could be added to pixman. The basic idea is that > "dithering" is a property of the destination image, not of the > gradient. I still think this is the right way to do it. Thank you for your input. Would it be possible to use your preferred way of doing it to my patch ? Le 27/03/2018 à 03:47, Bill Spitzak a écrit : > I don't understand why you would want to disable it when writing .png > files. There will be banding in the .png file, which I would think is > worse than the increased size. Depends. In many cases there is no visible banding, and some people may not want a x100 filesize increase for it (for files that are entirely made of gradients). > Also kind of fools the user if they did not look at the .png file and > only at InkScape's display. We already have such differences for filter quality, and of course it would be possible to switch it in the display windows for previewing. -- Marc ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [patch] Gradient dithering into pixman
a 100644
--- a/pixman/pixman-private.h
+++ b/pixman/pixman-private.h
@@ -352,6 +352,9 @@ typedef struct
pixman_repeat_t repeat;
pixman_bool_t need_reset;
+
+pixman_dithering_t dithering;
+uint32_tprng_state;
} pixman_gradient_walker_t;
void
@@ -367,6 +370,8 @@ uint32_t
_pixman_gradient_walker_pixel (pixman_gradient_walker_t *walker,
pixman_fixed_48_16_t x);
+uint32_t
+pixman_prng_get(uint32_t state);
/*
* Edges
*/
@@ -1121,6 +1126,12 @@ extern int timer_defined;
void pixman_timer_register (pixman_timer_t *timer);
+struct pixman_prng_state
+{
+int n;
+}
+
+
#define TIMER_BEGIN(tname) \
{ \
static pixman_timer_t timer ## tname; \
diff --git a/pixman/pixman-utils.c b/pixman/pixman-utils.c
index 4a3a835..aaeb50a 100644
--- a/pixman/pixman-utils.c
+++ b/pixman/pixman-utils.c
@@ -328,3 +328,12 @@ _pixman_log_error (const char *function, const char *message)
n_messages++;
}
}
+
+uint32_t
+pixman_prng_get(uint32_t x)
+{
+x ^= x << 13;
+x ^= x >> 17;
+x ^= x << 15;
+return x;
+}
diff --git a/pixman/pixman.h b/pixman/pixman.h
index 509ba5e..12612e8 100644
--- a/pixman/pixman.h
+++ b/pixman/pixman.h
@@ -316,6 +316,13 @@ typedef enum
PIXMAN_FILTER_SEPARABLE_CONVOLUTION
} pixman_filter_t;
+typedef enum
+{
+PIXMAN_DITHERING_NONE,
+PIXMAN_DITHERING_GOOD,
+ PIXMAN_DITHERING_BEST
+} pixman_dithering_t;
+
typedef enum
{
PIXMAN_OP_CLEAR = 0x00,
From f3405510ed7070cc2d0e903b3561ca287efbb41e Mon Sep 17 00:00:00 2001
From: Marc Jeanmougin <m...@jeanmougin.fr>
Date: Tue, 27 Mar 2018 00:05:37 +0200
Subject: [PATCH] Adds a gradient dithering function to pixman.
This dithering is random, resulting is no artifacts and very smooth result,
and uses a very fast prng (xorshift algorithm).
Also adds a pixman_image_set_dithering to toggle the feature,
and a pixman_dithering_t type for possible future other implementations.
Signed-off-by: Marc Jeanmougin <m...@jeanmougin.fr>
---
pixman/pixman-gradient-walker.c | 36
pixman/pixman-image.c | 15 +++
pixman/pixman-linear-gradient.c | 8 +++-
pixman/pixman-private.h | 12
pixman/pixman-utils.c | 9 +
pixman/pixman.h | 9 +
6 files changed, 80 insertions(+), 9 deletions(-)
diff --git a/pixman/pixman-gradient-walker.c b/pixman/pixman-gradient-walker.c
index 822f8e6..dbd7a92 100644
--- a/pixman/pixman-gradient-walker.c
+++ b/pixman/pixman-gradient-walker.c
@@ -27,6 +27,9 @@
#include
#endif
#include "pixman-private.h"
+#include
+#include
+#include
void
_pixman_gradient_walker_init (pixman_gradient_walker_t *walker,
@@ -48,6 +51,8 @@ _pixman_gradient_walker_init (pixman_gradient_walker_t *walker,
walker->repeat= repeat;
walker->need_reset = TRUE;
+walker->dithering = gradient->dithering;
+walker->prng_state = rand();
}
static void
@@ -169,6 +174,29 @@ gradient_walker_reset (pixman_gradient_walker_t *walker,
walker->need_reset = FALSE;
}
+static inline uint8_t
+_pixman_dither (pixman_gradient_walker_t *walker, float *in)
+{
+float f_rem = (*in-(float)((int)(*in)));
+uint32_t i_rem = f_rem * 4294967296.;
+
+switch (walker->dithering)
+{
+case PIXMAN_DITHERING_BEST:
+/* we want a8 = a with probability (1-(a-int(a))) and
+ * a8=a+1 with probability a-int(a)
+ */
+if ((walker->prng_state = pixman_prng_get(walker->prng_state)) < i_rem)
+return *in +1;
+else
+return *in;
+case PIXMAN_DITHERING_NONE:
+default:
+/* round */
+return *in + 0.5f;
+}
+}
+
uint32_t
_pixman_gradient_walker_pixel (pixman_gradient_walker_t *walker,
pixman_fixed_48_16_t x)
@@ -188,10 +216,10 @@ _pixman_gradient_walker_pixel (pixman_gradient_walker_t *walker,
g = a * (walker->g_s * y + walker->g_b);
b = a * (walker->b_s * y + walker->b_b);
-a8 = a + 0.5f;
-r8 = r + 0.5f;
-g8 = g + 0.5f;
-b8 = b + 0.5f;
+a8 = _pixman_dither(walker, );
+r8 = _pixman_dither(walker, );
+g8 = _pixman_dither(walker, );
+b8 = _pixman_dither(walker, );
v = ((a8 << 24) & 0xff00) |
((r8 << 16) & 0x00ff) |
diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
index 681864e..75f168e 100644
--- a/pixman/pixman-image.c
+++ b/pixman/pixman-image.c
@@ -102,6 +102,7 @@ _pixman_init_gradient (gradient_t * gradient,
gradient->n_stops = n_stops;
gradient->common.property_changed = gradient_property_chan
