[workspace] Use libpng from BCR

[bobpaw]

Loses drake specific intel msse4.1 SIMD instructions and addenda to pnglibconf.h to disable out-of-line functions and suppress exif warnings.

Note this will not be merged into master until after the zlib deprecation period ends (see #24814).

Toward #24792.

---

This change is 

[bobpaw]

+a:@tyler-yankee for feature review please

[tyler-yankee]

Basically LGTM.

@tyler-yankee reviewed 9 files and all commit messages, and made 4 comments.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on bobpaw).

---

-- commits line 4 at r1:

Suggestion:

Loses drake-specific Intel msse4.1 SIMD instructions and addenda to

---

tools/workspace/default.bzl line 168 at r1 (raw file):

        "zlib",
    ]
    for name in ALIAS_REPOSITORIES:

BTW this function is getting slightly ugly, and will likely need to grow up to define a proper mapping above, instead of a list where we pretend {name: actual} is the right spelling but then special-case it enough times that the cases are no longer so "special."

This PR doesn't necessarily have to do it, but if the intent is for this PR to live on a branch with the other conversions pending zlib deprecation removal, then it's something to consider. We could land that on master ahead of time.

---

tools/workspace/libpng_internal/png_drake_vendor.h line 9 at r1 (raw file):

/* Suppress benign eXIf warnings, as they produce unnecessary test output. */
#undef PNG_READ_eXIf_SUPPORTED

Wow, BCR writes a genrule that completely blocks this? Upstream defines a configure option that would theoretically allow us to swap in our own header, where we could copy their header (writing a lint test / upgrade script to sync it) and #undef what we need. I'm not sure how that would propagate to downstream drake projects though.

It'll be a question for platform in terms of how much we care to keep using these settings.

[bobpaw]

tools/workspace/default.bzl line 168 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

BTW this function is getting slightly ugly, and will likely need to grow up to define a proper mapping above, instead of a list where we pretend {name: actual} is the right spelling but then special-case it enough times that the cases are no longer so "special."

This PR doesn't necessarily have to do it, but if the intent is for this PR to live on a branch with the other conversions pending zlib deprecation removal, then it's something to consider. We could land that on master ahead of time.

Ok, I'll see about doing that after this commit.

[bobpaw]

tools/workspace/libpng_internal/png_drake_vendor.h line 9 at r1 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Wow, BCR writes a genrule that completely blocks this? Upstream defines a configure option that would theoretically allow us to swap in our own header, where we could copy their header (writing a lint test / upgrade script to sync it) and #undef what we need. I'm not sure how that would propagate to downstream drake projects though.

It'll be a question for platform in terms of how much we care to keep using these settings.

I'm not even really sure how necessary the drake-specifics are. Jeremy and Aiden are in the blame. @rpoyner-tri do you have an idea here?

[rpoyner-tri]

tools/workspace/libpng_internal/png_drake_vendor.h line 9 at r1 (raw file):

Previously, bobpaw (Aiden Woodruff) wrote…

I'm not even really sure how necessary the drake-specifics are. Jeremy and Aiden are in the blame. @rpoyner-tri do you have an idea here?

👀

[rpoyner-tri]

@rpoyner-tri made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on bobpaw and tyler-yankee).

---

tools/workspace/libpng_internal/png_drake_vendor.h line 9 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

👀

I don't know that either of these two undef changes are life-critical.

AFAICT the "avoid slow functions" thing is a matter of emphasis and not necessity. Looks to me like the macro implementations would still be the default choice, even without the undef. I suppose that in order to tell the difference, we'd have to either dig through build output, and/or write some benchmark.

As for the exif log spam reduction, can we just try some builds and see how bad it gets?

[bobpaw]

tools/workspace/libpng_internal/png_drake_vendor.h line 9 at r1 (raw file):

As for the exif log spam reduction, can we just try some builds and see how bad it gets?

I don't see any warnings output in the CI or by bazel test --test_output=all //geometry/render_vtk/.... Further relevant details are in #22728.

[bobpaw]

tools/workspace/libpng_internal/png_drake_vendor.h line 9 at r1 (raw file):

Previously, bobpaw (Aiden Woodruff) wrote…

As for the exif log spam reduction, can we just try some builds and see how bad it gets?
I don't see any warnings output in the CI or by bazel test --test_output=all //geometry/render_vtk/.... Further relevant details are in #22728.

Disregard my previous comment. I had built on the wrong branch. I do see these warnings popping up.

[tyler-yankee]

@tyler-yankee reviewed 7 files and all commit messages, and made 2 comments.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on bobpaw, rpoyner-tri, SeanCurtis-TRI, and tyler-yankee).

---

tools/workspace/default.bzl line 168 at r1 (raw file):

Previously, bobpaw (Aiden Woodruff) wrote…

Ok, I'll see about doing that after this commit.

(#24916)

---

-- commits line 9 at r3:
It's fair to push this commit here -- to verify that the warnings go away when switching to BCR -- but we should land the change in a separate PR to master. Probably @SeanCurtis-TRI will be interested.

[SeanCurtis-TRI]

-- commits line 9 at r3:

Previously, tyler-yankee (Tyler Yankee) wrote…

It's fair to push this commit here -- to verify that the warnings go away when switching to BCR -- but we should land the change in a separate PR to master. Probably @SeanCurtis-TRI will be interested.

I haven't run this code yet, but the sense I've gleaned is that:

1. Moving to BCR caused that libpng to spew to the console on some of our pngs.
2. The fix was to change the pngs.

If that basic gist is correct, it leads to the question of how do we avoid committing pngs that have chunks that libpng doesn't like? This feels like a data-integrity control issue.

If I'm wrong, could someone recap the situation for me? Thanks.

[tyler-yankee]

@tyler-yankee made 1 comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on bobpaw, rpoyner-tri, and SeanCurtis-TRI).

---

-- commits line 9 at r3:
That's mostly correct.

It's actually an upstream libpng change/release that caused the new warnings, which we disabled a while ago; see #22728, #22861.

The issue here is that when we move towards obtaining libpng from BCR, we lose control over its BUILD rules, and thus our prior warning suppression. So Aiden is proposing simply fixing drake's "defective" images (even though as Jeremy originally pointed out, the metadata issues are benign for the purposes of the tests).

how do we avoid committing pngs that have chunks that libpng doesn't like?

Short of a developer noticing the warnings once we turn them back on and fix the existing case, it seems like we'd have to assert on test case outputs or something, but I'm not familiar enough there to say.

[bobpaw]

-- commits line 9 at r3:

It's fair to push this commit here -- to verify that the warnings go away when switching to BCR -- but we should land the change in a separate PR to master.

Agreed and that was exactly my intention.

how do we avoid committing pngs that have chunks that libpng doesn't like

The fixed images were invalid PNGs, but most readers can handle them just fine. See Table 7 in Section 5.6 of the PNG spec. Specifically that eXIf chunks ought to be before IDAT chunks. On a personal developer level, I typically have a rule like this in my .gitconfig:

[diff "exif"]
	textconv = exiftool

and the following in my $XDG_CONFIG_HOME/git/attributes:

# Show image diffs with exiftool
*.png diff=exif
*.jpg diff=exif
*.jpeg diff=exif

Then git diff can show the exif output which includes the warning. Maybe it makes sense to add a github action that checks exiftool for Warning/Error messages for newly submitted images.

[SeanCurtis-TRI]

-- commits line 9 at r3:

Previously, bobpaw (Aiden Woodruff) wrote…

It's fair to push this commit here -- to verify that the warnings go away when switching to BCR -- but we should land the change in a separate PR to master.

Agreed and that was exactly my intention.

how do we avoid committing pngs that have chunks that libpng doesn't like

The fixed images were invalid PNGs, but most readers can handle them just fine. See Table 7 in Section 5.6 of the PNG spec. Specifically that eXIf chunks ought to be before IDAT chunks. On a personal developer level, I typically have a rule like this in my .gitconfig:

[diff "exif"]
	textconv = exiftool

and the following in my $XDG_CONFIG_HOME/git/attributes:

# Show image diffs with exiftool
*.png diff=exif
*.jpg diff=exif
*.jpeg diff=exif

Then git diff can show the exif output which includes the warning. Maybe it makes sense to add a github action that checks exiftool for Warning/Error messages for newly submitted images.

I'd advocate for something that runs pre-merge that would prevent us from accidentally merging invalid pngs.

[jwnimmer-tri]

The invalid pngs discussion lost the plot. I'll circle back within a few days to comment.

[jwnimmer-tri]

@jwnimmer-tri reviewed 9 files, made 5 comments, and resolved 1 discussion.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on bobpaw and tyler-yankee).

---

-- commits line 9 at r3:

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'd advocate for something that runs pre-merge that would prevent us from accidentally merging invalid pngs.

Drake must not spuriously warn about exif data that we don't even care about. We must fix the warning to not trigger.

---

tools/workspace/libpng_internal/png_drake_vendor.h line 9 at r1 (raw file):

AFAICT the "avoid slow functions" thing is a matter of emphasis and not necessity. Looks to me like the macro implementations would still be the default choice, even without the undef.

We need to confirm this before casually dropping it. Checking the disassembly manually locally shouldn't be very hard.

As for the exif log spam reduction, can we just try some builds and see how bad it gets?

We don't want Drake to emit spurious warnings.

We need to configure libpng to disable exif support, like we had previously. At first glance, it does seem like this will require an amendment to the BCR rule(s).

---

tools/workspace/libpng_internal/package.BUILD.bazel line 85 at r3 (raw file):

        # Don't allow the pngpriv.h auto-sensing of Neon to take effect.
        "-DPNG_ARM_NEON_OPT=0",
        "-DPNG_ARM_NEON_IMPLEMENTATION=0",

Please explain (in the review discussion here) why it's OK to re-enable Neon.

Code quote:

        # Don't allow the pngpriv.h auto-sensing of Neon to take effect.
        "-DPNG_ARM_NEON_OPT=0",
        "-DPNG_ARM_NEON_IMPLEMENTATION=0",

---

tools/workspace/libpng_internal/package.BUILD.bazel line 91 at r3 (raw file):

            "-msse4.1",
            "-DPNG_INTEL_SSE_OPT=1",
            "-DPNG_INTEL_SSE_IMPLEMENTATION=3",

Please check (locally -- no need to push new code to the pull request) that on Noble the auto-sensed value of PNG_INTEL_SSE_IMPLEMENTATION in intel/check.h defaults to 3 with each of our default compilers (GCC 13, Clang 20) in Drake's compile step. (For example, edit filter_sse2_intrinsics.c to static_assert on the value, and confirm that ==3 passes and ==4 fails.)

---

tools/workspace/vtk_internal/settings.bzl line 636 at r3 (raw file):

        ],
        "deps_extra": [
            "@libpng//:libpng",

nit Simplify

Suggestion:

"@libpng"

[bobpaw]

tools/workspace/libpng_internal/png_drake_vendor.h line 9 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

AFAICT the "avoid slow functions" thing is a matter of emphasis and not necessity. Looks to me like the macro implementations would still be the default choice, even without the undef.

We need to confirm this before casually dropping it. Checking the disassembly manually locally shouldn't be very hard.

As for the exif log spam reduction, can we just try some builds and see how bad it gets?

We don't want Drake to emit spurious warnings.

We need to configure libpng to disable exif support, like we had previously. At first glance, it does seem like this will require an amendment to the BCR rule(s).

Working on adding a config setting to the BCR rule (it seems like most people would have some cause to add to this file so they can modify config settings?).

[bobpaw]

tools/workspace/libpng_internal/package.BUILD.bazel line 85 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Please explain (in the review discussion here) why it's OK to re-enable Neon.

Working

I forgot that I needed to add a patch for the BCR BUILD.bazel to add these copts.

[jwnimmer-tri]

@jwnimmer-tri made 1 comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on bobpaw and tyler-yankee).

---

tools/workspace/libpng_internal/package.BUILD.bazel line 91 at r3 (raw file):

... in intel/check.h ...

Oops, this was referring to the libpng master branch. In the current version, the definition comes from pngpriv.h.