18
\$\begingroup\$

I've done a lot of reworking to this main file recently, in regards to using a new speech recognition engine and integrating the last reviews suggestions. My question before is going to be phrased very similarly to how it was previously.

How Khronos Works

The process starts by recording a .wav file with LibSndFile and PortAudio. PortAudio finds the default input device, and starts recording audio. Once finished, PortAudio passes along the raw PCM data to LibSndFile, which stores it in a .wav file.

This .wav file is passed to pocketsphinx for speech processing, which then returns the spoken text. Khronos takes this text and responds with its own speech synthesis software.

Here's what I would like reviewed:

  • Readability: Is everything easily understood?

  • Bottlenecks: Are there places to improve speed?

  • Memory management: In general I avoid manually managing memory, since it's easy to screw up and better for the machine to handle it. Do I go too far with that concept here?

  • Portability: Am I constraining this in any way?

There are parts of code that I haven't included (method calls, structure definitions, etc.). Posting them here would make the question larger, and I would like the reviews to be more specific to the given code.


Code:

/**
 * @file main.c
 * @brief Khronos client
 * @version 0.4
 */
#include <ctype.h>
#include <errno.h>
#include <fcntl.h>
#include <limits.h>
#include <pocketsphinx.h>
#include <portaudio.h>
#include <sphinxbase/err.h>
#include <stdlib.h>
#include <stdbool.h>
#include <time.h>
#include <tritium/tritium.h>
#include <unistd.h>

#include "audio.h"
#include "command.h"
#include "color.h"
#include "util.h"

cst_voice *register_cmu_us_rms(const char *str);

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

static const char* recognizeFromFile(ps_decoder_t *ps, const char* fileName)
{
    int16 adbuf[2048];
    FILE *file = NULL;
    const char *hyp = NULL;
    size_t k = 0;

    if ((file = fopen(fileName, "rb")) == NULL)
    {
        fprintf(stderr, "Failed to open file '%s' for reading", fileName);
    }

    // verify .wav file?  I trust libsndfile to make a valid one
    ps_start_utt(ps);
    bool uttStarted = false;

    while ((k = fread(adbuf, sizeof(int16), 2048, file)) > 0)
    {
        ps_process_raw(ps, adbuf, k, false, false);
        bool inSpeech = ps_get_in_speech(ps);
        if (inSpeech && !uttStarted) uttStarted = true;
        if (!inSpeech && uttStarted)
        {
            ps_end_utt(ps);
            hyp = ps_get_hyp(ps, NULL);
            ps_start_utt(ps);
            uttStarted = false;
        }
    }
    ps_end_utt(ps);

    if (uttStarted)
    {
        hyp = ps_get_hyp(ps, NULL);
    }

    fclose(file);
    return hyp;
}


int runKhronos(PaStream *stream, AudioData *data, AudioSnippet *sampleBlock, ps_decoder_t *ps)
{
    int err = 0;
    bool sampleComplete = false;
    int fd = createSafeFileDescriptor(getTmpDir());
    const char *fileName = getPathFromDescriptor(fd);
    if ((err = processStream(stream, data, sampleBlock, fd, &sampleComplete)))
    {
        fprintf(stderr, "Error recording FLAC file: %d\n", err);
        return err;
    }
    else if (sampleComplete)
    {
        const char *text = recognizeFromFile(ps, fileName);
        if (text)
        {
            fprintf(stdout, "Recognized text: %s\n", text);
        }
        else puts(RED_TEXT("No text recognized."));
        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;
                }
            }
            // there was some text, but a response was unknown
            if (!said) say("I could not understand what you said.");
        }
        sampleComplete = false;
    }
    close(fd);
    unlink(fileName);
    free((void*) fileName);
    return err;
}

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

int main(int argc, char **argv)
{
    int err = 0;
    srand ((unsigned) time(NULL));

    // turn off pocketsphinx output
    err_set_logfp(NULL);
    err_set_debug_level(0);

    // handle command line arguments
    if (argc > 1)
    {
        if ((err = parseArgs(argc, argv)))
        {
            fprintf(stderr, "Error parsing command line arguments.\n");
            return err;
        }
    }
    // initialize pocketsphinx stuff
    cmd_ln_t *config = cmd_ln_init(NULL, ps_args(), true,
                                   "-hmm", MODELDIR "/en-us/en-us",
                                   "-lm", MODELDIR "/en-us/en-us.lm.bin",
                                   "-dict", MODELDIR "/en-us/cmudict-en-us.dict",
                                   NULL);
    if (!config)
    {
        fprintf(stderr, "Failed to create config object, see log for details\n");
        return -1;
    }
    ps_decoder_t *ps = ps_init(config);
    if (!ps)
    {
        fprintf(stderr, "Failed to create recognizer, please update CMU Sphinx software\n");
        return -1;
    }

    // initialize stuff for PortAudio
    AudioData *data = allocAudioData();
    AudioSnippet *sampleBlock = &((AudioSnippet)
                                  {
                                      .size = 0,
                                      .snippet = NULL
                                  });

    PaStream *stream = NULL;
    err = init(&stream, data, sampleBlock);

    // main program loop
    while (!err)
    {
        err = runKhronos(stream, data, sampleBlock, ps);
    }

    //cleanup
    freeAudioData(&data);
    free(sampleBlock->snippet);
    free(stream);
    ps_free(ps);
    cmd_ln_free_r(config);
    Pa_Terminate();
    puts(Pa_GetErrorText(err));
    return err;
}

Test Run*:

There is more output visible right now just so the end user can see things are happening internally; hopefully this is reduced in the future. You can see I startup the program, say "test" and wait for the program to process and respond, then say "quit" where the program then exits. Very basic since few commands are implemented, but it shows how the program works.

*You can find the code hosted on Github if you would like to try out the code for yourself

$ ./Khronos
Listening: 0
Listening: 1
Listening: 2
Listening: 3
Listening: 4
Listening: 5
Listening: 6
Listening: 7
Listening: 8
Listening: 9
Wrote to file!!!!
Recognized text: test
Listening: 0
Listening: 1
Listening: 2
Listening: 3
Listening: 4
Listening: 5
Listening: 6
Listening: 7
Listening: 8
Listening: 9
Wrote to file!!!!
Recognized text: quit
$
\$\endgroup\$
  • \$\begingroup\$ Can we get some examples of the code being run? Just out of curiosity? \$\endgroup\$ – 13aal May 19 '16 at 22:41
  • \$\begingroup\$ @13aal Absolutely, give me a second to edit them in real quick. \$\endgroup\$ – syb0rg May 19 '16 at 22:42
  • \$\begingroup\$ Awesome, im excited to see these \$\endgroup\$ – 13aal May 19 '16 at 22:43
  • 1
    \$\begingroup\$ @13aal Edited in an example, feel free to visit my Github where you can download Khronos and try it out yourself (assuming it compiles, still working out a few kinks) \$\endgroup\$ – syb0rg May 19 '16 at 22:50
  • \$\begingroup\$ This is awesome, good job \$\endgroup\$ – 13aal May 19 '16 at 22:59
16
+50
\$\begingroup\$

Readability

Inconsistencies in coding style:

  • Decide where You put an asterisk. Now You have things like this: ps_decoder_t *ps, const char* fileName.
  • A few unreasonable var-names. What's size_t k? cst_voice* v? const Command *p?
  • Mixing cases. Now You can't really recognize if it's a function or variable name, just by bluntly looking at the code. That's not entirely Your fault, but still it looks a bit messy. EDIT: to clarify myself, APIs You're using apparently has a different coding style. So, You have API calls like register_cmu_us_rms(), tritium_textToSpeech() but recognizeFromFile(). Sadly, don't have any good solutions for that :( Also, adbuf, but fileName, sampleComplete, etc. I would use different casing for function names and variables. But that's for You to decide or to pick some already existing coding style :)

Why would You put Your variable definitions all over place? It's messy. EDIT: I would debate that putting all the variable definitions on top of Your function body makes it waaaay more easier to read. Having in mind that functions must be short enough and with reasonable nesting. But yeah, that's debatable.

Multiple exit points. This is one of the readability issues I personally hate the most. 4 return statements in Your main()? 3 exit() in Your parseArgs? Really? Also, why wouldn't You reuse Your err variable in Your main() function, if You still insists having multiple exit points?

EDIT: More on multiple exit points --- one of the ways would be something like this:

static const int recognizeFromFile(ps_decoder_t *ps,
    const char* fileName, char **outputBuffer)
{
    FILE *file = NULL;
    int retVal = 0;
    int tmpErr = 0;

    /* Sanitize inputs. */
    if (ps == NULL)
    {
        KHRONOS_LOGGER("Invalid (NULL) ps_decoder handle provided.");
        retVal = KHRONOS_ERR_INVALID;
        goto done;
    }

    if (fileName == NULL)
    {
        KHRONOS_LOGGER("Invalid (NULL) fileName provided.");
        retVal = KHRONOS_ERR_INVALID;
        goto done;
    }

    if (outputBuffer == NULL)
    {
        KHRONOS_LOGGER("Invalid (NULL) outputBuffer provided.");
        retVal = KHRONOS_ERR_INVALID;
        goto done;
    }

    /* Do the magic. */
    file = fopen(fileName, "rb");
    if (file == NULL)
    {
        KHRONOS_LOGGER("Failed to open file %s for reading.",
            fileName);
        retVal = KHRONOS_ERR_IO;
        goto done;
    }

    tmpErr = ps_start_utt(ps);
    if (tmpErr != 0)
    {
        KGRONOS_LOGGER("ps_start_utt failed with status: %d.",
            tmpErr);
        retVal = KHRONOS_ERR_API;
        goto done;
    }

    /* All the magic succeeded */
    /* So, fill the output buffer, or just set some return values */
    ...
    ...
done:
    /* Clean the stuff up. */
    if (file != NULL)
    {
        fclose(file)
    }

    return retVal;
}

Single exit point and single centralized space for all Your function's clean-up. Also added some more logging. END-of-EDIT

Memory management

I can't see const char *text and const char* out = p->fn(); being freed anywhere. Not sure how those functions work, but it looks like a blunt malloc beneath. Isn't it?

Functionality

Despite failing to open the file (line #41) You still insist trying to read it.

Why would You check for same recognizeFromFile() twice (line #87, #92)? As far as I recall, puts() does not change string itself, does it?

This part:

while ((k = fread(adbuf, sizeof(int16), 2048, file)) > 0)
{
    ps_process_raw(ps, adbuf, k, false, false);
    bool inSpeech = ps_get_in_speech(ps);
    if (inSpeech && !uttStarted) uttStarted = true;
    if (!inSpeech && uttStarted)
    {
        ps_end_utt(ps);
        hyp = ps_get_hyp(ps, NULL);
        ps_start_utt(ps);
        uttStarted = false;
    }
}

Looks fishy. Why not just do something like this:

while ((k = fread(adbuf, sizeof(int16), 2048, file)) > 0)
{
    ps_process_raw(ps, adbuf, k, false, false);
    bool inSpeech = ps_get_in_speech(ps);

    uttStarted = inSpeech && !uttStarted;
    if (!inSpeech && uttStarted)
    {
        ps_end_utt(ps);
        hyp = ps_get_hyp(ps, NULL);
        ps_start_utt(ps);
        uttStarted = false;
    }
}

Also, I would really comment parts like this. Doesn't exactly trivial to get, what's actually happening beneath.

Generic

On line #109 You have sampleComplete = false; despite the fact You never use this value afterwards.

Why wouldn't You use proper argument parser (i.e. usual getopt) instead of writing Your own cumbersome parser?

You're not sanitizing any of Your inputs. I imagine this piece of code being used as a library. User of that library might pull his/her hairs off trying to debug his/her code and pin-point the location of some NULL pointer being passed on.

ctype.h include is unused in Your code, as far as I see.

Why would You cast pointer while freeing the memory?

\$\endgroup\$
  • 1
    \$\begingroup\$ Lots of good points, thank you. Could you give an example of the mixed cases making it hard to recognize if it's a function or variable name? Also, declaring variables where they are used can make a program more efficient. \$\endgroup\$ – syb0rg May 12 '16 at 20:15
  • 4
    \$\begingroup\$ This is totally off-topic, just a thing I noticed about your English. The only pronoun that's capitalized in English is "I". So, "you" and "your" should be lower-case. I'm guessing your native tongue is German? :) \$\endgroup\$ – Blorgbeard May 17 '16 at 4:25
  • 1
    \$\begingroup\$ @Blorgbeard, nope. In my native language it's not capitalized neither :} That's my... personal rule. In whatever language. \$\endgroup\$ – Kamiccolo May 17 '16 at 15:36
  • 1
    \$\begingroup\$ What is your suggestion to avoid multiple returns? I think it is often more readable to have multiple exit points instead of checking a dozen times some flag to control the program flow. \$\endgroup\$ – exilit May 20 '16 at 11:39
  • 1
    \$\begingroup\$ @exilit, personally I follow a bit modified Linux Kernel Coding Style --- Chapter 7: Centralized exiting of function. Basically, just checking return values of used functions and in case of error - reporting, setting output variable and jumping to clean-up. \$\endgroup\$ – Kamiccolo May 20 '16 at 16:48
5
\$\begingroup\$

This isn't a full review, just a couple of points:

Help Text

You have a method printHelp, not included in your code. I'm assuming that it returns a formatted help text that tells the user how to run the program. To get this output currently you need to know the command line arguments for help. Rather than having the user guess until they get it right, why not have this as the default behaviour if the args can't be parsed correctly?

Stdout/Stderr

You've got several methods that refer to stderr and stdout (puts, fprintf). This may be OK if you only ever intend to run the application from the command line. If however, there is a chance that you might want it in a library, or to put a GUI in front of it then decoupling it now may be worthwhile. Putting a wrapper around puts/fprintf something like handleError or sendMessageToUser would minimally change the code, allow the output method to be easily changed in one position.

\$\endgroup\$

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.