Compilation fails when libpng does not support APNG

[sideeffect42]

Compilation fails when linking against a libpng without the apng patch set applied.

Would it be possible to detect this at compile-time and disable APNG support if libpng does not support it either?

FAILED: Lagom/Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/PNGLoader.cpp.o 
/usr/bin/c++ -DENABLE_COMPILETIME_FORMAT_CHECK -DLibGfx_EXPORTS -I/tmp/ladybird -I/tmp/ladybird/Userland/Services -I/tmp/ladybird/Userland/Libraries -I/tmp/ladybird/Build/ladybird/Lagom -I/tmp/ladybird/Build/ladybird/Lagom/Userland/Services -I/tmp/ladybird/Build/ladybird/Lagom/Userland/Libraries -I/tmp/ladybird/Meta/Lagom/../.. -I/tmp/ladybird/Meta/Lagom/../../Userland -I/tmp/ladybird/Meta/Lagom/../../Userland/Libraries -I/tmp/ladybird/Meta/Lagom/../../Userland/Services -I/tmp/ladybird/Build/ladybird -O2 -pipe -march=armv8-a+crc+crypto -mtune=cortex-a72.cortex-a53 -mabi=lp64 -fstack-protector-strong -mharden-sls=all -std=c++23 -fPIC -fdiagnostics-color=always -Wall -Wextra -fno-exceptions -ffp-contract=off -Wno-invalid-offsetof -Wno-unknown-warning-option -Wno-unused-command-line-argument -Werror -Wno-expansion-to-defined -Wno-literal-suffix -Wno-dangling-reference -fno-semantic-interposition -fvisibility-inlines-hidden -Wno-maybe-uninitialized -Wno-shorten-64-to-32 -fsigned-char -ggnu-pubnames -fPIC -D_FILE_OFFSET_BITS=64 -O2 -g1 -MD -MT Lagom/Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/PNGLoader.cpp.o -MF Lagom/Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/PNGLoader.cpp.o.d -o Lagom/Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/PNGLoader.cpp.o -c /tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp: In function ‘AK::ErrorOr<AK::NonnullRefPtr<Gfx::Bitmap> > Gfx::render_animation_frame(const AnimationFrame&, const AnimationFrame&, const Bitmap&)’:
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:210:10: error: ‘PNG_DISPOSE_OP_BACKGROUND’ was not declared in this scope
  210 |     case PNG_DISPOSE_OP_BACKGROUND:
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:213:10: error: ‘PNG_DISPOSE_OP_PREVIOUS’ was not declared in this scope
  213 |     case PNG_DISPOSE_OP_PREVIOUS:
      |          ^~~~~~~~~~~~~~~~~~~~~~~
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:220:10: error: ‘PNG_BLEND_OP_SOURCE’ was not declared in this scope
  220 |     case PNG_BLEND_OP_SOURCE:
      |          ^~~~~~~~~~~~~~~~~~~
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:223:10: error: ‘PNG_BLEND_OP_OVER’ was not declared in this scope
  223 |     case PNG_BLEND_OP_OVER:
      |          ^~~~~~~~~~~~~~~~~
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp: In member function ‘AK::ErrorOr<long unsigned int, AK::Error> Gfx::PNGLoadingContext::read_frames(png_structp, png_infop)’:
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:234:9: error: ‘png_get_acTL’ was not declared in this scope; did you mean ‘png_get_sCAL’?
  234 |     if (png_get_acTL(png_ptr, info_ptr, &frame_count, &loop_count)) {
      |         ^~~~~~~~~~~~
      |         png_get_sCAL
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:237:9: error: ‘png_set_acTL’ was not declared in this scope; did you mean ‘png_set_sCAL’?
  237 |         png_set_acTL(png_ptr, info_ptr, frame_count, loop_count);
      |         ^~~~~~~~~~~~
      |         png_set_sCAL
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:240:13: error: ‘png_read_frame_head’ was not declared in this scope
  240 |             png_read_frame_head(png_ptr, info_ptr);
      |             ^~~~~~~~~~~~~~~~~~~
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:250:51: error: ‘PNG_INFO_fcTL’ was not declared in this scope; did you mean ‘PNG_INFO_pCAL’?
  250 |             if (!png_get_valid(png_ptr, info_ptr, PNG_INFO_fcTL)) {
      |                                                   ^~~~~~~~~~~~~
      |                                                   PNG_INFO_pCAL
/tmp/ladybird/Userland/Libraries/LibGfx/ImageFormats/PNGLoader.cpp:254:13: error: ‘png_get_next_frame_fcTL’ was not declared in this scope
  254 |             png_get_next_frame_fcTL(png_ptr, info_ptr, &width, &height, &x, &y, &delay_num, &delay_den, &dispose_op, &blend_op);
      |             ^~~~~~~~~~~~~~~~~~~~~~~

[dzfrias]

I ran into a similar issue earlier. vcpkg should pull in a version of libpng with the correct patchset, because libpng is a vcpkg dependency for Ladybird. This problem arises because clang is trying to get use system's libpng.

The problem (on my system) was that I was overriding $CPATH and $LIBRARY_PATH (I overrode them trying to fix a separate build issue). I'm not sure if that specific fix is relevant to you, but hopefully it gives you a starting point.

[sideeffect42]

I'm neither using clang nor vcpkg. I build directly from source using system tools and GCC.

While I can USE=apng emerge media-libs/libpng on Gentoo to get libpng with the APNG patchset installed, this is a lot harder on other distros (e.g. Debian).

[ADKaster]

We're in a similar situation to Firefox in this regard. APNG is supported by all the other browsers, so we want it as well. So whatever those distros do for Firefox wanting the APNG patch is the same deal we're going to ask for

[sideeffect42]

I'm not against APNG, I'm just asking for APNG support being guarded behind #ifdef PNG_APNG_SUPPORTED. This way, when Ladybird is compiled against a patched libpng you get the niceties of animated PNGs and if not, well, not.

A browser without APNG is still better than no browser, right?

[circl-lastname]

I'm neither using clang nor vcpkg. I build directly from source using system tools and GCC.

That's weird, the build system is supposed to pull in libpng from vcpkg. Was that changed?

Edit: I was unaware that the PR purposefully used a non-vcpkg version of libpng

[ADKaster]

That's weird, the build system is supposed to pull in libpng from vcpkg. Was that changed?

vcpkg is a package manager, not a build system. its job is to find packages for us, and help us link to them. In a perfect world using a system package manager would work as well.

That being said, we simply must have APNG. It is supported by everyone.

https://github.com/pnggroup/libpng/issues/267 https://caniuse.com/apng

We should be looking for a solution to get APNG working in more environments, rather than disabling the feature because some distros don't want to ship a patch they consider less than ideal.

[vpzomtrrfrt]

Is there at least some way to give a better error message here?

[zack466]

I also had this issue on MacOS, and like @dzfrias said, it was because my system's libpng installation was getting included instead of the one installed by vcpkg. For me, the build system kept putting -isystem /opt/local/include into the compile command, and I couldn't figure out how to get rid of it, so I eventually just renamed /opt/local/include/png.h to /opt/local/include/png.h.bak and the error went away. Super hacky, but it seems to have worked.

[vitalyster]

@ADKaster while vcpkg currently preferred by developers, it is not the only way to find packages. If you must have APNG you need to check its existence and show correct error message if it is not present