From 71a4e18bf8ba5b2a9569981d664269ee5e40fbbf Mon Sep 17 00:00:00 2001 From: aplefull Date: Wed, 30 Apr 2025 14:57:35 +0200 Subject: [PATCH] LibGfx: Only include frames with fcTL chunks in the animation Before this change, IDAT data was mistakenly always included in the animation. Now we only include frames with explicit fcTL chunks. As per the PNG spec (third edition): "The static image may be included as the first frame of the animation by the presence of a single fcTL chunk before IDAT. Otherwise, the static image is not part of the animation." We also fall back to the IDAT data when APNG has acTL but no fcTL chunks. Test image is 062.png from fDAT-inherits-cICP.html from WPT. --- Libraries/LibGfx/ImageFormats/PNGLoader.cpp | 20 +++++++++++++++--- Tests/LibGfx/TestImageDecoder.cpp | 16 ++++++++++++++ Tests/LibGfx/test-inputs/png/apng-1-frame.png | Bin 0 -> 445 bytes Tests/LibWeb/TestConfig.ini | 4 ---- 4 files changed, 33 insertions(+), 7 deletions(-) create mode 100644 Tests/LibGfx/test-inputs/png/apng-1-frame.png diff --git a/Libraries/LibGfx/ImageFormats/PNGLoader.cpp b/Libraries/LibGfx/ImageFormats/PNGLoader.cpp index b6cda651507..6cc65ff92c5 100644 --- a/Libraries/LibGfx/ImageFormats/PNGLoader.cpp +++ b/Libraries/LibGfx/ImageFormats/PNGLoader.cpp @@ -267,6 +267,7 @@ ErrorOr PNGLoadingContext::read_frames(png_structp png_ptr, png_infop in // Conceptually, at the beginning of each play the output buffer must be completely initialized to a fully transparent black rectangle, with width and height dimensions from the `IHDR` chunk. auto output_buffer = TRY(Bitmap::create(BitmapFormat::BGRA8888, AlphaType::Unpremultiplied, size)); auto painter = Painter::create(output_buffer); + size_t animation_frame_count = 0; for (size_t frame_index = 0; frame_index < frame_count; ++frame_index) { png_read_frame_head(png_ptr, info_ptr); @@ -289,7 +290,8 @@ ErrorOr PNGLoadingContext::read_frames(png_structp png_ptr, png_infop in return static_cast(unsigned_duration_ms); }; - if (png_get_valid(png_ptr, info_ptr, PNG_INFO_fcTL)) { + bool has_fcTL = png_get_valid(png_ptr, info_ptr, PNG_INFO_fcTL); + if (has_fcTL) { png_get_next_frame_fcTL(png_ptr, info_ptr, &width, &height, &x, &y, &delay_num, &delay_den, &dispose_op, &blend_op); } else { width = png_get_image_width(png_ptr, info_ptr); @@ -316,7 +318,10 @@ ErrorOr PNGLoadingContext::read_frames(png_structp png_ptr, png_infop in VERIFY_NOT_REACHED(); } - frame_descriptors.append({ TRY(output_buffer->clone()), duration_ms() }); + if (has_fcTL) { + animation_frame_count++; + frame_descriptors.append({ TRY(output_buffer->clone()), duration_ms() }); + } switch (dispose_op) { case PNG_DISPOSE_OP_NONE: @@ -334,6 +339,15 @@ ErrorOr PNGLoadingContext::read_frames(png_structp png_ptr, png_infop in VERIFY_NOT_REACHED(); } } + + frame_count = animation_frame_count; + + // If we didn't find any valid animation frames with fcTL chunks, fall back to using the base IDAT data as a single frame. + if (frame_count == 0) { + auto frame_bitmap = TRY(decode_frame(size)); + frame_descriptors.append({ move(frame_bitmap), 0 }); + frame_count = 1; + } } else { // This is a single-frame PNG. frame_count = 1; @@ -342,7 +356,7 @@ ErrorOr PNGLoadingContext::read_frames(png_structp png_ptr, png_infop in auto decoded_frame_bitmap = TRY(decode_frame(size)); frame_descriptors.append({ move(decoded_frame_bitmap), 0 }); } - return this->frame_count; + return frame_count; } PNGImageDecoderPlugin::~PNGImageDecoderPlugin() = default; diff --git a/Tests/LibGfx/TestImageDecoder.cpp b/Tests/LibGfx/TestImageDecoder.cpp index c837821dc41..c2b9ee55179 100644 --- a/Tests/LibGfx/TestImageDecoder.cpp +++ b/Tests/LibGfx/TestImageDecoder.cpp @@ -363,6 +363,22 @@ TEST_CASE(test_png) TRY_OR_FAIL(expect_single_frame(*plugin_decoder)); } +TEST_CASE(test_apng) +{ + auto file = TRY_OR_FAIL(Core::MappedFile::map(TEST_INPUT("png/apng-1-frame.png"sv))); + EXPECT(Gfx::PNGImageDecoderPlugin::sniff(file->bytes())); + auto plugin_decoder = TRY_OR_FAIL(Gfx::PNGImageDecoderPlugin::create(file->bytes())); + + EXPECT_EQ(plugin_decoder->frame_count(), 1u); + EXPECT_EQ(plugin_decoder->loop_count(), 0u); + + auto frame = TRY_OR_FAIL(plugin_decoder->frame(0)); + + EXPECT_EQ(frame.duration, 1000); + EXPECT_EQ(frame.image->get_pixel(64, 32), Gfx::Color(117, 252, 76)); + EXPECT_EQ(frame.image->size(), Gfx::IntSize(128, 64)); +} + TEST_CASE(test_exif) { auto file = TRY_OR_FAIL(Core::MappedFile::map(TEST_INPUT("png/exif.png"sv))); diff --git a/Tests/LibGfx/test-inputs/png/apng-1-frame.png b/Tests/LibGfx/test-inputs/png/apng-1-frame.png new file mode 100644 index 0000000000000000000000000000000000000000..e50902adb9fe038676cd5f45eb6ecc9468dbb2b0 GIT binary patch literal 445 zcmeAS@N?(olHy`uVBq!ia0y~yU}#`qU~u4IV_;yobgjOgfq{V~+0!|IhnImdkNNR? z1_lO>#N-ek1_lO31_lO(ExIokFfcGo_H=O!shIQj(n3ZC1A&$ex9eZNmk@P1qI@IQ z{dygH0s~J2qX7fk0R{;MX0kbtcm&cLESQf-Dx6~|Wa}_?n8S!i$X4{m;Y}sodEXcq z7^Ko5P6oLc>^~3#j8hm=7#JA@o}FP}V3?hj;ur$*7|0`rf(!}*2R4Xz$lfVtHh($ literal 0 HcmV?d00001 diff --git a/Tests/LibWeb/TestConfig.ini b/Tests/LibWeb/TestConfig.ini index 8ca40189502..d55a269b482 100644 --- a/Tests/LibWeb/TestConfig.ini +++ b/Tests/LibWeb/TestConfig.ini @@ -323,7 +323,3 @@ Text/input/WebSocket/echo.html ; Times out due to us not implementing auto-commit the correct way. Text/input/wpt-import/IndexedDB/idbfactory_open.any.html - -; Inconsistently fails on CI - we seem to wrongly include the non-animated data in the animation. -; https://github.com/LadybirdBrowser/ladybird/issues/4519 -Ref/input/wpt-import/png/apng/fDAT-inherits-cICP.html