sf::Packet onSend and onReceive - Simple zlib Implementation

[AlanEdwardes]

Hello all,

This is more of a general C++ memory management/code design question than a networking thing, but since it involves sf::Packet I thought this would be a good place.

I'm trying to add zlib compression to the onSend / onReceive methods of a derrived sf:Packet class - much like the example here: http://www.sfml-dev.org/tutorials/2.1/network-packet.php#custom-packets

This is the compression portion:

const void* StreamSerialiser::onSend(std::size_t& size)
{
        const void* srcData = getData();
        uLong srcSize = getDataSize();
        uLong dstSize = compressBound(srcSize);

        const char* dstData = new char[dstSize];
        compress((Bytef*)dstData, &dstSize, (Bytef*)srcData, srcSize);

        size = dstSize;
        return dstData;
}

And the decompression portion:

void StreamSerialiser::onReceive(const void* data, std::size_t size)
{
        uLong dstSize = 1500;
        const char *dstData = new char[dstSize];

        uncompress((Bytef*)dstData, &dstSize, (Bytef*)data, size);
        append(dstData, dstSize);
        delete dstData;
}

In the first onSend portion, I'm allocating some memory (dstData), but I never see that pointer after I return it - so have no opportunity to free the memory.

Should dstData instead be a class variable, say, a 1500 byte char array that is re-used?

The onReceive portion seems to work OK, but I'm not happy about the memory allocation there either. Would that also benefit from being a re-used class variable, or is there a better way to do this?

Any input about this is appreciated - I'm still learning about memory management.

Cheers!

[Nexus]

Yes, the idea is to manage memory yourself. SFML cannot know how you allocated it, especially since you're returning a void pointer.

"Manage yourself" means use RAII and store the data in a member variable that survives the onSend() call, not a local variable. In your case, a good choice for a buffer of bytes would be std::vector<Bytef>. Then you don't need the casts (which should have been rather reinterpret_cast than C casts, to state the danger explicitly in code and tell the compiler unambiguously what you want).

If you're interested in RAII and why it's a good idea to avoid new/delete, you could read my article.

[panithadrum]

Amazing article by the way. You're such a genius, Nexus!

[AlanEdwardes]

Yes, the idea is to manage memory yourself. SFML cannot know how you allocated it, especially since you're returning a void pointer.

"Manage yourself" means use RAII and store the data in a member variable that survives the onSend() call, not a local variable. In your case, a good choice for a buffer of bytes would be std::vector<Bytef>. Then you don't need the casts (which should have been rather reinterpret_cast than C casts, to state the danger explicitly in code and tell the compiler unambiguously what you want).

If you're interested in RAII and why it's a good idea to avoid new/delete, you could read my article.

Excellent, thanks for the response. The article is a good read.

This is one of the few instances I have used new / delete at all in this codebase - I've mostly been using Object object; object.Method(); for my game objects, except where it has been unavoidable (and I'm going to change those to smart pointers having read that).

With regards to using a vector, I wasn't quite sure how I'd go about translating that to a Bytef pointer, as zlib requires for these calls. I guess I might have to use z_stream to feed zlib each byte at a time, but I'm not sure how I'd tackle that. I'd like to keep the solution simple.

So, for a non-leaking but still imperfect solution, I have:

class StreamSerialiser : public sf::Packet
{
        ...
private:
        virtual const void* onSend(std::size_t& size);
        virtual void onReceive(const void* data, std::size_t size);
        Bytef m_oCompressionBuffer[1500];
        Bytef m_oDecompressionBuffer[1500];
}
 

The compression portion:

const void* StreamSerialiser::onSend(std::size_t& size)
{
        const Bytef* srcData = reinterpret_cast<const Bytef*>(getData());

        uLong srcSize = getDataSize();
        uLong dstSize = compressBound(srcSize);

        compress(m_oCompressionBuffer, &dstSize, srcData, getDataSize());

        size = dstSize;
        return m_oCompressionBuffer;
}
 

The decompression portion:

void StreamSerialiser::onReceive(const void* data, std::size_t size)
{
        const Bytef* srcData = reinterpret_cast<const Bytef*>(data);

        uLong dstSize;
        uncompress(m_oDecompressionBuffer, &dstSize, srcData, size);

        append(m_oDecompressionBuffer, dstSize);
}
 

The obvious flaw here is that the buffers are only 1500 bytes, which suffices for usage with UDP packets, but makes this class very single-purpose. I perhaps could use only one buffer too - this class shouldn't need to be thread safe.

I agree a vector would be a better fit for this, but again I'm not sure how to implement that with the zlib calls.

Cheers!

[Nexus]

With regards to using a vector, I wasn't quite sure how I'd go about translating that to a Bytef pointer

Have you read the documentation? Hint: data() or address of first element.

So, for a non-leaking but still imperfect solution, I have:

Don't use hacks that eventually break when you can solve it robustly with std::vector.

By the way, void* -> Bytef* can be done with static_cast. reinterpret_cast is only needed when you convert between different typed pointers.

[AlanEdwardes]

With regards to using a vector, I wasn't quite sure how I'd go about translating that to a Bytef pointer

Have you read the documentation? Hint: data() or address of first element.

Ah, excellent! Ah, no, I hadn't read that well enough.

By the way, void* -> Bytef* can be done with static_cast. reinterpret_cast is only needed when you convert between different typed pointers.

Right, gotcha. I will change that.

I'll post the final code too, so that anyone looking for this functionality can use it. Thanks for your help!

[AlanEdwardes]

OK, so using a vector, this is the code I have so far. If anyone has any suggestions about how to optimise it, please let me know.

Header:

class StreamSerailiser : public sf::Packet
{
        [...]
private:
        virtual const void* onSend(std::size_t& size);
        virtual void onReceive(const void* data, std::size_t size);
        std::vector<Bytef> m_oCompressionBuffer;
}

Compression:

const void* StreamSerialiser::onSend(std::size_t& size)
{
        // We only support data with a maximum size of
        // an unsigned short (so the size can be sent
        // in the first two bytes of the packet)
        assert(size <= 65535);

        // Cast the data to a bytef pointer
        const Bytef* srcData = static_cast<const Bytef*>(getData());

        // Get the size of the packet to send
        uLong srcSize = getDataSize();

        // Compute the size of the compressed data
        uLong dstSize = compressBound(srcSize);

        // Resize the vector to accomodate the compressed data,
        // plus two bytes for our uncompressed size
        m_oCompressionBuffer.resize(dstSize + 2);

        // Take the first 8 bytes of srcSize
        m_oCompressionBuffer[0] = srcSize & 0xFF;

        // And the second 8 bytes
        m_oCompressionBuffer[1] = (srcSize >> 8) & 0xFF;

        // Compress the data into the rest of the buffer
        compress(m_oCompressionBuffer.data() + 2, &dstSize, srcData, srcSize);

        // Set the size to the compressed size plus
        // two bytes for the size marker
        size = (dstSize + 2);

        // Return data to send
        return m_oCompressionBuffer.data();
}

Decompression:

void StreamSerialiser::onReceive(const void* data, std::size_t size)
{
        // Cast the data to Bytef*, the format zlib deals with
        const Bytef* srcData = static_cast<const Bytef*>(data);

        // Extract the uncompressed data size from the first two
        // bytes in the packet so we can use it for the buffer
        sf::Uint16 uncompressedSize = srcData[1] << 8 | srcData[0];

        // Resize the vector to accomodate the uncompressed data
        m_oCompressionBuffer.resize(uncompressedSize);

        // Declare a variable for the destination size
        uLong dstSize;

        // Uncompress the data (remove the first two bytes)
        uncompress(m_oCompressionBuffer.data(), &dstSize, (srcData + 2), size - 2);

        // Assert that the uncompressed size is the same as the
        // size we were sent for the buffer
        assert(dstSize == uncompressedSize);

        // Append it to the packet
        append(m_oCompressionBuffer.data(), dstSize);
}

So there, because the buffer size isn't known on the receiving end ahead of time, I'm prepending that to the packet's data as an unsigned short split into two unsigned chars / Bytef, and reconstructing it at the receiving end.

I don't know if this is safe code with regards to endianness etc, so any input on this is appreciated. I suspect this could be greatly improved

Cheers.

[Nexus]

The code looks good.

One thing you could do is avoid the magic number 2 and replace it with a constant declared globally in the .cpp file. Also, keep in mind that assert is only a debugging help, it should not be used to validate user input. But if the conditions can't occur in a correct program in Release mode, then the use of assertions is the right choice.