3

I am trying to filter a Bitmap image to increase or decrease Hue, Saturation, and Lightness values.

My code is working perfectly, but it is slow.

I am locking two bitmaps in memory, the original source and the current destination. The user can move various trackbar controls to modify each value which is then converted to an HSL value. For example, the values on the trackbar correspond to a range of -1.0 to 1.0.

Each time an event is thrown that the trackbar value changed, I run a function which locks the destination bitmap and applies the HSL values with the source bitmap and then stores the result in the destination bitmap. Once finished, I unlock the destination bitmap and paint the image on the screen.

Previously I used a lookup table for my other filters since I was doing per-byte operations. However I do not know how to apply that using HSL instead. Here is the code I am using:

byte red, green, blue;

for (int i = 0; i < sourceBytes.Length; i += 3)
{
    blue = sourceBytes[i];
    green = sourceBytes[i + 1];
    red = sourceBytes[i + 2];

    Color newColor = Color.FromArgb(red, green, blue);

    if (ModifyHue)
        newColor = HSL.ModifyHue(newColor, Hue);

    if (ModifySaturation)
        newColor = HSL.ModifySaturation(newColor, Saturation);

    if (ModifyLightness)
        newColor = HSL.ModifyBrightness(newColor, Lightness);

    destBytes[i] = newColor.B;
    destBytes[i + 1] = newColor.G;
    destBytes[i + 2] = newColor.R;
}

And here's my ModifyBrightness function:

public static Color ModifyBrightness(Color color, double brightness)
{
    HSL hsl = FromRGB(color);
    hsl.L *= brightness;
    return hsl.ToRGB();
}

So basically if their brightness slider is in the very middle, its value will be 0 which I will convert to "1.0" when I pass it in to the function, so it multiplies the brightness by 1.0 which means it won't change. If they drag the slider all the way to the right it will have a value of 100 which will result in a modifier of 2.0, so I'll multiply the lightness value by 2.0 to double it.

Trevor Elliott
  • 11,292
  • 11
  • 63
  • 102
  • 1
    First thing I would do would be to cache results, and use unsafe code for accessing the arrays faster. – SimpleVar Jan 16 '13 at 18:05
  • +1. With microcode like that, the array access totally kills you. Every accessis a check for min/max value. Happy unsafe pointer code, please. And move from bytes to a struct with all values at the same time. – TomTom Jan 16 '13 at 18:13
  • Do you guys have any documented reference of this? Afaik array syntax is equal to pointer syntax when the array is a pointer, no? There are no min/max limits anyways. – Rotem Jan 16 '13 at 18:20
  • @OP if it's only HSL you need to modify you can just as well use ImageAttributes with a ColorMatrix, It will execute much faster than your current code. – Rotem Jan 16 '13 at 18:25
  • I use the same exact code structure for my brightness, contrast, gamma, invert, and grayscale filters and they are extremely fast using lookup tables. I have tried using pointers instead and I actually noticed a performance decrease. The array indexing is not the bottleneck here, it's the RGB to HSL conversion. As I said in my question, I want to use something similar to a lookup table but I don't know how to apply that to HSL since you can't make a lookup table for every possible double value, it's not the same as 256 distinct byte values. – Trevor Elliott Jan 16 '13 at 18:39
  • @Moozhe I know it's not what you want to hear, but all the effects you mentioned can be achieved easily using the `ImageAttributes` class using a single call to `DrawImage`. I don't see a reason to reinvent the wheel. Here are some links: http://www.graficaobscura.com/matrix/index.html http://www.codeproject.com/Articles/3772/ColorMatrix-Basics-Simple-Image-Color-Adjustment – Rotem Jan 16 '13 at 18:59
  • Thee is "only" 0x1000000 RGB values (assuming your current code which uses `byte` for each channel). I think it maybe OK to have look up table for all of them if it is indeed bottleneck (note that it may not give you saving as it will require much more memory accesses)... 70Mb (if HSL value is 4 bytes) maybe ok price to pay. – Alexei Levenkov Jan 16 '13 at 19:10
  • I ended up using the ImageAttributes and it worked well. – Trevor Elliott Jan 17 '13 at 17:14

2 Answers2

6

I ended up researching ImageAttributes and ColorMatrix and found the performance was excellent.

Here is how I implemented it for a Saturation and Brightness filter:

// Luminance vector for linear RGB
const float rwgt = 0.3086f;
const float gwgt = 0.6094f;
const float bwgt = 0.0820f;

private ImageAttributes imageAttributes = new ImageAttributes();
private ColorMatrix colorMatrix = new ColorMatrix();
private float saturation = 1.0f;
private float brightness = 1.0f;

protected override void OnPaint(object sender, PaintEventArgs e)
{
    base.OnPaint(sender, e);

    e.Graphics.DrawImage(_bitmap, BitmapRect, BitmapRect.X, BitmapRect.Y, BitmapRect.Width, BitmapRect.Height, GraphicsUnit.Pixel, imageAttributes);
}

private void saturationTrackBar_ValueChanged(object sender, EventArgs e)
{
    saturation = 1f - (saturationTrackBar.Value / 100f);

    float baseSat = 1.0f - saturation;

    colorMatrix[0, 0] = baseSat * rwgt + saturation;
    colorMatrix[0, 1] = baseSat * rwgt;
    colorMatrix[0, 2] = baseSat * rwgt;
    colorMatrix[1, 0] = baseSat * gwgt;
    colorMatrix[1, 1] = baseSat * gwgt + saturation;
    colorMatrix[1, 2] = baseSat * gwgt;
    colorMatrix[2, 0] = baseSat * bwgt;
    colorMatrix[2, 1] = baseSat * bwgt;
    colorMatrix[2, 2] = baseSat * bwgt + saturation;

    imageAttributes.SetColorMatrix(colorMatrix, ColorMatrixFlag.Default, ColorAdjustType.Bitmap);

    Invalidate();
}

private void brightnessTrackBar_ValueChanged(object sender, EventArgs e)
{
    brightness = 1f + (brightnessTrackBar.Value / 100f);

    float adjustedBrightness = brightness - 1f;

    colorMatrix[4, 0] = adjustedBrightness;
    colorMatrix[4, 1] = adjustedBrightness;
    colorMatrix[4, 2] = adjustedBrightness;

    imageAttributes.SetColorMatrix(colorMatrix, ColorMatrixFlag.Default, ColorAdjustType.Bitmap);

    Invalidate();
}
Trevor Elliott
  • 11,292
  • 11
  • 63
  • 102
  • 1
    The fun part is you can calculate many different effects using individual matrices then multiply the matrices to combine the effects. – Rotem Jan 17 '13 at 17:18
1

You need to profile you app and see where problems are.

Random suggestions:

  • use 32 bit-per-pixel format to avoid unaligned reads. (and read whole 32 as single operation)
  • avoid multiple RGB <-> HSL conversions
Alexei Levenkov
  • 98,904
  • 14
  • 127
  • 179
  • Well I use the same code for contrast, RGB brightness, gamma, invert, etc. And it is very fast. So the RGB/HSL conversions are the main choke point. Maybe you could elaborate on how I avoid multiple conversions. I thought of using a lookup table but how can I use a LUT for a double value? With byte values it's easy since there are only 256 possibilities. – Trevor Elliott Jan 16 '13 at 18:35
  • @Moozhe, You code does up to 2 useless transforms to start with (if you need to change all 3 of H S L). Otherwise - look at your `FromRGB` code and optimize. In worst case 16M of integer values for precomputed table is not end of the world... – Alexei Levenkov Jan 16 '13 at 18:55
  • @AlexeiLevenkov It's actually 16.7m colors * 4 bytes = 67MB, not very feasible. Not to mention these probably more values than actually exist in the image, making the LUT generation slower than the calculation. – Rotem Jan 16 '13 at 18:58
  • @Rotem 67Mb is about size of one image from modern camera, so it is not unheard of amount of memory per item. OP should be able to measure and see if CPU vs. memory in this case works for that particular case. – Alexei Levenkov Jan 16 '13 at 19:04
  • You're right that I should only convert it to HSL once instead of 3 times. Actually in my testing I was only doing the Lightness filter and that was extremely slow. The other filters were toggled off. So I can fix that but I still need to improve performance by other means. – Trevor Elliott Jan 16 '13 at 19:04
  • @AlexeiLevenkov It's not just the amount of memory that's the problem. I'm arguing that there would be more calculations in solving a LUT per iteration that actually solving the image. – Rotem Jan 16 '13 at 19:10
  • @Rotem - look up table need to be created once - so should be 0 cost per image, but it is very unclear if reading from huge look up table would be faster than computing result. – Alexei Levenkov Jan 16 '13 at 19:14
  • @AlexeiLevenkov It would need to be recalculated every time the user changes the parameters, which is interactive. We are not talking about processing many images using same parameters, we're talking about the same image with many parameters. – Rotem Jan 16 '13 at 19:27