2
\$\begingroup\$

Here is my TCP socket that sends eight images per second to a connected Client.
The images are taken from a thermal camera.

static void HandleServer()
        {
            Console.WriteLine("Server is starting...");
            byte[] data = new byte[1024];
            IPEndPoint ipep = new IPEndPoint(IPAddress.Any, 9050);
        Socket newsock = new Socket(AddressFamily.InterNetwork,
                        SocketType.Stream, ProtocolType.Tcp);
            newsock.Bind(ipep);
            newsock.Listen(10);
            Console.WriteLine("Waiting for a client...");

            Socket client = newsock.Accept();
            IPEndPoint newclient = (IPEndPoint)client.RemoteEndPoint;
            Console.WriteLine("Connected with {0} at port {1}",
                            newclient.Address, newclient.Port);
        HandleImage(client, newsock);

    }


    private static void HandleImage(Socket client, Socket s)
    {
        int sent;
        int imageCount = 0;
        double totalSize = 0;
        try
        {
            while (true)
            {

                Image bmp = getImage();
                MemoryStream ms = new MemoryStream();
                // Save to memory using the bmp format
                bmp.Save(ms, System.Drawing.Imaging.ImageFormat.Jpeg);

                byte[] bmpBytes = ms.ToArray();
                bmp.Dispose();
                ms.Close();

                sent = SendVarData(client, bmpBytes);
                imageCount++;
                totalSize += sent;
                Console.WriteLine("Sent " + sent + " bytes");
                Thread.Sleep(125);

            }
        }
        catch (Exception e)
        {
            Console.WriteLine("Exception catched, probably an interrupted connection. Restarting server...");
            s.Close();
            Console.WriteLine("Transferred {0} images with a total size of {1}", imageCount, totalSize);
            Console.WriteLine("Average JPEG size: " + totalSize / imageCount + " byte");
            HandleServer();
        }
    }
    private static int SendVarData(Socket s, byte[] data)
    {
        int total = 0;
        int size = data.Length;
        int dataleft = size;
        int sent;

        byte[] datasize = new byte[4];
        datasize = BitConverter.GetBytes(size);
        sent = s.Send(datasize);

        while (total < size)
        {
            sent = s.Send(data, total, dataleft, SocketFlags.None);
            total += sent;
            dataleft -= sent;
        }
        return total;
    }
}
}

Any feedback is appreciated:)

Update: getImage simply calls the driver DLL of the thermal camera and returns the actual Image. I do this every 125ms because the camera is specified with 8fps and does NOT provide a method or property to check whether there is a new image available.

\$\endgroup\$
3
  • \$\begingroup\$ I highly recommend reading up on the relationship between IDisposable-implementing objects (such as Image, MemoryStream, and Socket) and the using statement. \$\endgroup\$ Mar 19 '19 at 16:34
  • \$\begingroup\$ @Jesse thanks a lot for your feedback. So this is a Kind of try with ressources like in Java? \$\endgroup\$
    – ItFreak
    Mar 19 '19 at 16:39
  • \$\begingroup\$ I'm not terribly familiar with Java, but based on the way you phrased it, I would suspect so. \$\endgroup\$ Mar 19 '19 at 17:18
7
\$\begingroup\$

This isn't quite true:

sends eight images per second

The 125ms sleep means that your code will sit around blocking a thread for about ~1/8th of a second (Thread.Sleep shouldn't be use for accurate timing ever) after it sends every image. This means that the time between images is ~125ms plus the time to actually send an image (i.e. no more than ~8 images per second, could be much fewer). This pretty much guarantees that you will drop a frame now and then.

You say that getImage() may or may not be providing a new image, and there is no nice way to check if a new image is available; however, you might consider interrogating the image received and checking if it is the same the one you last sent (and then don't send it); you can do this cheaply enough by taking a hash of the data stream. If getImage() itself isn't too expensive, then this would enable a quicker response rate, and ensure 8fps, rather than the 7-or-8 fps that a method like this will give you (assuming sending the image data is reasonably fast). If this makes sense will depend on the resource available, and how much you care about receiving every frame.

It would also be nice if you could avoid locking up a whole thread, as that sort of methodology does not scale (you run out of threads). An asynchronous HandleServer/HandleImage combination which makes use of Task.Delay() to schedule work might be a good solution, and would not significantly increase the complexity of the implementation.


Don't leave comments to rot:

// Save to memory using the bmp format
bmp.Save(ms, System.Drawing.Imaging.ImageFormat.Jpeg);

Such a comment is probably not worth the space it consumes, and it's unhelpful when it is out of date like this.


Calling HandleServer() within HandleImage() means that every exception will consume a little more of the stack frame, until eventually the program crashes. You might consider putting the logic for handling exceptions outside of both of these methods, and putting it in a loop; for example, something like:

while (keepRunning)
{
    try
    {
        HandleServer();
    }
    catch (Exception ex)
    {
        // do whatever
    }        
}

Not only will this save slowly filling the stack, but it means that HandleImage can focus on handling images and not restarting the server when it goes wrong.

The difficulty with this is that you currently print some information when catching an exception, to which this alternative does not lend itself. (you could catch the expcetion, print everything out, and then throw it again: this way HandleImage is still only concerned with sending lots of images, and the code above it takes the role of deciding how to deal with other failures (e.g. network errors)).

You include the exception in your catch clause, but you don't report anything about it. It would be much better to print out the exception rather than the assumption that it is a network error: that way, when something goes wrong in production there is some hope of debugging it.


This code needlessly allocates an extra byte[].

byte[] datasize = new byte[4];
datasize = BitConverter.GetBytes(size);

It can be simplified to this:

byte[] dataSize = BitConverter.GetBytes(size);

Your code to send the data to the server is suspect: when sending the size-data, you don't check the return value, but you have a corresponding loop to send the image data. I'm not sure how socket and TCP get along, but if you don't trust data to send in one go, you should be giving sizeData the same treatment (you can put the loop in it's own method to keep it simple).


Consider using the using statement (as suggested by Jesse C. Slicer in the comments) to ensure proper disposal of disposables. For example, in HandleImage:

byte[] bmpBytes;

using (Image bmp = getImage())
using (MemoryStream ms = new MemoryStream())
{
    bmp.Save(ms, System.Drawing.Imaging.ImageFormat.Jpeg);
    bmpBytes = ms.ToArray();
}

This will dispose the resources in the event that an exception if thrown in there somehow, and moves the released resources into a different scope, which means they can't be misused as easily (you can't forget to dispose them, and you can't use once when are disposed).


It's generally a good idea to define variables as close to their usage as possible. sent in HandleImage, for example, is declared on the first line of the method, but only used inside the while loop; it's purpose would be clearer if it was defined and assigned on the same line, and it would be impossible to use outside of its meaningful scope. total in SendVarData is also initialised first thing, but has no role in first piece of logic in the method.


These methods would really benefit from some inline-documentation (///) explaining what their role and behaviour is: I would not assume a method called HandleImage() could ever start a TCP server, so this needs to be written down somewhere (though ideally that would be changed, either as suggested above or through some other refactoring).


Small things:

  • Your 'bytes sent' reporting doesn't include the 4 bytes sent for the length-prefix.

  • The past-participle of 'catch' is 'caught', rather than 'catched'.

  • The variable data in HandleServer is never used.

  • Be consistent with your use of white-space (e.g. the gaps between methods, and indentation in HandleServer, etc.)

  • You might consider reusing the memory stream; we don't know how large the files are, but taking a new memory stream for each image may ruin your program's memory characteristics, and will necessarily degrade performance to some extent.

  • static void HandleServer() is missing an accessibility modifier (which are otherwise present)

  • double totalSize = 0; should not be a double (64bit float-point); did you mean long (64-bit integer)?

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot! Nice points which I will take care of. The Code ist just for a Mini PC running on a multicopter to send the images, so you know a bot about the background. But thank you for your very helpful answer! \$\endgroup\$
    – ItFreak
    Mar 19 '19 at 20:15

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.