Using LockBits generates weird image

Question

I'm trying to write out a grayscale image using Lockbits, my current code looks is

/// <summary>
/// Save the content of the FrameProc out to a bitmap
/// </summary>
public void Save(string path)
{
    Bitmap bmp = new Bitmap(this.size.Width, this.size.Height
                           ,PixelFormat.Format32bppRgb);
    var data = bmp.LockBits(this.size, ImageLockMode.WriteOnly, bmp.PixelFormat);

    unsafe
    {
        for (int y = 0; y < this.size.Height; y++)
        {
            byte* row = (byte*)data.Scan0 + (y * data.Stride);
            for (int x = 0; x < this.size.Width; x++)
            {
                byte value = (byte)this.buffer[y, x];
                row[x*Bits+r] = value; 
                row[x*Bits+g] = value;
                row[x*Bits+b] = value;
            }
        }
    }

    bmp.UnlockBits(data);
    bmp.Save(path, ImageFormat.Bmp);
}

where

/// <summary>
/// The amount of Bytes per pixel in the image
/// </summary>
private const int Bits = 4;

/// <summary>
/// Image components
/// </summary>
private const int a=3, r = 2, g = 1, b = 0;

However the image i receive is not correct:

Maybe this is related to how i'm reading them in? So here's that code

    public FrameProc(Bitmap bmp)
    {
        this.size=new Rectangle(new Point(0,0), bmp.Size);
        var data = bmp.LockBits(this.size
                               ,ImageLockMode.ReadOnly
                               ,bmp.PixelFormat);
        this.buffer = new Matrix(this.size.Height, this.size.Width);

        unsafe
        {
            for (int y = 0; y < this.size.Height; y++)
            {
                byte* row = (byte*)data.Scan0 + (y * data.Stride);
                for (int x = 0; x < this.size.Width; x++)
                {
                    this.buffer[y,x] = 0.299*row[x*Bytes+r] 
                                     + 0.587*row[x*Bytes+g] 
                                     + 0.114*row[x*Bytes+b];
                }
            }
        }

        bmp.UnlockBits(data);
    }

Answer 1

Use this function indeed:

public Bitmap MakeGrayscale(Bitmap original)
{
   unsafe
   {
      //create an empty bitmap the same size as original
      Bitmap newBitmap = new Bitmap(original.Width, original.Height);

      //lock the original bitmap in memory
      BitmapData originalData = original.LockBits(
         new Rectangle(0, 0, original.Width, original.Height),
         ImageLockMode.ReadOnly, PixelFormat.Format24bppRgb);

      //lock the new bitmap in memory
      BitmapData newData = newBitmap.LockBits(
         new Rectangle(0, 0, original.Width, original.Height), 
         ImageLockMode.WriteOnly, PixelFormat.Format24bppRgb);

      //set the number of bytes per pixel
      // here is set to 3 because I use an Image with 24bpp
      int pixelSize = 3;

      for (int y = 0; y < original.Height; y++)
      {
         //get the data from the original image
         byte* oRow = (byte*)originalData.Scan0 + (y * originalData.Stride);

         //get the data from the new image
         byte* nRow = (byte*)newData.Scan0 + (y * newData.Stride);

         for (int x = 0; x < original.Width; x++)
         {
            //create the grayscale version
            byte grayScale = 
               (byte)((oRow[x * pixelSize] * .11) + //B
               (oRow[x * pixelSize + 1] * .59) +  //G
               (oRow[x * pixelSize + 2] * .3)); //R

            //set the new image's pixel to the grayscale version
            nRow[x * pixelSize] = grayScale; //B
            nRow[x * pixelSize + 1] = grayScale; //G
            nRow[x * pixelSize + 2] = grayScale; //R
         }
      }

      //unlock the bitmaps
      newBitmap.UnlockBits(newData);
      original.UnlockBits(originalData);

      return newBitmap;
   }
}

Source and other interesting examples (with theory behind) could be taken from here

Answer 2

From the results you're getting - it looks exactly as if each pixel is three bytes big and not four as you have declared it - and as one would expect. (Note: you called it Bits - but that's wrong - it should be namned Bytes, not Bits).

I'd experiment with any one of this:

• change from 4 to 3 bytes
• change from Format32bppRgb to Format32bppArgb and fill out the alpha with 255
• change from 4 to 3 bytes and from Format32bppRgb to from Format24bppRgb

I would also rewrite the loop slightly for performance (sorry, I can't help myself):

for (int x = 0; x < this.size.Width; x++, row += Bits) 
                { 
                    byte value = (byte)this.buffer[y, x]; 
                    row[r] = value;  
                    row[g] = value; 
                    row[b] = value; 
                } 

But were you really would get more speed if you get a pointer to this.buffer using the fixed keyword. Yes, you're not having any performance problems, but I couldn't help myself from mentioning it!