New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option for setting the PNG zlib compression level #10143
Conversation
b2b6105
to
e628f0a
Compare
Source/Core/Common/Image.cpp
Outdated
| // Disable warning for setjmp in C++ | ||
| #pragma warning(disable : 4611) | ||
| #endif | ||
| if (setjmp(png_jmpbuf(png_ptr)) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i seem to remember purposefully getting rid of setjmp usage in libpng-related code before; is it really required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The side effects (and reason for warning) can be pretty subtle and easy to get wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code previously used the simplified libpng API (png_image_write_to_memory); as far as I can tell there is no way to set the compression level with that API.
png_image_write_to_memory calls png_safe_execute, which uses setjmp internally. So setjmp was being used before, though not directly in Dolphin code.
I did try to isolate the setjmp call to its own function and have the C++ objects be constructed and destructed outside of that function, but I'm not sure if that's guaranteed to be safe (either by the standard or in practice due to inlining).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I'm the one who ported this over to the simplified API: IMO if you want to use the complex API, we should make a separate C (not C++) compilation unit and use the setjmp there. Otherwise this is inviting danger for anyone who touches the code in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a way to write it in C and have the write callback also be C that would be nice. It's not a huge deal to me either way but just a bit of unfortunate code pattern by libpng :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved SavePNG0 into a C file (which also required disabling precompiled headers for that file).
According to cppreference on longjmp,
No destructors for automatic objects are called. If replacing of
std::longjmpwiththrowandsetjmpwithcatchwould execute a non-trivial destructor for any automatic object, the behavior of suchstd::longjmpis undefined.
Since longjmp isn't called by the callback, I think it's fine for the callback to be in C++ code. I also don't want to try and port the old buffer-resizing code to C, partially because I think it'd be fiddly to do and partially because if the buffer ends up being too small, I don't want to run the compression process a second time (though that probably won't happen often for higher compression levels).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I suggested that the callback be C is because std::vector::insert can throw (which comes with it's own problems but might also cause dtors to be hit on paths you wouldn't expect). But, dolphin doesn't care about such exceptions at all anyway, so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently build dolphin with exceptions disabled, though I'm tempted to re-enable them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least for msvc we specify /EHsc, mostly to simplify debugging if something does throw.
e628f0a
to
d589439
Compare
d589439
to
c14e874
Compare
c14e874
to
e99ed2d
Compare
e99ed2d
to
94ccf76
Compare
When using large internal resolutions, it can take a while for the image to be compressed. This PR adds a setting to change the zlib compression level, which enables a trade-off between smaller files and faster saving. (Since PNG is lossless, the image quality is always the same).
Libpng notes this about the compression levels:
dolphin/Externals/libpng/png.h
Lines 1500 to 1509 in 890a5ed
Libpng presumably uses zlib's default of 6 by default:
dolphin/Externals/zlib/zlib.h
Lines 235 to 239 in 890a5ed
Though libpng also does have a "fast" option which uses a value of 3:
dolphin/Externals/libpng/pngwrite.c
Lines 2104 to 2116 in 890a5ed
Here are some results using frame 0 of the Melty Molten Galaxy fifolog with arbitrary mipmap detection enabled at auto aspect ratio and an internal resolution of 16x (producing a 12968 by 7296 image):
The size diff and time diff values are comparisons to level 0 followed by comparisons to the previous level.