How to improve speed in ImageSharp compared to Bitmap.LockBits?

[atuladhar]

I've been testing speed performance of ImageSharp and Bitmap.LockBits by running a function in loop to change a bitmap to grayscale. The result showed that a loop for 1000 times took approx. 11 seconds using Bitmap.LockBits while it took around 37 seconds using ImageSharp. Below is the code I've used in ImageSharp. Please suggest if there can be any improvements to improve the speed.

using (Image<Rgba32> image = (Image<Rgba32>)Image.Load("design1.jpg"))
            {
                Stopwatch stopWatch = new Stopwatch();
                stopWatch.Start();

                int wid = image.Width;
                int hgt = image.Height;

                SixLabors.ImageSharp.Color[] gs = new SixLabors.ImageSharp.Color[256];
                for (int i = 0; i < 256; ++i)
                {
                    gs[i] = new Rgba32((byte)i, (byte)i, (byte)i);
                }

                for (int i = 0; i < 1000; i++)
                {
                    for (int y = 0; y < hgt; y++)
                    {
                        Span<Rgba32> rowSpan = image.GetPixelRowSpan(y);
                        for (int x = 0; x < wid; x++)
                        {
                            Rgba32 pixelColor = rowSpan[x];
                            gray = (byte)((pixelColor.R + pixelColor.G + pixelColor.B) / 3);
                            rowSpan[x] = gs[gray];
                        }
                    }
                }
                stopWatch.Stop();
                TimeSpan ts = stopWatch.Elapsed;
                image.Save("resultGray.jpg");
            }

[JimBobSquarePants]

You should remove your bounds checks. You have three per loop. Bitmap.LockBits will have none for accessing the pixels since you're dealing with pointers so it's not really a fair comparison.

You can use ref Rgba32 rowBase = ref MemoryMarshal.GetReference(image.GetPixelRowSpan(y)) to get a reference to the row then Unsafe.Add(ref rowBase, x) to get the actual Rgba32 instance as a ref then you can update that instance rather than hitting the indexer again.

P.S There are built in methods available that do grayscale properly. Using the average loses too much information.

I recommend using BenchmarkDotNet for future benchmarking for better accuracy.

[atuladhar]

Hi, Thanks for your reply.
I am doing test on a basic function, hence chose grayscale function. The motive is to understand how ImageSharp does pixel by pixel operation on images without compromising the speed. I checked with the default grayscale function and performance is much better:

for (int i = 0; i < 1000; i++)
{
    image.Mutate(x => x
          .Grayscale()
         );

As suggested, I changed the code to the following:

for (int i = 0; i < 1000; i++)
{
    for (int y = 0; y < hgt; y++)
    {                        
        ref Rgba32 rowBase = ref MemoryMarshal.GetReference(image.GetPixelRowSpan(y));

        for (int x = 0; x < wid; x++)
        {
            Rgba32 pixelColor = Unsafe.Add(ref rowBase, x);
            gray = (byte)((pixelColor.R + pixelColor.G + pixelColor.B) / 3);
            rowBase = gs[gray]; //this needs to be changed I suppose?
        }
    }
}

I am not sure if this is the correct way as speed is not much improved with this change. Please suggest.

[antonfirsov]

I dont think omitting bounds check has any significant role here since the difference is around 3x. @JimBobSquarePants its not a good advice for users to go unsafe in the first round. In this case there's definitely an other problem out there.

@atuladhar are you sure you are benchmarking in Release mode?

[JimBobSquarePants]

@antonfirsov The key advice there was to use our built in functionality since it wasn't apparent that @atuladhar was simply testing pixel access performance. The original comparison is apples and oranges since Bitmap.LockBits is working against a source where there are no bounds checks.

Managed pixel access in System.Drawing is normally performed via GetPixel/SetPixel which is an order of magnitude slower than our per-pixel indexer. If a fair comparison was to take place then it has to be done using pointer arithmetic in the same manner as Bitmap.LockBits.

@atuladhar

Your second sample is not updating the correct reference. It would be useful to see your System.Drawing comparison code also.

ref Rgba32 grayBase = Unsafe.As<Color, Rgba32>(ref MemoryMarshal.GetReference<Color>(gs));

for (int i = 0; i < 1000; i++)
{
    for (int y = 0; y < hgt; y++)
    {                        
        ref Rgba32 rowBase = ref MemoryMarshal.GetReference(image.GetPixelRowSpan(y));

        for (int x = 0; x < wid; x++)
        {
            ref Rgba32 pixelColor = ref Unsafe.Add(ref rowBase, x);
            gray = (byte)((pixelColor.R + pixelColor.G + pixelColor.B) / 3);
            pixelColor = Unsafe.Add(ref, grayBase, gray); 
        }
    }
}

[antonfirsov]

The original comparison is apples and oranges since Bitmap.LockBits is working against a source where there are no bounds checks.

Bounds checks really do not matter that much, JIT is often capable to remove them (**). With newer runtime versions, Span<T> code should perform almost as fast as native pointer access, and in no circumstances would this explain a 3-4x difference as reported by #1417 (comment).

I believe there is some trivial mistake, like comparing performance of Debug builds.

It would be useful to see your System.Drawing comparison code also.

Agreed, we need to see the baseline code!

---

(**) @atuladhar speaking of bounds checks:

When working with spans or arrays, you should prefer span.Length in for loops:

Span<Rgba32> rowSpan = image.GetPixelRowSpan(y);
- for (int x = 0; x < wid; x++) { /* access rowSpan[i] */ }
+ for (int x = 0; x < rowSpan.Length; x++) { /* access rowSpan[i] */ }

This increases the chance for the JIT to "understand" that rowSpan[i] can never go outside the boundaries, enabling the removal of bounds-checks in generated ASM code.