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

Use Simplified libpng API for writing PNGs. #9289

Merged
merged 3 commits into from Dec 11, 2020

Conversation

AdmiralCurtiss
Copy link
Contributor

This makes the code way simpler and also actually writes RGB images when alpha discard is requested (instead of writing RGBA files that just have full alpha). Also deduplicates the PNG writing code.

One slight caveat here is that writing using this API automatically adds either an sRGB chunk (with a value of 0) or, if the PNG_IMAGE_FLAG_COLORSPACE_NOT_sRGB flag is set, a gAMA chunk (with a value of 45455). We previously wrote neither of those. It's unclear to me what this means in practice, if anything, though? I also have no idea whether things like screenshots or textures should be considered sRGB or not.

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 looks great. I can't say what impact the changes in colorspace you mention would bring.

I'll try to find time in the next day or two to test dynamic input textures to make sure everything still works properly.

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

LGTM codewise

[02:52:43] <iwubcode> leoetlino - not seeing any issues with 9289 from some quick testing
[02:53:28] <iwubcode> Generated images are still usable by Dolphin at least

@leoetlino leoetlino merged commit fd5c69d into dolphin-emu:master Dec 11, 2020
10 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the simple-png-api-write branch December 11, 2020 12:51
@Adamillo
Copy link

Idk if this pr has to something with the textures not dumping but after updating to the latest dev version (namely 5.0-13215), the textures wouldnt dump anymore. my theory states that the previous dolphin version that merged this pr (namely 5.0-13211) caused the issue not to dump the textures anymore

@AdmiralCurtiss
Copy link
Contributor Author

I'm certain I tested texture dumping but I'll take a look.

@AdmiralCurtiss
Copy link
Contributor Author

Yes, works totally fine here on 5.0-13215. (using the Vulkan backend)

@Adamillo
Copy link

I think finally found out whats going on. On the latest PR the textures wont dump if the Windows User has russian letters in their names, because after I moved Dolphin to the C: folder instead of Downloads and used portable.txt, the textures finally dumped. After that, I made Dolphin create a new Documents folder with the default settings and the textures don't dump! I think the issue I have is the russian name of the Windows User!

@JosJuice
Copy link
Member

Ah, yeah, it doesn't surprise me that that would happen on Windows... We probably need to replace the call to png_image_write_to_file with png_image_write_to_memory plus FileIO::File or similar.

@AdmiralCurtiss
Copy link
Contributor Author

Aaah yeah I could see how that could happen if libpng doesn't assume UTF8 strings. Well that's annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants