JdeRobot / base

GNU General Public License v3.0
94 stars 124 forks source link

Add md5sum to image interface #39

Closed rocapal closed 9 years ago

rocapal commented 9 years ago

Add md5sum to image interface. The image server can add the md5sum of the image transmitted and the client can check if everything goes well after the send.

ojgarciab commented 9 years ago

I think that checksum validation is redundant.

It's a great idea for testing or debugging purposes, but it has a performance impact that maybe must be avoided.

ZeroC ICE check within a request the validate of entire process of dispatch the return message: https://doc.zeroc.com/display/Ice/Automatic+Retries

  1. What kind of error caused the request to fail?

Ice does not bother retrying a request if it knows the same error is going to occur again. For example, Ice never retries an invocation that raises a MarshalException, which indicates that there was a problem while encoding or decoding a message. Retrying such an invocation is unlikely to change the outcome.

[...]

In addition to user exceptions and subclasses of RequestFailedException, a server can also return an instance of UnknownException, UnknownLocalException, or UnknownUserException to indicate that it encountered an unexpected exception while dispatching the request. These exceptions are eligible for retry.

ICE will not deliver an incomplete request to a client (despite about "automatic retries" in title), it will throw several exceptions based on where the error was located and in a few cases there is the option of automatically retry it.

Headers allow ICE to detect incomplete answers: https://doc.zeroc.com/display/Ice/Overview+of+the+Ice+Protocol

But not integrity: that task is again redundant because TCP and UDP has it (both of them has CRC checks at IP header level and TCP/UDP payload) and most of data link layers too!: ethernet, WiFi (both of them uses the same FCS), ppp, ect has CRC checks in every packet sent (or a complete packet or sequence like AAL5 PDU in ATM).

If using "built-in compression" (based on bzip2) there is a 4 byte value about uncompressed size (to allow preallocation of buffers), used to verify correct uncompression too (it will throw a Ice::CompressionException exception it doesn't match or there is another error while uncompressing data):

The message body of a compressed request, batch request, or reply message is encoded by first writing the size of the uncompressed message (including its header) as a four-byte integer, followed by the compressed message body (excluding the header). It follows that the size of a compressed message is 14 bytes for the header, plus four bytes to record the size of the uncompressed message, plus the number of bytes occupied by the compressed message body. Writing the uncompressed message size prior to the body enables the receiver to allocate a buffer that is large enough to accomodate the uncompressed message body.

Best regards.

rocapal commented 9 years ago

You are right. The idea to add checksum in the structure is that could help in the client side to detect if you are processing twice the same image. If you use cameraClient this feature is very useful. Also It was added for debugging.

The impact is around 300 micro-seconds. Anyway, it could be a good improvement if checksum can be enable/disable from configuration file.

ojgarciab commented 9 years ago

Ahh, parellelice has this problem, I thought about that issue some time ago when was proposed in developer list.

In that case is a good idea, checking md5 or checking timestamp (that should be the same) to avoid process same frame.

I thought it would be mandatory for all Image servers, it could take down my lightweight implementation of a pure video4linux cameraserver without opencv dependence (and overload).

Perhaps it should be used only with implementations that are sensitive to that problem.

What will the behavior of the client? will discard frames based on same md5 or reject them if md5 sum is incorrect?

A quick workarround (if only test md5 to avoid repeated frames and not to check data consistency) would be a incremental counter as md5 field until I implement md5 hash in androidcameraserver (now on github) and v4lserver (not yet in github).

Best regards.