Add a void * user cookie to SoOutputReallocCB

[marco-langer]

Some callbacks in the library, such as SoErrorCB, already have a void * data parameter, but other callbacks are missing this parameter currently.

I'd like to propose to add a void * data parameter to the reallocation callback SoOutputReallocCB defined in SoOutput.h:

typedef void * SoOutputReallocCB(void * ptr, size_t newSize, void * data);

This parameter could be made NULL by default if API backwards-compatibility is desired.

I could prepare a PR with the necessary changes.

[VolkerEnderlein]

This would break the ABI and needs a major version bump to 5. What's the use case for this change, as SoOutputReallocCB is only called in SoOutput_Writer.cpp to reallocate memory?

[marco-langer]

Actually, my initial statement was not correct: Even if the default parameter for the void * userData pointer
in SoOutput::setBuffer() would be NULL, it still would be an API break due to the additional parameter in the callback.

My pain point with the current API is that it enforces me to make a copy of the complete scene in order to serialize it into a custom container. I've followed the example code given in SoOutput.cpp for SoOutput::setBuffer() and came up with this solution:

#include <gsl/pointers>
#include <boost/intrusive_ptr.hpp>
#include <algorithm>
#include <cstddef>
#include <cstdlib>
#include <vector>

static std::size_t bufferSize = 0;
static std::byte * buffer;

std::vector<std::byte> serializeGraphs(gsl::not_null<boost::intrusive_ptr<SoSeparator>> root)
{
    bufferSize = 1024;
    buffer = static_cast<std::byte *>(std::malloc(bufferSize));
    auto reallocCallback = [](void * bufptr, std::size_t newSize) -> void *
    {
        buffer = static_cast<std::byte *>(std::realloc(bufptr, newSize));
        bufferSize = newSize;
        return buffer;
    };

    SoOutput output;
    output.setBuffer(buffer, bufferSize, reallocCallback);

    SoWriteAction wa(&output);
    wa.apply(root.get().get());

    std::vector<std::byte> serializedGraphs(bufferSize);
    std::copy_n(buffer, bufferSize, std::begin(serializedGraphs));

    std::free(buffer);

    return serializedGraphs;
}

I have to copy the raw buffer into the vector. The example code in SoOutput.cpp also creates a copy when constructing a SbString out of the buffer:

  SbString s(buffer);
  free(buffer);
  return s;

Using an additional void * parameter would allow me to stream directly into the vector without any overhead:

#include <gsl/pointers>
#include <boost/intrusive_ptr.hpp>
#include <cstddef>
#include <vector>

std::vector<std::byte> serializeGraphs(gsl::not_null<boost::intrusive_ptr<SoSeparator>> root)
{
    const std::size_t initialeSize = 1024;
    std::vector<std::byte> serializedGraphs(initialeSize);
    auto reallocCallback = [](void * bufptr, std::size_t newSize, void * userData) -> void *
    {
        auto * castedSerializedGraphs = static_cast<std::vector<std::byte> *>(userData);
        castedSerializedGraphs->resize(newSize);
        return castedSerializedGraphs->data();
    };

    SoOutput output;
    output.setBuffer(buffer, initialeSize, reallocCallback, &serializedGraphs);

    SoWriteAction wa(&output);
    wa.apply(root.get().get());

    return serializedGraphs;
}

I would prefer the second solution as I am currently looking for opportunities to employ custom memory allocators for std::vector in order to decrease the memory overhead of our application.

One possible way to retain API and ABI compatibility would be to implement a new function and deprecate the old one. This would allow users a smooth transition without any breaking change:

typedef void * SoOutputReallocCB2(void * ptr, size_t newSize, void * userData);


virtual void setBuffer2(void * bufPointer, size_t initSize,
                       SoOutputReallocCB2 * reallocFunc, void * userData, int32_t offset = 0)

This would result in quite some code duplication and maybe a version bump with API breakage would be a better solution.

[VolkerEnderlein]

Can you use a string_view class to manage the buffer allocated by std::realloc to avoid the copy? Its interface is very similar to std::vector so you can use it in most of the algorithms that require iterators. You would set buffer = nullptr; at the end of the first serializeGraphs implementation and manage the lifetime of the buffer via the string_view. When it comes to the destruction of the string_view you need to free the underlying pointer using free.
But maybe that's more hackery than a real solution.