[PNG] save_png() now works with std::vector and std::ostream

[pfeatherstone]

• save_png() works with std::vector<byte,alloc>
• save_png() works with std::ostream
• Optimization for bgr_pixel. No longer have to convert image to array2d<rgb_pixel>
• Added tests to image.cpp which checks save_png() works with matrix
• Added tests to image.cpp which checks the different ways of saving
• I've temporarily removed the overload for matrix which converts to array2d. The tests don't fail so I don't know why it's there. It unnecessarily incurs a huge allocation. I imagine it handles the case where matrix_exp is an expression not a matrix. Who does that? Can we have an overload for that specifically? I've added an overload for matrix and for matrix_exp. The latter does a copy. The former converts to const_image_view which is zero-copy.
• I've noticed the implementation of load_png() supports buffers but it's really verbose. I managed to get everything working for save_png using a callback. I imagine the same can be done for load_png() and reduce the code bloat. We could then also support std::istream Done it

[pfeatherstone]

At the moment, since the implementation details are located in a .cpp file, the callback is stored in a std::function, which means there could be a dynamic allocation if SBO is not possible, and there is an indirection which means the compiler is less likely to inline. This isn't a big deal. But if everything were in a header, then the callback could be templated and inlined. Is there a reason why we can't make everything header only?

[arrufat]

I've temporarily removed the overload for matrix which converts to array2d. The tests don't fail so I don't know why it's there. It unnecessarily incurs a huge allocation. I imagine it handles the case where matrix_exp is an expression not a matrix. Who does that? Can we have an overload for that specifically?

I do. I sometimes use join_rows or join_cols to display images side by side.

[pfeatherstone]

Ok. Can we add a test for that? Is there a way to ensure that the copy only happens when it's an expression not a matrix ?

[davisking]

I've temporarily removed the overload for matrix which converts to array2d. The tests don't fail so I don't know why it's there. It unnecessarily incurs a huge allocation. I imagine it handles the case where matrix_exp is an expression not a matrix. Who does that? Can we have an overload for that specifically?

I do. I sometimes use join_rows or join_cols to display images side by side.

Yeah same. It's a convenient overload. Definitely don't want to remove it.

Also be sure to update save_png_abstract.h too.

[davisking]

Totally keep it in a .cpp file too. It's to hide the ping header file. That way it doesn't come pollute caller code. But also so people's wacky build setups don't get more chance to screw up what #include <png.h>. You would be surprised about the number of emails I've gotten that are some form of "my system has a broken $XXX and I'm getting this error about libpng or libjpeg not working".

And in the limit if everything is in every header build times of client code go up which is a bummer too. So the std::function is fine.

[davisking]

Ok. Can we add a test for that? Is there a way to ensure that the copy only happens when it's an expression not a matrix ?

Yeah just add an overload specifically for dlib::matrix and you would be good to go.

[pfeatherstone]

Totally keep it in a .cpp file too. It's to hide the ping header file. That way it doesn't come pollute caller code. But also so people's wacky build setups don't get more chance to screw up what #include <png.h>. You would be surprised about the number of emails I've gotten that are some form of "my system has a broken $XXX and I'm getting this error about libpng or libjpeg not working".

And in the limit if everything is in every header build times of client code go up which is a bummer too. So the std::function is fine.

Ok cool. I was thinking of going in the same direction as the ffmpeg wrappers. Making everything header only eliminates problems associated with linking against the correct version of some library since nothing is compiled into libdlib.a

[davisking]

Totally keep it in a .cpp file too. It's to hide the ping header file. That way it doesn't come pollute caller code. But also so people's wacky build setups don't get more chance to screw up what #include <png.h>. You would be surprised about the number of emails I've gotten that are some form of "my system has a broken $XXX and I'm getting this error about libpng or libjpeg not working".

And in the limit if everything is in every header build times of client code go up which is a bummer too. So the std::function is fine.

Ok cool. I was thinking of going in the same direction as the ffmpeg wrappers. Making everything header only eliminates problems associated with linking against the correct version of some library since nothing is compiled into libdlib.a

Right. It does. But then you find out people have funky copies of png.h floating around that make things not compile. I don't know what the deal is with it. It seems to mostly be a png thing. Like someone did something naughty and broke a bunch of systems. I forget what all it was. Some of it was anaconda. Which is a perennial source of "someone changed something they shouldn't have and broke the system".

[davisking]

It's why I hate dependencies. People can't help but make them broken or incompatible somehow with the official version of that dependency. And then there is complaining when down stream stuff doesn't work with their odd ball version of it.

[pfeatherstone]

I think this is ready

[pfeatherstone]

I've refactored load_png() to also support iostreams using a similar callback mechanism. I might put that in a separate PR if people are interested. The implementation also mirrors this one which is cool. But it's not necessary since load_png() supports reading from memory in some form. It also saves nearly 100 lines of code.

[pfeatherstone]

I've pushed the refactored loading implementation details. I think it's nicer, supports iostreams and mirrors the saving implementations details. It's personal preference I know. I can always revert if people don't like.

[pfeatherstone]

I can update docs for load_png() later.

[pfeatherstone]

I think the macos-latest runner is stuck.

[pfeatherstone]

@davisking do you mind restarting the macos runner?

[pfeatherstone]

Ok, finally ready. I promise I won't touch anything.

[pfeatherstone]

@davisking not a fan ?

[davisking]

@davisking not a fan ?

Ah no it's great. I was just traveling and in Texas the whole week. Just got back and looking at this now.

[davisking]

Yeah that's great stuff :D

Totally keep the png_loader though. Some people totally use that thing.

[pfeatherstone]

Bugger. I'll revert the loading API changes. I don't quite see the use case for png_loader. It's not doing anything special except for RAII stuff. But ok

[davisking]

It tells you what kind of image you have, which is sometimes useful. Like you might want to know definitively if the PNG has an alpha channel. That could just be some separate global function though. The real reason is I don't want to break user code. It doesn't cost anything to let the code just sit there and your most loyal users are the ones who have being using a tool the longest. And breaking old APIs selectively causes trouble for that group, which at some level is the group you least want to bother :)

[pfeatherstone]

Fair enough I'll sort it out.

[pfeatherstone]

@davisking If everything passes, then this is done.

[pfeatherstone]

@davisking Can we merge this?

[davisking]

Yeah, this is real nice :D