Skip to content

Update g2lib png encode/decode to use correct png_voidp define#1977

Merged
islas merged 1 commit intowrf-model:developfrom
islas:png_voidFix
Jan 11, 2024
Merged

Update g2lib png encode/decode to use correct png_voidp define#1977
islas merged 1 commit intowrf-model:developfrom
islas:png_voidFix

Conversation

@islas
Copy link
Collaborator

@islas islas commented Jan 6, 2024

TYPE: bug fix

KEYWORDS: png

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem:

Incorrect define type used for png void type when compared to libpng functions and NCEP g2lib source code.

While voidp does exist, compile may fail when enabling GRIB2 I/O and enabling libpng compression in the makefiles. Additionally no version of libpng (back to 0.89) ever had those functions explicitly compatibly with voidp - likewise the ncep g2 lib version of these files (back to v2.5.0) never has voidp in the modified code.

This will NOT affect out-of-the-box native builds of WRF, grib2 enabled or not. One must enable the source code manually to exercise PNG compression and then this issue may appear.

Solution:
Switch to the appropriate define png_voidp, as can be seen here :
https://github.com/weathersource/g2clib/blob/24bcccafb8088f4b5918a878e6b8daf7acc86275/src/dec_png.c#L91
https://github.com/weathersource/g2clib/blob/24bcccafb8088f4b5918a878e6b8daf7acc86275/src/enc_png.c#L91

or here:
https://github.com/NOAA-EMC/NCEPLIBS-g2/blob/1dcdfb0d8db0caf2f31b5631abcf1ca04402a0aa/dec_png.c#L104
https://github.com/NOAA-EMC/NCEPLIBS-g2/blob/1dcdfb0d8db0caf2f31b5631abcf1ca04402a0aa/enc_png.c#L103

Note that NCEP's github version does not goes as far back as version 1.0.7 which WRF uses

LIST OF MODIFIED FILES:
M external/io_grib2/g2lib/dec_png.c
M external/io_grib2/g2lib/enc_png.c

RELEASE NOTE:
Update g2lib png encode/decode to use correct png_voidp define

@weiwangncar
Copy link
Collaborator

The regression test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

@mgduda
Copy link
Collaborator

mgduda commented Jan 9, 2024

I'm curious as to how you encountered the issue with the wrong typecast?

Regarding the remark in your commit message, I would guess that png_voidp and voidp are both typdefs of void *. For what it's worth, if I manually compile the enc_png.c file with the gcc 12.2.0 compilers, even with -Wall, there are no warnings generated with the original code. But that probably depends on the libpng and zlib library versions that I'm using.

Anyway, I do agree that we should fix the typecast to agree with the argument type; it's just a question of whether we should add more detail in the PR description (especially if the enc_png.c and dec_png.c files aren't even compiled under the current default WRF build).

@islas
Copy link
Collaborator Author

islas commented Jan 9, 2024

I found out this file was wrong just because I was trying to compile everything in the cmake build to just cover as much ground as possible. I searched libpng source and symbols in libraries, and while voidp does exist, it failed to compile for me (can't remember all the compilers I was trying at the time but this was on cheyenne) and no version of libpng (back to 0.89) ever had those functions explicitly compatibly with voidp - likewise the ncep g2 lib version of these files (back to v2.5.0) never has voidp in there as well - hence my utter confusion.

It could be that I was not using certain flags like automatic implicit type casting

Copy link
Collaborator

@mgduda mgduda left a comment

Choose a reason for hiding this comment

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

Before merging this PR, I think it would be good to add some further explanation in the PR description. For example, the issue doesn't turn up in the "out-of-the-box" builds of WRF because PNG compression isn't enabled, and this is expected to only be an issue if GRIB2 I/O is enabled and manual changes are made to enable PNG compression for GRIB2 output. Even the details mentioned in the discussion on this PR regarding versions of PNG libraries and NCEP g2lib code would be useful to record in the merge commit messsage (which comes from the PR description, if I understand correctly) for posterity.

@mgduda mgduda requested a review from weiwangncar January 11, 2024 00:46
@islas islas merged commit 9ca6807 into wrf-model:develop Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants