Save screenshots as PNG

[marijnvdwerf]

No description provided.

[AaronVanGeffen]

Depends on #362 and is pending a changelog entry, but other than that: LGTM.

[IntelOrca]

The warning is genuine, you shouldn't mix C++ exceptions with setjmp.

[AaronVanGeffen]

@IntelOrca Ah, interesting. Given that's it's literally what the libpng docs say, I assumed it was fine. But it's a C library, of course. How would you change the code to prevent the warning? Would just wrapping the lot in try { } catch { } have the same effect?

[IntelOrca]

Do you have a link to the docs? Maybe some compilers can do it. Anyway, it is a C library so you will have to do it the C way. Duplicate dispose calls on all tails, a goto end label or micro routines that return exit codes appropriately.

I always found setjmp a dodgy solution to error handling over exit codes anyway, I don't know if you can avoid setjmp in libpng or not.

[AaronVanGeffen]

I'm not seeing any errors on macOS or Linux, no, but it could just be that they've not been enabled.

The docs are over at http://libpng.org/pub/png/libpng-manual.txt

[janisozaur]

Libpng is infamous for the setjmp stuff. But we can still avoid it:

If you would rather avoid the complexity of setjmp/longjmp issues,
you can compile libpng with PNG_NO_SETJMP, in which case
errors will result in a call to PNG_ABORT() which defaults to abort().

You can #define PNG_ABORT() to a function that does something
more useful than abort(), as long as your function does not
return.

[IntelOrca]

On Linux it will load the shared version of libpng which won't behave how you want though right?

[AaronVanGeffen]

Thanks for the feedback. I had hoped I had come up with an acceptable solution in 3cd1761, but I'm getting the same error in AppVeyor.

Any thoughts? Do we really need to avoid setjmp entirely? If so, how does OpenRCT2 achieve this? It doesn't:

https://github.com/OpenRCT2/OpenRCT2/blob/9b5e8140f9c5c68957ccb87ff332eb87d5faba20/src/openrct2/core/Imaging.cpp#L10

https://github.com/OpenRCT2/OpenRCT2/blob/9b5e8140f9c5c68957ccb87ff332eb87d5faba20/src/openrct2/core/Imaging.cpp#L80

[IntelOrca]

Any thoughts? Do we really need to avoid setjmp entirely? If so, how does OpenRCT2 achieve this?

No you can use it, just isolate in a function without any C++ exception throws or catches.

[AaronVanGeffen]

It seems the Ubuntu Docker images were never updated to include libpng either, despite the Dockerfiles having been updated. The cause seems to be a broken webhook. We still need a solution for MinGW too.

@janisozaur Could you help out with this? I've posted some details in the team chat.

I have tested the PR locally in an Ubuntu 18.04 VM, following the Dockerfile instructions to install packages. With the patches from this PR, it compiles, links, and runs.