Sending & Receive Image Over TCP/IP

Question

I've created a program that can send & receive a screenshot. However, I feel like my method is a little bit...inefficient. I have to write to the stream twice, because from what I've observed, in order for the image to form properly, I must know the exact file size aka. the length. So, what I do is write the length to the the stream, than write the image to the stream. Is there a way to combine this into one step, or optimize it more?

Here's my code for the client

 else if (plainText.Contains("screenshot")) //check if received command is asking for screenshot
        {
            MemoryStream ms = new MemoryStream();
            Bitmap bitmap = new Bitmap(Screen.PrimaryScreen.Bounds.Width, Screen.PrimaryScreen.Bounds.Height);
            Graphics graphics = Graphics.FromImage(bitmap);
            graphics.CopyFromScreen(0, 0, 0, 0, bitmap.Size); 
            bitmap.Save(ms, System.Drawing.Imaging.ImageFormat.Jpeg);//save bitmap to MemoryStream so I can read length
            writeToStream(ms.ToArray().Length.ToString()); //Write the length to the NetWorkStream
            writeToStream(ms.ToArray()); //Write the actual image to the NetworkStream
            ms.Close(); //close the MemoryStream when done
        }

And for my server:

private void screenShotBTN_Click(object sender, EventArgs e)
    {
        screenShotBTN.Enabled = false;

        NetworkStream stream = getSelectedClient().GetStream();

        writeBuffer = Encoding.ASCII.GetBytes("screenshot"); 

        stream.Write(writeBuffer, 0, writeBuffer.Length); // send screenshot request

        int data = 0;

        byte[] readBuffer = new byte[getSelectedClient().ReceiveBufferSize];

        data = stream.Read(readBuffer, 0, readBuffer.Length);

        string pictureSize = Encoding.ASCII.GetString(readBuffer, 0, data);

        Console.WriteLine(pictureSize); //for debugging purposes so I can see the length

        string x = new Random().Next().ToString();

        FileStream f = new FileStream(x + ".bmp", FileMode.Create, FileAccess.Write);

        while (new FileInfo(x + ".bmp").Length != Convert.ToInt32(pictureSize))
        {
            data = stream.Read(readBuffer, 0, readBuffer.Length);
            f.Write(readBuffer, 0, data);
        }

        f.Close();

        Process.Start(x + ".bmp");

        screenShotBTN.Enabled = true;
    }

Answer 1

Bug

You are saving the screenshot bitmap as a Jpg, but on the server side you are saving the stream as file having a bitmap (bmp) extension.

Although this works with the default windows image viewer (Process.Start(x + ".bmp");) other applications will maybe throw an exception here.

Just save it with the jpg extension you you are on the safe side.

Answer 2

ms.Close(); //close the MemoryStream when done

It's a good thing that you close the ms (MemoryStream) but there are two more to dispose.

The bitmap and graphics instances also need to be disposed/closed. The same applies to the NetworkStream but I got the impression that you don't know the using statement that allows you to automatically handle resources so:

NetworkStream stream = getSelectedClient().GetStream();
..
stream.Close();

would become:

using(var networkStream = getSelectedClient().GetStream())
{
    ..
} // at this point it will be closed/disposed by the runtime

The benefit of this is that it will be correctly disposed even if an exception occurs. Without it you would need to write the try/finally yourself (what a using actually does for you (or rahter the compiler))

NetworkStream networkStream = null;

try
{
    networkStream = getSelectedClient().GetStream();
    ..
}
finally
{
    networkStream?.Close();
}

You can use the same pattern for bitmaps, graphics and anything else that is IDisposable.

Answer 3

You should call ToArray once:

var bytes = ms.ToArray();
writeToStream(bytes.Length);
writeToStream(bytes);

Note, that you probably don't want to deal with encoding when sending binary data. Avoid sending strings over TCP when you do not have to. In this case you can easily send bytes.Length as int (= 4 bytes).

Also if you are sending screenshots often (hopefully without a malicious intent ;( ), you probably want to re-use single MemoryStream instance between the calls, so it is not re-created for every request.

Finally, you can save some time by not calling ToArray at all. Instead you can write memory stream buffer directly:

networkStream.Write(ms.GetBuffer(), 0, ms.Position);

---

Methods should use PascalCase: writeToStream -> WriteToStream

---

    string x = new Random().Next().ToString();
    FileStream f = new FileStream(x + ".bmp", FileMode.Create, FileAccess.Write);

This is unsafe. Random =/= Unique. Random can easily generate a file name that is already in use. In which case your server will probably just crash.

Alternatively you can use the Path.GetRandomFileName Method which

[..] returns a cryptographically strong, random string that can be used as either a folder name or a file name.