Extracting, then passing raw data into another class - How to avoid copying twice while maintaining encapsulation?

Question

Consider a class Book with a stl container of class Page. each Page holds a screenshot, like page10.jpg in raw vector<char> form.

A Book is opened with a path to a zip, rar, or directory containing these screenshots, and uses respective methods of extracting the raw data, like ifstream inFile.read(buffer, size);, or unzReadCurrentFile(zipFile, buffer, size). It then calls the Page(const char* stream, int filesize) constructor.

Right now, it's clear that the raw data is being copied twice. Once to extract to Book's local buffer and a second time in the Page ctor to the Page::vector<char>. Is there a way to maintain encapsulation while getting rid of the middleman buffer?

Answer 1

use std::vector resize member to set the buffer size originally, and then use its buffer directly by using front()'s address.

std::vector<char> v;
v.resize(size);
strcpy(&v.front(), "testing");

Direct buffer access of std::vector is given by: &v.front()

Answer 2

You can introduce a third component that would hold all the images. The book would fill it in, the pages would read from it. If you want to restrict access you can close it down and make book and page its friends. If you have repetitions of images (say, each page has a footer and a header, or some pages have a logo, or whatever) you can make that third component a flyweight, thus making it even more efficient than what you strived for.

Make sure that you don't open all the pages when you open the book. That can be expensive. Have each page hold an identifier for its images (perhaps file-paths), and load the images only when you really want to view the page.

Answer 3

Use std::vector to hold image data is a bad idea. I will use raw pointer or shared_ptr for this purpose. This prevents the buffer is copied twice.

Since you do care memory, holding all image data in memory is also a bad idea to me. A better case is to encapsulate it into a separate class. For example, ImageData. This class contains the row pointer of image data. The class can be initialized with a file path at the first, and the image data is loaded from disk when it is required.

Answer 4

In terms of code changes based on what you have already, the simplest is probably to give Page a setter taking a non-const vector reference or pointer, and swap it with the vector contained in the Page. The caller will be left holding an empty vector, but since the problem is excessive copying, presumably the caller doesn't want to keep the data:

void Book::addPage(ifstream file, streampos size) {
    std::vector<char> vec(size);
    file.read(&vec[0], size);
    pages.push_back(Page()); // pages is a data member
    pages.back().setContent(vec);
}

class Page {
    std::vector<char> content;
public:
    Page() : content(0) {} // create an empty page
    void setContent(std::vector<char> &newcontent) {
        content.swap(newcontent);
    }
};

Some people (for example the Google C++ style guide) want reference parameters to be const, and would want you to pass the newcontent parameter as a pointer, to emphasise that it is non-const:

void setContent(std::vector<char> *newcontent) {
    content.swap(*newcontent);
}

swap is fast - you'd expect it just to exchange the buffer pointers and sizes of the two vector objects.

Alternatively, give Page two different constructors: one for the zip file and one for the regular file, and have it be responsible for reading its own data. This is probably the cleanest, and it allows Page to be immutable, rather than being modified after construction. But actually you might not want that, since as you've noticed in a comment, adding the Page to a container copies the Page. So there's some benefit to being able to modify the Page to add the data after it has been cheaply constructed in the container: it avoids that extra copy without you needing to mess with containers of pointers. Still, the setContent function could just as easily take the file stream / zip file info as take a vector.

You could find or write a stream class which reads from a zipfile, so that Page can be responsible for reading data with just one constructor taking a stream. Or perhaps not a whole stream class, perhaps just an interface you design which reads data from a stream/zip/rar into a specified buffer, and Page can specify its internal vector as the buffer.

Finally, you could "mess with containers of pointers". Make pages a std::vector<boost::shared_ptr<Page> >, then do:

void Book::addPage(ifstream file, streampos size) {
    boost::shared_ptr<Page> page(new Page(file, size));
    pages.push_back(page); // pages is a data member
}

A shared_ptr has a modest overhead relative to just a Page (it makes an extra memory allocation for a small node containing a pointer and a refcount), but is much cheaper to copy. It's in TR1 too, if you have some implementation of that other than Boost.

Answer 5

I would have the Page class read its own data directly from the source, and the Book would only read as much of the source that it needed in order to locate each individual page (and to read any data belonging to the Book in general, such as a title).

For example, in the case of the data being stored in a directory, the Book would retrieve the list of files in the directory. For each file, it would pass the filename to a Page constructor which would open the file and load its contents.

As for the case where the book was stored in a zip file, I'm making some guesses as to how the library you're using works. I think you're using Minizip, which I'm not familiar with, but at a glance it looks like opening a file via Minizip gives you a handle. You pass that handle to unzGoToFirstFile() and unzGoToNextFile() to set the active subfile within the zip file (in your case, the active page), and use unzReadCurrentFile() to load the active subfile into a buffer. If that's the case, then your Book class would open the file using Minizip and set it to the first subfile. It would then pass the handle to the zip file to a constructor in Page, which would do the work of reading the subfile from the zip file. The Book would then call unzGoToNextFile() to move to the next subfile, and would create another page by again passing the handle to Page. It would continue doing this until there were no subfiles remaining. It would look something like:

Page::Page(zipFile file)
{
    //  TODO:  Determine the required size of the buffer that will store the data
    unsigned buffer_size;

    data_.resize(buffer_size)

    unzReadCurrentFile(file, &data_[0], buffer_size);
}

void Book::open(const std::string &filename)
{
    zipFile file = unzOpen(filename.c_str());

    int result = unzGoToFirstFile(file);
    while (result == UNZ_OK)
    {
        pages_.push_back(Page(file));
        unzGoToNextFile(file);
    }
}

This is very simplified (and I might be using Minizip totally wrong, so beware), and it also assumes that Book stores a vector of Page objects named pages_, and that Page names its buffer data_.