Bug in color space conversion

[GoogleCodeExporter]

Hi,

The color space conversion from YCbCr to RGB simply does not work 
(Image.ChangeColorSpace). The actual code is:

            else if (_cm.colorspace == ColorSpace.YCbCr && cs == 
ColorSpace.RGB)
            {
                for (int x = 0; x < width; x++)
                    for (int y = 0; y < height; y++)
                    {
                        ycbcr[0] = (byte)_raster[0][x, y]; // 0 is LUMA
                        ycbcr[1] = (byte)_raster[1][x, y]; // 1 is BLUE
                        ycbcr[2] = (byte)_raster[2][x, y];

                        YCbCr.fromRGB(rgb, ycbcr);
                        YCbCr.toRGB(ycbcr, rgb);

                        _raster[0][x, y] = ycbcr[0];
                        _raster[1][x, y] = ycbcr[1];
                        _raster[2][x, y] = ycbcr[2];
                    }

                _cm.colorspace = ColorSpace.RGB;
            }

I found two bugs:

1) The "FromRGB" call must be removed, since it will overwrite the YCbCr 
array data with converted values from the previous RGB pixel,

2) The final assignment to the _raster array takes its value from the 
ycbcr array. It should be read from the rgb array.

Also, it is possible to get a performance boost by avoiding to write the 
raster data to a temporary array, and read the converted values back after 
the conversion.

The trick is to rewrite the "toRGB" and  "fromRGB" to accept three "by-
ref" byte values, one for each component. So, the _raster value could be 
read/written directly without the use of a temporary array.

Also, the two conversion routines could be rewritten in a more optimized 
way by converting the byte type to the floating point type only once (and 
in "ToRGB", by substracting 128 from chroma components only once also, 
this is why I appended a "2" to the variable names: Cb2, Cr2)

So, here's my new version:

    internal class YCbCr 
    {
        public static void toRGB(ref byte c1, ref byte c2, ref byte c3)
        {
            double dY   = (double)c1;
            double dCb2 = (double)c2 - 128;
            double dCr2 = (double)c3 - 128;

            c1 = (byte)Math.Max(0, Math.Min(255, (dY + 1.402   * dCr2)));
            c2 = (byte)Math.Max(0, Math.Min(255, (dY - 0.34414 * dCb2 - 
0.71414 * dCr2)));
            c3 = (byte)Math.Max(0, Math.Min(255, (dY + 1.772   * dCb2)));
        }

        public static void fromRGB(ref byte c1, ref byte c2, ref byte c3)
        {
            double dR = (double)c1;
            double dG = (double)c2;
            double dB = (double)c3;

            c1 = (byte)( 0.299   * dR + 0.587   * dG + 0.114   * dB);
            c2 = (byte)(-0.16874 * dR - 0.33126 * dG + 0.5     * dB + 128);
            c3 = (byte)( 0.5     * dR - 0.41869 * dG - 0.08131 * dB + 128);
        }
    }

The class still contains the (less-performant) "in-out array" version:

        public static void toRGB(byte[] colorIn, byte[] colorOut)
        {
            byte c1 = colorIn[0];
            byte c2 = colorIn[1];
            byte c3 = colorIn[2];

            toRGB(ref c1, ref c2, ref c3);

            colorOut[0] = c1;
            colorOut[1] = c2;
            colorOut[2] = c3;
        }

        public static void fromRGB(byte[] colorIn, byte[] colorOut)
        {
            byte c1 = colorIn[0];
            byte c2 = colorIn[1];
            byte c3 = colorIn[2];

            fromRGB(ref c1, ref c2, ref c3);

            colorOut[0] = c1;
            colorOut[1] = c2;
            colorOut[2] = c3;
        }

...but they exist only for future use, because now some parts of the 
ChangeColorSpace method can be rewritten to use the "ref byte" version:

            if (_cm.colorspace == ColorSpace.RGB && cs == ColorSpace.YCbCr)
            {
                for (int x = 0; x < width; x++)
                    for (int y = 0; y < height; y++)
                        YCbCr.fromRGB(ref _raster[0][x,y], 
                                      ref _raster[1][x,y], 
                                      ref _raster[2][x,y]);

                _cm.colorspace = ColorSpace.YCbCr;
            }
            else if (_cm.colorspace == ColorSpace.YCbCr && cs == 
ColorSpace.RGB)
            {
                for (int x = 0; x < width; x++)
                    for (int y = 0; y < height; y++)
                        YCbCr.toRGB(ref _raster[0][x,y], 
                                    ref _raster[1][x,y], 
                                    ref _raster[2][x,y]);

                _cm.colorspace = ColorSpace.RGB;
            }

Of course, I deleted the local ycbcr and rgb arrays since they are no 
longer used.

I attach the two files with this post.

I would greatly appreciate to be included as a member of this very cool 
project.

Original issue reported on code.google.com by nelliga...@gmail.com on 4 Dec 2008 at 5:53

Attachments:

• FJCore-FixedColorSpace.zip

[GoogleCodeExporter]

Also there is a bug in the JpegEncoder constructor converting (8-bit) Grayscale 
images.

Replace:
_input.Image.ChangeColorSpace(ColorSpace.YCbCr);

With this to fix:
_input = new DecodedJpeg(_input.Image.ChangeColorSpace(ColorSpace.YCbCr), 
_input.MetaHeaders);

Original comment by snakewar...@gmail.com on 3 Feb 2009 at 2:13

[GoogleCodeExporter]

Original comment by jeff.pow...@gmail.com on 19 Feb 2009 at 4:43

• Changed state: Accepted

[GoogleCodeExporter]

Resolved with revision 9.  snakeware95: the colorspace change happens in-place 
so I
think that edit isn't needed.  resubmit a bug if problems remain.

Original comment by jeff.pow...@gmail.com on 19 Feb 2009 at 10:20

• Changed state: Fixed