9
\$\begingroup\$

This is part of my kynnaugh-cc TeamSpeak 3 plugin implementing speech recognition for deaf/hearing-impaired users. One of the self-contained classes in my code has the job of converting incoming PCM samples from TeamSpeak into FLAC format for transmission to the Google Cloud Speech API. The Cloud Speech API only accepts input in WAV or FLAC format, so obviously I prefer FLAC due to its superior compression (these files will be streamed in real time to Google's servers from an end-user's computer, so bandwidth is a concern).

The general mode of operation of this code from a caller's perspective is:

  • Call the constructor (from any thread).
  • When you have some samples of raw PCM (which, by contract, must be Signed 16-bit Little Endian, interleaved, at 48000 Hz sample rate, and either 1 or 2 channels), call convert::convertRawToFlac().
  • convertRawToFlac() will block until it completes the conversion, and then return.
  • The retval member of your convert instance will have a QByteArray full of the FLAC-encoded bytes for you to pick up and carry on to the next part of the program.
  • If you later on get more samples of PCM, just call the convertRawToFlac() function again; you don't have to create a new instance of convert.
  • The QtGstreamer and Qt stuff I use inside this class has "thread affinity" (they care which thread you call them on), but I actively expect convertRawToFlac() to be called from any arbitrary thread, not necessarily the same thread every time. For this reason, I spawn and re-use the same thread each time for all of the GStreamer stuff, which prevents me from having to teardown the GstPipeline with each call. It stays bound to the same mainloop thread.
  • At a higher level architecturally (with code not visible directly in this question), each user (a real-life person with a microphone in a TeamSpeak 3 VOIP channel and something to say) will get their own convert instance. This allows us to use more than one CPU core to simultaneously convert the PCM samples of multiple users talking over one another. It also improves the responsiveness/latency.

The input PCM samples have to be conditionally downmixed (if 2 channels then downmix to 1, otherwise if input is mono, do nothing) and resampled from 48000 Hz to 16000 Hz. Then, once in the right PCM format, encoded into FLAC.

This code is "self-contained" and could possibly be reused for Google Cloud Speech API purposes (due to the particular FLAC format needed by it -- it has to be mono, and they recommend 16 kHz) outside of kynnaugh-cc. I have licensed it disjunctively under CC-BY-SA or Apache 2.0 for the purposes of this question.

If you wish to compile and link the code, you will need to build qt-gstreamer against any Qt 5.x release coupled with any GStreamer 1.x release, and have the gst-plugins-base and gst-plugins-good packages installed (the latter is for flacenc - the FLAC encoder). The code is cross-platform and is known to work on macOS Sierra and Windows 10 with the 64-bit Intel architecture.

What I'm Looking For

  • Suggestions to eliminate bugs or memory leaks.
  • C++ "correctness" or "convention" recommendations (I've the most experience writing Java and C#; C++ is somewhat new to me.)
  • Different ways of accomplishing the same thing while being architecturally simple and retaining the same dependencies (Yes, I am quite closely married to the GStreamer framework due to its extreme flexibility and the potential of using a different codec than FLAC in the future if the Google Cloud Speech API changes.)
  • Any specific style suggestions.
  • Areas of the code that you feel really need a comment to explain what's going on (or why). I've kept comments minimal for now, but if another programmer feels that something desperately needs a comment, I will add it.
  • Anything I explicitly specify in the code that just sets a value to be equal to its default anyway, assuming there is a reasonable assumption that the default is not going to change in the future. Or, even better, if any of the non-debugging statements are completely superfluous and unnecessary, point them out so I can delete them and make the code shorter.

By The Way

  • I'll be asking more questions with the other self-contained modules of kynnaugh-cc if this question is successful.
  • Comments generally about kynnaugh-cc or other parts of the codebase are welcome, but don't post an answer entirely about code that's not pasted into this question, please. Save that for when I ask another question about this :)
  • There's a unit test in the github repository linked above that actually plays back a .raw audio file after converting it to FLAC (and then decoding it to PCM again), through your default soundcard, that you can compile and run.

convert.h:

#ifndef CONVERT_H
#define CONVERT_H

#include <QtCore>
#include <QGst/Pipeline>
#include <QGst/Bin>
#include <QGst/Element>
#include <QGst/Utils/ApplicationSink>
#include <QGst/Utils/ApplicationSource>
#include <QGst/Init>

using namespace QGst;
using namespace QGst::Utils;

class convert : public QObject
{
    Q_OBJECT
public:
    explicit convert(QObject *parent = 0);
    ~convert();
    void convertRawToFlac(QIODevice *dat, qint32 channes);
    QByteArray retval;
public Q_SLOTS:
    void threadStart();
private:
    static QMutex initLock;
    static bool inited;
    void setupPipeline();
    QThread *callingThread;
    QThread thread;
    QReadWriteLock lock;
    QIODevice *data;
    qint32 channels;
    ElementPtr pipeline;
    ElementPtr flacenc;
    ApplicationSource *appsrc;
    ElementPtr audioconvert;
    ApplicationSink *appsink;
};

#endif // CONVERT_H

convert.cpp:

#include "convert.h"

#include <QGst/ElementFactory>
#include <QGlib/Error>
#include <QGlib/Connect>
#include <QGst/Bus>
#include <QGst/Parse>
#include <QGst/Message>
#include <QGst/Buffer>
#include <glib.h>
#include <gst/gstbuffer.h>

#define SP qDebug() << "convert::setupPipeline()" << i++;
#define TS qDebug() << "convert::threadStart()" << i++;
#define CRTF qDebug() << "convert::convertRawToFlac()" << i++;

bool convert::inited = false;
QMutex convert::initLock;

convert::convert(QObject *parent)
    : QObject(parent), thread(this), lock(QReadWriteLock::Recursive), data(NULL), channels(0)
{
    initLock.lock();
    if(!inited)
    {
        QGst::init();
        qDebug() << "GStreamer has been inited in convert constructor.";
        inited = true;
    }
    initLock.unlock();

    connect(&thread, &QThread::started, this, &convert::threadStart, Qt::DirectConnection);
}

//Assume `data` contains Signed 16-bit LE PCM samples at 48 kHz
void convert::convertRawToFlac(QIODevice *dat, qint32 channes)
{
    int i = 0;
    QWriteLocker locker(&lock);
    this->callingThread = QThread::currentThread();
    this->data = dat;
    this->channels = channes;
    CRTF
    thread.start(QThread::NormalPriority);
    this->moveToThread(&thread);
    CRTF
    QThread::yieldCurrentThread();
    CRTF
    thread.wait(); //TODO: Error handling if wait() returns false due to timeout
    CRTF
}

void convert::setupPipeline()
{
    int i = 0;
    if(pipeline.isNull())
    {
        SP
        PipelinePtr pipe;
        QString pipetext = QString("appsrc name=\"a\""
                                   " caps=\"audio/x-raw,format=S16LE,rate=48000,layout=interleaved,channels=%1\" ! "
                                   "audioconvert name=\"b\" ! "
                                   "audioresample name=\"c\" ! "
                                   "audio/x-raw,format=S16LE,rate=16000,channels=1,layout=interleaved ! "
                                   "flacenc name=\"d\" !"
                                   "appsink name=\"e\" caps=\"audio/x-flac\" sync=true")
                           .arg(this->channels);
        qDebug() << "Pipeline text: " << pipetext;
        pipeline = Parse::launch(pipetext);
        SP
        pipe = pipeline.dynamicCast<Pipeline>();
        SP
        appsrc = new ApplicationSource();
        appsrc->setElement(pipe->getElementByName("a"));
        appsrc->enableBlock(true);
        appsrc->setLive(false);
        appsrc->setStreamType(QGst::AppStreamTypeStream);
        appsrc->setCaps(Caps::fromString(QString("audio/x-raw,format=S16LE,rate=48000,layout=interleaved,channels=%1").arg(this->channels)));
        appsrc->setMaxBytes(1073741824);
        SP
        audioconvert = pipe->getElementByName("b");

        flacenc = pipe->getElementByName("d");

        appsink = new ApplicationSink();
        appsink->setElement(pipe->getElementByName("e"));
        appsink->enableDrop(false);
        appsink->setCaps(Caps::fromString("audio/x-flac"));
        appsink->setMaxBuffers(1073741824);
        SP
    }
}

convert::~convert()
{
    if(!this->pipeline.isNull())
    {
        this->pipeline->setState(QGst::StateNull);
    }
}

void convert::threadStart()
{
    int i = 0;
    TS
    setupPipeline();
    TS
    if(!this->data->isOpen())
    {
        this->data->open(QIODevice::ReadOnly);
    }
    QByteArray sourcedata = this->data->readAll();
    qDebug() << "sourcedata has length " << sourcedata.size();
    BufferPtr qbuf = Buffer::create(sourcedata.size());
    GstBuffer *buf = static_cast<GstBuffer*>(qbuf);
    SamplePtr sam;
    gst_buffer_fill(buf, 0, static_cast<gconstpointer>(sourcedata.data()), sourcedata.size());
    qDebug() << "GstBuffer has size " << qbuf->size();

    qDebug() << "Setting state to READY";
    pipeline->setState(QGst::StateReady);
    qDebug() << "READY set; setting state to PAUSED";
    pipeline->setState(QGst::StatePaused);
    qDebug() << appsrc->pushBuffer(qbuf);
    qDebug() << "pushed buffer; setting state to PLAYING";
    pipeline->setState(QGst::StatePlaying);
    qDebug() << "Set state to PLAYING";
    appsrc->endOfStream();
    qDebug() << "Sending EOS";
    TS
    do
    {
        sam = appsink->pullSample();
        if(!sam.isNull())
        {
            BufferPtr bp = sam->buffer();
            quint32 sz = bp->size();
            if(sz > 0)
            {
                gpointer raw = malloc(sz);
                if(raw != NULL)
                {
                    TS
                    bp->extract(0, raw, sz);
                    this->retval.append(static_cast<const char*>(raw), sz);
                    free(raw);
                }
            }
        }
    } while(!sam.isNull() && !appsink->isEos());
    TS
    this->thread.quit();
    this->pipeline->setState(QGst::StateReady);
    qDebug() << "FLAC encoding pipeline state set back to ready because we're done";
    this->moveToThread(this->callingThread);
}   
\$\endgroup\$
2
\$\begingroup\$

Global namespace pollution

Don't do this in a header:

using namespace QGst;
using namespace QGst::Utils;

This will add many, many names to the global namespace of every source file that includes this header. It's much less harmful to import vast numbers of names in your implementation file (though I still recommend using just the names you need, in the smallest reasonable scope).


Includes

<QtCore> is a big include, and will hit your compilation speed (again, for every translation unit that needs this header). Don't drag all of that in; include just what you need, and forward-declare what you can:

#include <QByteArray>
#include <QMutex>
#include <QObject>
#include <QReadWriteLock>
#include <QThread>

#include <QGst/Element>

class QIODevice;

namespace QGst {
    namespace Utils {
        class ApplicationSource;
        class ApplicationSink;
    }
}

Naming

Qt naming convention is to use PascalCase nouns for class names:

class Converter

That's still very broad; consider alternatives such as Downsampler or SpeechCompressor.

Other identifiers I'd suggest changing:

  • retval -> flacData
  • dat -> pcmData
  • channes -> channels

The debugging macros can be written with the do while(0) idiom so that they can be used like statements:

#define SP() do { qDebug() << "convert::setupPipeline()" << i++; } while (0)
#define TS() do { qDebug() << "convert::threadStart()" << i++; } while (0)
#define CRTF() do { qDebug() << "convert::convertRawToFlac()" << i++; } while (0)

That said, I don't really like them at all - macros are always a bit suspect, and these touch a variable called i which is extremely likely to collide with the code. Also, I don't think this code needs to be quite so chatty.


Initialization

We don't need these:

bool convert::inited = false;
QMutex convert::initLock;

Instead, we can make the language work for us with an immediately-invoked initializer expression for a static local:

convert::convert(QObject *parent)
    : QObject(parent),
      thread(this),
      lock(QReadWriteLock::Recursive),
      data(NULL),
      channels(0)
{
    static const bool inited = []{
        QGst::init();
        qDebug() << "GStreamer has been inited in convert constructor.";
        return true;
    }();
    (void)inited;               // suppress "unused variable" warning

When you do need a lock, prefer to use a scoped lock-guard, rather than manual lock() and unlock(). It's much easier to ensure that locks and unlocks are balanced that way, without having to examine all execution paths.


Signal connection

I don't think you want a direct connection here:

connect(&thread, &QThread::started, this, &convert::threadStart, Qt::DirectConnection);

It defeats the point of having a thread if we execute this code directly in the owning thread. We should let that just default, so that threadStart() is executed in convert's thread:

connect(&thread, &QThread::started, this, &convert::threadStart);

Tricky timing

This code is suspect:

thread.start(QThread::NormalPriority);
this->moveToThread(&thread);

We want to move this object to the thread before we start the thread - remember that there's a signal emitted by thread start that's connected to this object. Yes, Qt won't deliver it until it gets back into the correct event loop, but why write code that's so hard to reason about when we can simply re-order these two lines?


Useless use of thread

Here, we do nothing while we wait for the thread we just started:

thread.start(QThread::NormalPriority);
this->moveToThread(&thread);
CRTF();
QThread::yieldCurrentThread();
CRTF();
thread.wait(); //TODO: Error handling

So there really was no point running that in its own thread, after all.


Magic numbers

    appsrc->setMaxBytes(1073741824);

What's the significance of that number? It looks like 1 << 30 but it's hard to tell at a glance. Regardless, it's surprisingly big for 48kHz 16-bit input - over three hours' worth, even in stereo. It would be better to give it a good name near the start of the file.


Memory allocation

We prefer new/new[] and delete/delete[] over malloc() in C++ code. And we prefer self-reclaiming allocations using smart pointers over either of those:

        if(sz > 0)
        {
            auto const raw = std::make_unique<char[]>(sz);
            bp->extract(0, raw.get(), sz);
            retval.append(raw.get(), sz);
        }

(you'll want to catch std::bad_alloc outside of the loop, I think).

I don't think you need a buffer for this at all, though - it should be straightforward to resize retval and extract directly to its data() buffer without copying.

\$\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.