13
\$\begingroup\$

I wrote an extension method defined for the Image Class (System.Drawing) — which returns a Boolean value telling you if the image is opaque or transparent.

The color of each pixel in the image is represented as a 32-bit number: 8 bits each for alpha, red, green, and blue (ARGB). Each of the four components is a number from 0 through 255, with 0 representing no intensity and 255 representing full intensity.

The alpha component specifies the transparency of the color.

If any pixel in the image has a value not equal to 255 in its alpha channel, then the image is not opaque, and therefore transparent.

    public static bool IsOpaque(this Image image)
    {
        var bitmap = new Bitmap(image);
        var bitmapData = bitmap.LockBits(new Rectangle(0, 0, bitmap.Width, bitmap.Height), ImageLockMode.ReadOnly,
            PixelFormat.Format32bppArgb);
        unsafe
        {
            var p = (byte*)bitmapData.Scan0;
            for (var x = 0; x < bitmap.Width; x++)
            {
                for (var y = 0; y < bitmap.Height; y++)
                {
                    if (p[x * 4 + y * bitmapData.Stride + 3] == 255) continue;
                    bitmap.UnlockBits(bitmapData);
                    return false;
                }
            }
        }
        bitmap.UnlockBits(bitmapData);
        return true;
    }

Concerns:

  • Can performance be improved at all?
  • Is it possible to improve readability and reusability without affecting performance?
\$\endgroup\$
  • 1
    \$\begingroup\$ I'd normally use the word "transparent" to mean opacity == 0, and "translucent" for something between transparent and opaque. \$\endgroup\$ – Toby Speight Oct 24 '17 at 8:41
  • 4
    \$\begingroup\$ @Owen just a small addition, not enough to go to an answer: using try { ... } finally { bitmap.UnlockBits(bitmapData); } you avoid to repeat that code twice. \$\endgroup\$ – Adriano Repetti Oct 24 '17 at 8:53
  • 2
    \$\begingroup\$ I'd swap y and x loops (so that y is the outer one), because this way you are skipping ~stride number of bytes for each access (since you are scanning vertically). Might give you some performance boost - but when it comes to performance, as always, you should do some measurements and compare the alternatives. \$\endgroup\$ – Filip Milovanović Oct 24 '17 at 9:12
  • \$\begingroup\$ @AdrianoRepetti That would hurt readability by increasing nesting — with no effect on performance. \$\endgroup\$ – Owen Oct 26 '17 at 20:36
  • \$\begingroup\$ @TobySpeight I did it for the SEO. \$\endgroup\$ – Owen Oct 26 '17 at 20:37
12
\$\begingroup\$

Performance can definitely be improved, but it's a case of how far you want to take the tradeoff.

First, though, note that Bitmap is IDisposable. If you create a new one, you're responsible for disposing it, typically with a using.

Bearing in mind Heslacher's point about casting being faster than copying, and correcting it to account for the fact that not all images are bitmaps, we could refactor:

    private static bool IsOpaque(Bitmap bitmap)
    {
        ...
    }

    public static bool IsOpaque(this Image image)
    {
        var bitmap = image as Bitmap;
        if (bitmap != null)
        {
            return IsOpaque(bitmap);
        }

        using (bitmap = new Bitmap(image))
        {
            return IsOpaque(bitmap);
        }
    }

Now, taking into account memory locality we want to swap the loops, as pointed out by Roland:

            for (var y = 0; y < bitmap.Height; y++)
            {
                for (var x = 0; x < bitmap.Width; x++)
                {
                    if (p[x * 4 + y * bitmapData.Stride + 3] == 255) continue;
                    ...
                }
            }

But multiplication is slow, so replace with addition:

            int off = 3; // NB apply Roland's observation about endianness here
            int gap = bitmapData.Stride - bitmap.Width * 4;
            for (var y = 0; y < bitmap.Height; y++, off += gap)
            {
                for (var x = 0; x < bitmap.Width; x++, off += 4)
                {
                    if (p[off] == 255) continue;
                    ...
                }
            }

Now we come to the big tradeoff of speed vs complexity of code. Image.PixelFormat and LockBits. If the images are already in 32-bit ARGB then LockBits is relatively cheap. If they're in a different format, it can be quite expensive.

  • If the format doesn't support alpha (i.e. (image.PixelFormat & (PixelFormat.Indexed | PixelFormat.Alpha | PixelFormat.PAlpha)) == PixelFormat.Undefined) then you can return false without even looking at the data.
  • If the format is indexed, you can do a quick-reject by scanning the palette. (Or even a quick-accept if every palette index is translucent, although I imagine that case would be rare).
  • Failing those, you can replicate the scan on a per-format basis, locking the data in its native format and then scanning appropriately. For indexed images, you can further split on whether the palette contains only one index with transparency (so you just need to look for that index) vs whether you need to actually look up the alpha for each pixel.

Note that if you take the final option, it might be worth generating the code with T4 so that you only have to edit one place in the .tt file to fix bugs rather than having to fix them method by method.

Further note that although I said this was a tradeoff, strictly speaking your code is buggy: if the original image is in Format64bppArgb then the conversion to Format32bppArgb might turn an almost opaque pixel completely opaque.

\$\endgroup\$
  • \$\begingroup\$ I googled for 64 bits per pixel image and I couldn't find any example, can you share any image with Format64bppArgb and is there any camera able to capture 64 bits per pixel? \$\endgroup\$ – Jawad Al Shaikh Oct 24 '17 at 15:56
  • 1
    \$\begingroup\$ @JawadAlShaikh, I don't have any, but the MSDN documentation for PixelFormat shows that they are supported, and absent a guarantee that no-one will pass an image in that format it's good practice to assume that they might. Given that we're talking about images with alpha, cameras aren't directly relevant: you won't get ARGB images from a camera. But a lot of recent cameras will produce "raw" images with more than 8 bits per channel: e.g. my Nikon D5300 will save 12-bit or 14-bit NEFs, which would have to be imported into GDI as Format48bppRgb, Format64bppArgb, or Format64bppPArgb. \$\endgroup\$ – Peter Taylor Oct 24 '17 at 16:09
  • \$\begingroup\$ Cool, so only "raw" format can handle bits>8, I mean png, tiff, bmp can't store Format64bppArgb? \$\endgroup\$ – Jawad Al Shaikh Oct 24 '17 at 16:13
  • \$\begingroup\$ @JawadAlShaikh, all of those formats can, but JPEG can't, and I've never seen a camera which writes anything other than JPEG and raw. \$\endgroup\$ – Peter Taylor Oct 24 '17 at 16:16
  • \$\begingroup\$ Actually, on modern processors multiplication isn't any slower than addition. It's division that is slow. That being said, I still like your idea of using an incrementing offset. \$\endgroup\$ – Octopus Oct 25 '17 at 17:31
14
\$\begingroup\$

I don't know how good the C# compiler and runtime optimize this code out of the box, so here is what I would try:

  • Load bitmap.Width, bitmap.Height and bitmapData.Stride into local variables.
  • Swap the two for loops, making x the inner loop. Memory accesses are now linear instead of jumping around (to see this, print the array index in your current code).

Side node: You should check whether your code works correctly on big-endian platforms. The + 3 looks suspicious. You could use a uint * instead of the byte * and test the pixel value like this: (p[x + stride * y] >= 0xFF000000.

\$\endgroup\$
13
\$\begingroup\$

Readability

The readability of the code could be slightly improved by adding a new line before the unsafe. Assigning the Rectangle which is used to call LockBits() to a local variable will make the call to LockBits() more readable as well because it doesn't sprawl over two lines.


Performance

  • Why do you create a new Bitmap out of the passed Image if the only thing you do is using it readonly ? A simple cast to a Bitmap would be enough and faster as well.

    Just do:

    var bitmap = image as Bitmap ?? new Bitmap(image);
    
  • The calculation of the index of the pointer p could be improved by adjusting the outer loop to simply increment x by 4, because addition is faster then multiplication.

    for (var x = 0; x < bitmap.Width; x += 4)
    {
        for (var y = 0; y < bitmap.Height; y++)
        {
            if (p[x + y * bitmapData.Stride + 3] == 255) continue;
            bitmap.UnlockBits(bitmapData);
            return false;
        }
    }  
    

    If we want to improve this further we should use a new variable currentY, like so

    for (var x = 0; x < bitmap.Width; x += 4)
    {
        var currentY = -bitmapData.Stride;
        for (var y = 0; y < bitmap.Height; y++)
        {
            currentY += bitmapData.Stride;
            if (p[x + currentY + 3] == 255) continue;
            bitmap.UnlockBits(bitmapData);
            return false;
        }
    }
    

    but this improvement is almost not measurable. The main performance boost comes from the simple casting instead of creating new Bitmap.

\$\endgroup\$
  • \$\begingroup\$ image could theoretically be a System.Drawing.Imaging.Metafile, or some other class derived fromImage that isn't a Bitmap. But at that point, the check for transparency changes drastically. Perhaps the code should throw an ArgumentException if it isn't a Bitmap, then do the cast directly to Bitmap. \$\endgroup\$ – Bradley Uffner Oct 25 '17 at 13:42
  • 2
    \$\begingroup\$ @BradleyUffner, that would be terrible API design. If you're going to complain about an argument which isn't of type T, you should make the formal parameter type be T. \$\endgroup\$ – Peter Taylor Oct 26 '17 at 11:18
5
\$\begingroup\$

Always validate the arguments in a public method.

if (image == null)
{
    throw new ArgumentNullException(nameof(image));
}

As suggested by Heslacher check if the image is a Bitmap before converting it

Bitmap bitmap = image as Bitmap;

if (bitmap == null)
{
    bitmap = new Bitmap(image);
}

Use try-finally to reduce repetition and reduce the chance of a mistake

var bitmapData = bitmap.LockBits(
    new Rectangle(0, 0, width, height), 
    ImageLockMode.ReadOnly,
    PixelFormat.Format32bppArgb);

try
{
    ...
    return false;
}
finally
{
    bitmap.UnlockBits(bitmapData);
}
return true;

Put together with the suggestion by Roland Illig to scan the image with a uint*

public static bool IsOpaque(this Image image)
{
    if (image == null)
    {
        throw new ArgumentNullException(nameof(image));
    }

    Bitmap bitmap = image as Bitmap;

    if (bitmap == null)
    {
        bitmap = new Bitmap(image);
    }

    var width = bitmap.Width;
    var height = bitmap.Height;

    var bitmapData = bitmap.LockBits(
        new Rectangle(0, 0, width, height), 
        ImageLockMode.ReadOnly,
        PixelFormat.Format32bppArgb);

    try
    {
        unsafe
        {
            var pixels = (uint*)bitmapData.Scan0;
            var stride = bitmapData.Stride / sizeof(uint);

            for (var y = 0; y < height; y++)
            {
                var yOffset = y * stride;

                for (var x = 0; x < width; x++)
                {
                    if (pixels[x + yOffset] < 0xFF000000) 
                    {
                        return false;
                    }
                }
            }
        }
    }
    finally
    {
        bitmap.UnlockBits(bitmapData);
    }

    return true;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ +1 I was hoping someone would suggest unlocking in a finally block. (By the way, that second if on the last section of code should be if(bitmap == null)) \$\endgroup\$ – Pharap Oct 24 '17 at 21:38
  • \$\begingroup\$ @Pharap, Adriano Repetti already suggested it in a comment. \$\endgroup\$ – Peter Taylor Oct 26 '17 at 11:20
  • \$\begingroup\$ @PeterTaylor Then Adriano should have made it a proper answer instead of just a comment. \$\endgroup\$ – Pharap Oct 26 '17 at 16:00
  • \$\begingroup\$ All of that nesting hurts readability and doesn't increase performance. And you're right, I should be validating arguments of public methods. :S \$\endgroup\$ – Owen Oct 26 '17 at 17:10
4
\$\begingroup\$

As far as performance, the best you can do is to get those nested loops as tight as possible. I think the tightest you could get them would be

var maxY = bitmap.Height * bitmapData.stride + 3;
var maxX = bitmap.Width * 4;
for (var y = 3; y < maxY; y += bitmapData.stride)
{
    for (var x = 0; x < maxX; x +=4)
    {
        if (p[y+x] == 255) continue;

        bitmap.UnlockBits(bitmapData);
        return false;
    }
}

This is somewhat like previous answers, but it does not use additional variables to keep track of gap and offset, and does absolutely no multiplications inside the loops. This limits the code to the bare minimum necessary, increment x and y to their next spot in the bit array and check that spot.

\$\endgroup\$
0
\$\begingroup\$

If performance is not the most important goal, the following code is simple and beautiful:

public static bool IsTranslucent(this Bitmap bitmap)
{
    var width = bitmap.Width;
    var height = bitmap.Height;

    for (var y = 0; y < height; y++)
        for (var x = 0; x < width; x++)
            if (bitmap.GetPixel(x, y).A != 255)
                return false;
    return true;
}

Unless you are doing massive amounts of image processing, this is the code I would go with. It is as simple as possible and expresses your requirements closely.

To be worth optimizing, this code should be responsible for at least 20% of the total run time of the program.

\$\endgroup\$
  • \$\begingroup\$ Why would you ever downgrade performance? \$\endgroup\$ – Owen Dec 1 '17 at 10:17
  • \$\begingroup\$ @Owen Being safe from unsafe code is reason enough for me. \$\endgroup\$ – Roland Illig Dec 1 '17 at 11:02
  • \$\begingroup\$ Unverifiable doesn't always mean dangerous. \$\endgroup\$ – Owen Dec 3 '17 at 5:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.