Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

replace libpng with libspng #10889

Merged
merged 2 commits into from Jul 26, 2022
Merged

replace libpng with libspng #10889

merged 2 commits into from Jul 26, 2022

Conversation

shuffle2
Copy link
Contributor

@shuffle2 shuffle2 commented Jul 24, 2022

This only replaces the png library dolphin itself uses (mgba and Qt use their own).
It also allows getting rid of the gross C support code needed for libpng and make the External a submodule.

oof, i had a bug that made me think the speedup was a lot more than it really is. There is about 10% speedup for smaller png writes, for larger ones it's actually a bit slower (but still faster overall than current master, thanks to zlib-ng).

On the read path, I tested by comparing time taken for prefetching the hypatia wind waker hires texture set to load:
this PR: 66 seconds
zlib-ng branch: 95 seconds
master (zlib and libpng): 130 seconds

Not sure how to make it work with cmake.

@shuffle2 shuffle2 force-pushed the spng branch 2 times, most recently from 09dce67 to cc625c6 Compare July 24, 2022 11:55
@AdmiralCurtiss
Copy link
Contributor

The reason you can't get CMake to work is because spng's CMakeLists.txt does not set any include directory on the spng/spng_static target. So they don't get set when linking to it. It also unconditionally installs the spng headers, which we probably don't want.

Since its compilation steps are very simple I think we can just make a custom replacement for it instead of trying to make it behave. Replace the entirety of our spng CMakeLists.txt (the one outside the submodule) with:

cmake_minimum_required(VERSION 3.0)

project(spng C)

add_library(spng STATIC ${CMAKE_CURRENT_LIST_DIR}/libspng/spng/spng.c)
target_compile_definitions(spng PUBLIC SPNG_STATIC)
target_link_libraries(spng PUBLIC ZLIB::ZLIB)
target_include_directories(spng PUBLIC ${CMAKE_CURRENT_LIST_DIR}/libspng/spng/)
dolphin_disable_warnings_msvc(spng)

@shuffle2
Copy link
Contributor Author

Thanks!

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

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

Code wise this LGTM (just looked at the png changes). I tested Dynamic Input Textures, loading a texture pack, and dumping some textures. All seemed to work fine

@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • aeon-charge-attack on ogl-lin-radeon: diff
  • chibi-robo-fastdepth on ogl-lin-radeon: diff
  • chibi-robo-zfighting on ogl-lin-radeon: diff
  • dbz-depth on ogl-lin-radeon: diff
  • djfny-menu on ogl-lin-radeon: diff
  • DKCR-Char on ogl-lin-radeon: diff
  • ea-vp6 on ogl-lin-radeon: diff
  • find-mii on ogl-lin-radeon: diff
  • fog-adj on ogl-lin-radeon: diff
  • fortune-street on ogl-lin-radeon: diff
  • fortune-street-fog on ogl-lin-radeon: diff
  • fortune-street-white-box on ogl-lin-radeon: diff
  • fsa-layers on ogl-lin-radeon: diff
  • inverted-depth-range on ogl-lin-radeon: diff
  • kirby-shadows on ogl-lin-radeon: diff
  • line-width-test on ogl-lin-radeon: diff
  • luigi-shadows on ogl-lin-radeon: diff
  • mario-sluggers-bar on ogl-lin-radeon: diff
  • mario-tennis-menu on ogl-lin-radeon: diff
  • megaman-heat on ogl-lin-radeon: diff
  • melee-lighting on ogl-lin-radeon: diff
  • mii-channel on ogl-lin-radeon: diff
  • milotic-texture on ogl-lin-radeon: diff
  • mini-ninjas on ogl-lin-radeon: diff
  • mkdd-efb on ogl-lin-radeon: diff
  • mkwii-bluebox on ogl-lin-radeon: diff
  • monkeyball-fuse on ogl-lin-radeon: diff
  • mp7-text on ogl-lin-radeon: diff
  • mtennis-zfreeze on ogl-lin-radeon: diff
  • my-word-coach on ogl-lin-radeon: diff
  • nddemo-bumpmapping on ogl-lin-radeon: diff
  • nfsu-purplerect on ogl-lin-radeon: diff
  • nfsu-reflections on ogl-lin-radeon: diff
  • nsmbw-coins on ogl-lin-radeon: diff
  • nsmbw-intro on ogl-lin-radeon: diff
  • pm-hc-jp on ogl-lin-radeon: diff
  • puzzle-collection on ogl-lin-radeon: diff
  • pw-black-bars on ogl-lin-radeon: diff
  • rs2-glass on ogl-lin-radeon: diff
  • rs2-skybox on ogl-lin-radeon: diff
  • rs2-zfreeze on ogl-lin-radeon: diff
  • sadx-ui on ogl-lin-radeon: diff
  • sf-assault-flashing on ogl-lin-radeon: diff
  • smg2-fog on ogl-lin-radeon: diff
  • smg-marioeyes on ogl-lin-radeon: diff
  • sms-bubbles on ogl-lin-radeon: diff
  • sms-gc on ogl-lin-radeon: diff
  • soa-black on ogl-lin-radeon: diff
  • soniccolors-mm on ogl-lin-radeon: diff
  • sonicriderszg-gb on ogl-lin-radeon: diff
  • spyro-bloom on ogl-lin-radeon: diff
  • ssbm-pointsize on ogl-lin-radeon: diff
  • ss-map on ogl-lin-radeon: diff
  • super-sluggers-white-out on ogl-lin-radeon: diff
  • thps4-shadow on ogl-lin-radeon: diff
  • tos-invis-char on ogl-lin-radeon: diff
  • tsp3-pinkgrass on ogl-lin-radeon: diff
  • xenoblade-menu on ogl-lin-radeon: diff
  • zelda1-vc on ogl-lin-radeon: diff
  • ztp-grass on ogl-lin-radeon: diff
  • zww-armos on ogl-lin-radeon: diff
  • zww-water on ogl-lin-radeon: diff
  • zww-waves on ogl-lin-radeon: diff

automated-fifoci-reporter

@shuffle2
Copy link
Contributor Author

i don't think this PR should have made fifoci complain, nor the zlib-ng change, so idk what's up with that :(

@AdmiralCurtiss AdmiralCurtiss merged commit a9edf12 into dolphin-emu:master Jul 26, 2022
11 checks passed
@dvessel
Copy link
Contributor

dvessel commented Jul 27, 2022

Texture dumps can’t be read by anything using libpng. This looks like a bug with libspng.

❯ pngquant --verbose tex1_128x128_m_114e37a06b3ef23b_14_arb-bad.png
tex1_128x128_m_114e37a06b3ef23b_14_arb-bad.png:
  error: Read error (libpng failed)
  error: cannot decode image tex1_128x128_m_114e37a06b3ef23b_14_arb-bad.png
There were errors quantizing 1 file out of a total of 1 file.
❯ identify -verbose tex1_128x128_m_114e37a06b3ef23b_14_arb-bad.png
identify: Expected 8 bytes; found 0 bytes `tex1_128x128_m_114e37a06b3ef23b_14_arb-bad.png' @ warning/png.c/MagickPNGWarningHandler/1750.
identify: Read Exception `tex1_128x128_m_114e37a06b3ef23b_14_arb-bad.png' @ error/png.c/MagickPNGErrorHandler/1716.

example files:

tex1_128x128_m_114e37a06b3ef23b_14_arb-good
tex1_128x128_m_114e37a06b3ef23b_14_arb-bad

@shuffle2
Copy link
Contributor Author

a bug with libspng or with libpng 🤔 they seem like valid pngs to software on my system (and also github/browser, which displays them fine...)

@dvessel
Copy link
Contributor

dvessel commented Jul 27, 2022

Not sure but I filed a bug with libspng. All my gui apps reads them just fine but if some command line tool uses libpng, it will fail.

randy408/libspng#226

@shuffle2
Copy link
Contributor Author

@dvessel actually i took a look using the pngcheck tool of libpng and think this will fix it, can you check: #10907

@shuffle2 shuffle2 deleted the spng branch July 31, 2022 17:13
@Rumi-Larry
Copy link

Since this PR was made the file at "dolphin/Source/PCH/nopch/pch.h" is no longer necessary.

Rumi-Larry added a commit to Rumi-Larry/dolphin that referenced this pull request Oct 9, 2022
This was made as a workaround for having PCH ignore the file "ImageC.c" which could not be made into ".cpp" due to limitations of libpng, but since it has been replaced by libspng dolphin-emu#10889 this is no longer needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants