10
\$\begingroup\$

I wrote a tiny command line tool that converts ADF audio files from the Grand Theft Auto - Vice City game to MP3 files that can be played outside the game.

The ADF file format stores the game soundtracks and radio stations. These in turn are just MP3 files that had every byte XORed with the decimal constant 34.

My application is very simple, it only takes one or two file names and processes every byte of the input file, producing a MP3 as output.

Usage example:

$ ./adf2mp3 flash.adf

Looks for the file "flash.adf" in the current directory and writes a file named "flash.mp3" to the current directory as its output. You can also provide an explicit output filename as the third parameter.

adf2mp3.cpp:

// Standard C++ includes:
#include <algorithm>
#include <fstream>
#include <iostream>
#include <stdexcept>
#include <string>
#include <vector>

// Standard C / POSIX includes:
#include <cerrno>
#include <sys/types.h>
#include <sys/stat.h>

namespace adf2mp3
{

using ubyte = unsigned char;
using ulong = unsigned long;

//
// The GTA Vice City ADF files are MP3 files that
// had each byte XORed with this magic constant.
// 34 is 22 in hexadecimal and 42 in octal...
//
// Not sure who figured this out, but I got this
// info from the Xentax File Format Wiki.
//
constexpr ubyte GtaMagic = 34;

//
// Process the input file in chunks of this size in bytes.
// The chunk buffer is allocated dynamically (it is a std::vector)
// so this can be a fairly large value.
//
constexpr ulong ChunkSize = 8192;

// ========================================================

struct FileStats
{
    ulong fileLength;   // File length in bytes.
    bool  isDirectory;  // True if the path provided pointed to a directory.
    bool  isNormalFile; // True is the path provided pointed to a file.
};

FileStats fileStatsForPath(const std::string & pathname)
{
    errno = 0; // Clear the errno global, just in case.
    struct stat statBuf;

    if (stat(pathname.c_str(), &statBuf) != 0)
    {
        throw std::runtime_error("Path '" + pathname + "': " + std::string(std::strerror(errno)));
    }

    FileStats fs;
    fs.fileLength = static_cast<ulong>(statBuf.st_size);

#if _WIN32
    fs.isDirectory  = (statBuf.st_mode & _S_IFDIR) ? true : false;
    fs.isNormalFile = (statBuf.st_mode & _S_IFREG) ? true : false;
#else // !_WIN32
    fs.isDirectory  = S_ISDIR(statBuf.st_mode) ? true : false;
    fs.isNormalFile = S_ISREG(statBuf.st_mode) ? true : false;
#endif // _WIN32

    return fs;
}

std::string removeExtension(const std::string & filename)
{
    const auto lastDot = filename.find_last_of('.');
    if (lastDot == std::string::npos)
    {
        return filename;
    }
    return filename.substr(0, lastDot);
}

void printHelpText(const char * programName)
{
    std::cout << "\n";
    std::cout << "Usage:\n";
    std::cout << "$ " << programName << " <input_file> [output_file]\n";
    std::cout << "  Runs the tool normally. If the output filename is not provided\n" <<
        "  the input filename is used but the extension is replaced with '.mp3'.\n";

    std::cout << "\n";
    std::cout << "Usage:\n";
    std::cout << "$ " << programName << " --help | -h\n";
    std::cout << "  Prints this help text.\n";
    std::cout << "\n";
}

void handleCommandLine(int argc, const char * argv[],
                       std::string & inFileName, std::string & outFileName,
                       bool & printHelpOnly, bool & missingArgs)
{
    if (argc < 2)
    {
        missingArgs = true;
        return;
    }

    if (std::strcmp(argv[1], "--help") == 0 || std::strcmp(argv[1], "-h") == 0)
    {
        printHelpOnly = true;
        return;
    }

    // input_file + output_file
    if (argc >= 3)
    {
        inFileName  = argv[1];
        outFileName = argv[2];
    }
    else // Just input_file
    {
        inFileName = argv[1];
        outFileName.clear();
    }

    printHelpOnly = false;
    missingArgs   = false;
}

void processChunk(std::vector<ubyte> & chunk, ulong numBytes)
{
    for (ulong b = 0; b < numBytes; ++b)
    {
        chunk[b] ^= GtaMagic;
    }
}

void processFiles(const std::string & inFileName, const std::string & outFileName)
{
    const FileStats fileStats = fileStatsForPath(inFileName);

    if (fileStats.fileLength == 0)
    {
        throw std::runtime_error("Input file is empty!");
    }

    if (fileStats.isDirectory)
    {
        throw std::runtime_error("Input file '" + inFileName + "' is a directory!");
    }

    std::ifstream inFile(inFileName,   std::ifstream::in  | std::ifstream::binary);
    std::ofstream outFile(outFileName, std::ofstream::out | std::ofstream::binary);

    ulong bytesProcessed = 0;
    std::vector<ubyte> chunk(ChunkSize, 0);

    while (bytesProcessed != fileStats.fileLength)
    {
        const ulong bytesLeft = fileStats.fileLength - bytesProcessed;
        const ulong bytesThisIteration = std::min<ulong>(chunk.size(), bytesLeft);

        inFile.read(reinterpret_cast<char *>(chunk.data()), bytesThisIteration);
        processChunk(chunk, bytesThisIteration);
        outFile.write(reinterpret_cast<char *>(chunk.data()), bytesThisIteration);

        bytesProcessed += bytesThisIteration;
    }
}

void run(int argc, const char * argv[])
{
    bool printHelpOnly, missingArgs;
    std::string inFileName, outFileName;
    handleCommandLine(argc, argv, inFileName, outFileName, printHelpOnly, missingArgs);

    if (printHelpOnly)
    {
        printHelpText(argv[0]);
        return;
    }

    if (missingArgs)
    {
        std::cout << "Not enough arguments!\n";
        printHelpText(argv[0]);
        return;
    }

    // Replace .adf extension of source file with .mp3 and use
    // it for the output if no explicit filename was provided.
    if (outFileName.empty())
    {
        outFileName = removeExtension(inFileName) + ".mp3";
    }

    processFiles(inFileName, outFileName);
}

} // namespace adf2mp3 {}

int main(int argc, const char * argv[])
{
    try
    {
        adf2mp3::run(argc, argv);
        return 0;
    }
    catch (std::exception & e)
    {
        std::cerr << "ERROR: " << e.what() << std::endl;
        return EXIT_FAILURE;
    }
}

I also wrote a minimal makefile for it. Would appreciate comments on it:

COMPILER = c++
CPPFLAGS = -Wall -Wextra -Weffc++ -std=c++11 -O3
FILES    = adf2mp3.cpp
OUTPUT   = adf2mp3

all:
    $(COMPILER) $(CPPFLAGS) $(FILES) -o $(OUTPUT)

clean:
    rm -f $(OUTPUT)

I should note that I'm aware about the convention of reading/writing from/to stdin & stdout when no filename(s) are provided, but I don't think I'll add this feature at the moment.

Repository link. I'm looking for second opinions mostly. Is there anything I can do to improve this? Nitpicking is also welcome.

\$\endgroup\$
  • 2
    \$\begingroup\$ NB: CPPFLAGS should be reserved for pre-processor flags, e.g. -I<path> - use CXXFLAGS for compiler phase flags. \$\endgroup\$ – Alnitak Dec 29 '14 at 16:47
  • \$\begingroup\$ Humm, thanks @Alnitak, didn't know that. \$\endgroup\$ – glampert Dec 29 '14 at 16:49
  • 1
    \$\begingroup\$ There are built-in rules for converting c++ files to .o files, and those rules use pre-defined macro names such as CPPFLAGS, CFLAGS (for C) and CXXFLAGS and LDFLAGS. \$\endgroup\$ – Alnitak Dec 29 '14 at 16:58
  • \$\begingroup\$ FWIW, the only thing in the code that looks out-of-place to me is the use of std::vector<ubyte> when a char[] parameter to the fstream read and write functions would work perfectly well and without the casts. \$\endgroup\$ – Alnitak Dec 29 '14 at 17:03
  • 1
    \$\begingroup\$ Also, in order to avoid reinventing the wheel, use one of the existing command-line parsers such as mentioned in stackoverflow.com/questions/865668/parse-command-line-arguments \$\endgroup\$ – Snowbody Mar 11 '15 at 16:06
3
+50
\$\begingroup\$

Your program is overly complicated. That's my first thought I've had while reading your code. Don't overly complicate your problem but KISS.

  • You don't need to care whether you're trying to open a directory: you just can't open a directory as a file.

  • Following, you don't need FileStats.

    • => You don't need stat.
  • You should be throwing a system_error ( : public runtime_error ) which will provide a modern way to retrieve error messages (can be used in a cross-platform manner, too).

  • You don't need a namespace.

  • In printHelpText consider that each cout is a different call. You can chain them in just one thanks to string literal chaining (done by the preprocessor) and cascading calls:

    std::cout << "\n"
                 "Usage:\n"
                 "$ " << programName << " <input_file> [output_file]\n"
                 "Runs the tool normally. If the output filename is not provided\n" 
                 "the input filename is used but the extension is replaced with '.mp3'.\n"
                 "\n";
        // ...
    
  • In processFiles you're still complicating your life. A vector is "overkill": you just need an unique_ptr.

  • To get the file size, you can:

     std::ifstream in(filename, std::ifstream::ate | std::ifstream::binary);
     auto size = in.tellg();
     in.seekp(ios::begin);
    
  • I would avoid adf2mp3::run and, in general, run/init functions in the global namespace: that's what main is supposed to do.

Details

  • If you want to be pedantic, unsigned char may be larger than 8 bits (incredibly uncommon nowadays): you'd better use std::uint8_t.
  • I don't think a loop is needed as it should write it all in one time (unlike low-level I/O).
  • Setting errno to zero isn't really necessary (as you stated) because you're supposed to check it only if a (C) function returns a value indicating a failure and sets it accordingly.
  • I'm not sure if you can use const char *argv[] but the legal signatures of main are described here.
\$\endgroup\$
  • \$\begingroup\$ Hi @black, Thanks for your input. Your code formatting was broken, so I took the liberty of editing it. Please make sure I didn't change anything I shouldn't have. \$\endgroup\$ – glampert Mar 9 '15 at 18:50
2
\$\begingroup\$

My only objection is the overuse of std::vector() when none of the features of the vector are actually being used.

I would suggest instead making the processChunk function work directly on a char *:

void processChunk(char *p, size_t n) {
    while (n--) {
        *p++ ^= GtaMagic;
    }
}

and then having declared your std::vector (which should use the char type expected by ifstream::read, not ubyte) you only need to deference its data() element once:

std::vector<char> chunk(chunkSize, 0);
char *data = chunk.data();
inFile.read(data, bytesThisIteration);
processChunk(data, bytesThisIteration);
outFile.write(data, bytesThisIteration);
\$\endgroup\$
  • \$\begingroup\$ Is the XOR instruction affected by the signedness of char? I'm not sure. But if not, then my ubyte could very well just be replaced by a plain char. \$\endgroup\$ – glampert Dec 29 '14 at 17:28
  • 1
    \$\begingroup\$ AFAIK, the xor will not be affected by the sign - it'll just work on the raw binary representation. FWIW, the code above was replaced with a SSE 16-byte at a time loop using clang -O3 but the version using std::vector<char> only works on one byte at a time. \$\endgroup\$ – Alnitak Dec 29 '14 at 17:31
  • \$\begingroup\$ Humm, that's disappointing about the vector. I should check the assembly output more often... Not that I care about performance that much in this case, but one would expect the vector code to be inlined, producing similar results... \$\endgroup\$ – glampert Dec 29 '14 at 17:40
  • 1
    \$\begingroup\$ @glampert well, it was at least inlined into a tight four instruction loop, but the optimiser appeared unable to take the same leap into SSE instructions that it was able to with the "C style" loop. \$\endgroup\$ – Alnitak Dec 29 '14 at 17:42
  • 1
    \$\begingroup\$ In fact, only -O2 was required, and it also unrolled the loop to do 32 bytes per branch. That's a smart optimiser :) \$\endgroup\$ – Alnitak Dec 29 '14 at 17:45
1
\$\begingroup\$

Your processFiles() has several problems.

First, the name. It appears to process a single file, but the name suggests it processes multiple files. I'd change it to processFile().

Second, error checking. You have to check if the ifstream inFile and ofstream outFile were constructed successfully, that is, whether the files named inFileName and outfileName were opened successfully. One way is to check the .fail() method; another way is to set the failbit in the exception mask of each object and then handle the exception elsewhere (but then you'd have to construct it first, set the failbit, and finally open it).

Third, I don't think there's any benefit in keeping track of how bytesLeft or bytesProcessed. Just ask for ChunkSize bytes each time, then assign bytesThisTime = inFile.gcount(); for use by processChunk() and outFile.write(). Then the loop condition is to check for eof() or fail() (which is needed anyway since operations on a file can fail after the file is open too).

I also don't see any need for FileStats fileStats. The only thing you use it for is to get the file length, but that's of no benefit. You're not checking the files to determine if they're valid MP3s, so a 0-byte input file should process to a 0-byte output file, not an exception. Plus, checking for emptiness does not guarantee you'll be able to open the file successfully.

\$\endgroup\$
  • 1
    \$\begingroup\$ Interesting points, thanks! Error checking is truly lacking. About processFiles, the way I see it, it does handles 2 files, the input (ADF) and output (MP3). \$\endgroup\$ – glampert Mar 9 '15 at 18:09
  • \$\begingroup\$ So you're using both definitions of "processed" at the same time: the input to a process, and the output of a process. I've never seen that usage before, and certainly never with the active form of the verb. \$\endgroup\$ – Snowbody Mar 9 '15 at 18:11
  • \$\begingroup\$ Yeah, I don't know, I guess processFile (singular) would be good too, since it is one input file, the output file being the result of the processing. I'm starting to catch your drift. \$\endgroup\$ – glampert Mar 9 '15 at 18:12
0
\$\begingroup\$

Comments on the Makefile:

I suggest adding the -g flag, which enables debugging mode.

Moreover, I don't see why use c++ over g++. On the other hand I don't see why not. See: g++ and c++ - SO


I usually create/compile the individual files separately, but in your case you seem to have only one file, thus no need for this option. To be precise, when I use this option, I then use clean to delete all object files. This has pros and cons, of course.

The drawback is that you have to compile every file again, but that is something that the computer does. The main advantage, IMHO, is that you don't put more files in the workspace, which might have already many files.

\$\endgroup\$
  • \$\begingroup\$ You mean you didn't test the code, right? It's OK. I was under the impression that c++ was more portable then g++, but that might be wrong. g++ on OSX is an alias to Clang, so it should be fine to use it. Yes, ideally I should add more rules to the makefile to compile each file separately, but since I have only a single source file, I didn't bother. Thanks for the comments. Welcome to code review, btw! \$\endgroup\$ – glampert Dec 28 '14 at 16:19
  • \$\begingroup\$ Yes I didn't. Yes I agree with the one file you mention. However, the debugging flag seems nice. You are welcome and thanks! :) \$\endgroup\$ – gsamaras Dec 28 '14 at 16:26
  • 1
    \$\begingroup\$ "Production" builds shouldn't have the -g flag, and the use of c++ vs g++ is architecture specific - there's zero useful content in this answer. \$\endgroup\$ – Alnitak Dec 29 '14 at 16:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.