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

Pre-scan the root of texture packs for hash-named files. #17380

Merged
merged 4 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2481,6 +2481,7 @@ if(UNITTEST)
unittest/TestIRPassSimplify.cpp
unittest/TestX64Emitter.cpp
unittest/TestVertexJit.cpp
unittest/TestVFS.cpp
unittest/TestRiscVEmitter.cpp
unittest/TestSoftwareGPUJit.cpp
unittest/TestThreadManager.cpp
Expand Down
1 change: 1 addition & 0 deletions Common/File/VFS/VFS.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class VFSInterface {
public:
virtual ~VFSInterface() {}
virtual uint8_t *ReadFile(const char *path, size_t *size) = 0;
// If listing already contains files, it'll be cleared.
virtual bool GetFileListing(const char *path, std::vector<File::FileInfo> *listing, const char *filter = nullptr) = 0;
};

Expand Down
84 changes: 54 additions & 30 deletions Common/File/VFS/ZipFileReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,13 @@ ZipFileReader *ZipFileReader::Create(const Path &zipFile, const char *inZipPath,
return nullptr;
}

ZipFileReader *reader = new ZipFileReader();
reader->zip_file_ = zip_file;
truncate_cpy(reader->inZipPath_, inZipPath);
return reader;
// The inZipPath is supposed to be a folder, and internally in this class, we suffix
// folder paths with '/', matching how the zip library works.
std::string path = inZipPath;
if (!path.empty() && path.back() != '/') {
path.push_back('/');
}
return new ZipFileReader(zip_file, path);
}

ZipFileReader::~ZipFileReader() {
Expand All @@ -50,16 +53,15 @@ ZipFileReader::~ZipFileReader() {
}

uint8_t *ZipFileReader::ReadFile(const char *path, size_t *size) {
char temp_path[2048];
snprintf(temp_path, sizeof(temp_path), "%s%s", inZipPath_, path);
std::string temp_path = inZipPath_ + path;

std::lock_guard<std::mutex> guard(lock_);
// Figure out the file size first.
struct zip_stat zstat;
zip_stat(zip_file_, temp_path, ZIP_FL_NOCASE | ZIP_FL_UNCHANGED, &zstat);
zip_file *file = zip_fopen(zip_file_, temp_path, ZIP_FL_NOCASE | ZIP_FL_UNCHANGED);
zip_stat(zip_file_, temp_path.c_str(), ZIP_FL_NOCASE | ZIP_FL_UNCHANGED, &zstat);
zip_file *file = zip_fopen(zip_file_, temp_path.c_str(), ZIP_FL_NOCASE | ZIP_FL_UNCHANGED);
if (!file) {
ERROR_LOG(IO, "Error opening %s from ZIP", temp_path);
ERROR_LOG(IO, "Error opening %s from ZIP", temp_path.c_str());
return 0;
}
uint8_t *contents = new uint8_t[zstat.size + 1];
Expand All @@ -72,8 +74,10 @@ uint8_t *ZipFileReader::ReadFile(const char *path, size_t *size) {
}

bool ZipFileReader::GetFileListing(const char *orig_path, std::vector<File::FileInfo> *listing, const char *filter = 0) {
char path[2048];
snprintf(path, sizeof(path), "%s%s", inZipPath_, orig_path);
std::string path = std::string(inZipPath_) + orig_path;
if (!path.empty() && path.back() != '/') {
path.push_back('/');
}

std::set<std::string> filters;
std::string tmp;
Expand All @@ -95,25 +99,36 @@ bool ZipFileReader::GetFileListing(const char *orig_path, std::vector<File::File
// We just loop through the whole ZIP file and deduce what files are in this directory, and what subdirectories there are.
std::set<std::string> files;
std::set<std::string> directories;
GetZipListings(path, files, directories);
bool success = GetZipListings(path, files, directories);
if (!success) {
// This means that no file prefix matched the path.
return false;
}

listing->clear();

INFO_LOG(SYSTEM, "Listing %s", orig_path);

for (auto diter = directories.begin(); diter != directories.end(); ++diter) {
File::FileInfo info;
info.name = *diter;

// Remove the "inzip" part of the fullname.
info.fullName = Path(std::string(path).substr(strlen(inZipPath_))) / *diter;
std::string relativePath = std::string(path).substr(inZipPath_.size());
info.fullName = Path(relativePath + *diter);
info.exists = true;
info.isWritable = false;
info.isDirectory = true;
// INFO_LOG(SYSTEM, "Found file: %s (%s)", info.name.c_str(), info.fullName.c_str());
listing->push_back(info);
}

for (auto fiter = files.begin(); fiter != files.end(); ++fiter) {
std::string fpath = path;
File::FileInfo info;
info.name = *fiter;
info.fullName = Path(std::string(path).substr(strlen(inZipPath_))) / *fiter;
std::string relativePath = std::string(path).substr(inZipPath_.size());
info.fullName = Path(relativePath + *fiter);
info.exists = true;
info.isWritable = false;
info.isDirectory = false;
Expand All @@ -123,43 +138,52 @@ bool ZipFileReader::GetFileListing(const char *orig_path, std::vector<File::File
continue;
}
}
// INFO_LOG(SYSTEM, "Found dir: %s (%s)", info.name.c_str(), info.fullName.c_str());
listing->push_back(info);
}

std::sort(listing->begin(), listing->end());
return true;
}

void ZipFileReader::GetZipListings(const char *path, std::set<std::string> &files, std::set<std::string> &directories) {
size_t pathlen = strlen(path);
if (path[pathlen - 1] == '/')
pathlen--;
// path here is from the root, so inZipPath needs to already be added.
bool ZipFileReader::GetZipListings(const std::string &path, std::set<std::string> &files, std::set<std::string> &directories) {
_dbg_assert_(path.empty() || path.back() == '/');

std::lock_guard<std::mutex> guard(lock_);
int numFiles = zip_get_num_files(zip_file_);
bool anyPrefixMatched = false;
for (int i = 0; i < numFiles; i++) {
const char* name = zip_get_name(zip_file_, i, 0);
if (!name)
continue;
if (!memcmp(name, path, pathlen)) {
// The prefix is right. Let's see if this is a file or path.
const char *slashPos = strchr(name + pathlen + 1, '/');
continue; // shouldn't happen, I think
if (startsWith(name, path)) {
if (strlen(name) == path.size()) {
// Don't want to return the same folder.
continue;
}
const char *slashPos = strchr(name + path.size(), '/');
if (slashPos != 0) {
// A directory.
std::string dirName = std::string(name + pathlen + 1, slashPos - (name + pathlen + 1));
anyPrefixMatched = true;
// A directory. Let's pick off the only part we care about.
size_t offset = path.size();
std::string dirName = std::string(name + offset, slashPos - (name + offset));
// We might get a lot of these if the tree is deep. The std::set deduplicates.
directories.insert(dirName);
} else if (name[pathlen] == '/') {
const char *fn = name + pathlen + 1;
} else {
anyPrefixMatched = true;
// It's a file.
const char *fn = name + path.size();
files.insert(std::string(fn));
} // else, it was a file with the same prefix as the path. like langregion.ini next to lang/.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we protect against this case now?

So let's say path is "assets/lang". We did check it starts with "assets/lang", so that's good. But "assets/langregion" matches that and does not have a /. So won't we match it and add it to the list as "egion" or something?

I might also be missing something about name + pathlen. In my above scenario, "assets/lang/foo.ini" would not find a slash after name + pathlen + 1 (which is "foo.ini") but wouldn't fn become "/foo.ini"? Seems confusing.

It feels like this new logic might only be something that works when path == ""? Which does look like it was broken before.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

I somehow convinced myself that since all directories in a zip file always end wih '/', they'd be caught in the first case anyway - but that doesn't even make sense. Don't know what I was thinking.

I'll make a unit test for this today to really make sure all cases comes out right.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Actually this does work - just gotta follow the convention of directory paths ending with "/" consistently throughout.

The unit test revealed a number of bugs, but I've fixed it and it's working now.

Copy link
Collaborator

@unknownbrackets unknownbrackets May 3, 2023

Choose a reason for hiding this comment

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

Okay, yeah, making GetFileListing() add / makes sense.

But oops, now there's a source_assets/ziptest.zip. Oh, it's intentional for testing.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, we don't have a designated directory for testdata, at least not that I found. Though I suppose could have put the zip in /unittest too.

}
}
}
return anyPrefixMatched;
}

bool ZipFileReader::GetFileInfo(const char *path, File::FileInfo *info) {
struct zip_stat zstat;
char temp_path[1024];
snprintf(temp_path, sizeof(temp_path), "%s%s", inZipPath_, path);
std::string temp_path = inZipPath_ + path;

// Clear some things to start.
info->isDirectory = false;
Expand All @@ -168,7 +192,7 @@ bool ZipFileReader::GetFileInfo(const char *path, File::FileInfo *info) {

{
std::lock_guard<std::mutex> guard(lock_);
if (0 != zip_stat(zip_file_, temp_path, ZIP_FL_NOCASE | ZIP_FL_UNCHANGED, &zstat)) {
if (0 != zip_stat(zip_file_, temp_path.c_str(), ZIP_FL_NOCASE | ZIP_FL_UNCHANGED, &zstat)) {
// ZIP files do not have real directories, so we'll end up here if we
// try to stat one. For now that's fine.
info->exists = false;
Expand Down
6 changes: 4 additions & 2 deletions Common/File/VFS/ZipFileReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,11 @@ class ZipFileReader : public VFSBackend {
}

private:
void GetZipListings(const char *path, std::set<std::string> &files, std::set<std::string> &directories);
ZipFileReader(zip *zip_file, const std::string &inZipPath) : zip_file_(zip_file), inZipPath_(inZipPath) {}
// Path has to be either an empty string, or a string ending with a /.
bool GetZipListings(const std::string &path, std::set<std::string> &files, std::set<std::string> &directories);

zip *zip_file_ = nullptr;
std::mutex lock_;
char inZipPath_[256];
std::string inZipPath_;
};
4 changes: 4 additions & 0 deletions Common/StringUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ inline bool endsWithNoCase(const std::string &str, const std::string &what) {
return strncasecmp(str.c_str() + offset, what.c_str(), what.size()) == 0;
}

inline bool equalsNoCase(const std::string &str, const char *what) {
return strcasecmp(str.c_str(), what) == 0;
}

void DataToHexString(const uint8_t *data, size_t size, std::string *output);
void DataToHexString(int indent, uint32_t startAddr, const uint8_t* data, size_t size, std::string* output);

Expand Down
77 changes: 53 additions & 24 deletions GPU/Common/TextureReplacer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ bool TextureReplacer::LoadIni() {
bool iniLoaded = ini.LoadFromVFS(*dir, INI_FILENAME);

if (iniLoaded) {
if (!LoadIniValues(ini)) {
if (!LoadIniValues(ini, dir)) {
delete dir;
return false;
}
Expand All @@ -160,7 +160,7 @@ bool TextureReplacer::LoadIni() {
}

INFO_LOG(G3D, "Loading extra texture ini: %s", overrideFilename.c_str());
if (!LoadIniValues(overrideIni, true)) {
if (!LoadIniValues(overrideIni, nullptr, true)) {
delete dir;
return false;
}
Expand Down Expand Up @@ -195,7 +195,7 @@ bool TextureReplacer::LoadIni() {
return true;
}

bool TextureReplacer::LoadIniValues(IniFile &ini, bool isOverride) {
bool TextureReplacer::LoadIniValues(IniFile &ini, VFSBackend *dir, bool isOverride) {
auto options = ini.GetOrCreateSection("options");
std::string hash;
options->Get("hash", &hash, "");
Expand Down Expand Up @@ -231,13 +231,14 @@ bool TextureReplacer::LoadIniValues(IniFile &ini, bool isOverride) {
}

bool filenameWarning = false;

std::map<ReplacementCacheKey, std::map<int, std::string>> filenameMap;

if (ini.HasSection("hashes")) {
auto hashes = ini.GetOrCreateSection("hashes")->ToMap();
// Format: hashname = filename.png
bool checkFilenames = g_Config.bSaveNewTextures && !g_Config.bIgnoreTextureFilenames && !vfsIsZip_;

std::map<ReplacementCacheKey, std::map<int, std::string>> filenameMap;

for (const auto &item : hashes) {
ReplacementCacheKey key(0, 0);
int level = 0; // sscanf might fail to pluck the level, but that's ok, we default to 0. sscanf doesn't write to non-matched outputs.
Expand All @@ -256,31 +257,59 @@ bool TextureReplacer::LoadIniValues(IniFile &ini, bool isOverride) {
ERROR_LOG(G3D, "Unsupported syntax under [hashes]: %s", item.first.c_str());
}
}
}

// Now, translate the filenameMap to the final aliasMap.
for (auto &pair : filenameMap) {
std::string alias;
int mipIndex = 0;
for (auto &level : pair.second) {
if (level.first == mipIndex) {
alias += level.second + "|";
mipIndex++;
} else {
WARN_LOG(G3D, "Non-sequential mip index %d, breaking. filenames=%s", level.first, level.second.c_str());
break;
// Scan the root of the texture folder/zip and preinitialize the hash map.
std::vector<File::FileInfo> filesInRoot;
if (dir) {
dir->GetFileListing("", &filesInRoot, nullptr);
for (auto file : filesInRoot) {
if (file.isDirectory)
continue;
if (file.name.empty() || file.name[0] == '.')
continue;
Path path(file.name);
std::string ext = path.GetFileExtension();

std::string hash = file.name.substr(0, file.name.size() - ext.size());
if (!((hash.size() >= 26 && hash.size() <= 27 && hash[24] == '_') || hash.size() == 24)) {
continue;
}
// OK, it's hash-like enough to try to parse it into the map.
if (equalsNoCase(ext, ".ktx2") || equalsNoCase(ext, ".png") || equalsNoCase(ext, ".dds") || equalsNoCase(ext, ".zim")) {
ReplacementCacheKey key(0, 0);
int level = 0; // sscanf might fail to pluck the level, but that's ok, we default to 0. sscanf doesn't write to non-matched outputs.
if (sscanf(hash.c_str(), "%16llx%8x_%d", &key.cachekey, &key.hash, &level) >= 1) {
// INFO_LOG(G3D, "hash-like file in root, adding: %s", file.name.c_str());
filenameMap[key][level] = file.name;
}
}
if (alias == "|") {
alias = ""; // marker for no replacement
}
}

// Now, translate the filenameMap to the final aliasMap.
for (auto &pair : filenameMap) {
std::string alias;
int mipIndex = 0;
for (auto &level : pair.second) {
if (level.first == mipIndex) {
alias += level.second + "|";
mipIndex++;
} else {
WARN_LOG(G3D, "Non-sequential mip index %d, breaking. filenames=%s", level.first, level.second.c_str());
break;
}
// Replace any '\' with '/', to be safe and consistent. Since these are from the ini file, we do this on all platforms.
for (auto &c : alias) {
if (c == '\\') {
c = '/';
}
}
if (alias == "|") {
alias = ""; // marker for no replacement
}
// Replace any '\' with '/', to be safe and consistent. Since these are from the ini file, we do this on all platforms.
for (auto &c : alias) {
if (c == '\\') {
c = '/';
}
aliases_[pair.first] = alias;
}
aliases_[pair.first] = alias;
}

if (filenameWarning) {
Expand Down
2 changes: 1 addition & 1 deletion GPU/Common/TextureReplacer.h
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class TextureReplacer {
bool FindFiltering(u64 cachekey, u32 hash, TextureFiltering *forceFiltering);

bool LoadIni();
bool LoadIniValues(IniFile &ini, bool isOverride = false);
bool LoadIniValues(IniFile &ini, VFSBackend *dir, bool isOverride = false);
void ParseHashRange(const std::string &key, const std::string &value);
void ParseFiltering(const std::string &key, const std::string &value);
void ParseReduceHashRange(const std::string& key, const std::string& value);
Expand Down
1 change: 1 addition & 0 deletions android/jni/Android.mk
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,7 @@ ifeq ($(UNITTEST),1)
$(SRC)/unittest/TestSoftwareGPUJit.cpp \
$(SRC)/unittest/TestThreadManager.cpp \
$(SRC)/unittest/TestVertexJit.cpp \
$(SRC)/unittest/TestVFS.cpp \
$(TESTARMEMITTER_FILE) \
$(SRC)/unittest/UnitTest.cpp

Expand Down
Binary file added source_assets/ziptest.zip
Binary file not shown.
Loading