From 10ecc4afbd345696e966a94088ccc59aee0720c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 7 Mar 2023 20:53:50 +0100 Subject: [PATCH 1/7] Use the VFS in the replacer to remove duplicated code --- Core/TextureReplacer.cpp | 330 +++++++++++---------------------------- Core/TextureReplacer.h | 30 ++-- 2 files changed, 104 insertions(+), 256 deletions(-) diff --git a/Core/TextureReplacer.cpp b/Core/TextureReplacer.cpp index 875ded80fe..43e6fbf83b 100644 --- a/Core/TextureReplacer.cpp +++ b/Core/TextureReplacer.cpp @@ -21,11 +21,6 @@ #include #include #include -#ifdef SHARED_LIBZIP -#include -#else -#include "ext/libzip/zip.h" -#endif #include "ext/xxhash.h" @@ -35,6 +30,8 @@ #include "Common/Data/Format/PNGLoad.h" #include "Common/Data/Text/I18n.h" #include "Common/Data/Text/Parsers.h" +#include "Common/File/VFS/DirectoryReader.h" +#include "Common/File/VFS/ZipFileReader.h" #include "Common/File/FileUtil.h" #include "Common/File/VFS/VFS.h" #include "Common/LogReporting.h" @@ -63,17 +60,8 @@ TextureReplacer::TextureReplacer() { none_.prepareDone_ = true; } -void ReplacerZipInfo::Close() { - if (!z) - return; - - std::lock_guard guard(lock); - zip_close(z); - z = nullptr; -} - TextureReplacer::~TextureReplacer() { - zip_.Close(); + delete vfs_; } void TextureReplacer::Init() { @@ -98,7 +86,8 @@ void TextureReplacer::NotifyConfigChanged() { enabled_ = File::IsDirectory(basePath_); } else if (wasEnabled) { - zip_.Close(); + delete vfs_; + vfs_ = nullptr; Decimate(ReplacerDecimateMode::ALL); } @@ -107,46 +96,7 @@ void TextureReplacer::NotifyConfigChanged() { } } -static struct zip *ZipOpenPath(Path fileName) { - int error = 0; - if (fileName.Type() == PathType::CONTENT_URI) { - int fd = File::OpenFD(fileName, File::OPEN_READ); - return zip_fdopen(fd, 0, &error); - } - return zip_open(fileName.c_str(), 0, &error); -} - -static constexpr zip_uint64_t INVALID_ZIP_SIZE = 0xFFFFFFFFFFFFFFFFULL; -static zip_uint64_t ZipFileSize(zip *z, zip_int64_t i) { - zip_stat_t zstat; - if (zip_stat_index(z, i, 0, &zstat) != 0) - return INVALID_ZIP_SIZE; - if ((zstat.valid & ZIP_STAT_SIZE) == 0) - return INVALID_ZIP_SIZE; - return zstat.size; -} - -static bool LoadIniZip(IniFile &ini, zip *z, const std::string &filename) { - zip_int64_t i = zip_name_locate(z, filename.c_str(), ZIP_FL_NOCASE); - if (i < 0) - return false; - - std::string inistr; - zip_uint64_t sz = ZipFileSize(z, i); - if (sz == INVALID_ZIP_SIZE) - return false; - inistr.resize(sz); - - zip_file_t *zf = zip_fopen_index(z, i, 0); - inistr.resize(zip_fread(zf, &inistr[0], inistr.size())); - zip_fclose(zf); - - std::stringstream sstream(inistr); - return ini.Load(sstream); -} - bool TextureReplacer::LoadIni() { - // TODO: Use crc32c? hash_ = ReplacedTextureHash::QUICK; aliases_.clear(); hashranges_.clear(); @@ -160,32 +110,21 @@ bool TextureReplacer::LoadIni() { // Prevents dumping the mipmaps. ignoreMipmap_ = false; - zip_.Close(); - - IniFile ini; - bool iniLoaded = false; + delete vfs_; + vfs_ = nullptr; // First, check for textures.zip, which is used to reduce IO. - zip *z = ZipOpenPath(basePath_ / ZIP_FILENAME); - if (z) { - iniLoaded = LoadIniZip(ini, z, INI_FILENAME); - // Require the zip have textures.ini to use it. - if (iniLoaded) { - iniLoaded = true; - std::lock_guard guard(zip_.lock); - zip_.z = z; - } else { - zip_close(z); - z = nullptr; - } + VFSBackend *dir = ZipFileReader::Create(basePath_ / ZIP_FILENAME, ""); + if (!dir) { + dir = new DirectoryReader(basePath_); } - if (!iniLoaded) { - iniLoaded = ini.LoadFromVFS(g_VFS, (basePath_ / INI_FILENAME).ToString()); - } + IniFile ini; + bool iniLoaded = ini.LoadFromVFS(g_VFS, (basePath_ / INI_FILENAME).ToString()); if (iniLoaded) { if (!LoadIniValues(ini)) { + delete dir; return false; } @@ -194,26 +133,26 @@ bool TextureReplacer::LoadIni() { if (ini.GetOrCreateSection("games")->Get(gameID_.c_str(), &overrideFilename, "")) { if (!overrideFilename.empty() && overrideFilename != INI_FILENAME) { IniFile overrideIni; - if (zip_.z) { - std::lock_guard guard(zip_.lock); - iniLoaded = LoadIniZip(overrideIni, zip_.z, overrideFilename); - } else { - iniLoaded = overrideIni.LoadFromVFS(g_VFS, (basePath_ / overrideFilename).ToString()); - } + iniLoaded = overrideIni.LoadFromVFS(*dir, overrideFilename); if (!iniLoaded) { ERROR_LOG(G3D, "Failed to load extra texture ini: %s", overrideFilename.c_str()); + // TODO: Should we just move on instead? + delete dir; return false; } INFO_LOG(G3D, "Loading extra texture ini: %s", overrideFilename.c_str()); if (!LoadIniValues(overrideIni, true)) { + delete dir; return false; } } } } - // The ini doesn't have to exist for it to be valid. + vfs_ = dir; + + // The ini doesn't have to exist for the texture directory or zip to be valid. return true; } @@ -481,6 +420,7 @@ ReplacedTexture &TextureReplacer::FindReplacement(u64 cachekey, u32 hash, int w, // Okay, let's construct the result. ReplacedTexture &result = cache_[replacementKey]; + result.vfs_ = this->vfs_; if (!g_Config.bReplaceTexturesAllowLate || budget > 0.0) { PopulateReplacement(&result, cachekey, hash, w, h); } @@ -498,27 +438,24 @@ void TextureReplacer::PopulateReplacement(ReplacedTexture *result, u64 cachekey, for (int i = 0; i < MAX_MIP_LEVELS; ++i) { const std::string hashfile = LookupHashFile(cachekey, hash, i); - const Path filename = basePath_ / hashfile; if (hashfile.empty()) { // Out of valid mip levels. Bail out. break; } + const Path filename = basePath_ / hashfile; + ReplacedTextureLevel level; level.fmt = Draw::DataFormat::R8G8B8A8_UNORM; level.file = filename; bool good; bool logError = hashfile != HashName(cachekey, hash, i) + ".png"; - if (zip_.z) { - level.zinfo = &zip_; - std::lock_guard guard(zip_.lock); - level.zi = zip_name_locate(zip_.z, hashfile.c_str(), ZIP_FL_NOCASE); - good = PopulateLevelFromZip(level, !logError); - } else { - good = PopulateLevelFromPath(level, !logError); - } + VFSFileReference *fileRef = vfs_->GetFile(hashfile.c_str()); + + level.fileRef = fileRef; + good = PopulateLevel(level, !logError); // We pad files that have been hashrange'd so they are the same texture size. level.w = (level.w * w) / newW; @@ -554,7 +491,7 @@ enum class ReplacedImageType { INVALID, }; -static ReplacedImageType Identify(const uint8_t magic[4]) { +static inline ReplacedImageType IdentifyMagic(const uint8_t magic[4]) { if (strncmp((const char *)magic, "ZIMG", 4) == 0) return ReplacedImageType::ZIM; if (magic[0] == 0x89 && strncmp((const char *)&magic[1], "PNG", 3) == 0) @@ -562,92 +499,57 @@ static ReplacedImageType Identify(const uint8_t magic[4]) { return ReplacedImageType::INVALID; } -static ReplacedImageType Identify(FILE *fp) { +static ReplacedImageType Identify(VFSBackend *vfs, VFSOpenFile *openFile, std::string *outMagic) { uint8_t magic[4]; - if (fread(magic, 1, 4, fp) != 4) + if (vfs->Read(openFile, magic, 4) != 4) { + *outMagic = "FAIL"; return ReplacedImageType::INVALID; - rewind(fp); - - return Identify(magic); -} - -static ReplacedImageType Identify(zip_file_t *zf) { - uint8_t magic[4]; - if (zip_fread(zf, magic, 4) != 4) - return ReplacedImageType::INVALID; - - return Identify(magic); -} - -bool TextureReplacer::PopulateLevelFromPath(ReplacedTextureLevel &level, bool ignoreError) { - bool good = false; - - FILE *fp = File::OpenCFile(level.file, "rb"); - if (!fp) { - if (!ignoreError) - ERROR_LOG(G3D, "Error opening replacement texture file '%s'", level.file.c_str()); - return false; } - - auto imageType = Identify(fp); - if (imageType == ReplacedImageType::ZIM) { - fseek(fp, 4, SEEK_SET); - good = fread(&level.w, 4, 1, fp) == 1; - good = good && fread(&level.h, 4, 1, fp) == 1; - int flags; - if (good && fread(&flags, 4, 1, fp) == 1) { - good = (flags & ZIM_FORMAT_MASK) == ZIM_RGBA8888; + // Turn the signature into a readable string that we can display in an error message. + *outMagic = std::string((const char *)magic, 4); + for (int i = 0; i < outMagic->size(); i++) { + if ((s8)(*outMagic)[i] < 32) { + (*outMagic)[i] = '_'; } - } else if (imageType == ReplacedImageType::PNG) { - png_image png = {}; - png.version = PNG_IMAGE_VERSION; - if (png_image_begin_read_from_stdio(&png, fp)) { - // We pad files that have been hashrange'd so they are the same texture size. - level.w = png.width; - level.h = png.height; - good = true; - } else { - ERROR_LOG(G3D, "Could not load texture replacement info: %s - %s", level.file.ToVisualString().c_str(), png.message); - } - png_image_free(&png); - } else { - ERROR_LOG(G3D, "Could not load texture replacement info: %s - unsupported format", level.file.ToVisualString().c_str()); } - fclose(fp); - - return good; + vfs->Rewind(openFile); + return IdentifyMagic(magic); } -bool TextureReplacer::PopulateLevelFromZip(ReplacedTextureLevel &level, bool ignoreError) { +bool TextureReplacer::PopulateLevel(ReplacedTextureLevel &level, bool ignoreError) { bool good = false; // Everything in here is within a lock, so we don't need to relock. - if (!level.zinfo || !level.zinfo->z || level.zi < 0) { + if (!level.fileRef) { if (!ignoreError) ERROR_LOG(G3D, "Error opening replacement texture file '%s' in textures.zip", level.file.c_str()); return false; } + File::FileInfo info; + vfs_->GetFileInfo(level.fileRef, &info); + size_t zsize = info.size; - zip_file_t *zf = zip_fopen_index(level.zinfo->z, level.zi, 0); - if (!zf) + VFSOpenFile *file = vfs_->OpenFileForRead(level.fileRef); + if (!file) { return false; + } - auto imageType = Identify(zf); - zip_fclose(zf); + std::string magic; + auto imageType = Identify(vfs_, file, &magic); - zf = zip_fopen_index(level.zinfo->z, level.zi, 0); if (imageType == ReplacedImageType::ZIM) { uint32_t ignore = 0; - good = zip_fread(zf, &ignore, 4) == 4; - good = good && zip_fread(zf, &level.w, 4) == 4; - good = good && zip_fread(zf, &level.h, 4) == 4; - int flags; - if (good && zip_fread(zf, &flags, 4) == 4) { - good = (flags & ZIM_FORMAT_MASK) == ZIM_RGBA8888; - } + struct ZimHeader { + uint32_t magic; + uint32_t w; + uint32_t h; + uint32_t flags; + } header; + good = vfs_->Read(file, &header, sizeof(header)) == sizeof(header); + good = (header.flags & ZIM_FORMAT_MASK) == ZIM_RGBA8888; } else if (imageType == ReplacedImageType::PNG) { PNGHeaderPeek headerPeek; - good = zip_fread(zf, &headerPeek, sizeof(headerPeek)) == sizeof(headerPeek); + good = vfs_->Read(file, &headerPeek, sizeof(headerPeek)) == sizeof(headerPeek); if (good && headerPeek.IsValidPNGHeader()) { level.w = headerPeek.Width(); level.h = headerPeek.Height(); @@ -657,9 +559,9 @@ bool TextureReplacer::PopulateLevelFromZip(ReplacedTextureLevel &level, bool ign good = false; } } else { - ERROR_LOG(G3D, "Could not load texture replacement info: %s - unsupported format (zip)", level.file.ToVisualString().c_str()); + ERROR_LOG(G3D, "Could not load texture replacement info: %s - unsupported format %s", level.file.ToVisualString().c_str(), magic.c_str()); } - zip_fclose(zf); + vfs_->CloseFile(file); return good; } @@ -986,7 +888,7 @@ float TextureReplacer::LookupReduceHashRange(int& w, int& h) { class ReplacedTextureTask : public Task { public: - ReplacedTextureTask(ReplacedTexture &tex, LimitedWaitable *w) : tex_(tex), waitable_(w) {} + ReplacedTextureTask(VFSBackend *vfs, ReplacedTexture &tex, LimitedWaitable *w) : vfs_(vfs), tex_(tex), waitable_(w) {} TaskType Type() const override { return TaskType::IO_BLOCKING; @@ -997,16 +899,18 @@ public: } void Run() override { - tex_.Prepare(); + tex_.Prepare(vfs_); waitable_->Notify(); } private: ReplacedTexture &tex_; LimitedWaitable *waitable_; + VFSBackend *vfs_; }; bool ReplacedTexture::IsReady(double budget) { + _assert_(vfs_ != nullptr); lastUsed_ = time_now_d(); if (threadWaitable_ && !threadWaitable_->WaitFor(budget)) { return false; @@ -1028,14 +932,14 @@ bool ReplacedTexture::IsReady(double budget) { if (threadWaitable_) delete threadWaitable_; threadWaitable_ = new LimitedWaitable(); - g_threadManager.EnqueueTask(new ReplacedTextureTask(*this, threadWaitable_)); + g_threadManager.EnqueueTask(new ReplacedTextureTask(vfs_, *this, threadWaitable_)); if (threadWaitable_->WaitFor(budget)) { // If we finished all the levels, we're done. return initDone_ && !levelData_.empty(); } } else { - Prepare(); + Prepare(vfs_); _assert_(initDone_); return true; } @@ -1044,8 +948,9 @@ bool ReplacedTexture::IsReady(double budget) { return false; } -void ReplacedTexture::Prepare() { +void ReplacedTexture::Prepare(VFSBackend *vfs) { std::unique_lock lock(mutex_); + this->vfs_ = vfs; if (cancelPrepare_) { initDone_ = true; return; @@ -1069,83 +974,47 @@ void ReplacedTexture::PrepareData(int level) { // We must lock around access to levelData_ in case two textures try to load it at once. std::lock_guard guard(levelData_[level]->lock); - const ReplacedTextureLevel &info = levels_[level]; + ReplacedTextureLevel &info = levels_[level]; std::vector &out = levelData_[level]->data; // Already populated from cache. if (!out.empty()) return; - FILE *fp = nullptr; - zip_file_t *zf = nullptr; ReplacedImageType imageType; - std::unique_lock zguard; - if (info.zinfo && info.zinfo->z) { - zguard = std::unique_lock(info.zinfo->lock); - zf = zip_fopen_index(info.zinfo->z, info.zi, 0); - if (!zf) - return; - imageType = Identify(zf); - // Can't assume we can seek. Reopen. - zip_fclose(zf); - zf = zip_fopen_index(info.zinfo->z, info.zi, 0); - } else { - fp = File::OpenCFile(info.file, "rb"); - if (!fp) { - // Leaving the data sized at zero means failure. - return; - } - - imageType = Identify(fp); + File::FileInfo fileInfo; + if (!vfs_->GetFileInfo(info.fileRef, &fileInfo)) { + ERROR_LOG(G3D, "Failed to get file info for level"); + return; } + size_t fileSize = fileInfo.size; + + VFSOpenFile *openFile = vfs_->OpenFileForRead(info.fileRef); + std::string magic; + imageType = Identify(vfs_, openFile, &magic); auto cleanup = [&] { - if (zf) - zip_fclose(zf); - if (fp) - fclose(fp); + vfs_->CloseFile(openFile); }; if (imageType == ReplacedImageType::ZIM) { - size_t zimSize = 0; - if (fp) { - zimSize = File::GetFileSize(fp); - } else if (zf) { - zip_uint64_t zsize = ZipFileSize(info.zinfo->z, info.zi); - zimSize = zsize == INVALID_ZIP_SIZE ? 0 : (size_t)zsize; - } else { - _assert_(false); - } - std::unique_ptr zim(new uint8_t[zimSize]); + std::unique_ptr zim(new uint8_t[fileSize]); if (!zim) { ERROR_LOG(G3D, "Failed to allocate memory for texture replacement"); cleanup(); return; } - if (fp) { - if (fread(&zim[0], 1, zimSize, fp) != zimSize) { - ERROR_LOG(G3D, "Could not load texture replacement: %s - failed to read ZIM", info.file.c_str()); - cleanup(); - return; - } - } else if (zf) { - if (zip_fread(zf, &zim[0], zimSize) != zimSize) { - ERROR_LOG(G3D, "Could not load texture replacement: %s - failed to read ZIM (zip)", info.file.c_str()); - cleanup(); - return; - } - // Try to unlock early to prevent blocking threads. - zguard.unlock(); - } else { - _assert_(false); + if (vfs_->Read(openFile, &zim[0], fileSize) != fileSize) { + ERROR_LOG(G3D, "Could not load texture replacement: %s - failed to read ZIM", info.file.c_str()); + cleanup(); + return; } int w, h, f; uint8_t *image; - - if (LoadZIMPtr(&zim[0], zimSize, &w, &h, &f, &image)) { + if (LoadZIMPtr(&zim[0], fileSize, &w, &h, &f, &image)) { if (w > info.w || h > info.h) { ERROR_LOG(G3D, "Texture replacement changed since header read: %s", info.file.c_str()); cleanup(); @@ -1171,31 +1040,13 @@ void ReplacedTexture::PrepareData(int level) { png_image png = {}; png.version = PNG_IMAGE_VERSION; - // Needs to survive for a little while, used for zip only. std::string pngdata; - if (fp) { - if (!png_image_begin_read_from_stdio(&png, fp)) { - ERROR_LOG(G3D, "Could not load texture replacement info: %s - %s", info.file.c_str(), png.message); - cleanup(); - return; - } - } else if (zf) { - zip_uint64_t zsize = ZipFileSize(info.zinfo->z, info.zi); - if (zsize != INVALID_ZIP_SIZE) - pngdata.resize(zsize); - if (!pngdata.empty()) { - pngdata.resize(zip_fread(zf, &pngdata[0], pngdata.size())); - } - // Try to unlock early to prevent blocking threads. - zguard.unlock(); - - if (!png_image_begin_read_from_memory(&png, &pngdata[0], pngdata.size())) { - ERROR_LOG(G3D, "Could not load texture replacement info: %s - %s (zip)", info.file.c_str(), png.message); - cleanup(); - return; - } - } else { - _assert_(false); + pngdata.resize(fileSize); + vfs_->Read(openFile, &pngdata[0], fileSize); + if (!png_image_begin_read_from_memory(&png, &pngdata[0], pngdata.size())) { + ERROR_LOG(G3D, "Could not load texture replacement info: %s - %s (zip)", info.file.c_str(), png.message); + cleanup(); + return; } if (png.width > (uint32_t)info.w || png.height > (uint32_t)info.h) { ERROR_LOG(G3D, "Texture replacement changed since header read: %s", info.file.c_str()); @@ -1280,6 +1131,7 @@ bool ReplacedTexture::Load(int level, void *out, int rowPitch) { if (data.empty()) return false; + if (rowPitch < info.w * 4) { ERROR_LOG_REPORT(G3D, "Replacement rowPitch=%d, but w=%d (level=%d)", rowPitch, info.w * 4, level); return false; diff --git a/Core/TextureReplacer.h b/Core/TextureReplacer.h index cc9e4abed1..849186bc5f 100644 --- a/Core/TextureReplacer.h +++ b/Core/TextureReplacer.h @@ -27,6 +27,7 @@ #include "Common/CommonTypes.h" #include "Common/MemoryUtil.h" #include "Common/File/Path.h" +#include "Common/File/VFS/VFS.h" #include "Common/GPU/DataFormat.h" #include "GPU/Common/TextureDecoder.h" @@ -37,7 +38,7 @@ class TextureCacheCommon; class TextureReplacer; class ReplacedTextureTask; class LimitedWaitable; -struct zip; +class VFSBackend; // These must match the constants in TextureCacheCommon. enum class ReplacedTextureAlpha { @@ -52,13 +53,6 @@ enum class ReplacedTextureHash { XXH64, }; -struct ReplacerZipInfo { - zip *z = nullptr; - std::mutex lock; - - void Close(); -}; - struct ReplacedTextureLevel { int w; int h; @@ -66,9 +60,7 @@ struct ReplacedTextureLevel { Path file; // Can be ignored for hashing/equal, since file has all uniqueness. // To be able to reload, we need to be able to reopen, unfortunate we can't use zip_file_t. - ReplacerZipInfo *zinfo = nullptr; - int64_t zi = -1; - + VFSFileReference *fileRef; bool operator ==(const ReplacedTextureLevel &other) const { if (w != other.w || h != other.h || fmt != other.fmt) return false; @@ -206,7 +198,7 @@ struct ReplacedTexture { bool Load(int level, void *out, int rowPitch); protected: - void Prepare(); + void Prepare(VFSBackend *vfs); void PrepareData(int level); void PurgeIfOlder(double t); @@ -216,10 +208,13 @@ protected: double lastUsed_ = 0.0; LimitedWaitable *threadWaitable_ = nullptr; std::mutex mutex_; + bool cancelPrepare_ = false; bool initDone_ = false; bool prepareDone_ = false; + VFSBackend *vfs_ = nullptr; + friend TextureReplacer; friend ReplacedTextureTask; }; @@ -282,21 +277,22 @@ protected: std::string LookupHashFile(u64 cachekey, u32 hash, int level); std::string HashName(u64 cachekey, u32 hash, int level); void PopulateReplacement(ReplacedTexture *result, u64 cachekey, u32 hash, int w, int h); - bool PopulateLevelFromPath(ReplacedTextureLevel &level, bool ignoreError); - bool PopulateLevelFromZip(ReplacedTextureLevel &level, bool ignoreError); + bool PopulateLevel(ReplacedTextureLevel &level, bool ignoreError); bool enabled_ = false; bool allowVideo_ = false; bool ignoreAddress_ = false; bool reduceHash_ = false; - float reduceHashSize = 1.0; // default value with reduceHash to false - float reduceHashGlobalValue = 0.5; // Global value for textures dump pngs of all sizes, 0.5 by default but can be set in textures.ini + float reduceHashSize = 1.0f; // default value with reduceHash to false + float reduceHashGlobalValue = 0.5f; // Global value for textures dump pngs of all sizes, 0.5 by default but can be set in textures.ini + double lastTextureCacheSizeGB_ = 0.0; bool ignoreMipmap_ = false; std::string gameID_; Path basePath_; ReplacedTextureHash hash_ = ReplacedTextureHash::QUICK; - ReplacerZipInfo zip_; + + VFSBackend *vfs_ = nullptr; typedef std::pair WidthHeightPair; std::unordered_map hashranges_; From 16b0e1103a00991fda4a2ccd2816a451ab3ab983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 7 Mar 2023 20:59:34 +0100 Subject: [PATCH 2/7] Check file existence early. Although, not sure if this is the right thing on Android... --- Common/File/VFS/DirectoryReader.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Common/File/VFS/DirectoryReader.cpp b/Common/File/VFS/DirectoryReader.cpp index 999807ed0a..af05120450 100644 --- a/Common/File/VFS/DirectoryReader.cpp +++ b/Common/File/VFS/DirectoryReader.cpp @@ -44,8 +44,13 @@ public: }; VFSFileReference *DirectoryReader::GetFile(const char *path) { + Path filePath = path_ / path; + if (!File::Exists(filePath)) { + return nullptr; + } + DirectoryReaderFileReference *reference = new DirectoryReaderFileReference(); - reference->path = path_ / path; + reference->path = filePath; return reference; } From 41fcb7a79f958dc053dd29099ef92cb71f18b9f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 7 Mar 2023 22:02:44 +0100 Subject: [PATCH 3/7] Add necessary check --- Core/TextureReplacer.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Core/TextureReplacer.cpp b/Core/TextureReplacer.cpp index 43e6fbf83b..98e20a480e 100644 --- a/Core/TextureReplacer.cpp +++ b/Core/TextureReplacer.cpp @@ -453,6 +453,10 @@ void TextureReplacer::PopulateReplacement(ReplacedTexture *result, u64 cachekey, bool logError = hashfile != HashName(cachekey, hash, i) + ".png"; VFSFileReference *fileRef = vfs_->GetFile(hashfile.c_str()); + if (!fileRef) { + // If the file doesn't exist, let's just bail immediately here. + break; + } level.fileRef = fileRef; good = PopulateLevel(level, !logError); @@ -525,8 +529,11 @@ bool TextureReplacer::PopulateLevel(ReplacedTextureLevel &level, bool ignoreErro ERROR_LOG(G3D, "Error opening replacement texture file '%s' in textures.zip", level.file.c_str()); return false; } + File::FileInfo info; - vfs_->GetFileInfo(level.fileRef, &info); + if (!vfs_->GetFileInfo(level.fileRef, &info)) { + return false; + } size_t zsize = info.size; VFSOpenFile *file = vfs_->OpenFileForRead(level.fileRef); From ce9000e7e04e7d258bda00fb30cda9058f0e3d69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 8 Mar 2023 08:23:13 +0100 Subject: [PATCH 4/7] Address most of the feedback --- Core/TextureReplacer.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Core/TextureReplacer.cpp b/Core/TextureReplacer.cpp index 98e20a480e..6d2cab9f60 100644 --- a/Core/TextureReplacer.cpp +++ b/Core/TextureReplacer.cpp @@ -136,7 +136,8 @@ bool TextureReplacer::LoadIni() { iniLoaded = overrideIni.LoadFromVFS(*dir, overrideFilename); if (!iniLoaded) { ERROR_LOG(G3D, "Failed to load extra texture ini: %s", overrideFilename.c_str()); - // TODO: Should we just move on instead? + // Since this error is most likely to occure for texture pack creators, let's just bail here + // so that the creator is more likely to look in the logs for what happened. delete dir; return false; } @@ -523,7 +524,6 @@ static ReplacedImageType Identify(VFSBackend *vfs, VFSOpenFile *openFile, std::s bool TextureReplacer::PopulateLevel(ReplacedTextureLevel &level, bool ignoreError) { bool good = false; - // Everything in here is within a lock, so we don't need to relock. if (!level.fileRef) { if (!ignoreError) ERROR_LOG(G3D, "Error opening replacement texture file '%s' in textures.zip", level.file.c_str()); @@ -553,6 +553,8 @@ bool TextureReplacer::PopulateLevel(ReplacedTextureLevel &level, bool ignoreErro uint32_t flags; } header; good = vfs_->Read(file, &header, sizeof(header)) == sizeof(header); + level.w = header.w; + level.h = header.h; good = (header.flags & ZIM_FORMAT_MASK) == ZIM_RGBA8888; } else if (imageType == ReplacedImageType::PNG) { PNGHeaderPeek headerPeek; @@ -911,9 +913,9 @@ public: } private: + VFSBackend *vfs_; ReplacedTexture &tex_; LimitedWaitable *waitable_; - VFSBackend *vfs_; }; bool ReplacedTexture::IsReady(double budget) { @@ -981,7 +983,7 @@ void ReplacedTexture::PrepareData(int level) { // We must lock around access to levelData_ in case two textures try to load it at once. std::lock_guard guard(levelData_[level]->lock); - ReplacedTextureLevel &info = levels_[level]; + const ReplacedTextureLevel &info = levels_[level]; std::vector &out = levelData_[level]->data; // Already populated from cache. From e277db63c0586904db20bfa612585098ca22998b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 8 Mar 2023 09:11:25 +0100 Subject: [PATCH 5/7] Optimize away those extra GetFileInfo, since both backends can get the size during Open efficiently. --- Common/File/VFS/DirectoryReader.cpp | 8 ++++++-- Common/File/VFS/DirectoryReader.h | 2 +- Common/File/VFS/VFS.h | 4 +++- Common/File/VFS/ZipFileReader.cpp | 12 ++++++++++-- Common/File/VFS/ZipFileReader.h | 2 +- Core/TextureReplacer.cpp | 20 +++++--------------- 6 files changed, 26 insertions(+), 22 deletions(-) diff --git a/Common/File/VFS/DirectoryReader.cpp b/Common/File/VFS/DirectoryReader.cpp index af05120450..981dff905b 100644 --- a/Common/File/VFS/DirectoryReader.cpp +++ b/Common/File/VFS/DirectoryReader.cpp @@ -1,3 +1,5 @@ +#include + #include "Common/Common.h" #include "Common/Log.h" #include "Common/File/VFS/DirectoryReader.h" @@ -64,13 +66,15 @@ void DirectoryReader::ReleaseFile(VFSFileReference *vfsReference) { delete reference; } -VFSOpenFile *DirectoryReader::OpenFileForRead(VFSFileReference *vfsReference) { +VFSOpenFile *DirectoryReader::OpenFileForRead(VFSFileReference *vfsReference, size_t *size) { DirectoryReaderFileReference *reference = (DirectoryReaderFileReference *)vfsReference; FILE *file = File::OpenCFile(reference->path, "rb"); if (!file) { return nullptr; } - + fseek(file, 0, SEEK_END); + *size = ftell(file); + fseek(file, 0, SEEK_SET); DirectoryReaderOpenFile *openFile = new DirectoryReaderOpenFile(); openFile->file = file; return openFile; diff --git a/Common/File/VFS/DirectoryReader.h b/Common/File/VFS/DirectoryReader.h index cc6d971371..f742c6b3da 100644 --- a/Common/File/VFS/DirectoryReader.h +++ b/Common/File/VFS/DirectoryReader.h @@ -14,7 +14,7 @@ public: bool GetFileInfo(VFSFileReference *vfsReference, File::FileInfo *fileInfo) override; void ReleaseFile(VFSFileReference *vfsReference) override; - VFSOpenFile *OpenFileForRead(VFSFileReference *vfsReference) override; + VFSOpenFile *OpenFileForRead(VFSFileReference *vfsReference, size_t *size) override; void Rewind(VFSOpenFile *vfsOpenFile) override; size_t Read(VFSOpenFile *vfsOpenFile, void *buffer, size_t length) override; void CloseFile(VFSOpenFile *vfsOpenFile) override; diff --git a/Common/File/VFS/VFS.h b/Common/File/VFS/VFS.h index 67d737d5f7..5d585e309e 100644 --- a/Common/File/VFS/VFS.h +++ b/Common/File/VFS/VFS.h @@ -46,7 +46,9 @@ public: virtual bool GetFileInfo(VFSFileReference *vfsReference, File::FileInfo *fileInfo) = 0; virtual void ReleaseFile(VFSFileReference *vfsReference) = 0; - virtual VFSOpenFile *OpenFileForRead(VFSFileReference *vfsReference) = 0; + // Must write the size of the file to *size. Both backends can do this efficiently here, + // avoiding a call to GetFileInfo. + virtual VFSOpenFile *OpenFileForRead(VFSFileReference *vfsReference, size_t *size) = 0; virtual void Rewind(VFSOpenFile *vfsOpenFile) = 0; virtual size_t Read(VFSOpenFile *vfsOpenFile, void *buffer, size_t length) = 0; virtual void CloseFile(VFSOpenFile *vfsOpenFile) = 0; diff --git a/Common/File/VFS/ZipFileReader.cpp b/Common/File/VFS/ZipFileReader.cpp index 694ef95864..a03307ed72 100644 --- a/Common/File/VFS/ZipFileReader.cpp +++ b/Common/File/VFS/ZipFileReader.cpp @@ -238,21 +238,29 @@ void ZipFileReader::ReleaseFile(VFSFileReference *vfsReference) { delete reference; } -VFSOpenFile *ZipFileReader::OpenFileForRead(VFSFileReference *vfsReference) { +VFSOpenFile *ZipFileReader::OpenFileForRead(VFSFileReference *vfsReference, size_t *size) { ZipFileReaderFileReference *reference = (ZipFileReaderFileReference *)vfsReference; ZipFileReaderOpenFile *openFile = new ZipFileReaderOpenFile(); openFile->reference = reference; + *size = 0; // We only allow one file to be open for read concurrently. It's possible that this can be improved, // especially if we only access by index like this. lock_.lock(); - openFile->zf = zip_fopen_index(zip_file_, reference->zi, 0); + zip_stat_t zstat; + if (zip_stat_index(zip_file_, reference->zi, 0, &zstat) != 0) { + lock_.unlock(); + return false; + } + openFile->zf = zip_fopen_index(zip_file_, reference->zi, 0); if (!openFile->zf) { WARN_LOG(G3D, "File with index %d not found in zip", reference->zi); lock_.unlock(); return nullptr; } + *size = zstat.size; + // Intentionally leaving the mutex locked, will be closed in CloseFile. return openFile; } diff --git a/Common/File/VFS/ZipFileReader.h b/Common/File/VFS/ZipFileReader.h index e3c50b38bf..f39058beed 100644 --- a/Common/File/VFS/ZipFileReader.h +++ b/Common/File/VFS/ZipFileReader.h @@ -28,7 +28,7 @@ public: bool GetFileInfo(VFSFileReference *vfsReference, File::FileInfo *fileInfo) override; void ReleaseFile(VFSFileReference *vfsReference) override; - VFSOpenFile *OpenFileForRead(VFSFileReference *vfsReference) override; + VFSOpenFile *OpenFileForRead(VFSFileReference *vfsReference, size_t *size) override; void Rewind(VFSOpenFile *vfsOpenFile) override; size_t Read(VFSOpenFile *vfsOpenFile, void *buffer, size_t length) override; void CloseFile(VFSOpenFile *vfsOpenFile) override; diff --git a/Core/TextureReplacer.cpp b/Core/TextureReplacer.cpp index 6d2cab9f60..bc81d0e032 100644 --- a/Core/TextureReplacer.cpp +++ b/Core/TextureReplacer.cpp @@ -530,13 +530,8 @@ bool TextureReplacer::PopulateLevel(ReplacedTextureLevel &level, bool ignoreErro return false; } - File::FileInfo info; - if (!vfs_->GetFileInfo(level.fileRef, &info)) { - return false; - } - size_t zsize = info.size; - - VFSOpenFile *file = vfs_->OpenFileForRead(level.fileRef); + size_t fileSize; + VFSOpenFile *file = vfs_->OpenFileForRead(level.fileRef, &fileSize); if (!file) { return false; } @@ -992,14 +987,9 @@ void ReplacedTexture::PrepareData(int level) { ReplacedImageType imageType; - File::FileInfo fileInfo; - if (!vfs_->GetFileInfo(info.fileRef, &fileInfo)) { - ERROR_LOG(G3D, "Failed to get file info for level"); - return; - } - size_t fileSize = fileInfo.size; + size_t fileSize; + VFSOpenFile *openFile = vfs_->OpenFileForRead(info.fileRef, &fileSize); - VFSOpenFile *openFile = vfs_->OpenFileForRead(info.fileRef); std::string magic; imageType = Identify(vfs_, openFile, &magic); @@ -1051,7 +1041,7 @@ void ReplacedTexture::PrepareData(int level) { std::string pngdata; pngdata.resize(fileSize); - vfs_->Read(openFile, &pngdata[0], fileSize); + pngdata.resize(vfs_->Read(openFile, &pngdata[0], fileSize)); if (!png_image_begin_read_from_memory(&png, &pngdata[0], pngdata.size())) { ERROR_LOG(G3D, "Could not load texture replacement info: %s - %s (zip)", info.file.c_str(), png.message); cleanup(); From 74cad6b5211a2f821e4ceae379e67cbaf6395cae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 8 Mar 2023 09:17:26 +0100 Subject: [PATCH 6/7] Quick buildfix --- Common/File/VFS/ZipFileReader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Common/File/VFS/ZipFileReader.cpp b/Common/File/VFS/ZipFileReader.cpp index a03307ed72..35a8aa4afa 100644 --- a/Common/File/VFS/ZipFileReader.cpp +++ b/Common/File/VFS/ZipFileReader.cpp @@ -249,7 +249,7 @@ VFSOpenFile *ZipFileReader::OpenFileForRead(VFSFileReference *vfsReference, size zip_stat_t zstat; if (zip_stat_index(zip_file_, reference->zi, 0, &zstat) != 0) { lock_.unlock(); - return false; + return nullptr; } openFile->zf = zip_fopen_index(zip_file_, reference->zi, 0); From 306fdde9d0dd606fad5732de917709573b52e393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 8 Mar 2023 09:29:19 +0100 Subject: [PATCH 7/7] Correct error handling for missing ini (only accept it in directories, not zip) --- Common/File/VFS/ZipFileReader.cpp | 35 +++++++++++++------------------ Core/TextureReplacer.cpp | 13 ++++++++++++ Core/TextureReplacer.h | 1 + 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/Common/File/VFS/ZipFileReader.cpp b/Common/File/VFS/ZipFileReader.cpp index 35a8aa4afa..d4e4f69e66 100644 --- a/Common/File/VFS/ZipFileReader.cpp +++ b/Common/File/VFS/ZipFileReader.cpp @@ -15,25 +15,6 @@ #include "Common/File/VFS/ZipFileReader.h" #include "Common/StringUtils.h" -static uint8_t *ReadFromZip(zip *archive, const char* filename, size_t *size) { - // Figure out the file size first. - struct zip_stat zstat; - zip_file *file = zip_fopen(archive, filename, ZIP_FL_NOCASE|ZIP_FL_UNCHANGED); - if (!file) { - ERROR_LOG(IO, "Error opening %s from ZIP", filename); - return 0; - } - zip_stat(archive, filename, ZIP_FL_NOCASE|ZIP_FL_UNCHANGED, &zstat); - - uint8_t *contents = new uint8_t[zstat.size + 1]; - zip_fread(file, contents, zstat.size); - zip_fclose(file); - contents[zstat.size] = 0; - - *size = zstat.size; - return contents; -} - ZipFileReader *ZipFileReader::Create(const Path &zipFile, const char *inZipPath) { int error = 0; zip *zip_file; @@ -69,7 +50,21 @@ uint8_t *ZipFileReader::ReadFile(const char *path, size_t *size) { snprintf(temp_path, sizeof(temp_path), "%s%s", inZipPath_, path); std::lock_guard guard(lock_); - return ReadFromZip(zip_file_, temp_path, size); + // 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); + if (!file) { + ERROR_LOG(IO, "Error opening %s from ZIP", temp_path); + return 0; + } + uint8_t *contents = new uint8_t[zstat.size + 1]; + zip_fread(file, contents, zstat.size); + zip_fclose(file); + contents[zstat.size] = 0; + + *size = zstat.size; + return contents; } bool ZipFileReader::GetFileListing(const char *orig_path, std::vector *listing, const char *filter = 0) { diff --git a/Core/TextureReplacer.cpp b/Core/TextureReplacer.cpp index bc81d0e032..89b71963d2 100644 --- a/Core/TextureReplacer.cpp +++ b/Core/TextureReplacer.cpp @@ -116,7 +116,10 @@ bool TextureReplacer::LoadIni() { // First, check for textures.zip, which is used to reduce IO. VFSBackend *dir = ZipFileReader::Create(basePath_ / ZIP_FILENAME, ""); if (!dir) { + vfsIsZip_ = false; dir = new DirectoryReader(basePath_); + } else { + vfsIsZip_ = true; } IniFile ini; @@ -149,9 +152,19 @@ bool TextureReplacer::LoadIni() { } } } + } else { + if (vfsIsZip_) { + // We don't accept zip files without inis. + ERROR_LOG(G3D, "Texture pack lacking ini file: %s", basePath_.c_str()); + delete dir; + return false; + } else { + WARN_LOG(G3D, "Texture pack lacking ini file: %s", basePath_.c_str()); + } } vfs_ = dir; + INFO_LOG(G3D, "Texture pack activated from '%s'", basePath_.c_str()); // The ini doesn't have to exist for the texture directory or zip to be valid. return true; diff --git a/Core/TextureReplacer.h b/Core/TextureReplacer.h index 849186bc5f..9281edf3ef 100644 --- a/Core/TextureReplacer.h +++ b/Core/TextureReplacer.h @@ -293,6 +293,7 @@ protected: ReplacedTextureHash hash_ = ReplacedTextureHash::QUICK; VFSBackend *vfs_ = nullptr; + bool vfsIsZip_ = false; typedef std::pair WidthHeightPair; std::unordered_map hashranges_;