4
\$\begingroup\$

I'm testing some methods for checking if an area of the screen changes, can you see any problems with how I'm doing this, and is there a better way for me to do this?

the _memoryImage2 will not change in the final app, it will be a read only array that I will check the memoryImage against.

namespace ScreenShot_BoundsTest
{
    public partial class Form1 : Form
    {
        Bitmap _memoryImage1;
        Bitmap _memoryImage2;

        Color[,] _first;
        Color[,] _second;

        private string _valueX;

        public Form1()
        {
            InitializeComponent();
        }

        private void Button1Click(object sender, EventArgs e)
        {
            backgroundWorker1.RunWorkerAsync();
        }

        private void GetImage()
        {
            _memoryImage2 = _memoryImage1;
            CaptureScreen();
            pictureBox2.Image = pictureBox1.Image;
            if (_memoryImage1 != null) _first = Bitmap2Imagearray(_memoryImage1);

            pictureBox1.Image = _memoryImage1;
            if (_memoryImage2 != null) _second = Bitmap2Imagearray(_memoryImage2);

            var x = CompareArrays();

            _valueX = x != true ? "false" : "true";
        }

        private bool CompareArrays()
        {
            for (var i = 0; i < _memoryImage1.Width; i++)
            {
                for (var j = 0; j < _memoryImage1.Height; j++)
                {
                    if (_first == null) continue;
                    if (_second != null)
                        if (_first[i,j] != _second[i,j])
                        {
                            return false;
                        }
                }
            }
            return _first != null && _second != null;
        }

        private void CaptureScreen()
        {
            Size s;
            using (var myGraphics = CreateGraphics())
            {
                s = new Size(100, 50);
                _memoryImage1 = new Bitmap(s.Width, s.Height, myGraphics);
            }

            var memoryGraphics = Graphics.FromImage(_memoryImage1);
            memoryGraphics.CopyFromScreen(100, 100, 10, 10, s);
        }

        private static Color[,] Bitmap2Imagearray(Bitmap b)
        {
            var imgArray = new Color[b.Width, b.Height];
            for (var y = 0; y < b.Height; y++)
            {
                for (var x = 0; x < b.Width; x++)
                {
                    imgArray[x, y] = b.GetPixel(x, y);
                }
            }
            return imgArray;
        }

        private void BackgroundWorker1DoWork(object sender, DoWorkEventArgs e)
        {
            GetImage();
            Thread.Sleep(100);
        }

        private void BackgroundWorker1RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
        {
            label1.Text = _valueX;
            backgroundWorker1.RunWorkerAsync();
        }    
    }
}
\$\endgroup\$
8
\$\begingroup\$

Let's start with your algorithm. You're using Bitmap.GetPixel to convert the entire bitmap image pixel by pixel into matrix of colors. I tried this approach several years ago - it was pretty slow on .Net 2.0 and I suppose it is still slow. You should convert image into byte[] and extract color bytes out of this array, it should be hundred times faster. Look into Bitmap.Lockbits method and BitmapData class.

Remaining points are about your code readability. There are several general rules which may improve readability a lot:

  1. If method has some result of execution then return this result instead of assigning it to some field. Also use method parameters to accept arguments instead of class fields.
  2. If variable is not needed outside of one method's scope then do not declare a field for it, use local variable instead.
  3. Be strongly typed. Do not cast anything to string until you really need string.

These simple rules being applied to your code mean:

  1. Rules #1 and #3 mean that your methods should have following signature:

    • void GetImage() -> bool CompareCurrentImageWithPrevious()
    • bool CompareArrays() -> static bool CompareImageArrays(Color[,] imageArray1, Color[,] imageArray2)
    • void CaptureScreen() -> Bitmap CaptureScreen()
  2. This

    var x = CompareArrays();
    _valueX = x != true ? "false" : "true";
    

    Should be replaced with:

    return CompareArrays(); 
    
  3. This

    for (var i = 0; i < _memoryImage1.Width; i++)
    {
        for (var j = 0; j < _memoryImage1.Height; j++)
        {
            if (_first == null) continue;
            if (_second != null)
                if (_first[i,j] != _second[i,j])
                {
                    return false;
                }
        }
    }
    return _first != null && _second != null;
    

    Should be:

     if (_first == null || _second == null) return false;
     if (_memoryImage1.Width != _memoryImage2.Width ||
         _memoryImage1.Height != _memoryImage2.Height)
         return false;
     return _first.SequenceEquals(_second);
    
  4. Do not assign result of GetImage() to class field, DoWorkEventArgs have Result property which can be used to return value computed by background worker.

    private void BackgroundWorker1DoWork(object sender, DoWorkEventArgs e)
    {
        e.Result = GetImage();
        Thread.Sleep(100);
    }
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thank you very mutch, it was realy helpful :) I will implement the changes and start using the ways you describe in future projects. +1 for helping in making me a better developer. \$\endgroup\$ – Darkmage Jun 22 '11 at 12:00

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.