HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Raw PCM to FLAC conversion using QtGstreamer in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewcppstackoverflowc++audioqt
conversionflacrawusingqtgstreamerpcm

Problem

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 simu

Solution

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

` 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 
#include 
#include 
#include 
#include 

#include 

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(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.

Code Snippets

using namespace QGst;
using namespace QGst::Utils;
#include <QByteArray>
#include <QMutex>
#include <QObject>
#include <QReadWriteLock>
#include <QThread>

#include <QGst/Element>

class QIODevice;

namespace QGst {
    namespace Utils {
        class ApplicationSource;
        class ApplicationSink;
    }
}
class Converter
#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)
bool convert::inited = false;
QMutex convert::initLock;

Context

StackExchange Code Review Q#149902, answer score: 2

Revisions (0)

No revisions yet.