diff --git a/doc/changelog-plugins.dox b/doc/changelog-plugins.dox index 1f80200de..8959bf093 100644 --- a/doc/changelog-plugins.dox +++ b/doc/changelog-plugins.dox @@ -264,7 +264,8 @@ namespace Magnum { - @relativeref{Trade,PngImporter} and @relativeref{Trade,PngImageConverter} no longer asserts if the patch version of libpng changed, such as when a system package was upgraded from 1.6.37 to 1.6.38. If the major or minor - version changes, it's still an assert. + version changes, it's now a runtime error, produced by libpng itself, + instead of an assert. @subsection changelog-plugins-latest-deprecated Deprecated APIs diff --git a/package/ci/appveyor.yml b/package/ci/appveyor.yml index 24f0e87fa..98d1b8ff0 100644 --- a/package/ci/appveyor.yml +++ b/package/ci/appveyor.yml @@ -2,32 +2,32 @@ clone_depth: 1 environment: matrix: - - TARGET: desktop - COMPILER: msvc - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015 - APPVEYOR_JOB_NAME: windows-msvc2015 - - TARGET: desktop - COMPILER: msvc - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017 - APPVEYOR_JOB_NAME: windows-msvc2017 + # - TARGET: desktop + # COMPILER: msvc + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015 + # APPVEYOR_JOB_NAME: windows-msvc2015 + # - TARGET: desktop + # COMPILER: msvc + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017 + # APPVEYOR_JOB_NAME: windows-msvc2017 - TARGET: desktop COMPILER: msvc APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019 APPVEYOR_JOB_NAME: windows-msvc2019 - - TARGET: desktop - COMPILER: msvc - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022 - APPVEYOR_JOB_NAME: windows-msvc2022 - - TARGET: desktop - COMPILER: msvc - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019 - APPVEYOR_JOB_NAME: windows-static-msvc2019 - BUILD_STATIC: ON - - TARGET: desktop - COMPILER: msvc - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022 - APPVEYOR_JOB_NAME: windows-static-msvc2022 - BUILD_STATIC: ON + # - TARGET: desktop + # COMPILER: msvc + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022 + # APPVEYOR_JOB_NAME: windows-msvc2022 + # - TARGET: desktop + # COMPILER: msvc + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019 + # APPVEYOR_JOB_NAME: windows-static-msvc2019 + # BUILD_STATIC: ON + # - TARGET: desktop + # COMPILER: msvc + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022 + # APPVEYOR_JOB_NAME: windows-static-msvc2022 + # BUILD_STATIC: ON - TARGET: desktop COMPILER: msvc-clang APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019 @@ -36,29 +36,29 @@ environment: COMPILER: msvc-clang APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022 APPVEYOR_JOB_NAME: windows-msvc2022-clang - - TARGET: desktop - COMPILER: msvc - # Same reasoning as in Corrade for /EHsc - COMPILER_EXTRA: -DCMAKE_CXX_FLAGS="/permissive- /EHsc" -DMSVC_COMPATIBILITY=OFF - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019 - APPVEYOR_JOB_NAME: windows-conforming-msvc2019 - - TARGET: desktop - COMPILER: msvc - # Not playing with fire and using /EHsc on 2022 as well - COMPILER_EXTRA: -DCMAKE_CXX_FLAGS="/permissive- /EHsc" -DMSVC_COMPATIBILITY=OFF - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022 - APPVEYOR_JOB_NAME: windows-conforming-msvc2022 - - TARGET: desktop - COMPILER: mingw - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017 - APPVEYOR_JOB_NAME: windows-mingw - - TARGET: rt - # This actually isn't needed anywhere, but without this the %COMPILE:~0,4% - # expressions cause a "The syntax of the command is incorrect." sometimes - # but not in all cases. What the fuck, cmd.exe. - COMPILER: msvc - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017 - APPVEYOR_JOB_NAME: windows-rt-msvc2017 + # - TARGET: desktop + # COMPILER: msvc + # # Same reasoning as in Corrade for /EHsc + # COMPILER_EXTRA: -DCMAKE_CXX_FLAGS="/permissive- /EHsc" -DMSVC_COMPATIBILITY=OFF + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019 + # APPVEYOR_JOB_NAME: windows-conforming-msvc2019 + # - TARGET: desktop + # COMPILER: msvc + # # Not playing with fire and using /EHsc on 2022 as well + # COMPILER_EXTRA: -DCMAKE_CXX_FLAGS="/permissive- /EHsc" -DMSVC_COMPATIBILITY=OFF + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022 + # APPVEYOR_JOB_NAME: windows-conforming-msvc2022 + # - TARGET: desktop + # COMPILER: mingw + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017 + # APPVEYOR_JOB_NAME: windows-mingw + # - TARGET: rt + # # This actually isn't needed anywhere, but without this the %COMPILE:~0,4% + # # expressions cause a "The syntax of the command is incorrect." sometimes + # # but not in all cases. What the fuck, cmd.exe. + # COMPILER: msvc + # APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017 + # APPVEYOR_JOB_NAME: windows-rt-msvc2017 install: # Ninja. `cinst ninja` started 503ing in late November 2019 and wasn't really @@ -91,7 +91,8 @@ install: # libpng, for MSVC 2022, 2019 and clang-cl. MSVC 2017 seems to work well with # the 2019 build, MinGW doesn't work at all due to ABI issues. -- IF "%TARGET%" == "desktop" IF "%COMPILER:~0,4%" == "msvc" appveyor DownloadFile https://ci.magnum.graphics/libpng-1.6.39-windows-2019.zip && 7z x libpng-1.6.39-windows-2019.zip -o%APPVEYOR_BUILD_FOLDER%\deps +- IF "%TARGET%" == "desktop" IF "%COMPILER%" == "msvc" appveyor DownloadFile https://ci.magnum.graphics/libpng-1.6.39-windows-2019.zip && 7z x libpng-1.6.39-windows-2019.zip -o%APPVEYOR_BUILD_FOLDER%\deps +- IF "%TARGET%" == "desktop" IF "%COMPILER%" == "msvc-clang" appveyor DownloadFile https://ci.magnum.graphics/libpng-1.6.39-windows-2019-clang-cl.zip && 7z x libpng-1.6.39-windows-2019-clang-cl.zip -o%APPVEYOR_BUILD_FOLDER%\deps # OpenAL - IF NOT "%TARGET%" == "rt" IF NOT EXIST %APPVEYOR_BUILD_FOLDER%\openal-soft-1.19.1-bin.zip appveyor DownloadFile https://openal-soft.org/openal-binaries/openal-soft-1.19.1-bin.zip diff --git a/src/MagnumPlugins/PngImageConverter/PngImageConverter.cpp b/src/MagnumPlugins/PngImageConverter/PngImageConverter.cpp index fb81f9bf7..1b1f7e608 100644 --- a/src/MagnumPlugins/PngImageConverter/PngImageConverter.cpp +++ b/src/MagnumPlugins/PngImageConverter/PngImageConverter.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -109,31 +110,71 @@ Containers::Optional> PngImageConverter::doConvertToData return {}; } - png_structp file = png_create_write_struct(PNG_LIBPNG_VER_STRING, nullptr, nullptr, nullptr); - /** @todo this will assert if the PNG major/minor version doesn't match, - with "libpng warning: Application built with libpng-1.7.0 but running - with 1.6.38" being printed to stdout, the proper fix is to set error - callbacks directly in the png_create_write_struct() call */ - CORRADE_INTERNAL_ASSERT(file); - png_infop info = png_create_info_struct(file); - CORRADE_INTERNAL_ASSERT(info); + /* Structures for writing the file. To avoid leaks, everything that needs + to be destructed when exiting the function has to be defined *before* + the setjmp() call below. */ + struct PngState { + png_structp file; + png_infop info; + } png{}; + /** @todo a capturing ScopeGuard would be nicer :( */ + Containers::ScopeGuard pngStateGuard{&png, [](PngState* state) { + /* Although undocumented, this function gracefully handles the case + when the file/info is null, no need to check that explicitly. */ + png_destroy_write_struct(&state->file, &state->info); + }}; std::string output; - - /* Error handling routine. Since we're replacing the png_default_error() - function, we need to call std::longjmp() ourselves -- otherwise the - default error handling with stderr printing kicks in. */ - if(setjmp(png_jmpbuf(file))) { - png_destroy_write_struct(&file, &info); + /* For exiting out of error callbacks via longjmp. Can't use the builtin + png_jmpbuf() because the errors can happen inside + png_create_read_struct(), thus even before the png.file gets filled, and + thus even before we can call setjmp(png_jmpbuf(png.file)). Yet the + official documentation says absolutely nothing about handling such case, + F.F.S. */ + std::jmp_buf jmpbuf; + /* We may arrive here from the longjmp from the error callback */ + if(setjmp(jmpbuf)) return {}; - } - png_set_error_fn(file, nullptr, [](const png_structp file, const png_const_charp message) { + + /* Create the write structure. Directly set up also error/warning + callbacks because if done here and not in a subsequent + png_set_error_fn() call, it'll gracefully handle also libpng version + mismatch errors. */ + png.file = png_create_write_struct(PNG_LIBPNG_VER_STRING, &jmpbuf, [](const png_structp file, const png_const_charp message) { Error{} << "Trade::PngImageConverter::convertToData(): error:" << message; - std::longjmp(png_jmpbuf(file), 1); - }, [](png_structp, const png_const_charp message) { - Warning{} << "Trade::PngImageConverter::convertToData(): warning:" << message; + /* Since we're replacing the png_default_error() function, we need to + call std::longjmp() ourselves -- otherwise the default error + handling with stderr printing kicks in. See above for why + png_jmpbuf() can't be used. */ + std::longjmp(*static_cast(png_get_error_ptr(file)), 1); + }, [](const png_structp file, const png_const_charp message) { + /* If there's a mismatch in the passed PNG_LIBPNG_VER_STRING major or + minor version (but not patch version), png_create_read_struct() + returns a nullptr. However, such a *fatal* error is treated as a + warning by the library, directed to the warning callback: + https://github.com/glennrp/libpng/blob/07b8803110da160b158ebfef872627da6c85cbdf/png.c#L219-L240 + That may cause great confusion ("why convertToData() returns a + NullOpt if it was just a warning??"), so I'm detecting that, + annotating it as an error instead, and jumping to the error return + so we don't end up with a nullptr `png.file` below. + + Unfortunately this case is annoyingly hard to test automatically, so + the following branch is uncovered. */ + if(Containers::StringView{message}.hasPrefix("Application built with libpng-"_s)) { + Error{} << "Trade::PngImageConverter::convertToData(): error:" << message; + std::longjmp(*static_cast(png_get_error_ptr(file)), 1); + } else + Warning{} << "Trade::PngImageConverter::convertToData(): warning:" << message; }); + /* If png_create_write_struct() failed, png.file is a nullptr. Apart from + that we may arrive here from the longjmp from the error callback. */ + if(!png.file || setjmp(png_jmpbuf(png.file))) + return {}; + + png.info = png_create_info_struct(png.file); + CORRADE_INTERNAL_ASSERT(png.info); - png_set_write_fn(file, &output, [](png_structp file, png_bytep data, png_size_t length){ + /* Set functions for writing */ + png_set_write_fn(png.file, &output, [](png_structp file, png_bytep data, png_size_t length){ auto&& output = *reinterpret_cast(png_get_io_ptr(file)); std::size_t oldSize = output.size(); output.resize(output.size() + length); @@ -141,10 +182,10 @@ Containers::Optional> PngImageConverter::doConvertToData }, [](png_structp){}); /* Write header */ - png_set_IHDR(file, info, image.size().x(), image.size().y(), + png_set_IHDR(png.file, png.info, image.size().x(), image.size().y(), bitDepth, colorType, PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE); - png_write_info(file, info); + png_write_info(png.file, png.info); /* Get data properties and calculate the initial slice based on subimage offset */ @@ -155,20 +196,19 @@ Containers::Optional> PngImageConverter::doConvertToData /* Write rows in reverse order, properly take stride into account */ if(bitDepth == 8) { for(Int y = 0; y != image.size().y(); ++y) - png_write_row(file, const_cast(data.exceptPrefix((image.size().y() - y - 1)*dataProperties.second.x()).data())); + png_write_row(png.file, const_cast(data.exceptPrefix((image.size().y() - y - 1)*dataProperties.second.x()).data())); /* For 16 bit depth we need to swap to big endian */ } else if(bitDepth == 16) { #ifndef CORRADE_TARGET_BIG_ENDIAN - png_set_swap(file); + png_set_swap(png.file); #endif for(Int y = 0; y != image.size().y(); ++y) { - png_write_row(file, const_cast(reinterpret_cast(data.exceptPrefix((image.size().y() - y - 1)*dataProperties.second.x()).data()))); + png_write_row(png.file, const_cast(reinterpret_cast(data.exceptPrefix((image.size().y() - y - 1)*dataProperties.second.x()).data()))); } } - png_write_end(file, nullptr); - png_destroy_write_struct(&file, &info); + png_write_end(png.file, nullptr); /* Copy the string into the output array (I would kill for having std::string::release()) */ Containers::Array fileData{NoInit, output.size()}; diff --git a/src/MagnumPlugins/PngImporter/PngImporter.cpp b/src/MagnumPlugins/PngImporter/PngImporter.cpp index c23ba512c..a1b4b8933 100644 --- a/src/MagnumPlugins/PngImporter/PngImporter.cpp +++ b/src/MagnumPlugins/PngImporter/PngImporter.cpp @@ -39,6 +39,7 @@ #include /* setjmp(), libpng why are you still insane */ #include #include +#include #include #include #include @@ -47,6 +48,8 @@ namespace Magnum { namespace Trade { +using namespace Containers::Literals; + PngImporter::PngImporter() = default; PngImporter::PngImporter(PluginManager::AbstractManager& manager, const Containers::StringView& plugin): AbstractImporter{manager, plugin} {} @@ -84,40 +87,78 @@ void PngImporter::doOpenData(Containers::Array&& data, DataFlags dataFlags UnsignedInt PngImporter::doImage2DCount() const { return 1; } Containers::Optional PngImporter::doImage2D(UnsignedInt, UnsignedInt) { - /* Structures for reading the file */ - png_structp file = png_create_read_struct(PNG_LIBPNG_VER_STRING, nullptr, nullptr, nullptr); - /** @todo this will assert if the PNG major/minor version doesn't match, - with "libpng warning: Application built with libpng-1.7.0 but running - with 1.6.38" being printed to stdout, the proper fix is to set error - callbacks directly in the png_create_read_struct() call */ - CORRADE_INTERNAL_ASSERT(file); - png_infop info = png_create_info_struct(file); - CORRADE_INTERNAL_ASSERT(info); - /** @todo a capturing ScopeGuard would be nicer :( */ + /* Structures for reading the file. To avoid leaks, everything that needs + to be destructed when exiting the function has to be defined *before* + the setjmp() call below. */ struct PngState { png_structp file; png_infop info; - } pngState{file, info}; - Containers::ScopeGuard pngStateGuard{&pngState, [](PngState* state) { + } png{}; + /** @todo a capturing ScopeGuard would be nicer :( */ + Containers::ScopeGuard pngStateGuard{&png, [](PngState* state) { + /* Although undocumented, this function gracefully handles the case + when the file/info is null, no need to check that explicitly. */ png_destroy_read_struct(&state->file, &state->info, nullptr); }}; Containers::Array rows; Containers::Array data; - - /* Error handling routine. Since we're replacing the png_default_error() - function, we need to call std::longjmp() ourselves -- otherwise the - default error handling with stderr printing kicks in. */ - if(setjmp(png_jmpbuf(file))) return Containers::NullOpt; - png_set_error_fn(file, nullptr, [](const png_structp file, const png_const_charp message) { + /* For exiting out of error callbacks via longjmp. Can't use the builtin + png_jmpbuf() because the errors can happen inside + png_create_read_struct(), thus even before the png.file gets filled, and + thus even before we can call setjmp(png_jmpbuf(png.file)). Yet the + official documentation says absolutely nothing about handling such case, + F.F.S. */ + std::jmp_buf jmpbuf; + /* We may arrive here from the longjmp from the error callback */ + if(setjmp(jmpbuf)) + return {}; + + /* Create the read structure. Directly set up also error/warning + callbacks because if done here and not in a subsequent + png_set_error_fn() call, it'll gracefully handle also libpng version + mismatch errors. */ + png.file = png_create_read_struct(PNG_LIBPNG_VER_STRING, &jmpbuf, [](const png_structp file, const png_const_charp message) { Error{} << "Trade::PngImporter::image2D(): error:" << message; - std::longjmp(png_jmpbuf(file), 1); - }, [](png_structp, const png_const_charp message) { - Warning{} << "Trade::PngImporter::image2D(): warning:" << message; + /* Since we're replacing the png_default_error() function, we need to + call std::longjmp() ourselves -- otherwise the default error + handling with stderr printing kicks in. See above for why + png_jmpbuf() can't be used. */ + std::longjmp(*static_cast(png_get_error_ptr(file)), 1); + }, [](const png_structp file, const png_const_charp message) { + /* If there's a mismatch in the passed PNG_LIBPNG_VER_STRING major or + minor version (but not patch version), png_create_read_struct() + returns a nullptr. However, such a *fatal* error is treated as a + warning by the library, directed to the warning callback: + https://github.com/glennrp/libpng/blob/07b8803110da160b158ebfef872627da6c85cbdf/png.c#L219-L240 + That may cause great confusion ("why image2D() returns a NullOpt if + it was just a warning??"), so I'm detecting that, annotating it as + an error instead, and jumping to the error return so we don't end up + with a nullptr `png.file` below. + + Unfortunately this case is annoyingly hard to test automatically, so + the following branch is uncovered. */ + if(Containers::StringView{message}.hasPrefix("Application built with libpng-"_s)) { + Error{} << "Trade::PngImporter::image2D(): error:" << message; + std::longjmp(*static_cast(png_get_error_ptr(file)), 1); + } else + Warning{} << "Trade::PngImporter::image2D(): warning:" << message; }); + /* If png_create_read_struct() failed with an error or "failed with a + warning" due to the version mismatch), in both cases it longjmp()'d and + already returned above. So if we get to this point, the file pointer + should always be there. */ + CORRADE_INTERNAL_ASSERT(png.file); + + // FFS + if(setjmp(jmpbuf)) + return {}; + + png.info = png_create_info_struct(png.file); + CORRADE_INTERNAL_ASSERT(png.info); /* Set functions for reading */ Containers::ArrayView input = _in; - png_set_read_fn(file, &input, [](const png_structp file, const png_bytep data, const png_size_t length) { + png_set_read_fn(png.file, &input, [](const png_structp file, const png_bytep data, const png_size_t length) { auto&& input = *reinterpret_cast*>(png_get_io_ptr(file)); if(input.size() < length) png_error(file, "file too short"); std::memcpy(data, input.begin(), length); @@ -125,15 +166,15 @@ Containers::Optional PngImporter::doImage2D(UnsignedInt, UnsignedIn }); /* Read file information */ - png_read_info(file, info); + png_read_info(png.file, png.info); /* Image size */ - const Vector2i size(png_get_image_width(file, info), png_get_image_height(file, info)); + const Vector2i size(png_get_image_width(png.file, png.info), png_get_image_height(png.file, png.info)); /* Image channels and bit depth */ - png_uint_32 bits = png_get_bit_depth(file, info); - png_uint_32 channels = png_get_channels(file, info); - png_uint_32 colorType = png_get_color_type(file, info); + png_uint_32 bits = png_get_bit_depth(png.file, png.info); + png_uint_32 channels = png_get_channels(png.file, png.info); + png_uint_32 colorType = png_get_color_type(png.file, png.info); /* Check image format, convert if necessary */ switch(colorType) { @@ -143,7 +184,7 @@ Containers::Optional PngImporter::doImage2D(UnsignedInt, UnsignedIn /* Convert to 8-bit */ if(bits < 8) { - png_set_expand_gray_1_2_4_to_8(file); + png_set_expand_gray_1_2_4_to_8(png.file); bits = 8; } @@ -163,7 +204,7 @@ Containers::Optional PngImporter::doImage2D(UnsignedInt, UnsignedIn /* Palette needs to be converted */ case PNG_COLOR_TYPE_PALETTE: - png_set_palette_to_rgb(file); + png_set_palette_to_rgb(png.file); /* png_get_bit_depth(file, info); would return the original value here (which can be < 8), expecting the png_set_*() function to give back 8-bit channels */ @@ -181,8 +222,8 @@ Containers::Optional PngImporter::doImage2D(UnsignedInt, UnsignedIn } /* Convert transparency mask to alpha */ - if(png_get_valid(file, info, PNG_INFO_tRNS)) { - png_set_tRNS_to_alpha(file); + if(png_get_valid(png.file, png.info, PNG_INFO_tRNS)) { + png_set_tRNS_to_alpha(png.file); channels += 1; if(channels == 2) colorType = PNG_COLOR_TYPE_GRAY_ALPHA; @@ -209,14 +250,14 @@ Containers::Optional PngImporter::doImage2D(UnsignedInt, UnsignedIn /* Endianness correction for 16 bit depth */ #ifndef CORRADE_TARGET_BIG_ENDIAN - if(bits == 16) png_set_swap(file); + if(bits == 16) png_set_swap(png.file); #endif /* Read image row by row */ rows = Containers::Array{std::size_t(size.y())}; for(Int i = 0; i != size.y(); ++i) rows[i] = reinterpret_cast(data.data()) + (size.y() - i - 1)*stride; - png_read_image(file, rows); + png_read_image(png.file, rows); /* 8-bit images */ PixelFormat format;