5
\$\begingroup\$

I wrote this code to get familiar with structs in C. It allows the creation and exporting of a 24 bit TGA image and changing the color of individual pixels.

My concern is the setPixel function, and how I could improve its performance.

First the conversion from x,y to an array index, how can I minimize/eliminate the need for this? I think I could just use a two dimensional array.

Second, is the setting of the pixel's RGB values. How could this be optimized? Would a single assignment for all three at once improve it? How would I do this?

Lastly, I was wondering if there's anyway to make makeImage and saveImage less verbose. I wasn't able to find a better solution.

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

#define HEADER_BYTES 18 // Ammount of bytes header should take up

typedef struct TGAImg_t{
    uint8_t idLength;           // Length of image ID field (0-255)
    uint8_t colorMapType;       // If a color map is present (1) or not (0)
    uint8_t imageType;          // Compression and color types (0-3 and 9-11)
    uint16_t colorMapOrigin;    // Index of first color map entry
    uint16_t colorMapLength;    // Count of color map entries
    uint8_t colorMapEntrySize;  // Number of bits in each color map entry. 16 for Targa 16, 24 for Targa 24...
    uint16_t xOrigin;           // X coord of the lower left corner of the image
    uint16_t yOrigin;           // Y coord of the lower left corner of the image
    uint16_t width;             // Width of the image in pixels
    uint16_t height;            // Height of the image in pixels
    uint8_t imagePixelSize;     // Number of bits in a stored pixel index
    uint8_t imageDescriptorByte;// Document says to just keep this byte as 0
    uint8_t imageDataField[0];  // Array of image pixels.
} TGAImg;

typedef struct RGB_t{
    uint8_t red;
    uint8_t green;
    uint8_t blue;
} RGB;

TGAImg* makeImage(uint8_t idLength, uint8_t colorMapType,
                        uint8_t imageType, uint16_t colorMapOrigin,
                        uint16_t colorMapLength, uint8_t colorMapEntrySize,
                        uint16_t xOrigin, uint16_t yOrigin,
                        uint16_t width, uint16_t height,
                        uint8_t imagePixelSize, uint8_t imageDescriptorByte){
    // Allocate memory for header + image pixels (using info from header params)
    uint32_t dataFieldBytes = width * height * (imagePixelSize / 8);
    TGAImg* img = calloc(HEADER_BYTES + dataFieldBytes, 1 );
    if (!img){
        printf("ERROR: calloc fail for img @ makeImg");
        exit(EXIT_FAILURE);
    }
    // Set header values
    img->idLength = idLength;
    img->colorMapType = colorMapType;
    img->imageType = imageType;
    img->colorMapOrigin = colorMapOrigin;
    img->colorMapLength = colorMapLength;
    img->colorMapEntrySize = colorMapEntrySize;
    img->xOrigin = xOrigin;
    img->yOrigin = yOrigin;
    img->width = width;
    img->height = height;
    img->imagePixelSize = imagePixelSize;
    img->imageDescriptorByte = imageDescriptorByte;
    return img;
}

// Writes contents of a TGAImg struct to a image of name imageName
void saveImage(char imageName[], TGAImg* img){
    FILE* imageFile = fopen(imageName, "wb");

    fwrite(&img->idLength, sizeof(img->idLength), 1, imageFile);
    fwrite(&img->colorMapType, sizeof(img->colorMapType), 1, imageFile);
    fwrite(&img->imageType, sizeof(img->imageType), 1, imageFile);
    fwrite(&img->colorMapOrigin, sizeof(img->colorMapOrigin), 1, imageFile);
    fwrite(&img->colorMapLength, sizeof(img->colorMapLength), 1, imageFile);
    fwrite(&img->colorMapEntrySize, sizeof(img->colorMapEntrySize), 1, imageFile);
    fwrite(&img->xOrigin, sizeof(img->xOrigin), 1, imageFile);
    fwrite(&img->yOrigin, sizeof(img->yOrigin), 1, imageFile);
    fwrite(&img->width, sizeof(img->width), 1, imageFile);
    fwrite(&img->height, sizeof(img->height), 1, imageFile);
    fwrite(&img->imagePixelSize, sizeof(img->imagePixelSize), 1, imageFile);
    fwrite(&img->imageDescriptorByte, sizeof(img->imageDescriptorByte), 1, imageFile);
    fwrite(&img->imageDataField, img->width * img->height * (img->imagePixelSize / 8), 1, imageFile);

    fclose(imageFile);
}

void setPixel(TGAImg* img, RGB color, int x, int y){
    uint32_t index = ((y * img->width) + x) * 3; // Convert x and y to index for array
    // Apply pixel change to all colors
    img->imageDataField[index] = color.blue;
    img->imageDataField[index+1] = color.green;
    img->imageDataField[index+2] = color.red;
}
\$\endgroup\$
1
  • \$\begingroup\$ “I think I could just use a two dimensional array.” That would be much less efficient. I’ve written answers in this site explaining why, I suggest you search for those! \$\endgroup\$ Commented Mar 23, 2022 at 3:07

3 Answers 3

3
\$\begingroup\$

Performance

Row vs. Pixel

In addition to the slow setPixel(), offer a setLine() that offers reduced repeated calculations in setting many pixels of the same color.

Block vs. Pixel

In addition to the slow setPixel(), offer a copyBlock() that offers reduced repeated calculations in setting a rectangle of pixels. Employ memcpy() when possible.


Portability

File vs. Code structures

TGAImg is app to contain implementation specific padding and the TARGA file have a fixed (likely no padding) definition.

This is easy to in C code if a some of extended language qualifier like packed is allowed.

// typedef struct TGAImg_t{
typedef struct packed TGAImg_t{  // example

If no such language extension is available, it is work to write portable code. For the most part, code needs to reads the file header as an array of bytes and then form TGAImg one member at a time - as OP did with writing saveImage().

Endian

Byte order may differ form file format to code. Various endian functions exist.

// fwrite(&img->colorMapOrigin, sizeof(img->colorMapOrigin), 1, imageFile);
uint16_t u16 = endian_host_to_file16(img->colorMapOrigin);
fwrite(&u16, sizeof u16, 1, imageFile);

Pedantic: int may be 16-bit.

Various code needs to account for that.

// void setPixel(TGAImg* img, RGB color, int x, int y){
void setPixel(TGAImg* img, RGB color, int_fast32_t, x, int_fast32_t y){

Lastly, I was wondering if there's anyway to make makeImage and saveImage less verbose. I wasn't able to find a better solution.

Unclear why code does not simple use and let the caller fill in most members.

TGAImg *makeImage(const TGAImg *init_values)

Bug?

Trouble when imagePixelSize is not a multiple of 8. Pixel depth 15 allowed.

// uint32_t dataFieldBytes = width * height * (imagePixelSize / 8);
// Possible solution - needs review.
uint32_t dataFieldBytes = width * height * ((imagePixelSize + 7) / 8);
\$\endgroup\$
2
\$\begingroup\$

My concern is the setPixel function, and how I could improve its performance.

I don't think you can do much, there are some things but they're small. You could try flipping the definition of that RGB struct around (blue first) and then memcpy'ing the pixel, but 3 bytes is not a Nice Size (I tried this and GCC liked to implement such a 3-byte memcpy as 2 stores on x64, which is something, not amazing though). With the function being what it is, the arithmetic is inevitable, but you can improve it slightly (and only in some specific contexts) by using uint32_t for x and y, at least GCC 11.2 emitted a useless cdqe sometimes (although I don't quite understand when or why, as it doesn't happen normally and seems entirely redundant given that index is uint32_t).

Looking at a bigger picture, a "bulk API" changes the scenario considerably (they often do, which is why they exist), a function to write/copy a contiguous region of pixels could:

  • Avoid pixel-by-pixel calculations of index.
  • Use bigger memcpys to copy horizontal lines, if the source has the same format. Even rectangle-fill could use that, by filling one line first and then copying it. That's not as crazy it sounds, memcpy can be very efficient, it's difficult to reach that level of speed normally (you can though).
  • Somehow (might require padding the RGB struct out with a dummy byte) use memcpy with a size of 4, hopefully resulting in an unaligned 32-bit store on platforms on which that is better than 3 individual byte stores (eg x64), while not forcing an unaligned store on platforms on which that is bad. Writing the extra byte "outside" the current pixel doesn't matter if you know you're going to overwrite it in the next iteration, which is something that a single-pixel API couldn't know, but a bulk API can.
  • Use a fancy SIMD-shuffle based copy if the source has a contiguous format that's different from packed RGB, or even a planar format. Similarly, SIMD could be used to fill regions quickly. That's not -friendly at all though.
  • Add bounds checking (or prevent out-of-bounds access by clipping, if you prefer) at a reasonable cost, because the cost would be amortized over a bunch of pixels.

And probably more, bulk APIs create a lot of flexibility and opportunity to implement tricks and abstract them away, unlike single-action APIs. Naturally the kind of API that you want is up to you, though.

Lastly, I was wondering if there's anyway to make makeImage and saveImage less verbose.

It's a limited-portability solution (the Portability Police may not even want me to tell you this), but if your image struct has exactly the same field order / types as the file format and it is marked #pragma pack(1) (not fully portable, but widely implemented) (unfortunately TGA does not have naturally aligned header fields), then you can fwrite the whole thing in one go. TGA has little-endian fields, which also happens to be the most popular endianness for present-day processors, so you can target a lot of hardware with that. But you would be targeting some set of hardware, it's not a general solution. Some RISC processors may not like accesses to the unaligned fields, but repeatedly accessing the fields is not a good thing anyway.

\$\endgroup\$
1
\$\begingroup\$

Instead of hard-coding the number of bytes the 'header' of the structure should take up, you can just use sizeof() to get it. sizeof(), when applied to the structure, will return its size as if the zero-length array were not there.

Zero-length arrays are an obsolete GCC extension. Instead, you should use a flexible array member. You can do this by simply omitting the zero between the brackets.

Since you typedef your structures, you don't have to give them names. You can just leave them out, like this:

typedef struct {
    ...
} TGAImg;
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Unfortunately due to memory alignment (I'm pretty sure that's the term), sizeof() returns an improper value for TGAImg. Since the TGA format always has a header of 18 bytes, is that considered hard coded? Thanks for all the other advice. \$\endgroup\$
    – AidanTrent
    Commented Mar 22, 2022 at 0:04
  • 1
    \$\begingroup\$ I assume you mean the sizeof operator. sizeof and () are separate things. Note that sizeof *img will include any padding it contains, which is probably not appropriate for the file format. \$\endgroup\$ Commented Mar 22, 2022 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.