5
\$\begingroup\$

I have a byte array and need to fill an image data struct using it. The below method is written to fill the structure.

In the code, char *sfimageData is the byte array received from a USB camera. I need to generate this C++ struct. The method is working properly but I need to review this.

typedef struct image_struct
    {
        // Header
        uint32_t id;
        uint32_t timestamp;
        uint16_t nHeight;
        uint16_t nWidth;
        // Buffer
        float *depth_map;
        uint16_t *amplitude_map;

        uint16_t getWidth(){ return nWidth;};
        uint16_t getHeight(){return nHeight;};

    } IMAGE_STRUCT;

This method is working to fill the struct member:

void atImageEvent( short eventType, int size, short imageFormat, char* sfimageData, int dataLength, std::string& pScannerData )
{    
    std::string image_format;
    switch ( imageFormat )
    {
        case JPEG_FILE_SECTION:
            image_format = ".jpg";
            break;
        case TIFF_FILE_SECTION:
            image_format = ".tiff";
            break;
        case BMP_FILE_SECTION:
            image_format = ".bmp";
            break;
        case RAW_FILE_SELECTION:
            image_format = ".deb";
            break;
        case RAW_PROCESSED_FILE_SELECTION:
            image_format = ".dpf";
            break;
        case DEPTH_FRAME_FILE_SELECTION:
            image_format = ".dpf";
            break;
        default:
            cout << "Different type of image format, but saving as .dpf." << endl;
            image_format = ".dpf";
            break;
    } 

    string sFileName;
    stringstream ss;
    ss << "imagedata";
    ss << this->iImageFileNumber++; 
    ss << image_format;
    sFileName = ss.str();


    // cast image data into depth frame buffer.
    IMAGE_STRUCT output_image_struct;


    // define lengths for image struct
    int dataIndex=0;
    int idLength = sizeof(uint32_t);
    int tstampLength = sizeof(uint32_t);
    int heightLength = sizeof(uint16_t);
    int widthLength = sizeof(uint16_t);


    uint8_t *id, *timeStamp, *imgHeight, *imgWidth, *depthFrameData, *amplitudeData;

    id = (uint8_t *)malloc(sizeof(uint32_t));
    memset(id, 0x00, idLength);
    dataIndex = idLength;
    memcpy(id, sfimageData, sizeof(uint32_t));

    union
    {
        uint32_t _id;
        uint8_t *_byteArray;
    }uId;
    memcpy(&uId._byteArray, id, sizeof(uint32_t));
    free(id);

    output_image_struct.id = uId._id;

    timeStamp = (uint8_t *)malloc(sizeof(uint32_t));
    memset(timeStamp, 0x00, tstampLength);
    memcpy(timeStamp, sfimageData+dataIndex, sizeof(uint32_t));
    dataIndex += tstampLength;

    union
    {
        uint32_t _timeStamp;
        uint8_t *_byteArray;
    }uTimeStamp;
    memcpy(&uTimeStamp._byteArray, timeStamp, tstampLength);
    free(timeStamp);
    output_image_struct.timestamp = uTimeStamp._timeStamp;

    imgHeight = (uint8_t *)malloc(heightLength);
    memcpy(imgHeight, sfimageData+dataIndex, sizeof(uint16_t));
    dataIndex += heightLength;

    union
    {
        uint16_t _imgHeight;
        uint8_t *_byteArray;
    }uImgHeight;
    memcpy(&uImgHeight._byteArray, imgHeight, heightLength);
    free(imgHeight);   
    output_image_struct.nHeight = uImgHeight._imgHeight;

    imgWidth = (uint8_t *)malloc(widthLength);
    memset(imgWidth, 0x00, widthLength);
    memcpy(imgWidth, sfimageData+dataIndex, sizeof(uint16_t));
    dataIndex += widthLength;

    union
    {
        uint32_t _imgWidth;
        uint8_t *_byteArray;
    }uImgWidth;

    memcpy(&uImgWidth._byteArray, imgWidth, widthLength);
    free(imgWidth);
    output_image_struct.nWidth = uImgWidth._imgWidth;

    int depthFrameLength =  output_depth_frame.nHeight * output_depth_frame.nWidth * sizeof(float);
    int amplitudeMapLength = output_depth_frame.nHeight * output_depth_frame.nWidth * sizeof(uint16_t);

    depthFrameData = (uint8_t *)malloc(depthFrameLength);
    memset(depthFrameData, 0x00, depthFrameLength);
    memcpy(depthFrameData, sfimageData+dataIndex, depthFrameLength);
    dataIndex += depthFrameLength;

    union 
    {
        uint8_t *byteArray;
        float *floatArray;
    } uDepthFrame;

    uDepthFrame.byteArray = depthFrameData;

    output_image_struct.depth_map = uDepthFrame.floatArray;

    amplitudeData = (uint8_t *)malloc(amplitudeMapLength);
    memset(amplitudeData, 0x00, amplitudeMapLength);
    memcpy(amplitudeData, sfimageData + dataIndex, amplitudeMapLength);
    dataIndex += amplitudeMapLength;

    union 
    {
        uint8_t *byteArray;
        uint16_t *uint16Array;
    } uAmplitude;

    uAmplitude.byteArray = amplitudeData;

    output_image_struct.amplitude_map = uAmplitude.uint16Array;

    int middleIndex = (output_image_struct.getHeight()+1)*output_image_struct.getWidth()/2;

    // Print output_image_struct (primitive members)
    cout << "-----------------------------------------------------" << endl;
    cout << "ID:            " << output_image_struct.id << endl;
    cout << "TimeStamp:     " << output_image_struct.timestamp << endl;
    cout << "Img Height:    " << output_image_struct.nHeight << endl;
    cout << "Img Width:     " << output_image_struct.nWidth << endl;  
    cout << "First Index:   " << 0 << "    value:  " << output_image_struct.depth_map[0] << endl;
    cout << "Middle Index:  " << middleIndex << "    value:  " << output_image_struct.depth_map[middleIndex] << endl;
    cout << "last Index:    " << (output_image_struct.getHeight() * output_image_struct.getWidth()) - 1 << "    value:  " << output_image_struct.depth_map[ (output_image_struct.getHeight() * output_image_struct.getWidth()) - 1 ] << endl;
    cout << "-----------------------------------------------------" << endl;

    // print data if interested
    for(int x=0; x<depthFrameLength/sizeof(float); x++)
    {
        cout << " " << output_depth_frame.depth_map[x];
    }

    for(int x=0; x<amplitudeMapLength/sizeof(uint16_t); x++)
    {
        cout << " " << output_image_struct.amplitude_map[x];
    }

    cout << "END ********************" << endl;    

    free(depthFrameData);
    free(amplitudeData);
}

Here, I have use many malloc and memcpy calls as well. Is it a bad practice when writing good code?

\$\endgroup\$
  • \$\begingroup\$ (If and when you edit your question (avoid trivial, make substantial edits), please correct the title and capitalise characters at the beginning of sentences.) \$\endgroup\$ – greybeard Dec 14 '17 at 13:56
  • \$\begingroup\$ Doesn't compile - you're missing at least one #include and a main() function. That's not ready for review. \$\endgroup\$ – Toby Speight Dec 14 '17 at 17:30
10
\$\begingroup\$

You are doing several things in this function. You determine a filename, which you then never use, you are decoding the data buffer into a structure, and printing the structure elements to stdout.

Since the name of the function indicates this is an event handler of some kind, I really don't have a sense of what you're "trying" to do.

My recommendation would be to decompose your existing large function into several smaller functions. In particular, here are some obvious candidates:

const char * fileExtensionOfFormat(img_fmt);   // JPEG_FILE_SECTION -> ".jpg"

typedef uint8_t byte;

IMAGE_STRUCT * decodeImageData(byte * img_data);

    uint16_t read_uint16(byte *);
    uint32_t read_uint32(byte *);
    float    read_float(byte *);

There are a couple of things to be aware of:

  1. When you are reformatting raw data from one device to another, be wary of Byte Ordering. Your code suggests this is not an issue - it may be that you simply aren't aware of if, or it may be that some lower-level library is handling this for you.

  2. When you are fetching single values, you don't need to allocate and free memory. Just examine the source memory area and return a single object. You can construct integer objects by fetching bytes, shifting them some multiple of 8 bits, and using a bitwise or to merge them together:

    uint16_t read_short_le(byte * p) { return *p | *(p + 1) << 8; }

    uint16_t read_short_be(byte * p) { return *p << 8 | *(p + 1); }

In cases where you know the endianness of the source data and target data is the same, you can sometimes just use a pointer cast:

uint16_t read_short(byte * p) {
    return *(uint16_t *)p;
}

But you have to be wary of alignment requirements: some CPUs cannot read misaligned data, or if they can perform the read they do it at a high performance cost. For this reason, most code is written to do bytewise assembly using shift and binary or, with the (frequently incorrect) expectation that the compiler will figure out and replace the operations if type punning is a viable alternative.

\$\endgroup\$
  • \$\begingroup\$ You are discussing the Single Responsibility Principle at the beginning of your answer, you might want to quote it directly and add this link en.wikipedia.org/wiki/Single_responsibility_principle. You might also want to give some examples of sub functions. \$\endgroup\$ – pacmaninbw Dec 14 '17 at 16:43
6
\$\begingroup\$

Use C++ not C

First of all, you are making a lot usage of C, not C++. In C++ you either use the keyword class or struct to declare a class/struct, not together with the keyword typedef.

You are using uint32_t a lot. This is also C style, the proper C++ style is the std::unit32_t type. But most of the times you don't really need to specify the width of your integer directly. This only makes sense if you want to serialize/deserialize them.

Use new and delete instead of malloc and free. Better: use smartpointers like std::unique_ptr and std::shared_ptr.

declare properties private (encapsulate them)

The problem here is, you are using public available objects inside of the IMAGE_STRUCT struct, which can be modified from the outside.

split-up your functions

Your atImageEvent function is huge! Split it up into smaller parts.

name conventions

There are no explicit name conventions at all, but you should not declare your classes/structs with only capital characters (like IMAGE_STRUCT). Most of the times words with only capital characters are macros. It's not a mistake rather than a bit of taste.

if I am not totally wrong names beginning with _ are reserved by the standard and should not be used for custom code. To be honest, I am using them by myself, but I think its worth to mention this.

undefined behaviour

union
{
    uint16_t _imgHeight;
    uint8_t *_byteArray;
}uImgHeight;
memcpy(&uImgHeight._byteArray, imgHeight, heightLength);
free(imgHeight);   
output_image_struct.nHeight = uImgHeight._imgHeight;

You are running into undefined behavior here, because you are reading from a union property without writing to it first. Unions should not be used to convert types to each other. It is not safe!

I am sure, the code doesn't do what you expect. Your code says, you are converting a uint8_t pointer to uint16_t integer. You are not accessing the data, just the address!


In the end I did not review the correctness of your code, I just stepped over the obviously mistakes you made in your code.

\$\endgroup\$
3
\$\begingroup\$

First of all, please clarify what language you're programming in. You tagged the question both C and C++. The code has elements of both. I'll be commenting from the C angle since the code feels like a C function that was refactored to use cout <<, but the comments should apply in either case.

You make a few repeated mistakes throughout the function. I'll use this block as a representative sample and discuss individual issues in the order that I found them:

id = (uint8_t *)malloc(sizeof(uint32_t));      // #1
memset(id, 0x00, idLength);                    // #2
dataIndex = idLength;
memcpy(id, sfimageData, sizeof(uint32_t));     // #3

union
{
    uint32_t _id;
    uint8_t *_byteArray;
}uId;
memcpy(&uId._byteArray, id, sizeof(uint32_t)); // #4
free(id);

output_image_struct.id = uId._id;              // #5

#1 Dynamically allocating something as small as a 32-bit integer is frequently pointless. The pointer to this new buffer is likely at least 32 bits long. Just declare a 32-bit stack variable and use that instead (bonus perk: no cleanup required).

#2 When you're allocating a buffer and initializing it with memset, make sure you use the exact same size parameters for both function calls. Here, you allocate sizeof(uint32_t) bytes, but you're writing zeroes to idLength bytes. There's nothing forcing those two things to always be equal. Use sizeof(uint32_t) in both places and you don't have to worry about memory corruption due to a variable getting changed.

#3 Why zero out the variable at all? The next thing you do is to memcpy different data into it. The first memset is unnecessary here.

#4 So you've already copied four bytes of sfimageData into buffer id, and now you're copying it into uId._byteArray. Why the additional copy steps?

#5 Okay, now this is the third time you've copied this one piece of data. Just copy it directly here from the beginning. You can replace this entire code block with memcpy(&output_image_struct.id, sfimageData, sizeof(uint32_t)).

Your function has a series of code blocks that all follow this same format, and all seem to suffer from the same problems.

General Comments

Your code makes unsafe assumptions about the way that unions are implemented. The only union member that you can legally read from is the last one that you wrote to. Writing to one member and reading from another is undefined behavior and you can't rely on that code to behave a certain way. You're likely doing that to avoid doing shady things with pointer casting like id = *(uint32_t*)(sfimageData), but the behavior of both techniques are equally undefined. If you eliminate the redundant copy operations, this problem should go away as well.

It appears that the fields in the structure are listed in the same order that they appear in the data buffer. Use this to your advantage and simplify the bulk of your function to:

#pragma pack(push,1)  // structure packing syntax may vary
struct image_struct_data
{
    uint32_t id;
    uint32_t timestamp;
    uint16_t nHeight;
    uint16_t nWidth;
};
#pragma pack(pop)
struct image_struct
{
    image_struct_data data;
    // Buffers
    float *depth_map;
    uint16_t *amplitude_map;
    uint16_t getWidth() {return data.nWidth;};
    uint16_t getHeight() {return data.nHeight;};
};
...
image_struct_data output_image_struct;
memcpy(&output_image_struct, sfimageData, sizeof(output_image_struct));
sfimageData += sizeof(output_image_struct);

int depthFrameLength =  output_image_struct.nHeight * output_image_struct.nWidth * sizeof(float);
int amplitudeMapLength = output_image_struct.nHeight * output_image_struct.nWidth * sizeof(uint16_t);
output_image_struct.depth_map = malloc(depthFrameLength);
memcpy(output_image_struct.depth_map, sfimageData, depthFrameLength);
sfimageData += depthFrameLength;
output_image_struct.amplitude_map = malloc(amplitudeMapLength);
memcpy(output_image_struct.amplitude_map, sfimageData, amplitudeMapLength);
sfimageData += amplitudeMapLength;
...

Instead of manipulating fields individually, you're directly copying the first sizeof(struct image_data) bytes from the input buffer into a local variable, which is essentially what your current code does (but in a very indirect way). From here, all you have left to do is allocate the two variable-length buffers.

Your function takes an arbitrary-sized data buffer for input. You've forced the caller to specify the size of the buffer via dataLength, which is excellent. However, your function never so much as looks at this parameter. That makes it very easy for this function to make an invalid memory access and corrupt memory or crash your program. You need to ensure you never attempt to access beyond offset dataLength into the input buffer. How exactly to handle this is up to you. You can error out of the function if dataLength is less than what you expect, you can read up to dataLength bytes and try using default values for any remaining fields, etc etc.

Also, don't forget that malloc can return NULL. Every time you call malloc, you must check the return value before you try to use it.

\$\endgroup\$
  • \$\begingroup\$ thanks for the good code review, I am reading your post and have a question about "output_image_struct.depth_map = malloc(depthFrameLength);" depth_map is not a member of image_struct_data structure but you have directly using it for malloc. Will it work ? I am not clear there. \$\endgroup\$ – Dig The Code Dec 18 '17 at 6:49
  • \$\begingroup\$ The depth_map pointer is part of struct image_struct. All this code is doing is allocating a buffer and storing that buffer's address into that pointer. The buffer itself is not part of the structure, only a pointer to it. \$\endgroup\$ – bta Dec 18 '17 at 19:18

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.