From f9efcfb8d6445fbc40480c0f482e554e7c4f787e Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 16 May 2013 20:26:20 +0900 Subject: [PATCH 1/6] Fixed the test for ID3V2's compressed frame --- tests/test_id3v2.cpp | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index 48faf306..42d0f138 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -1,5 +1,6 @@ #include #include +#include // so evil :( #define protected public #include @@ -15,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -580,13 +582,27 @@ public: { MPEG::File f(TEST_FILE_PATH_C("compressed_id3_frame.mp3"), false); CPPUNIT_ASSERT(f.ID3v2Tag()->frameListMap().contains("APIC")); - ID3v2::AttachedPictureFrame *frame = - static_cast(f.ID3v2Tag()->frameListMap()["APIC"].front()); + +#ifdef HAVE_ZLIB + + ID3v2::AttachedPictureFrame *frame + = dynamic_cast(f.ID3v2Tag()->frameListMap()["APIC"].front()); CPPUNIT_ASSERT(frame); CPPUNIT_ASSERT_EQUAL(String("image/bmp"), frame->mimeType()); CPPUNIT_ASSERT_EQUAL(ID3v2::AttachedPictureFrame::Other, frame->type()); CPPUNIT_ASSERT_EQUAL(String(""), frame->description()); CPPUNIT_ASSERT_EQUAL(TagLib::uint(86414), frame->picture().size()); + +#else + + // Skip the test if ZLIB is not installed. + // The message "Compressed frames are currently not supported." will be displayed. + + ID3v2::UnknownFrame *frame + = dynamic_cast(f.ID3v2Tag()->frameListMap()["APIC"].front()); + CPPUNIT_ASSERT(frame); + +#endif } void testW000() From 36d9fc1973e592b41af5e25848ae205861ac5f5a Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 18 May 2013 23:30:15 +0900 Subject: [PATCH 2/6] Small change in Win9x support --- taglib/toolkit/tiostream.cpp | 43 +++++++++++++----------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/taglib/toolkit/tiostream.cpp b/taglib/toolkit/tiostream.cpp index 2d4f66d8..b7773975 100644 --- a/taglib/toolkit/tiostream.cpp +++ b/taglib/toolkit/tiostream.cpp @@ -29,30 +29,26 @@ using namespace TagLib; #ifdef _WIN32 -// MSVC 2008 or later can't produce the binary for Win9x. -#if !defined(_MSC_VER) || (_MSC_VER < 1500) +# include namespace { + // Check if the running system has CreateFileW() function. + // Windows9x systems don't have CreateFileW() or can't accept Unicode file names. - // Determines whether or not the running system is WinNT. - // In other words, whether the system supports Unicode. - - bool isWinNT() + bool supportsUnicode() { - OSVERSIONINFOA ver = {}; - ver.dwOSVersionInfoSize = sizeof(OSVERSIONINFOA); - if(GetVersionExA(&ver)) { - return (ver.dwPlatformId == VER_PLATFORM_WIN32_NT); - } - else { - return false; - } + const FARPROC p = GetProcAddress(GetModuleHandleA("kernel32"), "CreateFileW"); + return (p != NULL); } + + // Indicates whether the system supports Unicode file names. - const bool IsWinNT = isWinNT(); + const bool SystemSupportsUnicode = supportsUnicode(); // Converts a UTF-16 string into a local encoding. + // This function should only be used in Windows9x systems which don't support + // Unicode file names. std::string unicodeToAnsi(const std::wstring &wstr) { @@ -71,20 +67,11 @@ namespace // If Win9x, converts and stores it into m_name to avoid calling Unicode version functions. FileName::FileName(const wchar_t *name) - : m_wname(IsWinNT ? name : L"") - , m_name(IsWinNT ? "" : unicodeToAnsi(name)) + : m_wname(SystemSupportsUnicode ? name : L"") + , m_name (SystemSupportsUnicode ? "" : unicodeToAnsi(name)) { } -#else - -FileName::FileName(const wchar_t *name) - : m_wname(name) -{ -} - -#endif - FileName::FileName(const char *name) : m_name(name) { @@ -92,7 +79,7 @@ FileName::FileName(const char *name) FileName::FileName(const FileName &name) : m_wname(name.m_wname) - , m_name(name.m_name) + , m_name (name.m_name) { } @@ -116,7 +103,7 @@ const std::string &FileName::str() const return m_name; } -#endif +#endif // _WIN32 //////////////////////////////////////////////////////////////////////////////// // public members From dcf11b95861a72f3c5c23ff0763ee2d74d948449 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 19 May 2013 02:33:17 +0900 Subject: [PATCH 3/6] Small refactoring of FileStream --- taglib/toolkit/tbytevector.cpp | 8 +- taglib/toolkit/tfilestream.cpp | 228 +++++++++++++++------------------ 2 files changed, 106 insertions(+), 130 deletions(-) diff --git a/taglib/toolkit/tbytevector.cpp b/taglib/toolkit/tbytevector.cpp index 471af477..b3c8897f 100644 --- a/taglib/toolkit/tbytevector.cpp +++ b/taglib/toolkit/tbytevector.cpp @@ -729,9 +729,11 @@ TagLib::uint ByteVector::size() const ByteVector &ByteVector::resize(uint size, char padding) { - detach(); - d->data->data.resize(d->offset + size, padding); - d->length = size; + if(size != d->length) { + detach(); + d->data->data.resize(d->offset + size, padding); + d->length = size; + } return *this; } diff --git a/taglib/toolkit/tfilestream.cpp b/taglib/toolkit/tfilestream.cpp index 2ce5ab41..7a931cbe 100644 --- a/taglib/toolkit/tfilestream.cpp +++ b/taglib/toolkit/tfilestream.cpp @@ -51,9 +51,10 @@ namespace typedef FileName FileNameHandle; -# define INVALID_FILE INVALID_HANDLE_VALUE + typedef HANDLE FileHandle; + const FileHandle InvalidFileHandle = INVALID_HANDLE_VALUE; - HANDLE openFile(const FileName &path, bool readOnly) + inline FileHandle openFile(const FileName &path, bool readOnly) { const DWORD access = readOnly ? GENERIC_READ : (GENERIC_READ | GENERIC_WRITE); @@ -62,29 +63,32 @@ namespace else if(!path.str().empty()) return CreateFileA(path.str().c_str(), access, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); else - return INVALID_FILE; + return InvalidFileHandle; } - size_t fread(void *ptr, size_t size, size_t nmemb, HANDLE stream) + inline void closeFile(FileHandle file) { - DWORD readLen; - if(ReadFile(stream, ptr, size * nmemb, &readLen, NULL)) - return (readLen / size); + CloseHandle(file); + } + + inline size_t readFile(FileHandle file, ByteVector &buffer) + { + DWORD length; + if(ReadFile(file, buffer.data(), static_cast(buffer.size()), &length, NULL)) + return static_cast(length); else return 0; } - size_t fwrite(const void *ptr, size_t size, size_t nmemb, HANDLE stream) + inline size_t writeFile(FileHandle file, const ByteVector &buffer) { - DWORD writtenLen; - if(WriteFile(stream, ptr, size * nmemb, &writtenLen, NULL)) - return (writtenLen / size); + DWORD length; + if(WriteFile(file, buffer.data(), static_cast(buffer.size()), &length, NULL)) + return static_cast(length); else return 0; } -# if _DEBUG - // Convert a string in a local encoding into a UTF-16 string. // This function should only be used to generate an error message. @@ -111,78 +115,59 @@ namespace } } -# endif - #else -# define INVALID_FILE 0 - struct FileNameHandle : public std::string { FileNameHandle(FileName name) : std::string(name) {} operator FileName () const { return c_str(); } }; - FILE *openFile(const FileName &path, bool readOnly) + typedef FILE* FileHandle; + const FileHandle InvalidFileHandle = 0; + + inline FileHandle openFile(const FileName &path, bool readOnly) { return fopen(path, readOnly ? "rb" : "rb+"); } + inline void closeFile(FileHandle file) + { + fclose(file); + } + + inline size_t readFile(FileHandle file, ByteVector &buffer) + { + return fread(buffer.data(), sizeof(char), buffer.size(), file); + } + + inline size_t writeFile(FileHandle file, const ByteVector &buffer) + { + return fwrite(buffer.data(), sizeof(char), buffer.size(), file); + } + #endif } class FileStream::FileStreamPrivate { public: - FileStreamPrivate(const FileName &fileName, bool openReadOnly); - -#ifdef _WIN32 - - HANDLE file; - -#else - - FILE *file; - -#endif + FileStreamPrivate(const FileName &fileName, bool openReadOnly) + : file(InvalidFileHandle) + , name(fileName) + , readOnly(openReadOnly) + , size(0) + { + } + FileHandle file; FileNameHandle name; - bool readOnly; ulong size; + static const uint bufferSize = 1024; }; -FileStream::FileStreamPrivate::FileStreamPrivate(const FileName &fileName, bool openReadOnly) : - file(INVALID_FILE), - name(fileName), - readOnly(true), - size(0) -{ - // First try with read / write mode, if that fails, fall back to read only. - - if(!openReadOnly) - file = openFile(name, false); - - if(file != INVALID_FILE) - readOnly = false; - else - file = openFile(name, true); - - if(file == INVALID_FILE) - { -# ifdef _WIN32 - - debug("Could not open file " + fileNameToString(name)); - -# else - - debug("Could not open file " + String((const char *) name)); - -# endif - } -} - //////////////////////////////////////////////////////////////////////////////// // public members //////////////////////////////////////////////////////////////////////////////// @@ -190,21 +175,30 @@ FileStream::FileStreamPrivate::FileStreamPrivate(const FileName &fileName, bool FileStream::FileStream(FileName file, bool openReadOnly) : d(new FileStreamPrivate(file, openReadOnly)) { + // First try with read / write mode, if that fails, fall back to read only. + + if(!openReadOnly) + d->file = openFile(file, false); + + if(d->file != InvalidFileHandle) + d->readOnly = false; + else + d->file = openFile(d->name, true); + + if(d->file == InvalidFileHandle) + { +# ifdef _WIN32 + debug("Could not open file " + fileNameToString(d->name)); +# else + debug("Could not open file " + String(static_cast(d->name))); +# endif + } } FileStream::~FileStream() { -#ifdef _WIN32 - if(isOpen()) - CloseHandle(d->file); - -#else - - if(isOpen()) - fclose(d->file); - -#endif + closeFile(d->file); delete d; } @@ -224,16 +218,16 @@ ByteVector FileStream::readBlock(ulong length) if(length == 0) return ByteVector::null; - if(length > FileStreamPrivate::bufferSize && - length > ulong(FileStream::length())) - { - length = FileStream::length(); - } + const ulong streamLength = static_cast(FileStream::length()); + if(length > bufferSize() && length > streamLength) + length = streamLength; - ByteVector v(static_cast(length)); - const int count = fread(v.data(), sizeof(char), length, d->file); - v.resize(count); - return v; + ByteVector buffer(static_cast(length)); + + const size_t count = readFile(d->file, buffer); + buffer.resize(static_cast(count)); + + return buffer; } void FileStream::writeBlock(const ByteVector &data) @@ -248,7 +242,7 @@ void FileStream::writeBlock(const ByteVector &data) return; } - fwrite(data.data(), sizeof(char), data.size(), d->file); + writeFile(d->file, data); } void FileStream::insert(const ByteVector &data, ulong start, ulong replace) @@ -269,10 +263,10 @@ void FileStream::insert(const ByteVector &data, ulong start, ulong replace) return; } else if(data.size() < replace) { - seek(start); - writeBlock(data); - removeBlock(start + data.size(), replace - data.size()); - return; + seek(start); + writeBlock(data); + removeBlock(start + data.size(), replace - data.size()); + return; } // Woohoo! Faster (about 20%) than id3lib at last. I had to get hardcore @@ -295,64 +289,41 @@ void FileStream::insert(const ByteVector &data, ulong start, ulong replace) long readPosition = start + replace; long writePosition = start; - ByteVector buffer; + ByteVector buffer = data; ByteVector aboutToOverwrite(static_cast(bufferLength)); - // This is basically a special case of the loop below. Here we're just - // doing the same steps as below, but since we aren't using the same buffer - // size -- instead we're using the tag size -- this has to be handled as a - // special case. We're also using File::writeBlock() just for the tag. - // That's a bit slower than using char *'s so, we're only doing it here. - - seek(readPosition); - int bytesRead = fread(aboutToOverwrite.data(), sizeof(char), bufferLength, d->file); - readPosition += bufferLength; - - seek(writePosition); - writeBlock(data); - writePosition += data.size(); - - buffer = aboutToOverwrite; - - // In case we've already reached the end of file... - - buffer.resize(bytesRead); - - // Ok, here's the main loop. We want to loop until the read fails, which - // means that we hit the end of the file. - - while(!buffer.isEmpty()) { - + while(true) + { // Seek to the current read position and read the data that we're about // to overwrite. Appropriately increment the readPosition. - + seek(readPosition); - bytesRead = fread(aboutToOverwrite.data(), sizeof(char), bufferLength, d->file); + const size_t bytesRead = readFile(d->file, aboutToOverwrite); aboutToOverwrite.resize(bytesRead); readPosition += bufferLength; // Check to see if we just read the last block. We need to call clear() // if we did so that the last write succeeds. - if(ulong(bytesRead) < bufferLength) + if(bytesRead < bufferLength) clear(); // Seek to the write position and write our buffer. Increment the // writePosition. seek(writePosition); - fwrite(buffer.data(), sizeof(char), buffer.size(), d->file); + writeBlock(buffer); + + // We hit the end of the file. + + if(bytesRead == 0) + break; + writePosition += buffer.size(); // Make the current buffer the data that we read in the beginning. - + buffer = aboutToOverwrite; - - // Again, we need this for the last write. We don't want to write garbage - // at the end of our file, so we need to set the buffer size to the amount - // that we actually read. - - bufferLength = bytesRead; } } @@ -370,11 +341,9 @@ void FileStream::removeBlock(ulong start, ulong length) ByteVector buffer(static_cast(bufferLength)); - ulong bytesRead = 1; - - while(bytesRead != 0) { + while(true) { seek(readPosition); - bytesRead = fread(buffer.data(), sizeof(char), bufferLength, d->file); + const size_t bytesRead = readFile(d->file, buffer); readPosition += bytesRead; // Check to see if we just read the last block. We need to call clear() @@ -384,9 +353,14 @@ void FileStream::removeBlock(ulong start, ulong length) clear(); seek(writePosition); - fwrite(buffer.data(), sizeof(char), bytesRead, d->file); + writeFile(d->file, buffer); + + if(bytesRead == 0) + break; + writePosition += bytesRead; } + truncate(writePosition); } @@ -397,7 +371,7 @@ bool FileStream::readOnly() const bool FileStream::isOpen() const { - return (d->file != INVALID_FILE); + return (d->file != InvalidFileHandle); } void FileStream::seek(long offset, Position p) From 5c3f096fe4a77219f73e94e9b9f66677ad515ddb Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 19 May 2013 11:09:43 +0900 Subject: [PATCH 4/6] Fixed initialization of FileStream --- taglib/toolkit/tfile.cpp | 9 +-------- taglib/toolkit/tfile.h | 2 +- taglib/toolkit/tfilestream.cpp | 36 +++++++++++++++------------------- taglib/toolkit/tiostream.cpp | 12 +++++++++--- 4 files changed, 27 insertions(+), 32 deletions(-) diff --git a/taglib/toolkit/tfile.cpp b/taglib/toolkit/tfile.cpp index fdb5237f..a229e85f 100644 --- a/taglib/toolkit/tfile.cpp +++ b/taglib/toolkit/tfile.cpp @@ -29,21 +29,14 @@ #include "tdebug.h" #include "tpropertymap.h" -#include -#include -#include - #ifdef _WIN32 -# include # include # include -# define ftruncate _chsize #else +# include # include #endif -#include - #ifndef R_OK # define R_OK 4 #endif diff --git a/taglib/toolkit/tfile.h b/taglib/toolkit/tfile.h index 30e53f93..67f6f80f 100644 --- a/taglib/toolkit/tfile.h +++ b/taglib/toolkit/tfile.h @@ -213,7 +213,7 @@ namespace TagLib { bool isOpen() const; /*! - * Returns true if the file is open and readble. + * Returns true if the file is open and readable. */ bool isValid() const; diff --git a/taglib/toolkit/tfilestream.cpp b/taglib/toolkit/tfilestream.cpp index 7a931cbe..1798134f 100644 --- a/taglib/toolkit/tfilestream.cpp +++ b/taglib/toolkit/tfilestream.cpp @@ -27,20 +27,13 @@ #include "tstring.h" #include "tdebug.h" -#include -#include -#include - #ifdef _WIN32 -# include # include -# include #else +# include # include #endif -#include - using namespace TagLib; namespace @@ -89,11 +82,12 @@ namespace return 0; } +# ifndef NDEBUG + // Convert a string in a local encoding into a UTF-16 string. - // This function should only be used to generate an error message. - // In actual use, file names in local encodings are passed to CreateFileA() - // without any conversions. + // Debugging use only. In actual use, file names in local encodings are passed to + // CreateFileA() without any conversions. String fileNameToString(const FileName &name) { @@ -115,7 +109,9 @@ namespace } } -#else +# endif + +#else // _WIN32 struct FileNameHandle : public std::string { @@ -146,16 +142,16 @@ namespace return fwrite(buffer.data(), sizeof(char), buffer.size(), file); } -#endif +#endif // _WIN32 } class FileStream::FileStreamPrivate { public: - FileStreamPrivate(const FileName &fileName, bool openReadOnly) + FileStreamPrivate(const FileName &fileName) : file(InvalidFileHandle) , name(fileName) - , readOnly(openReadOnly) + , readOnly(true) , size(0) { } @@ -172,23 +168,23 @@ public: // public members //////////////////////////////////////////////////////////////////////////////// -FileStream::FileStream(FileName file, bool openReadOnly) - : d(new FileStreamPrivate(file, openReadOnly)) +FileStream::FileStream(FileName fileName, bool openReadOnly) + : d(new FileStreamPrivate(fileName)) { // First try with read / write mode, if that fails, fall back to read only. if(!openReadOnly) - d->file = openFile(file, false); + d->file = openFile(fileName, false); if(d->file != InvalidFileHandle) d->readOnly = false; else - d->file = openFile(d->name, true); + d->file = openFile(fileName, true); if(d->file == InvalidFileHandle) { # ifdef _WIN32 - debug("Could not open file " + fileNameToString(d->name)); + debug("Could not open file " + fileNameToString(fileName)); # else debug("Could not open file " + String(static_cast(d->name))); # endif diff --git a/taglib/toolkit/tiostream.cpp b/taglib/toolkit/tiostream.cpp index b7773975..2832e414 100644 --- a/taglib/toolkit/tiostream.cpp +++ b/taglib/toolkit/tiostream.cpp @@ -29,6 +29,8 @@ using namespace TagLib; #ifdef _WIN32 +# include "tstring.h" +# include "tdebug.h" # include namespace @@ -50,14 +52,18 @@ namespace // This function should only be used in Windows9x systems which don't support // Unicode file names. - std::string unicodeToAnsi(const std::wstring &wstr) + std::string unicodeToAnsi(const wchar_t *wstr) { - const int len = WideCharToMultiByte(CP_ACP, 0, &wstr[0], -1, NULL, 0, NULL, NULL); + if(SystemSupportsUnicode) { + debug("unicodeToAnsi() - Should not be used on WinNT systems."); + } + + const int len = WideCharToMultiByte(CP_ACP, 0, wstr, -1, NULL, 0, NULL, NULL); if(len == 0) return std::string(); std::string str(len, '\0'); - WideCharToMultiByte(CP_ACP, 0, &wstr[0], -1, &str[0], len, NULL, NULL); + WideCharToMultiByte(CP_ACP, 0, wstr, -1, &str[0], len, NULL, NULL); return str; } From 79f3edebc06529baa1b044fd264556665565a168 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 19 May 2013 11:59:37 +0900 Subject: [PATCH 5/6] Added myself to AUTHORS --- AUTHORS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/AUTHORS b/AUTHORS index 3683f3fb..e996d941 100644 --- a/AUTHORS +++ b/AUTHORS @@ -10,6 +10,8 @@ Teemu Tervo Numerous bug reports and fixes Mathias Panzenböck Mod, S3M, IT and XM metadata implementations +Tsuda Kageyu + A lot of fixes and improvements, i.e. memory copy reduction etc. Please send all patches and questions to taglib-devel@kde.org rather than to individual developers! From bbec1c7f81b8f59567fc8868aca61deb38acafeb Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 19 May 2013 14:39:45 +0900 Subject: [PATCH 6/6] Changed String::number() to use a standard function --- ConfigureChecks.cmake | 14 ++++++++++++++ config.h.cmake | 4 ++++ taglib/toolkit/tstring.cpp | 36 ++++++++++++++++++------------------ 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/ConfigureChecks.cmake b/ConfigureChecks.cmake index 440a4414..46611a00 100644 --- a/ConfigureChecks.cmake +++ b/ConfigureChecks.cmake @@ -183,6 +183,20 @@ if(NOT HAVE_GCC_BYTESWAP_16 OR NOT HAVE_GCC_BYTESWAP_32 OR NOT HAVE_GCC_BYTESWAP endif() endif() +# Determine whether your compiler supports some safer version of sprintf. + +check_cxx_source_compiles(" + #include + int main() { char buf[20]; snprintf(buf, 20, \"%d\", 1); return 0; } +" HAVE_SNPRINTF) + +if(NOT HAVE_SNPRINTF) + check_cxx_source_compiles(" + #include + int main() { char buf[20]; sprintf_s(buf, \"%d\", 1); return 0; } + " HAVE_SPRINTF_S) +endif() + # Determine whether your compiler supports codecvt. check_cxx_source_compiles(" diff --git a/config.h.cmake b/config.h.cmake index 436ca440..15d2f997 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -24,6 +24,10 @@ #cmakedefine HAVE_WIN_ATOMIC 1 #cmakedefine HAVE_IA64_ATOMIC 1 +/* Defined if your compiler supports some safer version of sprintf */ +#cmakedefine HAVE_SNPRINTF 1 +#cmakedefine HAVE_SPRINTF_S 1 + /* Defined if you have libz */ #cmakedefine HAVE_ZLIB 1 diff --git a/taglib/toolkit/tstring.cpp b/taglib/toolkit/tstring.cpp index cf745f81..7b4d2040 100644 --- a/taglib/toolkit/tstring.cpp +++ b/taglib/toolkit/tstring.cpp @@ -33,7 +33,8 @@ #include "trefcounter.h" #include -#include +#include +#include #if defined(HAVE_MSC_BYTESWAP) # include @@ -597,31 +598,30 @@ bool String::isAscii() const String String::number(int n) // static { - if(n == 0) - return String("0"); + static const size_t BufferSize = 11; // Sufficient to store "-214748364". + static const char *Format = "%d"; - String charStack; + char buffer[BufferSize]; + int length; - bool negative = n < 0; +#if defined(HAVE_SNPRINTF) - if(negative) - n = n * -1; + length = snprintf(buffer, BufferSize, Format, n); - while(n > 0) { - int remainder = n % 10; - charStack += char(remainder + '0'); - n = (n - remainder) / 10; - } +#elif defined(HAVE_SPRINTF_S) - String s; + length = sprintf_s(buffer, Format, n); - if(negative) - s += '-'; +#else - for(int i = charStack.d->data.size() - 1; i >= 0; i--) - s += charStack.d->data[i]; + length = sprintf(buffer, Format, n); - return s; +#endif + + if(length > 0) + return String(buffer); + else + return String::null; } TagLib::wchar &String::operator[](int i)