Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deimplementing complications in libpng18 #659

Open
wants to merge 3 commits into
base: libpng18
Choose a base branch
from

Conversation

ctruta
Copy link
Member

@ctruta ctruta commented Feb 21, 2025

Here are three honking commits for your viewing and reviewing pleasure.

FYI, projects/owatcom and scripts/makefile.intel are about to go, because they depend on what I'm about to remove in this PR. And I could fix those but I ain't gonna. And CMake should be able to do their job, anyway. I haven't tested Open Watcom recently, but last time I did give it a try, about two years ago, its IDE died a horrible death when I attempted to do stuff with projects/owatcom on my Windows 10 laptop.

Also about Watcom: fun fact: I used to live and work in Waterloo, Ontario for aboot a decade, and two former Watcom compiler engineers used to be in my team at BlackBerry for a while. Because, you know... Waterloo... compiler... wink-wink 😉

Remove #ifdef sections and other workarounds for old Windows compilers
that lacked proper support for Win32, including, especially, support
for the Win32 stdio API.
We should use `FILE *` instead of `FILE*` or `(FILE*)`, consistently,
as we should for all other pointer types. Moreover, when we refer to
standard stdio file objects in comments and in documentation, we should
use the term "FILE objects" consistently.

Lastly, we clarify in a comment in example.c that `PNG_STDIO_SUPPORTED`
is true only when the stdio support is both available in the system and
accessible in the user's libpng build.
The computer industry has spoken rather clearly: we shall have
8-bit bytes, powers-of-2- and multiples-of-8-bit ints and floats,
one-size-fits-all pointers, and C API calling conventions for
system-level interoperability between applications and libraries.

So we're going to have all that and we're going to like it. Which
we do. We use the opportunity to shed annotations and make libpng
less baroque, both in interface and in implementation.
@ctruta
Copy link
Member Author

ctruta commented Feb 21, 2025

@jbowler I wanted to add you in as a reviewer, but you being in and out of this group resulted in me having to summon you hereby, because GitHub's "Reviewers" feature doesn't let me add you in there.

Copy link
Contributor

@jbowler jbowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Mostly copy edit so it should be fine if it compiles. I can't test the maybe-significant change which is the removal of PNGAPI/PNGCBAPI; they were always no-ops on the systems I use. PNGFAPI looks fine but I need to do a gh pr pull and check the minconfigs; again any issues will be fail-to-compile, not some fundamental problem with the change.

@jbowler
Copy link
Contributor

jbowler commented Feb 21, 2025

The configure build fails (on AMD64) thus:

rm -f pnglibconf.c pnglibconf.tf[45]
rm -f scripts/pnglibconf/symbols.out scripts/pnglibconf/symbols.tf[12]
make: *** No rule to make target 'scripts/pnglibconf/symbols.def', needed by 'scripts/pnglibconf/symbols.chk'.  Stop.

The cmake build doesn't try to build it (I think you removed it), however it fails thus:

     -g -O2 --std=c90 -pedantic -Wcast-align -Wshadow -Werror -fPIC -MD -MT CMakeFiles/png_shared.dir/pngwutil.c.o -MF CMakeFiles/png_shared.dir/pngwutil.c.o.d -o CMakeFiles/png_shared.dir/pngwutil.c.o -c /home/jbowler/src/libpng/github/git/pngwutil.c
In file included from /home/jbowler/src/libpng/github/git/pngpriv.h:205,
                 from /home/jbowler/src/libpng/github/git/png.c:13:
/home/jbowler/src/libpng/github/git/png.h:1039:43: error: ISO C does not allow extra ‘;’ outside of a function [-Werror=pedantic]
 1039 |     png_const_timep ptime),PNG_DEPRECATED);

I.e. that pesky macro again.

@jbowler
Copy link
Contributor

jbowler commented Feb 21, 2025

cmake is fixed by removing the three(?) spurious ";" characters from the end of the PNG_REMOVED macros in png.h.

configure build is fixed by changing Makefile.am to refer to scripts/symbols.def, not scripts/pnglibconf/symbols.def. (Maybe none of the "symbols" stuff should be in scripts/pnglibconf, i.e. not symbols.c, checksym.awk but it looks like you intended symbols.def to be there too).

The configure "make check" fails at pngtest-all, which is weird, but there is not corresponding cmake test so far as I can see. It fails on:

pngtest contrib/testpngs/badpal/test-palette-8.png

Because the error message that is emitted:

libpng error: Wrote palette index exceeding num_palette

Doesn't seem to be found:

MISSING REPORT: contrib/testpngs/badpal/test-palette-8.png: IDAT: Read palette index exceeding num_palette
rwrwrwrwrwrwrwrwrwrwrwrwrwrwrwrw=============== FAIL(EXPECTED) contrib/testpngs/badpal/test-palette-8.png ====================
MISSING REPORT: contrib/testpngs/badpal/test-palette-8.png: Wrote palette index exceeding num_palette
FAIL tests/pngtest-all (exit status: 1)

@jbowler
Copy link
Contributor

jbowler commented Feb 21, 2025

You removed STDERR from pngtest.c, so now the output does not go to stdout.

EDIT: redirecting to stdout fixes the problem.

@jbowler
Copy link
Contributor

jbowler commented Feb 21, 2025

With those changes (Makefile.am scripts.def, png.h PNG_REMOVED and tests/pngtest-all 2>&1 in TEST) the base builds all pass except for aarch64. The latter is not new; the assembler does not compile with -pedantic, from cmake:

In file included from /home/jbowler/src/libpng/github/git/arm/arm_init.c:78,
                 from /home/jbowler/src/libpng/github/git/pngsimd.c:57:
/home/jbowler/src/libpng/github/git/arm/palette_neon_intrinsics.c: In function ‘png_riffle_palette_neon’:
/home/jbowler/src/libpng/github/git/arm/palette_neon_intrinsics.c:21:7: error: initializer element is not computable at load time [-Werror=pedantic]
   21 |       vdupq_n_u8(0x00),
      |       ^~~~~~~~~~
/home/jbowler/src/libpng/github/git/arm/palette_neon_intrinsics.c:22:7: error: initializer element is not computable at load time [-Werror=pedantic]
   22 |       vdupq_n_u8(0x00),
      |       ^~~~~~~~~~
/home/jbowler/src/libpng/github/git/arm/palette_neon_intrinsics.c:23:7: error: initializer element is not computable at load time [-Werror=pedantic]
   23 |       vdupq_n_u8(0x00),
      |       ^~~~~~~~~~
/home/jbowler/src/libpng/github/git/arm/palette_neon_intrinsics.c:24:7: error: initializer element is not computable at load time [-Werror=pedantic]
   24 |       vdupq_n_u8(0xff),
      |       ^~~~~~~~~~
/home/jbowler/src/libpng/github/git/arm/palette_neon_intrinsics.c:20:22: error: initializer element is not computable at load time [-Werror=pedantic]
   20 |    uint8x16x4_t w = {{
      |                      ^
/home/jbowler/src/libpng/github/git/arm/arm_init.c: In function ‘png_target_do_expand_palette_neon’:
/home/jbowler/src/libpng/github/git/arm/arm_init.c:148:10: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  148 |          png_bytep dp = row + (4/*RGBA*/*row_width - 1);
      |          ^~~~~~~~~

It's the same in libpng-1.6, I've been ignoring it.

@jbowler
Copy link
Contributor

jbowler commented Feb 21, 2025

The "minconfig" builds with configure seem to be as expected. The ones with cmake seem to have stopped working - it looks like everything gets built.

@jbowler
Copy link
Contributor

jbowler commented Feb 22, 2025

cmake "minconfig" builds seem consistent too; I had typo'ed (or probably edito'ed) DFA_XTRA to DFA_XTA on the command line for cmake. I did notice that despite setting 'VERBOSE=1' on the corresponding "make" the cmake sub-command doesn't seem to generate any output - the cmake for OUTPUT=pnglibconf.h is recorded in the log file but not the executed commands.

@jbowler
Copy link
Contributor

jbowler commented Feb 22, 2025

My three fixes are in #660

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants