Generalize and move ImageLoader outside of the priv namespace

[korczurekk]

While sf::Image is really slow and rarely useful, loading images is a thing that most of us likes (right?). Now if someone implements his own class to keep and manage an image (I'm working on an immutable one right now) they need to first load it with sf::Image and then copy back, and we all know how slow allocations and copies can be. It leaves a programmer with two choices:

• Get over it and do those copies
• Switch to another library like Boost.GIL

Both approaches are pretty bad, first one wastes CPU time, second one – mine (and SFML's API is really neat compared to Boost.GIL).

The solution that I'd like to suggest is to:

• Move ImageLoader outside of the priv namespace
• Make it accept any pointer to a pre–allocated memory instead of a reference to the std::vector

What do you think?

[eXpl0it3r]

Why is sf::Image slow?

I don't think this is really useful. I mean just use stb_image directly if the SFML interface doesn't suit your very specific needs.

Apropos, what are the real use cases for this?

[FRex]

Might as well allow it.., why force people to bundle another copy of STBI and reimplement the glue code, the interface is clear enough and there are 0 issues with exposing it IMO. I'd say no if it asked for stuff like supporting all stuff ImageMagicks does or non R8G8B8A8 images but this seems harmless.

Perhaps it should be made into a bunch of free or static functions also on this occasion..: https://en.sfml-dev.org/forums/index.php?topic=17108.msg123025#msg123025

[korczurekk]

Why is sf::Image slow?

• It's mutable and thus requires synchronization between threads if one can't enforce not using non–const methods
• Doesn't use SIMD (and I managed to make some functions 14x faster with Boost.SIMD)

Neither of them can be easily fixed in SFML TBH, first one doesn't suit some people (immutability enforces some additional copies in certain situations), and SIMD requires huge libraries to handle, that's not suitable for SFML too.

I don't think this is really useful. I mean just use stb_image directly if the SFML interface doesn't suit your very specific needs.

I don't think loading an array of pixels is a very specific need.

Apropos, what are the real use cases for this?

I have to process a few hundred images repeatedly as fast as possible and SFML seems to be just right as a library to load an image (and sf::Color comes in handy too). The exact project that I'm working is reproducing certain images using genetic algorithms, but it's not really relevant.

[Laurent]

Yes, we can certainly do something about ImageLoader. But keep in mind that it will still be inefficient: internally, stb_image returns its own allocated buffer, so you will never be able to directly load the pixels into your own external buffer anyway, there will still be an extra (useless) allocation & copy.

Make it accept any pointer to a pre–allocated memory instead of a reference to the std::vector

How can you pre-allocate the buffer to the correct size if you don't know the width and height of the image?

I have to process a few hundred images repeatedly as fast as possible

Then SFML is definitely NOT the right library. As you said, sf::Image is just a basic and limited container for pixels with a few loading functions. SFML was never designed for advanced image manipulation.

If you plan to make your own implementation of SIMD etc. then use stb_image, if you look at ImageLoader implementation you can see that SFML does almost nothing on top of it.
Otherwise I'm sure you can find other, dedicated libraries that will better suit your needs.

[korczurekk]

Yes, we can certainly do something about ImageLoader. But keep in mind that it will still be inefficient: internally, stb_image returns its own allocated buffer, so you will never be able to directly load the pixels into your own external buffer anyway, there will still be an extra (useless) allocation & copy.

1 copy is a lot better than two of them.

How can you pre-allocate the buffer to the correct size if you don't know the width and height of the image?

Right, stb_image obtains the size of an image and allocates the memory in the same function. Maybe something more flexible like std::back_insert_iterator? It would work with all containers from STL and a lot from other libraries. It however requires some templates, so loadFromFile/Stream/Memory would have to be moved to the ImageLoader.hpp file.

Then SFML is definitely NOT the right library. As you said, sf::Image is just a basic and limited container for pixels with a few loading functions. SFML was never designed for advanced image manipulation.

Exactly, TBH I couldn't find any decent library providing an immutable image class. That's why I wrote it myself.

If you plan to make your own implementation of SIMD etc. then use stb_image, if you look at ImageLoader implementation you can see that SFML does almost nothing on top of it.

SFML wraps C into nice C++ API making my code easier to maintain, provides sf::Vector2<T> and sf::Color minimizing the boilerplate code that I'd have to write, and is a popular library with a big community that can provide me some support when needed. That's why it's a better choice instead of the raw stb_image.

Otherwise I'm sure you can find other, dedicated libraries that will better suit your needs.

I think I covered this above.

[Laurent]

1 copy is a lot better than two of them.

If you really need fast loading, one extra allocation and copy is already too much. But of course you're the only one who knows what's acceptable and what's not

Maybe something more flexible like std::back_insert_iterator?

Flexible but in my opinion or poor choice for good performances. It will resize the container one by one element (even if std::vector for example internally uses exponential growth, that's still log(n) allocations) and copy the pixels one by one too, which is easily outperformed by a single allocation + memcpy.

SFML wraps C into nice C++ API making my code easier to maintain, provides sf::Vector2<T> and sf::Color minimizing the boilerplate code that I'd have to write, and is a popular library with a big community that can provide me some support when needed. That's why it's a better choice instead of the raw stb_image.

But in the end, with stb_image you'd have your code working instead of arguing here I still think that SFML is more limiting that helping in this specific context.

I don't get it. You request a lower-level function that just loads an image into some sort of pixel container, without extra allocations and copies. SFML doesn't provide that, but this is exactly what stb_image does with a single and easy to use function (see stbi_load). If it was me, I would definitely use the latter.

[korczurekk]

If you really need fast loading, one extra allocation and copy is already too much. But of course you're the only one who knows what's acceptable and what's not

I want relatively fast loading, it's important, but I won't waste too much time on that as I can do more important things.

Flexible but in my opinion or poor choice for good performances. It will resize the container one by one element (even if std::vector for example internally uses exponential growth, that's still log(n) allocations) and copy the pixels one by one too, which is easily outperformed by a single allocation + memcpy.

Didn't SFML already use push_back to allocate memory internally? I thought that it did, but if it's not the case then it would be huge regression, it deserves some additional thought, apparently.

But in the end, with stb_image you'd have your code working instead of arguing here I still think that SFML is more limiting that helping in this specific context.

Others may benefit from this change as well, am I a martyr now?

I don't get it. You request a lower-level function that just loads an image into some sort of pixel container, without extra allocations and copies. SFML doesn't provide that, but this is exactly what stb_image does with a single and easy to use function (see stbi_load). If it was me, I would definitely use the latter.

Well, I'll probably go for it now. It doesn't change the fact that SFML has a relatively useful feature and hides it in priv namespace. Are you hiding some bad code there and want to keep it hidden, Laurent? 

[Laurent]

Didn't SFML already use push_back to allocate memory internally?

Of course not, it uses a single resize + memcpy (the code is simple and short, don't hesitate to have a look).

Others may benefit from this change as well, am I a martyr now?

It doesn't change the fact that SFML has a relatively useful feature and hides it in priv namespace

Don't forget that I started my reply with "Yes, we can certainly do something about ImageLoader"