GNOME Bugzilla – Bug 332324
'remove unused colors' doesn't work right.
Last modified: 2006-04-19 10:51:29 UTC
The 'Remove unused colors' option in the Convert to Indexed dialog is broken. It seems to remove the colors but doesn't remap the image to the new palette -- hence the image looks bizarre usually. Steps to reproduce: 1. Pick a palette with >= 16 colors 2. Draw some areas of four of the colors from the palette onto the image 3. Convert to indexed, specifying custom palette (the one chosen before) and making sure 'remove unused colors' is ON.
I can't reproduce this in GIMP 2.0.6, which is the latest version I have. So guessing it might be something that happened after this.
I cannot reproduce that with current CVS (2.3.8++) nor with 2.2.10. FYI, I selected the "Reds And Purples" palette in both cases and I ended up with an indexed image containing only 4 colors (according to the colormap display). However, it looks like the CVS version is misbehaving: I start with a white image on which I draw 3 or 4 dots with the pencil tool, and I end up with a mostly black image after converting to indexed mode (there is no white in the palette, but there is a light purple that is correctly selected by 2.2.10). So there is definitely something wrong there. Maybe some memory corruption?
you just said basically, ' i can't reproduce this bug', then 'i reproduced this bug.' :) the colormap starts out like this : 0123456789 then the unused colors are removed, which leaves: 0149 but the color indices are not remapped, so the colors are FUBAR. Try this: * Create a 6-entry palette with the primarys and secondarys, like: R Y G C B M * Create a new 32x32 image * Fill it with red * Paint strokes of yellow, cyan, and magenta on it. * Indexize to that palette with 'remove unused colors' ON. The optimized palette looks like this: RMCY so: Yellow(index #1 in the original palette) becomes Magenta. Cyan (index #3 in the original palette) becomes Yellow. Magenta had an index of #5, so it gets its color from some undefined bit of memory just past the end of the colormap; in my case this meant black. Red stays the same, because the color at index #0 is still Red. It's very simple. The colormap is optimized but the image is not adjusted to match.
David, is it correct to assume that you only see this problem in the CVS version, not in GIMP 2.2?
Yes; This bug, at a guess, was introduced near the start of the 2.3.x series. I was tweaking some parts of gimpimage-convert at the time so I thought the error was due to my change. But it appears in a clean checkout (the one I was using a few minutes ago).
It is possible that the problem was introduced in the changes Sven made to get version 1.153 of gimpimage-convert.c, with the ChangeLog comment: 2005-02-18 Sven Neumann <sven@gimp.org> * app/core/gimpimage-convert.c: some simple loop unrolling, converted tabs to spaces and sprinkled the code with const qualifiers. The changes certainly affected relevant code.
I have reviewed this change and I think that it is correct. In the meantime I have undone one level of unrolling here for the sake of code readability. I also fixed what I believe was a bug in the function generate_histogram_gray(): 2006-02-25 Sven Neumann <sven@gimp.org> * app/core/gimpimage-convert.c (generate_histogram_gray): only count pixels with an alpha value > 127.
On second review I found that Bill was right and the code was indeed buggy. I have corrected this now: 2006-02-25 Sven Neumann <sven@gimp.org> * app/core/gimpimage-convert.c (remap_indexed_layer): fixed bug introduced by optimization (bug #332324). I am not yet closing this bug report because I wonder if the code in remap_indexed_layer() is doing the right thing. Why are the alpha value of the pixels checked at all? If we are remapping the image to a different colormap, why would we not touch transparent pixels?
Too long ago for me to actually remember, but I'd guess it's because the remap_table[] value for indices which are only used for totally transparent pixels may be undefined, since totally transparent pixels are (or should be, IIRC) excluded from all aspects of palette-building. So the transparency-test in remap_indexed_layer() is probably trying to avoid introducing indices which are out of the range of the official destination palette size. If that's the reason, and it's the only one I can think of, then the test can go away if we ensure that remap_table[] in gimp_image_convert() is all zero-initialized before make_remap_table() is called - that'd be kind of a hack, but it's correct. It'd also make the bogus-transparent-index-avoidance more likely to actually work, which I'm not 100% sure is really the case in the existing remap_indexed_layer(). (Referring to 2.0.6 source.)
(...or that function could be completely fine for different reasons not obvious to me in a five-minute code browse. Or bogus ;) But the zero-init alternative still looks safe to me.)
Well, let's close this report then. The problem seems to be fixed and it doesn't make much sense to speculate about the code based on memories.