Merge lp:~phablet-team/qtubuntu-camera/fix-trust-prompt-bugs into lp:qtubuntu-camera/stable

[Jim Hodapp]

Refactor media recorder code to:
- ensure proper cleanups
- prevent double start/stop of recordings
- allow for soundless recordings
- do not start recording before microphone access is allowed/forbidden by user

Refactor media recorder code to:
- ensure proper cleanups
- prevent double start/stop of recordings
- allow for soundless recordings
- do not start recording before microphone access is allowed/forbidden by user

Fixes:
Bug #1487126: Camera/mic trust: initial video corrupt
Bug #1487131: Camera/mic trust: mic denial in camera-app freezes camera
Bug #1487159: revoking mic permission in system-settings, camera appears to record but video is unplayable

[lp:~phablet-team/qtubuntu-camera/fix-trust-prompt-bugs]

149. By Florian Boucault on 2015-08-21

Refactor media recorder code to:
- ensure proper cleanups
- prevent double start/stop of recordings
- allow for soundless recordings
- do not start recording before microphone access is allowed/forbidden by user

150. By Florian Boucault on 2015-08-21

Slightly clearer if/else

151. By Jim Hodapp on 2015-08-21

Fix package version and libqtubuntu-media-signals-dev dependency version.

[Jim Hodapp (jhodapp)]

A few comments/questions inline.

review: Needs Fixing (code)

[lp:~phablet-team/qtubuntu-camera/fix-trust-prompt-bugs]

152. By Florian Boucault on 2015-08-21

Small status fix

153. By Florian Boucault on 2015-08-21

Reset m_audioCaptureAvailable in cleanup.

[lp:~phablet-team/qtubuntu-camera/fix-trust-prompt-bugs]

154. By Florian Boucault on 2015-08-21

In the case of a timeout error signalled by pulseaudio when trying to use the microphone, be smart and prevent any further recording.

[Jim Hodapp (jhodapp)]

review: Approve (code)

=== modified file 'debian/changelog'
--- debian/changelog	2015-07-10 18:12:33 +0000
+++ debian/changelog	2015-08-21 20:10:16 +0000
@@ -1,4 +1,4 @@
-qtubuntu-camera (0.3.3+15.10.20150710-0ubuntu1) wily; urgency=medium
+qtubuntu-camera (0.3.3+15.04.20150710-0ubuntu1) vivid; urgency=medium
 
   [ CI Train Bot ]
   * New rebuild forced.
@@ -9,7 +9,7 @@
 
  -- CI Train Bot <[e-mail]>  Fri, 10 Jul 2015 18:12:33 +0000
 
-qtubuntu-camera (0.3.3+15.10.20150706-0ubuntu1) wily; urgency=medium
+qtubuntu-camera (0.3.3+15.04.20150706-0ubuntu1) vivid; urgency=medium
 
   [ Alberto Aguirre ]
   * Remove dependency on platform-api. Use QScreen instead to obtain
@@ -20,7 +20,7 @@
 
  -- CI Train Bot <[e-mail]>  Mon, 06 Jul 2015 18:22:45 +0000
 
-qtubuntu-camera (0.3.3+15.10.20150629-0ubuntu1) wily; urgency=medium
+qtubuntu-camera (0.3.3+15.04.20150629-0ubuntu1) vivid; urgency=medium
 
   [ Florian Boucault ]
   * Write GPS location even if no altitude is given. (LP: #1447689)

=== modified file 'debian/control'
--- debian/control	2015-07-10 18:07:53 +0000
+++ debian/control	2015-08-21 20:10:16 +0000
@@ -8,7 +8,7 @@
                libhybris-dev (>= 0.1.0+git20131207+e452e83-0ubuntu34),
                libpulse-dev,
                libqt5opengl5-dev,
-               libqtubuntu-media-signals-dev (>=0.3+15.10.20150618.1-0ubuntu1),
+               libqtubuntu-media-signals-dev (>=0.3+15.04.20141104-0ubuntu1),
                pkg-config,
                qt5-default,
                qtmultimedia5-dev,

=== modified file 'src/aalmediarecordercontrol.cpp'
--- src/aalmediarecordercontrol.cpp	2015-01-26 16:21:39 +0000
+++ src/aalmediarecordercontrol.cpp	2015-08-21 20:10:16 +0000
@@ -25,7 +25,6 @@
 #include <QDebug>
 #include <QFile>
 #include <QFileInfo>
-#include <QThread>
 #include <QTimer>
 
 #include <hybris/camera/camera_compatibility_layer.h>
@@ -65,7 +64,7 @@
     m_currentState(QMediaRecorder::StoppedState),
     m_currentStatus(QMediaRecorder::UnloadedStatus),
     m_recordingTimer(0),
-    m_workerThread(0)
+    m_audioCaptureAvailable(false)
 {
 }
 
@@ -83,6 +82,8 @@
                 << errno << ")";
     }
     deleteRecorder();
+    m_audioCaptureThread.quit();
+    m_audioCaptureThread.wait();
 }
 
 /*!
@@ -158,76 +159,44 @@
 /*!
  * \brief Starts the main microphone reader/writer loop in AudioCapture (run)
  */
-void AalMediaRecorderControl::onStartThread()
+void AalMediaRecorderControl::startAudioCaptureThread()
 {
-    qDebug() << "Starting microphone reader/writer worker thread";
-    // Start the microphone read/write worker thread
-    m_workerThread->start();
-    Q_EMIT startWorkerThread();
+    qDebug() << "Starting microphone reader/writer thread";
+    // Start the microphone read/write thread
+    m_audioCaptureThread.start();
+    Q_EMIT audioCaptureThreadStarted();
 }
 
 /*!
  * \brief AalMediaRecorderControl::init makes sure the mediarecorder is
  * initialized
  */
-void AalMediaRecorderControl::initRecorder()
+bool AalMediaRecorderControl::initRecorder()
 {
     if (m_mediaRecorder == 0) {
         m_mediaRecorder = android_media_new_recorder();
-
-        m_audioCapture = new AudioCapture(m_mediaRecorder);
-        m_workerThread = new QThread;
-
-        if (m_audioCapture == 0) {
-            qWarning() << "Unable to create new audio capture, audio recording won't function";
-            Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "Unable to create new audio capture, audio recording won't function");
-        }
-        else
-        {
-            bool ret = false;
-
-            // Make sure that m_audioCapture is executed within the m_workerThread affinity
-            m_audioCapture->moveToThread(m_workerThread);
-
-            // Finished signal is for when the workerThread is completed. Important to connect this so that
-            // resources are cleaned up in the proper order and not leaked
-            ret = connect(m_audioCapture, SIGNAL(finished()), m_workerThread, SLOT(quit()));
-            if (!ret)
-                qWarning() << "Failed to connect quit() to the m_audioCapture finished signal";
-            ret = connect(m_audioCapture, SIGNAL(finished()), m_audioCapture, SLOT(deleteLater()));
-            if (!ret)
-                qWarning() << "Failed to connect deleteLater() to the m_audioCapture finished signal";
-            // Clean up the worker thread after we finish recording
-            ret = connect(m_workerThread, SIGNAL(finished()), m_workerThread, SLOT(deleteLater()));
-            if (!ret)
-                qWarning() << "Failed to connect deleteLater() to the m_workerThread finished signal";
-            // startWorkerThread signal comes from an Android layer callback that resides down in
-            // the AudioRecordHybris class
-            ret = connect(this, SIGNAL(startWorkerThread()), m_audioCapture, SLOT(run()));
-            if (!ret)
-                qWarning() << "Failed to connect run() to the local startWorkerThread signal";
-
-            // Call onStartThreadCb when the reader side of the named pipe has been setup
-            m_audioCapture->init(&AalMediaRecorderControl::onStartThreadCb, this);
-        }
-
         if (m_mediaRecorder == 0) {
             qWarning() << "Unable to create new media recorder";
             Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "Unable to create new media recorder");
+            return false;
+        }
+
+        int audioInitError = initAudioCapture();
+        if (audioInitError == 0) {
+            m_audioCaptureAvailable = true;
         } else {
-            setStatus(QMediaRecorder::LoadedStatus);
-            android_recorder_set_error_cb(m_mediaRecorder, &AalMediaRecorderControl::errorCB, this);
-            android_camera_unlock(m_service->androidControl());
+            m_audioCaptureAvailable = false;
+            if (audioInitError == AudioCapture::AUDIO_CAPTURE_TIMEOUT_ERROR) {
+                deleteRecorder();
+                return false;
+            }
         }
-    }
-
-    if (m_recordingTimer == 0) {
-        m_recordingTimer = new QTimer(this);
-        m_recordingTimer->setInterval(DURATION_UPDATE_INTERVAL);
-        m_recordingTimer->setSingleShot(false);
-        QObject::connect(m_recordingTimer, SIGNAL(timeout()),
-                         this, SLOT(updateDuration()));
-    }
+
+        android_recorder_set_error_cb(m_mediaRecorder, &AalMediaRecorderControl::errorCB, this);
+        android_camera_unlock(m_service->androidControl());
+    }
+
+    return true;
 }
 
 /*!
@@ -236,6 +205,8 @@
  */
 void AalMediaRecorderControl::deleteRecorder()
 {
+    deleteAudioCapture();
+
     if (m_mediaRecorder == 0)
         return;
 
@@ -245,6 +216,43 @@
     setStatus(QMediaRecorder::UnloadedStatus);
 }
 
+int AalMediaRecorderControl::initAudioCapture()
+{
+    // setting up audio recording; m_audioCapture is executed within the m_workerThread affinity
+    m_audioCapture = new AudioCapture(m_mediaRecorder);
+    int audioInitError = m_audioCapture->setupMicrophoneStream();
+    if (audioInitError != 0)
+    {
+        qWarning() << "Failed to setup PulseAudio microphone recording stream";
+        delete m_audioCapture;
+        m_audioCapture = 0;
+    } else {
+        m_audioCapture->moveToThread(&m_audioCaptureThread);
+
+        // startWorkerThread signal comes from an Android layer callback that resides down in
+        // the AudioRecordHybris class
+        connect(this, SIGNAL(audioCaptureThreadStarted()), m_audioCapture, SLOT(run()));
+
+        // Call recorderReadAudioCallback when the reader side of the named pipe has been setup
+        m_audioCapture->init(&AalMediaRecorderControl::recorderReadAudioCallback, this);
+    }
+    return audioInitError;
+}
+
+void AalMediaRecorderControl::deleteAudioCapture()
+{
+    if (m_audioCapture == 0)
+        return;
+
+    m_audioCapture->stopCapture();
+    m_audioCaptureThread.quit();
+    m_audioCaptureThread.wait();
+
+    delete m_audioCapture;
+    m_audioCapture = 0;
+    m_audioCaptureAvailable = false;
+}
+
 /*!
  * \brief AalMediaRecorderControl::errorCB handles errors from the android layer
  * \param context
@@ -285,10 +293,7 @@
 
     switch (state) {
     case QMediaRecorder::RecordingState: {
-        int ret = startRecording();
-        if (ret == -1) {
-            setStatus(QMediaRecorder::LoadedStatus);
-        }
+        startRecording();
         break;
     }
     case QMediaRecorder::StoppedState: {
@@ -349,17 +354,21 @@
         return RECORDER_INITIALIZATION_ERROR;
     }
 
+    if (m_currentStatus != QMediaRecorder::UnloadedStatus) {
+        qWarning() << "Can't start a recording while another one is in progess";
+        return RECORDER_NOT_AVAILABLE_ERROR;
+    }
+
+    setStatus(QMediaRecorder::LoadingStatus);
+
     m_duration = 0;
     Q_EMIT durationChanged(m_duration);
 
-    initRecorder();
-    if (m_mediaRecorder == 0) {
-        deleteRecorder();
+    if (!initRecorder()) {
+        setStatus(QMediaRecorder::UnloadedStatus);
         return RECORDER_NOT_AVAILABLE_ERROR;
     }
 
-    setStatus(QMediaRecorder::StartingStatus);
-
     QVideoEncoderSettings videoSettings = m_service->videoEncoderControl()->videoSettings();
 
     int ret;
@@ -369,12 +378,15 @@
         Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "android_recorder_setCamera() failed\n");
         return RECORDER_INITIALIZATION_ERROR;
     }
-    //state initial / idle
-    ret = android_recorder_setAudioSource(m_mediaRecorder, ANDROID_AUDIO_SOURCE_CAMCORDER);
-    if (ret < 0) {
-        deleteRecorder();
-        Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "android_recorder_setAudioSource() failed");
-        return RECORDER_INITIALIZATION_ERROR;
+    // state initial / idle
+    if (m_audioCaptureAvailable) {
+        ret = android_recorder_setAudioSource(m_mediaRecorder, ANDROID_AUDIO_SOURCE_CAMCORDER);
+        if (ret < 0) {
+            deleteRecorder();
+            Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "android_recorder_setAudioSource() failed");
+            return RECORDER_INITIALIZATION_ERROR;
+        }
+
     }
     ret = android_recorder_setVideoSource(m_mediaRecorder, ANDROID_VIDEO_SOURCE_CAMERA);
     if (ret < 0) {
@@ -382,19 +394,21 @@
         Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "android_recorder_setVideoSource() failed");
         return RECORDER_INITIALIZATION_ERROR;
     }
-    //state initialized
+    // state initialized
     ret = android_recorder_setOutputFormat(m_mediaRecorder, ANDROID_OUTPUT_FORMAT_MPEG_4);
     if (ret < 0) {
         deleteRecorder();
         Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "android_recorder_setOutputFormat() failed");
         return RECORDER_INITIALIZATION_ERROR;
     }
-    //state DataSourceConfigured
-    ret = android_recorder_setAudioEncoder(m_mediaRecorder, ANDROID_AUDIO_ENCODER_AAC);
-    if (ret < 0) {
-        deleteRecorder();
-        Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "android_recorder_setAudioEncoder() failed");
-        return RECORDER_INITIALIZATION_ERROR;
+    // state DataSourceConfigured
+    if (m_audioCaptureAvailable) {
+        ret = android_recorder_setAudioEncoder(m_mediaRecorder, ANDROID_AUDIO_ENCODER_AAC);
+        if (ret < 0) {
+            deleteRecorder();
+            Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "android_recorder_setAudioEncoder() failed");
+            return RECORDER_INITIALIZATION_ERROR;
+        }
     }
     // FIXME set codec from settings
     ret = android_recorder_setVideoEncoder(m_mediaRecorder, ANDROID_VIDEO_ENCODER_H264);
@@ -466,7 +480,11 @@
         Q_EMIT error(RECORDER_INITIALIZATION_ERROR, "android_recorder_prepare() failed");
         return RECORDER_INITIALIZATION_ERROR;
     }
-    //state prepared
+
+    setStatus(QMediaRecorder::LoadedStatus);
+    setStatus(QMediaRecorder::StartingStatus);
+
+    // state prepared
     ret = android_recorder_start(m_mediaRecorder);
     if (ret < 0) {
         close(m_outfd);
@@ -481,6 +499,13 @@
 
     setStatus(QMediaRecorder::RecordingStatus);
 
+    if (m_recordingTimer == 0) {
+        m_recordingTimer = new QTimer(this);
+        m_recordingTimer->setInterval(DURATION_UPDATE_INTERVAL);
+        m_recordingTimer->setSingleShot(false);
+        QObject::connect(m_recordingTimer, SIGNAL(timeout()),
+                         this, SLOT(updateDuration()));
+    }
     m_recordingTimer->start();
 
     return 0;
@@ -496,8 +521,9 @@
         qWarning() << "Can't stop recording properly, m_mediaRecorder is NULL";
         return;
     }
-    if (m_audioCapture == 0) {
-        qWarning() << "Can't stop recording properly, m_audioCapture is NULL";
+
+    if (m_currentStatus != QMediaRecorder::RecordingStatus) {
+        qWarning() << "Can't stop a recording that has not started";
         return;
     }
 
@@ -514,7 +540,9 @@
     // NOTE: This must come after the android_recorder_stop call, otherwise the
     // RecordThread instance will block the MPEG4Writer pthread_join when trying to
     // cleanly stop recording.
-    m_audioCapture->stopCapture();
+    if (m_audioCapture != 0) {
+        m_audioCapture->stopCapture();
+    }
 
     android_recorder_reset(m_mediaRecorder);
 
@@ -542,9 +570,10 @@
     android_recorder_setParameters(m_mediaRecorder, param.toLocal8Bit().data());
 }
 
-void AalMediaRecorderControl::onStartThreadCb(void *context)
+void AalMediaRecorderControl::recorderReadAudioCallback(void *context)
 {
     AalMediaRecorderControl *thiz = static_cast<AalMediaRecorderControl*>(context);
-    if (thiz != NULL)
-        thiz->onStartThread();
+    if (thiz != NULL) {
+        thiz->startAudioCaptureThread();
+    }
 }

=== modified file 'src/aalmediarecordercontrol.h'
--- src/aalmediarecordercontrol.h	2015-01-23 02:49:03 +0000
+++ src/aalmediarecordercontrol.h	2015-08-21 20:10:16 +0000
@@ -21,6 +21,7 @@
 #include <QMediaRecorderControl>
 #include <QSize>
 #include <QUrl>
+#include <QThread>
 
 #include <stdint.h>
 
@@ -58,23 +59,25 @@
     virtual void setMuted(bool muted);
     virtual void setState(QMediaRecorder::State state);
     virtual void setVolume(qreal gain);
-    void onStartThread();
+    void startAudioCaptureThread();
 
 signals:
-    void startWorkerThread();
+    void audioCaptureThreadStarted();
 
 private Q_SLOTS:
     virtual void updateDuration();
     void handleError();
+    void deleteAudioCapture();
 
 private:
-    void initRecorder();
+    bool initRecorder();
     void deleteRecorder();
+    int initAudioCapture();
     void setStatus(QMediaRecorder::Status status);
     int startRecording();
     void stopRecording();
     void setParameter(const QString &parameter, int value);
-    static void onStartThreadCb(void *context);
+    static void recorderReadAudioCallback(void *context);
 
     AalCameraService *m_service;
     MediaRecorderWrapper *m_mediaRecorder;
@@ -85,7 +88,8 @@
     QMediaRecorder::State m_currentState;
     QMediaRecorder::Status m_currentStatus;
     QTimer *m_recordingTimer;
-    QThread *m_workerThread;
+    QThread m_audioCaptureThread;
+    bool m_audioCaptureAvailable;
 
     static const int RECORDER_GENERAL_ERROR = -1;
     static const int RECORDER_NOT_AVAILABLE_ERROR = -2;

=== modified file 'src/audiocapture.cpp'
--- src/audiocapture.cpp	2014-12-03 17:08:38 +0000
+++ src/audiocapture.cpp	2015-08-21 20:10:16 +0000
@@ -37,6 +37,8 @@
 
 AudioCapture::~AudioCapture()
 {
+    android_recorder_set_audio_read_cb(m_mediaRecorder, NULL, NULL);
+
     if (m_audioPipe >= 0)
         close(m_audioPipe);
     if (m_paStream != NULL)
@@ -46,10 +48,10 @@
 /*!
  * \brief Initializes AudioCapture so that it's ready to read microphone data from Pulseaudio
  */
-bool AudioCapture::init(StartWorkerThreadCb cb, void *context)
+bool AudioCapture::init(RecorderReadAudioCallback callback, void *context)
 {
-    // The MediaRecorderLayer will call method (cb) when it's ready to encode a new audio buffer
-    android_recorder_set_audio_read_cb(m_mediaRecorder, cb, context);
+    // The MediaRecorderLayer will call method (callback) when it's ready to encode a new audio buffer
+    android_recorder_set_audio_read_cb(m_mediaRecorder, callback, context);
 
     return true;
 }
@@ -68,17 +70,12 @@
  */
 void AudioCapture::run()
 {
+    m_flagExit = false;
     qDebug() << __PRETTY_FUNCTION__;
 
     int bytesWritten = 0, bytesRead = 0;
     const size_t readSize = sizeof(m_audioBuf);
 
-    if (!setupMicrophoneStream())
-    {
-        qWarning() << "Failed to setup PulseAudio microphone recording stream";
-        return;
-    }
-
     if (!setupPipe())
     {
         qWarning() << "Failed to open /dev/socket/micshm, cannot write data to pipe";
@@ -101,8 +98,6 @@
         pa_simple_free(m_paStream);
         m_paStream = NULL;
     }
-
-    Q_EMIT finished();
 }
 
 /*!
@@ -122,21 +117,9 @@
 }
 
 /*!
- * \brief Signals AalMediaRecorderControl to start the main thread loop.
- * \detail This is necessary due to thread contexts. Starting of the main thread loop
- * for AudioCapture must be done in the main thread context and not in the AudioCapture
- * thread context, otherwise the loop start signal will never be seen.
- */
-void AudioCapture::startThreadLoop()
-{
-    Q_EMIT startThread();
-    qDebug() << "Emitted startThread(), should start reading from mic";
-}
-
-/*!
  * \brief Sets up the Pulseaudio microphone input channel
  */
-bool AudioCapture::setupMicrophoneStream()
+int AudioCapture::setupMicrophoneStream()
 {
     // FIXME: Get these parameters more dynamically from the control
     static const pa_sample_spec ss = {
@@ -150,10 +133,14 @@
     if (m_paStream == NULL)
     {
         qWarning() << "Failed to open a PulseAudio channel to read the microphone: " << pa_strerror(error);
-        return false;
+        if (error == PA_ERR_TIMEOUT) {
+            return AUDIO_CAPTURE_TIMEOUT_ERROR;
+        } else {
+            return AUDIO_CAPTURE_GENERAL_ERROR;
+        }
     }
 
-    return true;
+    return 0;
 }
 
 /*!

=== modified file 'src/audiocapture.h'
--- src/audiocapture.h	2014-07-30 14:50:39 +0000
+++ src/audiocapture.h	2015-08-21 20:10:16 +0000
@@ -34,28 +34,24 @@
 {
     Q_OBJECT
 
-    typedef void (*StartWorkerThreadCb)(void *context);
+    typedef void (*RecorderReadAudioCallback)(void *context);
 public:
+    static const int AUDIO_CAPTURE_GENERAL_ERROR = -1;
+    static const int AUDIO_CAPTURE_TIMEOUT_ERROR = -2;
+
     explicit AudioCapture(MediaRecorderWrapper *mediaRecorder);
     ~AudioCapture();
 
-    bool init(StartWorkerThreadCb cb, void *context);
+    bool init(RecorderReadAudioCallback callback, void *context);
     /* Terminates the Pulseaudio reader/writer QThread */
+    int setupMicrophoneStream();
     void stopCapture();
 
-signals:
-    void startThread();
-    void finished();
-
 public Q_SLOTS:
     void run();
 
-private Q_SLOTS:
-    void startThreadLoop();
-
 private:
     int readMicrophone();
-    bool setupMicrophoneStream();
     bool setupPipe();
     ssize_t loopWrite(int fd, const void *data, size_t len);
     int writeDataToPipe();

=== modified file 'unittests/stubs/audiocapture_stub.cpp'
--- unittests/stubs/audiocapture_stub.cpp	2014-07-30 14:50:39 +0000
+++ unittests/stubs/audiocapture_stub.cpp	2015-08-21 20:10:16 +0000
@@ -29,7 +29,7 @@
 {
 }
 
-bool AudioCapture::init(StartWorkerThreadCb cb, void *context)
+bool AudioCapture::init(RecorderReadAudioCallback cb, void *context)
 {
     Q_UNUSED(cb);
     Q_UNUSED(context);
@@ -40,14 +40,14 @@
 {
 }
 
+int AudioCapture::setupMicrophoneStream()
+{
+}
+
 void AudioCapture::run()
 {
 }
 
-void AudioCapture::startThreadLoop()
-{
-}
-
 int AudioCapture::readMicrophone()
 {
     return 0;