###Validation
Validation
###Naming
Naming
###Possible problems
Possible problems
###Performance
Performance
###Validation
###Naming
###Possible problems
###Performance
Validation
Naming
Possible problems
Performance
Until @PeterTylor doesn't change its code please see my comment: Getting the dominant RGB color of a bitmap Getting the dominant RGB color of a bitmap
Until @PeterTylor doesn't change its code please see my comment: Getting the dominant RGB color of a bitmap
Until @PeterTylor doesn't change its code please see my comment: Getting the dominant RGB color of a bitmap
###Validation
Because the method is public
you shouldn't assume that you get a valid non null
Bitmap
. You should add a null
check otherwise you are exposing implementation details of your method.
###Naming
Based on the C# Naming guidelines methods should be named using PascalCase
casing. Method level variables should be named using camelCase
casing. Hence getDominantColor
->GetDominantColor
and IntPtr Scan0
->IntPtr scan0
.
###Possible problems
You state in your question that this method is used to get the dominant color of your desktop. If you use it only for that, then all will be good.
A problem could come up if you use this method with some different bitmap.
- If the bitmap which is passed is of DIN A4 size with e.g 300dpi your
int[] totals
will overflow.
###Performance
I would suggest to use pointer arithmetic instead of calculating each time the idx
value. I also would remove the most inner loop like @Zefick posted.
public System.Drawing.Color GetDominantColor(Bitmap bmp)
{
if (bmp == null)
{
throw new ArgumentNullException("bmp");
}
BitmapData srcData = bmp.LockBits(new Rectangle(0, 0, bmp.Width, bmp.Height), ImageLockMode.ReadOnly, bmp.PixelFormat);
int bytesPerPixel = Image.GetPixelFormatSize(srcData.PixelFormat) / 8;
int stride = srcData.Stride;
IntPtr scan0 = srcData.Scan0;
long[] totals = new long[] { 0, 0, 0 };
int width = bmp.Width * bytesPerPixel;
int height = bmp.Height;
unsafe
{
byte* p = (byte*)(void*)scan0;
for (int y = 0; y < height; y++)
{
for (int x = 0; x < width; x += bytesPerPixel)
{
totals[0] += p[x + 0];
totals[1] += p[x + 1];
totals[2] += p[x + 2];
}
p += stride;
}
}
long pixelCount = bmp.Width * height;
int avgB = Convert.ToInt32(totals[0] / pixelCount);
int avgG = Convert.ToInt32(totals[1] / pixelCount);
int avgR = Convert.ToInt32(totals[2] / pixelCount);
bmp.UnlockBits(srcData);
return Color.FromArgb(avgR, avgG, avgB);
}
Benchmarking with BechnmarkDotNet with x64 compiled yields
Yours: 17.5252 ms
EBrown's: 14.6109 ms
Mine: 8.4846 ms
Peter Taylor's: 4.6419 ms
Until @PeterTylor doesn't change its code please see my comment: Getting the dominant RGB color of a bitmap