5
\$\begingroup\$

I have a C library written by a third-party company. It calculates photo positions on a book page, position of a photo inside an area (which part of the photo to crop), it fills a book with random photos...
I have created a .Net wrapper around the C library. I'm very new to C++ so I don't know the conventions (naming, ordering...).

The idea for the .Net wrapper is to create a "usable" library, not to bind the methods one-to-one. For instance, from a C# perspective, it seems stupid to send the size of an array to a method.
I also want to use "real" data types, for instance a Unix timestamp used by the C library is transformed to and from a DateTime for .Net use.

The C library (which I can't really modify) is composed of this:

/**
 * A frame is defined by four measures
 *
 *     +-----------------------------+
 *     |        ^             ^      |
 *     |        |top          |      |
 *     |        V             |      |
 *     |<------>+------------+|      |
 *     |  left  |            ||bottom|
 *     |        +------------+V      |
 *     |<-------------------->       |
 *     |          right              |
 *     +-----------------------------+
 */
typedef struct {
  int left;
  int top;
  int right;
  int bottom;
} frame_t;

/**
 * An image
 */
typedef struct {
  /* IN */
  int     width;     // The width of the original file (used by page and book functions)
  int     height;    // The height of the original file (page and book)
  int     group;     // Images from different groups should not be put on the same page (book only)
  int     nb_hints;  // The number of hints
  frame_t *hints;    // Hints are important areas that should not be cropped

  /* OUT */
  frame_t view;      // The viewport, to crop the original image (set by page and book functions)
  frame_t dest;      // The destination, to place the cropped image (page and book)
  int     page;      // The page number of a book (book only)
} image_t;

/**
 * Resize the image to fit within the specified dimensions while retaining the
 * aspect ratio of the original image. If necessary, crop the image in the
 * larger dimension. Also, try to preserve in the image the faces and features
 * (= the hints).
 *
 * @param image The image to resize
 * @param width The width of the resized image
 * @param height The height of the resized image
 * @return the score (higher is better)
 */
int resize_to_fill(image_t *image, int width, int height);

/**
 * Resize the image to fit within the specified dimensions while retaining the
 * aspect ratio of the original image. The image may be shorter or narrower
 * than specified in the smaller dimension but will not be larger than the
 * specified values.
 *
 * @param image The image to resize
 * @param width The width of the resized image
 * @param height The height of the resized image
 * @return the score (higher is better)
 */
int resize_to_fit(image_t *image, int width, int height);

/**
 * Parameters for packing images on a page
 */
typedef struct {
  int page_width;   // The width of the page
  int page_height;  // The height of the page
  int margin;       // The margin thickness
  int nb_run;       // The number of tries for each algorithm
} parameters_t;

/**
 * Pack the images on the page. It fills the position, offset and crop of the
 * images in a pleasant way.
 *
 * @param images The images to pack
 * @param nb_images The number of images
 * @param params The parameters (size of the page and margin)
 * @return the score of the best allocation found
 */
int pack(image_t *images, int nb_images, parameters_t params);

/**
 * Select an image and position it on a cover.
 *
 * @param images The images to pack
 * @param nb_images The number of images
 * @param params The parameters (size of the page and margin)
 * @return the index of the selected image
 */
int cover(image_t *images, int nb_images, parameters_t params);

/**
 * Parameters for packing images in a book
 */
typedef struct {
  int          min;      // The minimum number of images per page
  int          max;      // The maximum number of images per page
  int          nb_pages; // The number of pages
  int          nb_books; // The number of tries for generating a book
  parameters_t page;     // The parameters for packing images on each page
} book_parameters_t;

/**
 * Pack the images in a book.
 *
 * @param images The images to pack
 * @param nb_images The number of images
 * @param params The parameters (size of the page and margin)
 * @return the score of the best allocation found
 */
int pack_book(image_t *images, int nb_images, book_parameters_t params);

/**
 * The time when a photo was taken, to clusterize photos in groups.
 * When this time is not known, put 0 in timestamp.
 */
typedef struct {
  /* IN */
  int timestamp;  // When the photo was taken

  /* OUT */
  int group;      // The identifier of the group for this photo
} photo_timestamp_t;

/**
 * Create groups of photos.
 * Images are expected to be sorted by timestamp.
 *
 * @param images The images to group
 * @param nb_images The number of images
 * @return the number of groups
 */
int clusterize(photo_timestamp_t *images, int nb_images);

And the C++/CLI library this around it. The comments are C#-style to provide intellisense in the calling project.

/// <summary>
/// Position of an image on a page
/// </summary>
public ref class Frame {
public:
    /// <summary>
    /// Position of the left side of the frame
    /// </summary>
    int Left;

    /// <summary>
    /// Position of the right top side of the frame
    /// </summary>
    int Top;

    /// <summary>
    /// Position of the right side of the frame relative to the left of the page
    /// </summary>
    int Right;

    /// <summary>
    /// Position of the bottom side of the frame relative to the top of the page
    /// </summary>
    int Bottom;

    /// <summary>
    /// Width of the frame
    /// </summary>
    property int Width {
        int get() {
            return (Right - Left);
        }
    }

    /// <summary>
    /// Height of the frame
    /// </summary>
    property int Height {
        int get() {
            return (Bottom - Top);
        }
    }

    /// <summary>
    /// Initializes a new instance of the Frame class
    /// </summary>
    Frame() {
        Top = 0;
        Left = 0;
        Bottom = 0;
        Right = 0;
    }

internal:
    /// <summary>
    /// Copies the managed frame to a native structure
    /// </summary>
    frame_t ToNative() {
        frame_t f;
        f.left = Left;
        f.top = Top;
        f.right = Right;
        f.bottom = Bottom;
        return f;
    }

    /// <summary>
    /// Creates a managed frame from a native structure
    /// </summary>
    /// <remarks>
    /// see: stackoverflow.com/a/3158445/6776
    /// </remarks>
    Frame(const frame_t &f) {
        Left = f.left;
        Top = f.top;
        Right = f.right;
        Bottom = f.bottom;
    }
};

/// <summary>
/// Describes an image
/// </summary>
public ref class Image {
private:
    void init(int nb_hints) {
        this->view = gcnew Frame();
        this->dest = gcnew Frame();
        this->page = 0;
        this->Hints = gcnew array<Frame^>(nb_hints);
    }

public:
    /// <summary>
    /// The width of the original file
    /// </summary>
    int Width;

    /// <summary>
    /// The height of the original file
    /// </summary>
    int Height;

    /// <summary>
    /// Gets the group of the image
    /// </summary>
    property int Group {
        int get() {
            return group;
        }
    }

    /// <summary>
    /// The list of important areas that should not be cropped
    /// </summary>
    array<Frame^>^ Hints;

    /// <summary>
    /// Gets the page number of the image
    /// </summary>
    property int Page {
        int get() {
            return this->page;
        }
    }

    /// <summary>
    /// Gets the viewport (to crop the image)
    /// </summary>
    property Frame^ View {
        Frame^ get() {
            return this->view;
        }
    }

    /// <summary>
    /// Gets the destination (to place the image on the page)
    /// </summary>
    property Frame^ Dest {
        Frame^ get() {
            return this->dest;
        }
    }

    /// <summary>
    /// Initializes a new instance of the Image class
    /// </summary>
    Image(int nb_hints) {
        this->init(nb_hints);
    }

    /// <summary>
    /// Initializes a new instance of the Image class
    /// </summary>
    Image() {
        this->init(0);
    }

internal:
    /// <summary>
    /// The viewport (to crop the image)
    /// </summary>
    Frame^ view;

    /// <summary>
    /// The destination (to place the image on the page)
    /// </summary>
    Frame^ dest;

    /// <summary>
    /// The page number
    /// </summary>
    int page;

    /// <summary>
    /// The group of the image
    /// </summary>
    int group;

    /// <summary>
    /// Initializes a new instance of the Image class
    /// </summary>
    Image(image_t native) {
        this->Width = native.width;
        this->Height = native.height;

        this->Hints = gcnew array<Frame^>(native.nb_hints);
        for (int i = 0; i < native.nb_hints; i++)
        {
            this->Hints[i] = gcnew Frame(native.hints[i]);
        }

        this->group = native.group;
        this->page = native.page;
        this->dest = gcnew Frame(native.dest);
        this->view = gcnew Frame(native.view);
    }

    /// <summary>
    /// Copies the managed image to a native structure
    /// </summary>
    image_t ToNative() {
        image_t i;
        i.width = this->Width;
        i.height = this->Height;
        i.group = this->Group;

        // hints
        i.nb_hints = this->Hints->Length;
        i.hints = new frame_t[this->Hints->Length];
        for (int nb = 0; nb < this->Hints->Length; nb++)
        {
            i.hints[nb] = this->Hints[nb]->ToNative();
        }

        // output values
        i.page = this->Page;
        i.view = this->View->ToNative();
        i.dest = this->Dest->ToNative();

        return i;
    }
};

/// <summary>
/// Parameters for packing images on a page
/// </summary>
public ref class Parameters {
public:
    /// <summary>
    /// The width of the page
    /// </summary>
    int PageWidth;

    /// <summary>
    /// The height of the page
    /// </summary>
    int PageHeight;

    /// <summary>
    /// The margin between images
    /// </summary>
    int Margin;

    /// <summary>
    /// The number of tries for each algorithm (generally in the hundreds)
    /// </summary>
    int NbRun;

internal:
    /// <summary>
    /// Copies the managed parameter to a native structure
    /// </summary>
    parameters_t ToNative() {
        parameters_t p;
        p.page_width = this->PageWidth;
        p.page_height = this->PageHeight;
        p.margin = this->Margin;
        p.nb_run = this->NbRun;
        return p;
    }
};

/// <summary>
/// Parameters for packing images in a book
/// </summary>
public ref class BookParameters {
public:
    /// <summary>
    /// The minimum number of images per page
    /// </summary>
    int MinPerPage;

    /// <summary>
    /// The maximum number of images per page
    /// </summary>
    int MaxPerPage;

    /// <summary>
    /// The number of pages
    /// </summary>
    int NbPages;

    /// <summary>
    /// The number of "tentative" books generated (generally several thousands)
    /// </summary>
    int NbRun;

    /// <summary>
    /// The parameters for each page
    /// </summary>
    Parameters^ PageParam;

    /// <summary>
    /// Initializes a new instance of the BookParameters class
    /// </summary>
    BookParameters(Parameters^ parameters) {
        this->PageParam = parameters;
    }

internal:
    /// <summary>
    /// Copies the managed parameter to a native structure
    /// </summary>
    book_parameters_t ToNative() {
        book_parameters_t p;
        p.max = this->MaxPerPage;
        p.min = this->MinPerPage;
        p.nb_pages = this->NbPages;
        p.nb_books = this->NbRun;
        p.page = this->PageParam->ToNative();
        return p;
    }
};

/// <summary>
/// The time when a photo was taken, to clusterize photos in groups.
/// When this time is not known, put 0 in timestamp
/// </summary>
public ref class PhotoTimestamp {
public:
    /// <summary>
    /// When the photo was taken
    /// </summary>
    System::DateTime^ Timestamp;

    /// <summary>
    /// The identifier of the group for this photo
    /// </summary>
    property int Group {
        int get() {
            return this->group;
        }
    }

    /// <summary>
    /// Initializes a new instance of the PhotoTimestamp class
    /// </summary>
    PhotoTimestamp()
    {
        this->group = 0;
    }

internal:
    /// <summary>
    /// The calculated group of the image
    /// </summary>
    int group;

    /// <summary>
    /// Initializes a new instance of the PhotoTimestamp class
    /// </summary>
    PhotoTimestamp(photo_timestamp_t t) {
        this->group = t.group;

        System::DateTime^ timestamp = gcnew DateTime(1970, 1, 1, 0, 0, 0, 0, System::DateTimeKind::Utc);
        timestamp = timestamp->AddSeconds(t.timestamp).ToLocalTime();
        this->Timestamp = timestamp;
    }

    /// <summary>
    /// Copies the managed timestamp to a native structure
    /// </summary>
    photo_timestamp_t ToNative() {
        photo_timestamp_t t;
        t.group = this->Group;

        // convert the date to a UNIX timestamp
        t.timestamp = this->Timestamp->ToUniversalTime().Subtract(System::DateTime(1970, 1, 1)).TotalSeconds;

        return t;
    }
};

/// <summary>
/// Packing of the images
/// </summary>
public ref class Packer {
private:
    /// <summary>
    /// The images list
    /// </summary>
    array<Image^>^ images;

    /// <summary>
    /// The packing parameters
    /// </summary>
    Parameters^ params;

    /// <summary>
    /// References the native images structures
    /// </summary>
    image_t *nativeImages;

    /// <summary>
    /// Gets the values from the native images and stores them to the managed ones
    /// </summary>
    void NativeToManagedImages() {
        for (int i = 0; i < this->images->Length; i++) {
            images[i]->view = gcnew Frame(this->nativeImages[i].view);
            images[i]->dest = gcnew Frame(this->nativeImages[i].dest);
            images[i]->page = this->nativeImages[i].page;
            images[i]->group = this->nativeImages[i].group;
        }
    }

public:
    /// <summary>
    /// Initializes a new instance of the Packer class
    /// </summary>
    /// <param name="images">The images to pack</param>
    /// <param name="params">The parameters for the packing</param>
    Packer(array< Image^ >^ %images, Parameters^ params) {
        srand(time(NULL));

        this->images = images;
        this->params = params;

        // create a native array of image_t to send to the native library
        this->nativeImages = new image_t[this->images->Length];
        for (int i = 0; i < this->images->Length; i++) {
            this->nativeImages[i] = this->images[i]->ToNative();
        }
    }

    /// <summary>
    /// Deallocates the class and frees up memory
    /// </summary>
    ~Packer() {
        delete[] this->nativeImages;
    }

    /// <summary>
    /// Resizes an image to fill within the specified dimensions
    /// while retaining the aspect ratio of the original. If necessary,
    /// crop the image in the larger dimension.
    /// </summary>
    /// <param name="image">The image to resize</param>
    /// <param name="width">The width of the destination area</param>
    /// <param name="height">The height of the destination area</param>
    /// <returns>
    /// The score of the resize
    /// </returns>
    static int ResizeToFill(Image^ %image, int width, int height) {
        image_t native = image->ToNative();

        int result = resize_to_fill(&native, width, height);

        image = gcnew Image(native);

        return result;
    }

    /// <summary>
    /// Resizes the image to fit within the specified dimensions
    /// while retaining the aspect ration of the original. The image
    /// may be shorter or narrower than specified.
    /// </summary>
    /// <param name="image">The image to resize</param>
    /// <param name="width">The width of the destination area</param>
    /// <param name="height">The height of the destination area</param>
    /// <returns>
    /// The score of the resize
    /// </returns>
    static int ResizeToFit(Image^ %image, int width, int height) {
        image_t native = image->ToNative();

        int result = resize_to_fit(&native, width, height);

        image = gcnew Image(native);

        return result;
    }

    /// <summary>
    /// Creates groups of photos from their timestamps.
    /// </summary>
    /// <remarks>
    /// Images are expected to be sorted by timestamp.
    /// </remarks>
    /// <param name="images">The images infos to clusterize</param>
    /// <returns>
    /// The number of groups
    /// </returns>
    static int Clusterize(array<PhotoTimestamp^>^ %images) {
        photo_timestamp_t *timestamps = new photo_timestamp_t[images->Length];
        for (int i = 0; i < images->Length; i++) {
            timestamps[i] = images[i]->ToNative();
        }

        int result = clusterize(timestamps, images->Length);

        for (int i = 0; i < images->Length; i++)
        {
            images[i] = gcnew PhotoTimestamp(timestamps[i]);
        }

        return result;
    }

    /// <summary>
    /// Selects an image to position on a cover
    /// </summary>
    /// <returns>
    /// The index of the selected image
    /// </returns>
    int Cover() {
        int result = cover(nativeImages, this->images->Length, params->ToNative());

        return result;
    }

    /// <summary>
    /// Packs the images on the sheet
    /// </summary>
    /// <returns>
    /// The score of the best allocation found
    /// </returns>
    int Pack() {
        int result = pack(this->nativeImages, this->images->Length, this->params->ToNative());

        this->NativeToManagedImages();

        return result;
    }

    /// <summary>
    /// Packs the images in a book
    /// </summary>
    /// <param name="bookParameters">The parameters for the book packing</param>
    /// <returns>
    /// The score of the best allocation found
    /// </returns>
    int PackBook(BookParameters^ bookParameters) {
        int result = pack_book(this->nativeImages, this->images->Length, bookParameters->ToNative());

        this->NativeToManagedImages();

        return result;
    }
};

I am specifically concerned about several things.

First, could there be a better way to convert native to and from managed? I feel this way is not very memory efficient (lots of copying data around), but I fear using more efficient methods (like using pin_ptr) will add complexity to the code (and memory corruption risks) for a negligible performance gain. The app already takes several hundred Mbs and several seconds/minutes to call this method so I don't care about shaving a few milliseconds if it means refactoring everything.

While copying this code, I have noticed that the native-to-managed constructors Frame(const frame_t &f) and Image(image_t native) do not follow the same "signature style" (I have written them months apart). Is there a significant difference (apart from pointer vs value), and potential problem that could arise ?

Is there a risk somewhere (some kind of red flag or anti-pattern, perhaps) for potential memory leak or corruption ?
I was pointed on SO to the risk in the ResizeToFit wrapper method that the image_t instance I create may be "bound" in some way to the Image instance, because the image_t structure has a pointer (* hints) so it creates a shallow copy when passing it to the Image(image_t native) constructor. Have I understood correctly?

Is there anything else I'm missing?

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't get what this has to do with C++ at all. You're showing C and C# code. \$\endgroup\$ – iveqy Apr 2 '15 at 8:30
  • \$\begingroup\$ This is not C#. It's C++/CLI. Admittedly it's a little different from standard C++, but I thought not enough that C++ reviewers wouldn't have anything to say. It's just written in C# style (comments style, CamelCase...). \$\endgroup\$ – thomasb Apr 2 '15 at 9:12
  • \$\begingroup\$ but the Internal: keyword is not in C++ but exists in C#. That's what's confusing me. Shouldn't it be "protected" or "public private:" instead of "internal:"? \$\endgroup\$ – iveqy Apr 4 '15 at 12:39
  • \$\begingroup\$ Internal makes the member visible to all objects inside the library, so the ToNative methods can be called by other classes of the project, but not from the "calling" project. \$\endgroup\$ – thomasb Apr 5 '15 at 9:40
  • 1
    \$\begingroup\$ I have removed the C++ tag because none of the code is in C++, and the question itself has nothing to do with it. \$\endgroup\$ – utnapistim Apr 7 '15 at 9:28
4
\$\begingroup\$

First of all, why are you using C++/CLI? The C API looks relatively simple, you should be able to write the wrapper in C# using PInvoke, with the advantage that you won't have to write all that copying code.


int Left;

Public fields are considered bad practice in .Net. Instead, you can use shorthand property syntax:

property int Left;

Frame() {
    Top = 0;
    Left = 0;
    Bottom = 0;
    Right = 0;
}

All fields of managed types are automatically initialized to the default value, so I think this is unnecessary code.


Image(int nb_hints) {
    this->init(nb_hints);
}

Image() {
    this->init(0);
}

You can use delegating constructor for this:

Image(int nb_hints) {
    // actual initialization code here
}

Image() : Image(0) {
}

Consider changing the type of Hints to something like List<Frame^>^ or `IList^. That way, working with it will be easier.


System::DateTime^ Timestamp;

DateTime is a structure (value class in C++/CLI, struct in C#), so there is no need to use a handle here.


array< Image^ >^ %images

I don't see any reason why this should be a tracking reference.


Packer(array< Image^ >^ %images, Parameters^ params) {
    srand(time(NULL));

    this->images = images;
    this->params = params;

    // create a native array of image_t to send to the native library
    this->nativeImages = new image_t[this->images->Length];
    for (int i = 0; i < this->images->Length; i++) {
        this->nativeImages[i] = this->images[i]->ToNative();
    }
}

~Packer() {
    delete[] this->nativeImages;
}

Since Image::ToNative() allocates unmanaged memory using new and you're never deallocating it, this is going to cause a memory leak. I think the right solution here is to iterate through nativeImages in ~Packer and delete[] their hints there.


If I understand how destructors and finalizers work in C++/CLI correctly, then cleanup of unmanaged resources (like your arrays) should be done in the finalizer, which should then also be called from the destructor:

~Packer() {
    this->!Packer();
}

!Packer() {
    delete[] this->nativeImages;
}

This way, the unmanaged memory is (eventually) released even if you forgot to call the destructor (by using delete or stack semantics in C++/CLI and Dispose() or using in C#).

\$\endgroup\$
  • \$\begingroup\$ Thanks! A few remarks: constructor chaining (delegating) is not supported in C++/CLI. I couldn't make the code work without the handle to the timestamp. The %images tracking reference is because the values are modified in the C function. Thanks for the memory management tips! \$\endgroup\$ – thomasb Apr 21 '15 at 13:21
1
\$\begingroup\$
static int ResizeToFill(Image^ %image, int width, int height) {
    image_t native = image->ToNative();

    int result = resize_to_fill(&native, width, height);

    image = gcnew Image(native);

    return result;
}

This method (and the others) does not really change the image but it creates a new one which is not obvious and the user of this method would think he does not need to dispose the old image. I suggest adding another [Out] parameter for the result while changing the first parameter from % (ref) to normal by-ref parameter.

static int ResizeToFill(Image^ image, int width, int height, [Out] Image^ %result)
{
    image_t native = image->ToNative();

    int score = resize_to_fill(&native, width, height);

    result = gcnew Image(native);

    return score;
}
\$\endgroup\$

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.