After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 358851 - RGBA to indexed with dithering of transparency causes transparent pixels at fixed locations
RGBA to indexed with dithering of transparency causes transparent pixels at f...
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: General
git master
Other All
: Normal normal
: 2.4
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2006-10-01 20:48 UTC by Aurimas Juška
Modified: 2006-11-27 16:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Zoomed in screenshot (267 bytes, image/png)
2006-10-01 21:01 UTC, Aurimas Juška
  Details
example of error (still) (188 bytes, image/png)
2006-10-04 00:36 UTC, david gowers
  Details
Defect remains (56.37 KB, image/png)
2006-10-04 20:09 UTC, Aurimas Juška
  Details
Fix other functions in similar fashion (12.36 KB, patch)
2006-10-26 18:33 UTC, Aurimas Juška
committed Details | Review

Description Aurimas Juška 2006-10-01 20:48:16 UTC
This effect is seen on any RGBA layer that is converted to indexed mode using dithering of transparency.

Steps to reproduce:
* create new image
* delete default layer
* create new one transparent
* fill with black
* convert to index with Enable dithering of transparency
Comment 1 weskaggs 2006-10-01 20:56:33 UTC
Several of us have confirmed this in HEAD (it affects one pixel out of each 32x32 block, so you might have to zoom in to see it), but it does not happen in 2.2.  I think it was probably caused by the changes committed in version 1.166 of gimpimage-convert.c, with the ChangeLog entry:

2006-01-02  Sven Neumann  <sven@gimp.org>

	* app/core/gimpimage-convert-data.h
	* app/core/gimpimage-convert.c: applied patch by Adam D. Moss that
	replaces the default dither matrix by a 32x32 Bayer pattern (see
	bug #136604).

Comment #19 in bug #136604 makes the situation pretty clear.
Comment 2 Aurimas Juška 2006-10-01 21:01:02 UTC
Created attachment 73772 [details]
Zoomed in screenshot

Screenshot of zoomed in image with this defect.
Comment 3 Tor Lillqvist 2006-10-01 22:43:09 UTC
The immediate cause for the problem seems to be this test in gimpimage-convert.c at line 3294:

if ((has_alpha && (alpha_dither ?
		   ((src[alpha_pix]) > (DM[foo][bar])) :
		   (src[alpha_pix] > 127)))
    || !has_alpha)

(reformatted and complex subexpressions replaced with "foo" and "bar")

If the test is FALSE, the alpha of the destination pixel is set to zero. This causes the zero-alpha pixels we see.

The interesting thing here is that DM contains values from 1 to 255. Thus whenever we are looking at the (only) "255" value in DM, data[ALPHA_PIX] can never be > DM[foo][bar], and the FALSE branch is taken.

I certainly can't claim to understand this code, but would changing the ">" to ">=" here be a good idea? It fixes the problem in this bug, but what side-effects it has, I have no idea. 




Comment 4 Tor Lillqvist 2006-10-03 11:10:00 UTC
(Heh, when I say data[ALPHA_PIX] above, I meant src[alpha_pix], of course, I was looking at another place in gimpimage-convert.c and mistyped.)

Actually, after pondering this in the shower, I think it probably is safer to keep the comparison operator as ">", but instead add another condition: If src[alpha_pix]==255, then don't bother checking any firther against the DM. I.e.:

if ((has_alpha && (src[alpha_pix] == 255 ||
		   (alpha_dither ?
		    ((src[alpha_pix]) > (DM[foo][bar])) :
		    (src[alpha_pix] > 127))))
    || !has_alpha)

Then only the behaviour for fully opaque source pixels will change, they will always be fully opaque also in the result. For other pixels the behaviour will not change.
Comment 5 weskaggs 2006-10-03 19:11:32 UTC
I re-wrote the Adam-code to make it easier to understand, and once this was done, it became clear that the suggestion in comment #3 is the correct one.  (The direction of the inequalities is reversed in the rewritten code, though.)  This should get some testing, but I'm pretty confident the fix is correct.

For HEAD:

2006-10-03  Bill Skaggs  <weskaggs@primate.ucdavis.edu>

	* app/core/gimpimage-convert.c
	(median_cut_pass2_nodestruct_dither_rgb): adjust alpha-dithering
	and make code more readable; fixes bug #358851 as suggested
	by Tor Lillqvist.

Comment 6 david gowers 2006-10-04 00:36:17 UTC
Created attachment 73982 [details]
example of error (still)

This image was created by making a new 64x48 image, adding transparency, and 
indexizing (with the alpha-dither option ON, of course.)

I tested it on a larger image, and the wrongly transparent pixel is at [0,31] in each 'dither-tile' (the dithermatrix being 32x32.); so now, 1 of every 1024 pixels are wrongly transparent.
Comment 7 weskaggs 2006-10-04 16:36:47 UTC
This does not happen for me.  I'm betting that you weren't working with a fully up-to-date cvs.
Comment 8 Aurimas Juška 2006-10-04 20:09:21 UTC
Created attachment 74021 [details]
Defect remains

Tested your changes on current CVS. Black or white images don't cause problems any more. Painting some other colors causes no harm either. However, more than 256 colors... seems the defect is back. It had deeper roots, I guess. (Or maybe I should just get some sleep :)
Comment 9 Tor Lillqvist 2006-10-04 20:24:16 UTC
I guess similar love needs to be applied to some of the other functions in gimpimage-convert.c, then. What got fixed was only the function that is used when the number of colours in the original RGB(A) image is less or equal to the number we want in the destination indexed image
Comment 10 Aurimas Juška 2006-10-10 17:03:19 UTC
Have to reopen this one because I can still reproduce the bug when image has more than colors than we want to have in a palette. (Image must have alpha channel, of course.)

Comment 9 seems to have good solution. Quick investigation in a debugger shows that at least median_cut_pass2_no_dither_rgb should be changed too.
Comment 11 Aurimas Juška 2006-10-26 18:33:15 UTC
Created attachment 75465 [details] [review]
Fix other functions in similar fashion

This should fix the bug. Same love is applied to other functions that deal with alpha dithering.
Comment 12 Michael Natterer 2006-10-27 09:12:22 UTC
Adding Adam to CC list.
Comment 13 david gowers 2006-10-28 04:01:44 UTC
This patch works fine for me. I had hoped it would incidentally fix bug #349882, but that didn't occur. I think it is likely that the cause of bug #349882 resides nearby to the areas changed in this patch, though.
Comment 14 Sven Neumann 2006-11-27 16:39:42 UTC
Thanks a lot for the patch.

2006-11-27  Sven Neumann  <sven@gimp.org>

        * app/core/gimpimage-convert.c: applied patch from Aurimas Juška
        that fixes conversion with dithering of transparency (bug #358851).