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

Common: Write PNGs in two steps to allow Unicode target paths. #9339

Merged
merged 1 commit into from
Dec 20, 2020

Conversation

AdmiralCurtiss
Copy link
Contributor

Fixes writes to paths with non-ASCII characters, as discussed after the merge in #9289.

@iwubcode
Copy link
Contributor

So, writing to memory and then to a file seems slower. Is it? If so, I wonder if it'd make sense to somehow determine if we are using a unicode pathes and then take this slower approach (but if ascii write directly to file?).

@AdmiralCurtiss
Copy link
Contributor Author

In theory yes, in practice who really cares, fileio is slow enough as-is. I doubt it makes a noticeable difference.

@JosJuice
Copy link
Member

JosJuice commented Dec 19, 2020

If we want something that's more similar speed-wise to what master does, we could use png_set_write_fn to set a callback with which libpng can write directly to an IOFile. I don't know if it would be more or less complicated than what this PR does, but at least I prefer it over maintaining two different code paths.

@AdmiralCurtiss
Copy link
Contributor Author

I don't think you can use png_set_write_fn in combination with the simplified API. We could use png_image_write_to_stdio but that requires a raw FILE* which I would also like to avoid. Meh.

@smurf3tte
Copy link
Contributor

I'm curious. What is your reasoning for avoiding png_image_write_to_stdio? I believe it would collapse this PR down to just 4 new lines and 1 changed line:

  File::IOFile file;
  file.Open(path, "wb");
  if (!file)
    return false;
  png_image_write_to_stdio(&png, file.GetHandle(), 0, input, stride, nullptr);

@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Dec 20, 2020

It's the file.GetHandle() part. I have a long-term plan of switching the internals of IOFile to use the Win32 API instead of C-style file handles on Windows, see eg. #9023, so we can gain the advantages of that without having two separate file handling classes.

Though I suppose it might really be better to just use png_image_write_to_stdio here and then think about the other part when we get to that...

@lioncash
Copy link
Member

I agree that it likely Just Doesn't Matter That Much and that this is fine to avoid the usage of GetHandle() on IOFile. This can always be revisited if it becomes an issue/hotspot

@smurf3tte
Copy link
Contributor

It's the file.GetHandle() part. I have a long-term plan of switching the internals of IOFile to use the Win32 API instead of C-style file handles on Windows, see eg. #9023, so we can gain the advantages of that without having two separate file handling classes.

Though I suppose it might really be better to just use png_image_write_to_stdio here and then think about the other part when we get to that...

I think there are other ways to accomplish what #9023 is doing. It's possible to get the Win32 HANDLE from a file descriptor (FILE*) and it's also possible to construct a file descriptor around a HANDLE (which is what fopen() does). I agree that maintaining parallel file wrappers is something to be avoided.

break;
default:
return false;
}

png_image_write_to_file(&png, path.c_str(), 0, input, stride, nullptr);
if (png.warning_or_error & PNG_IMAGE_ERROR)
// libpng doesn't handle non-ASCII characters in path, so write in two steps:
Copy link
Contributor

@smurf3tte smurf3tte Dec 20, 2020

Choose a reason for hiding this comment

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

This... doesn't have anything to do with ASCII, as I understand it. It's about the active Windows code page not being UTF-8, which is what Dolphin operates in exclusively. libpng is just calling fopen on the string that is passed in.

Edit: perhaps this was a bit of an overstatement on my part. ASCII is related, in that it contains the subset of UTF-8 encoded characters that will work without this fix.

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

Successfully merging this pull request may close these issues.

5 participants