Image saving

[Ventus]

One of the issues with any design is, that it would need to be adapted for audio and font as well.

While I'm not particularly familiar with the audio internals, I'll definitely take a look.

With the current design, one needs to be aware that the buffer can be resized by the save function. It should also be defined what happens with an already allocated buffer and with an over-sized buffer.

I can think of two approaches: clear the buffer before inserting data, or append the data to the end. Since it's a vector, it can be easily resized to either data.size() or buffer.size() + data.size().

Also one thing to consider whether sf::Unit8 is the best binary data representation.

If SFML's spec was set to C++11, then using signed or unsigned wouldn't make a lot of difference, execution-wise. However, since the idea is to target C++03 compilers as well, we need to remember that char can have some padding added to it by the compiler. While unsigned char has always been guaranteed by the standard to have no padding added to it iirc.
There's also the convention that char represents text while unsigned char represents binary data, although signed char could be used instead.
These are the points I can think of for unsigned, but I'd love to hear what everyone else thinks about it.

[FRex]

What is that padding in char that you speak of? Char is often signed and that's the 'problem' but char, signed char and unsigned char are three same sized and distinct types (even though char is 100% like one of the other two).

[Ventus]

[C++11: 3.9.1/1]: [..] A char, a signed char, and an unsigned char occupy the same amount of storage and have the same alignment requirements (3.11); that is, they have the same object representation. For character types, all bits of the object representation participate in the value representation. [..]

From my understanding, this only became part of the standard in C++11, but I could be wrong.

[FRex]

That part is there in C++98 and I've never heard of any 'padding' in chars (that'd then not be there for signed/unsigned chars). char, unsigned char and void ptrs get used to mean 'binary data' all the time in APIs.

[Ventus]

That part is there in C++98 and I've never heard of any 'padding' in chars (that'd then not be there for signed/unsigned chars). char, unsigned char and void ptrs get used to mean 'binary data' all the time in APIs.

I see, good point. Well, then I personally have no preference for one or the other. What are your thoughts?

[FRex]

Well, SFML uses void now because everywhere the buffer size is known and 'big enough'.

This can't happen with std::vector so a different API would have to be made to allow user to alloc a buffer and handle failure nicely, something like:

bool sf::Image::saveToMemory(void * buff, std::size_t size, std::size_t& outsize);

Where return bool would say if there was enough space to write it all out or not and outsize would always contain how much bytes would be written if there was enough space. Some C, POSIX and WinAPI functions do it that or similar way (like some signed size type as return and -1 there as error), even allowing calling with NULL ptr and 0 size to get just the output size but that's starting to get too un-SFML-like (and kind of cumbersome to use, I always wrap these myself to just do right thing with dynamic allocation in C and C++ when I need them or make sure the buffer is way bigger than required so they can't fail).

With std::vector (which I don't mind since it solves the size problem and SFML already uses STL a lot and isn't some tight no-lib-side-mem-alloc-ever low-level hardcore library) either char or uchar are fine. Either choice will have someone preferring the other one, since both have decent reasons for being used (char is 'default' byte type to use, and uchar has the nice property of being unsigned so no sign and sign extensions shenanigans take place).

Alternatively a callback function or class could be used and use a const void ptr there but that still leaves a sort of gap in API where reading can be done from 3 source (stream, file, memory) and writing to 2 (file, stream) with memory missing/part of stream (while for reading there is memory input stream but also outright loadFromMemory call that takes just const void ptr and size).

[Ventus]

While I don't think it's a particularly good idea, nothing really stops us from having two overloads, one with char and one with unsigned char.

Particularly, I think using std::vector fits SFML a lot more, especially since getting an array from it is trivial. I suppose it just makes things a lot more simple.

[Nexus]

While I don't think it's a particularly good idea, nothing really stops us from having two overloads, one with char and one with unsigned char.

And one with signed char? Those are 3 distinct types, even if char is treated the same as one of the other two.

Alternatively a callback function or class could be used and use a const void ptr there but that still leaves a sort of gap in API where reading can be done from 3 source (stream, file, memory) and writing to 2 (file, stream) with memory missing/part of stream (while for reading there is memory input stream but also outright loadFromMemory call that takes just const void ptr and size).

True, but you need to choose one of the following:

• saveToMemory(void* buff, std::size_t size, std::size_t& outSize);
  
  This would be the direct counterpart of readFromMemory(), and as such the only way to have no "gap".
  
  However, this is very low-level, and user needs to take care of reallocations/retries. Quite user-unfriendly, even the C API of STB is nicer with the callback.
  
  
• saveToMemory(std::vector<sf::Uint8>& outMemory);
  
  Easy to use, quite limited though. Expects that the user needs exactly a std::vector, and that the elements are exactly sf::Uint8 (aka unsigned char). As soon as a different format is needed, the user needs to copy or reinterpret pointers.
  
  
• saveToStream(sf::OutputStream& stream);
  
  This is effectively a C++ translation of the C callback API (which is great, by the way).

Since saving to memory is not the typical game use case, it is probably occurring mostly when interfacing with a different rendering engine or image processing systems. Those usually have a rather low-level API based on char*/size_t pairs. Even if std::vector supports this format, for any scenario where ownership is transferred to a third-party system, the std::vector leads to a useless temporary copy.


In my opinion, sf::OutputStream is the cleanest and most flexible solution.
std::vector is convenient, but I would add it only once we gather some knowledge about common use cases (e.g. which element type).

[Ventus]

Since saving to memory is not the typical game use case, it is probably occurring mostly when interfacing with a different rendering engine or image processing systems. Those usually have a rather low-level API based on char*/size_t pairs. Even if std::vector supports this format, for any scenario where ownership is transferred to a third-party system, the std::vector leads to a useless temporary copy.

Usually, these other systems have methods to load RAW RGBA data, which makes getPixelsPtr and width * height * 4 perfect for this use case. The whole idea of saveToMemory is to encode this data in a specified format. Otherwise we could just copy the internal pixel data.

std::vector is convenient, but I would add it only once we gather some knowledge about common use cases (e.g. which element type).

Two particular use cases I've ran into were the following:
1-) Saving a project file in json. One of the data types stored were base64 encoded images. Encoding directly from getPixelsPtr was particularly easy, however the final size transformed a 78KB JPEG into 5,6MB RAW RGBA. That's where saveToMemory comes in, encoding the image without accessing the filesystem (yet). By using std::vector, there is a minimal code difference here.
2-) A server application which modifies files and returns them through the network. Without adding additional libraries, saveToMemory could remove the need to write to the filesystem as a middleman, reducing SSD wear and API response times.

That said, a saveToStream really does sound like the better option, so we could propose something like:

bool sf::Image::saveToStream(sf::OutputStream& stream, const std::string& format = "png");

[Nexus]

Thanks for listing the use cases, they sound reasonable indeed. The web didn't come to my mind, as other languages are often more prominent than C++.

In JSON, the binary representation of an image (whether you represent it as number array or Base-64 string) could also be constructed in a stream-based manner, not requiring to copy the whole image data to an intermediate buffer. This would be a perfect use case for sf::OutputStream.

bool sf::Image::saveToStream(sf::OutputStream& stream, const std::string& format = "png");

looks good, except for the default parameter. I think the user should explicitly specify the format.

Whether enum or string -- I'm inclined to enum for type safety and to immediately see the available options. However, in C++ it's quite a pain to convert it to/from strings (such as in file extensions), and as such, strings might prove easier to use due to fewer conversions.

[Ventus]

Unfortunately, this ultimately points us to asking even bigger questions, such as "what would sf::OutputStream look like?". And with the talk of switching to standard streams further on, it looks out of reach for the moment. Unless we could define an sf::OutputStream now.

For now, without implementing an output stream, we're looking at something like this:

bool sf::Image::saveToMemory(const std::string& format, std::vector<sf::Uint8>& buffer);

Or

bool sf::Image::saveToMemory(const sf::Image::Format& format, std::vector<sf::Uint8>& buffer);

With sf::Image::Format being an enum of png, jpg, bmp, etc. Although I personally feel that adding an enum is unecessary.

Discussion in the SFML Discord server pointed to a large preference towards sf::Uint8, so for the moment I plan on going with that.

[Nexus]

sf::InputStream looks like this:

class InputStream
{
public:
    virtual ~InputStream() {}

    virtual Int64 read(void* data, Int64 size) = 0;
    virtual Int64 seek(Int64 position) = 0;
    virtual Int64 tell() = 0;
    virtual Int64 getSize() = 0;
};

So the 1:1 counterpart would be:

class OutputStream
{
public:
    virtual ~OutputStream() {}

    virtual Int64 write(const void* data, Int64 size) = 0;
    virtual Int64 seek(Int64 position) = 0;
    virtual Int64 tell() = 0; // put pointer (bytes written to here)
    virtual Int64 getSize() = 0; // total bytes written (or available?)
};

I'm not sure if this is the best design however -- random access seems like a big constraint. This is already the case for input streams; requiring random access makes it impossible to continuously and incrementally process incoming data (i.e. streaming). For output streams, often you just want to have a sink where you put data in a fire&forget manner.

As such, there could be a more minimal version, even if this creates a slight asymmetry:

class OutputStream
{
public:
    virtual ~InputStream() {}
    virtual Int64 write(const void* data, Int64 size) = 0;
};

In Java for example, OutputStream is very minimal too.

We need to check why sf::InputStream provides seek/tell random access. I guess it's to allow partial loading of resources (e.g. audio).

[eXpl0it3r]

If you want discuss OutputStream, I suggest to create a new topic on this subject, as I'm sure it will be more than just a simple API design that needs discussion for it.

Personally, I think both saveToMemory and saveToStream are things that could be added, but as it stands saveToMemory doesn't really require yet another feature to be written first and it's something Ventus seems to be willing to implement, so maybe lets first focus on that here, while in parallel we can discuss implementations for sf::OutputStream in another thread.

[Ventus]

The Pull Request is now open: https://github.com/SFML/SFML/pull/1597
Thank you for contributing in the discussion!