Where it all begins: Khronos

Question

This is the main.c file for Khronos, a personal project of mine. This is basically where the whole program is setup, run and then quit. I'll post the short summery of what this file does from the README.md:

How Khronos Works

The process starts off by recording a .flac file with the help of LibSndFile and PortAudio working together. PortAudio scans for the default input device (set by your operating system), and starts the audio recording process. Once finished, PortAudio passes along the raw PCM data that was just recorded to LibSndFile, which performs the necessary instructions to store it in a .flac file.

This .flac file is then sent off to Google for speech processing. We specify what the sample rate is of the audio, and what sort of response we would like back from Google (JSON, XML, etc.). Google sends back a nicely packed JSON file for processing to extract what was said during the recording. This processing is done with my custom JSON parser that has proven to be faster when compared to the benchmarks of other JSON parsers.

Khronos then takes this processed information and responds to it in an appropriate way with it's own speech synthesis engine.

So here's what I would like reviewed:

• Readability: is everything easily understandable to you? Can you flow through the code with ease?

• Bottlenecks: are there places where I could improve speed?

• Memory management: in general I try to avoid having to manage memory manually, as it can be easy to screw up and it's better for the machine to handle most of it in my opinion. Do I go too far with that concept here?

• Portability: this code is being designed with the concept of being portable and runnable on a wide range of devices (currently it is being developed and tested using an older version Raspberry Pi). Am I constraining that in any way here?

Understandably there are parts to this code that I haven't included (some method calls, structure definitions, etc.). Posting all that here would make this question quite large and I would like the reviews to be more specific to certain pieces of code than very general and spread out over multiple files.

---

/**
 * @file main.c
 * @brief Khronos client
 * @version 0.3
 */
#include <assert.h>
#include <time.h>
#include <ctype.h>
#include <tritium/tritium.h>
#include <errno.h>

#include "audio.h"
#include "internet.h"
#include "command.h"
#include "color.h"
#include "parcel.h"

cst_voice *register_cmu_us_rms(const char *str);

const char* getTmpDir(void)
{
    char *tmpdir = NULL;
    if ((tmpdir = getenv("TEMP"))) return tmpdir;
    else if ((tmpdir = getenv("TMP"))) return tmpdir;
    else if ((tmpdir = getenv("TMPDIR"))) return tmpdir;
    else return "/tmp/";
}

static void say(const char *str)
{
    cst_voice* v = register_cmu_us_rms(NULL);
    tritium_textToSpeech(str, v, "play");
}

int main(int argc, char *argv[])
{
    srand ((unsigned) time(NULL));
    AudioData *data = allocAudioData();
    AudioSnippet *sampleBlock = &((AudioSnippet)
                                  {
                                      .size = 0,
                                      .snippet = NULL
                                  });
    ServerResponse *resp = NULL;
    char *flacFileBuf;
    size_t flacFileLen;
    double confidence = 0.0;
    int fd = -1;

    while (argc--)
    {
        if (streq("--help", argv[argc]) || streq("-h", argv[argc]) || streq("help", argv[argc]))
        {
            puts(printHelp());
            return 0;
        }
        if (streq("--version", argv[argc]) || streq("-v", argv[argc]) || streq("version", argv[argc]))
        {
            puts(printVersionInfo());
            return 0;
        }
        if (streq("--say", argv[argc]) || streq("-s", argv[argc]) || streq("say", argv[argc]))
        {
            say(argv[argc + 1]);
            return 0;
        }
    }

    if (testConnection())
    {
        fprintf(stderr, "Speech recognition offline.\n");
        say("Speech recognition offline.");
        return -1;
    }

    PaStream *stream = NULL;
    PaError err = init(&stream, data, sampleBlock);
    const char *fileRoot = getTmpDir();
    bool flag = false;

    while (!err)
    {
        // Creates temporary file safely
        char flacFile[FILENAME_MAX] = "";
        snprintf(flacFile, FILENAME_MAX, "%sXXXXXX.flac", fileRoot);
        fd = mkstemps(flacFile, 5); // the 5 is for the length of the suffix ".flac"

        if ((err = processStream(stream, data, sampleBlock, fd, &flag)))
        {
            fprintf(stderr, "Error recording FLAC file: %d\n", err);
            break;
        }
        else if (flag)
        {
            if ((err = getFileContents(flacFile, (void**)&flacFileBuf, &flacFileLen)))
            {
                fprintf(stdout, "Error reading FLAC file: %s\n", strerror(errno));
                break;
            }
            resp = sendAudioData(flacFileBuf, (int)flacFileLen, "en-US", data->sampleRate);
            if (!resp)
            {
                fprintf(stderr, "Error sending audio.");
                break;
            }

            const char *text = parcel_getItemFromJSON(resp->data, "transcript");
            if (text) confidence = strtod(parcel_getItemFromJSON(resp->data, "confidence") ?: "0", NULL) * 100;
            fprintf(stdout, "Recognized text: %s\n", text ?: RED_TEXT("No text recognized."));
            fprintf(stdout, "Confidence: %g%%\n", confidence);
            if (text)
            {
                bool said = false;
                for (size_t i = 0; i < ARRAY_SIZE(commands); ++i)
                {
                    const Command *p = commands + i;
                    if (strstr(text, p->cmd))
                    {
                        const char* out = p->fn();
                        fprintf(stdout, "%s\n", out);
                        say(out);
                        said = true;
                    }
                }
                if (!said) say("I could not understand what you said."); // there was some text, but a response was unknown
            }
            remove(flacFile);
            flag = false;
        }
        close(fd);
    }
    freeAudioData(&data);
    freeResponse(resp);
    free(sampleBlock->snippet);
    free(stream);
    Pa_Terminate();
    puts(Pa_GetErrorText(err));
    return err;
}

Answer 1

Without more of the code, it's not really going to be possible to address memory management or bottlenecks. So instead, I'll look instead at readability and portability.

Use the appropriate #includes

This program fragment requires two headers, which should be included but are not:

#include <stdlib.h>  // for getenv(), srand(), fprintf(), etc.
#include <stdbool.h>  // for "true" and "false" and "bool"

Use only standard calls for portable code

The mkstemps routine is common to many Unix-like environments but is not standard -- not even POSIX. The mktemp routine is part of the C standard but has security problems and should not be used. mkstemp is a POSIX standard, so it may be your best bet.

Add more error checking

The calls to snprintf and strtod can fail. In both cases, the code should check for errors and handle them as may be appropriate to your situation.

Don't abuse the ternary operator

The code currently has this line

if (text) confidence = strtod(parcel_getItemFromJSON(resp->data, "confidence") ?: "0", NULL) * 100;

First, that would be a lot more readable if it didn't pack everything on a single line. Second, omitting the second operand is a GNU extension and not portable.

Break things up into functions

The main routine has quite a lot going on inside of it that would benefit from being broken out into separate routines. Your English language description is quite clear and suggests a way that this could be broken into separate functions.

Use unlink instead of remove

Both are defined in the standard, but remove will remove a directory just as happily as it will remove a file. If you wish only to remove a file, it's safer to use unlink.

Don't erase an open file

The use of remove as mentioned above, precedes closing the file. That is not a good idea. Better is to close the file first and then delete it. I've been asked about this, so I'm providing more detail. It's counterintuitive, but many Linux filesystems allow deleting the file before closing the file. However, we can't count on this behavior (e.g. the file could be mounted via NFS) and the possible problems are easily avoided by rearranging the code. See this answer for more details on the general topic of "things that might happen to an open file."