Use of unique_ptr for class members

Question

I'm implementing a C++ program to read images from a TGA file. I got a struct for the header.

struct TGA_HEADER
{
// length of id string
char    id_length;

// image storage info
char    colour_map_type;
char    image_type;

// colour Map
short   first_entry;
short   num_entries;
char    bits_per_entry;

// image description
short   x_origin;
short   y_origin;
short   width;
short   height;
char    bits_per_pixel;
char    descriptor;
};

The image class looks like this:

class image
{
private:

public:
TGA_HEADER header;
std::unique_ptr> pixel_data;

image(const std::string& image_path);   
~image();

static void save_image(const std::string& file_name, image& image);
static void load_image(const std::string& path, const std::unique_ptr>& pixel_data, TGA_HEADER& header);
};

The constructor for my image class:

image::image(const std::string& image_path) 
: 
pixel_data(new std::vector)
{
load_image(image_path, this->pixel_data, this->header);
}

And my loading class:

void image::load_image(const std::string& path, const std::unique_ptr>& pixel_data, TGA_HEADER& header)
{
std::ifstream file_stream(path, std::ios::in | std::ios::binary | std::ios::ate);

if (file_stream.is_open())
{
    file_stream.seekg(0, std::ios::beg);

    int tgaDesc = 0;

    /* read header */
    file_stream.read(&header.id_length, sizeof(header.id_length));
    file_stream.read(&header.colour_map_type, sizeof(header.colour_map_type));
    file_stream.read(&header.image_type, sizeof(header.image_type));

    file_stream.read((char*)(&header.first_entry), sizeof(header.first_entry));
    file_stream.read((char*)(&header.num_entries), sizeof(header.num_entries));
    file_stream.read(&header.bits_per_entry, sizeof(header.bits_per_entry));

    file_stream.read((char*)(&header.x_origin), sizeof(header.x_origin));
    file_stream.read((char*)(&header.y_origin), sizeof(header.y_origin));
    file_stream.read((char*)(&header.width), sizeof(header.width));
    file_stream.read((char*)(&header.height), sizeof(header.height));
    file_stream.read(&header.bits_per_pixel, sizeof(header.bits_per_pixel));
    file_stream.read(&header.descriptor, sizeof(header.descriptor));

    // Skip the ID String
    char* skip = new char[256];
    file_stream.read(skip, header.id_length);

    // Skip the colour map if it doesn't exist
    if (!(tgaDesc & 4))
    {
        int colourMapSize = header.colour_map_type * header.num_entries;
        file_stream.read(skip, colourMapSize);
    }

    delete skip;

    int imageDataSize = header.width * header.height * (header.bits_per_pixel / 8);
    pixel_data->resize(imageDataSize);

    int originalPosition = (int)file_stream.tellg();

    /* read image data */
    file_stream.read(pixel_data->data(), imageDataSize);
}

My first question is, if I'm using the unique_ptr for my pixel_data in a correct way, especially in the constructor, and my second question would be if I initalize the TGA_HEADER object correctly respectively if I have to delete it manually or use smart pointers.

I'm quite new to C++, so feel free to comment on other issues you might come across.

Best regards

Miles Budnek · Accepted Answer

There is nothing technically wrong with your use of std::unique_ptr here, but it's totally unnecessary in this situation. std::vector is already essentially a unique pointer to an underlying array. Instead of having your class have a std::unique_ptr> member it should likely just have a std::vector member.

I would also question why load_image and save_image are static if they need references/pointers to the class's members (or the object itself, in the case of save_image). It seems like it would make much more sense for them to be non-static member functions. That way they would implicitly have access to the members of the object on which they're called.

There's also a potential memory leak in your load_image function. You dynamically allocate the array pointed to by skip. If any of the read operations between there and when you delete skip were to throw an exception you would leak that array. 256 bytes may also not be enough to read the entire color map. You should probably use a std::vector. Better yet, rather than reading the values you don't want, you could just seek past them:

// Skip the ID String
file_stream.seekg(header.id_length, std::ios::cur);

// Skip the colour map if it doesn't exist
if (!(tgaDesc & 4)) {
    int colourMapSize = header.colour_map_type * header.num_entries;
    file_stream.seekg(colourMapSize, std::ios::cur);
}

Writing that example made me notice that tgaDesc is always 0, so that if block will always run. Did you mean to check header.colour_map_type here? Of course, if there's no color map then header.num_entries should be 0, so I'm not sure the if is even needed at all.

While we're in load_image, you pass the std::ios::ate flag when opening file_stream but then immediately seekg back to the beginning of the file. If you remove the std::ios::ate flag then the stream will initially be positioned at the beginning of the file and the extra seekg could be eliminated.

The way you read the file header is mostly fine. Byte ordering (AKA endianness) could be a possible issue, but both TGA and most modern CPUs use little-endian byte order so you're probably fine unless you want to support some esoteric platform.

Putting that all together would give you an image class that looks something like this:

class image {
public:
    TGA_HEADER header;
    std::vector pixel_data;

    image(const std::string& image_path);
    ~image();

    void save_image(const std::string& file_name);
    void load_image(const std::string& path);
};

image::image(const std::string& image_path)
{
    load_image(image_path);
}

void image::load_image(const std::string& path)
{
    std::ifstream file_stream(path, std::ios::in | std::ios::binary);

    if (file_stream.is_open()) {
        int tgaDesc = 0;

        /* read header */
        file_stream.read(&header.id_length, sizeof(header.id_length));
        file_stream.read(&header.colour_map_type, sizeof(header.colour_map_type));
        file_stream.read(&header.image_type, sizeof(header.image_type));

        file_stream.read((char*)(&header.first_entry), sizeof(header.first_entry));
        file_stream.read((char*)(&header.num_entries), sizeof(header.num_entries));
        file_stream.read(&header.bits_per_entry, sizeof(header.bits_per_entry));

        file_stream.read((char*)(&header.x_origin), sizeof(header.x_origin));
        file_stream.read((char*)(&header.y_origin), sizeof(header.y_origin));
        file_stream.read((char*)(&header.width), sizeof(header.width));
        file_stream.read((char*)(&header.height), sizeof(header.height));
        file_stream.read(&header.bits_per_pixel, sizeof(header.bits_per_pixel));
        file_stream.read(&header.descriptor, sizeof(header.descriptor));

        // Skip the ID String
        file_stream.seekg(header.id_length, std::ios::cur);

        // Skip the colour map if it doesn't exist
        if (!(tgaDesc & 4)) {
            int colourMapSize = header.colour_map_type * header.num_entries;
            file_stream.seekg(colourMapSize, std::ios::cur);
        }


        int imageDataSize = header.width * header.height * (header.bits_per_pixel / 8);
        pixel_data.resize(imageDataSize);

        /* read image data */
        file_stream.read(pixel_data.data(), imageDataSize);
    }
}

Use of unique_ptr for class members

Answers (2)

Related Questions