Skip to content

Run GIF tests again, and fix regression for short reads

Simon McVittie requested to merge wip/run-gif-tests-again into master
  • build: Do not install .test files when test is skipped

    From: Jan Tojnar (from !94 (merged))

    The skipping prevented the executable from being installed but not the .test file that referenced it. This caused installed tests to fail:

          Running test: gdk-pixbuf/pixbuf-pixdata.test
          Caught exception during testing: Failed to execute child process ?/nix/store/kqmj2776mw24qxyswfbqlmybpws4g4yn-gdk-pixbuf-2.42.0-installedTests/libexec/installed-tests/gdk-pixbuf/pixbuf-pixdata? (No such file or directory)
  • gif: Do all of gif_init() with a single read

    As documented in the introductory comment, the interface of the various functions in the GIF loader is that they read all the bytes they need, or return -1 if not enough are available. Since commit 5212d69f "gif: Replace old buffer management code with GByteArray", the incremental loader strictly depends on that assumption.

    Unfortunately, gif_init() didn't conform to that interface. If there were enough bytes available for the GIF signature (GIF87a or GIF89a) but not enough bytes for the screen descriptor, it would return -1 having already consumed the first 6 bytes of the stream. A subsequent retry with more data available would start from the beginning of the screen descriptor, and immediately raise an error because the screen descriptor is extremely unlikely to start with another copy of the "GIF8" magic number.

    The regression test tests/pixbuf-short-gif-write.c would have detected this, but was accidentally disabled by commit 7f0b214a "tests: Conditionally build and run tests".

    This seems most easily fixed by reading the whole of the 13-byte fixed-length header in one go. Adjust the offsets into the buffer used to parse the screen descriptor accordingly.

  • tests: Don't check whether bmp and gif loaders are enabled

    This is conceptually similar to commit 2fd7d21f "tests: Fix GIF tests being permanently disabled". One way or another, gdk-pixbuf always supports these two formats: on Windows with the native gdiplus loader enabled, it covers these two formats; otherwise, format-specific loaders are used.

    This means we will run the GIF tests, as intended.

    Fixes: 7f0b214a "tests: Conditionally build and run tests"


This continues from !94 (merged) to resurrect the GIF tests.

In my initial attempt at this, without the second commit, tests/pixbuf-short-gif-write.c failed:

ERROR:../tests/pixbuf-short-gif-write.c:32:loader_write_from_channel: assertion failed (error == NULL): File does not appear to be a GIF file (gdk-pixbuf-error-quark, 0)

I bisected to find the regression and found that 5212d69f "gif: Replace old buffer management code with GByteArray" breaks that test. Reverting that commit fixed it, but broke tests/pixbuf-gif.c instead:

# Start of gif tests
# GLib-GIO-DEBUG: _g_io_module_get_default: Found default implementation gvfs (GDaemonVfs) for ?gio-vfs?
Bail out! ERROR:../tests/pixbuf-gif.c:150:run_gif_test: assertion failed (delay_time == expected_delay_time): (20 == 100)

It isn't clear to me why that happened, but fixing gif_init() to conform to the required interface seems to fix both tests.

/cc @jtojnar @ebassi @robert.ancell

Edited by Simon McVittie

Merge request reports