Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Validation

Validation

###Naming

Naming

###Possible problems

Possible problems

###Performance

Performance

###Validation

###Naming

###Possible problems

###Performance

Validation

Naming

Possible problems

Performance

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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

Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

###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

lang-cs

AltStyle によって変換されたページ (->オリジナル) /