recorder: fix some recorder bugs

+ Fix recording in an audio only call
+ Fix crash if deinit the recorder without remote video
+ Add some tests

Change-Id: Ie78a632f6a2ecb3eae4f53ae8f25c8ecbf5c5091
GitLab: #719

src/conference.cpp

@@ -1019,7 +1019,7 @@ Conference::toggleRecording()
     bool newState = not isRecording();
     if (newState)
         initRecorder(recorder_);
-    else
+    else if (recorder_)
         deinitRecorder(recorder_);
 
     // Notify each participant

src/media/video/video_receive_thread.cpp

@@ -297,13 +297,17 @@ VideoReceiveThread::getHeight() const
 AVPixelFormat
 VideoReceiveThread::getPixelFormat() const
 {
-    return videoDecoder_->getPixelFormat();
+    if (videoDecoder_)
+        return videoDecoder_->getPixelFormat();
+    return {};
 }
 
 MediaStream
 VideoReceiveThread::getInfo() const
 {
-    return videoDecoder_->getStream("v:remote");
+    if (videoDecoder_)
+        return videoDecoder_->getStream("v:remote");
+    return {};
 }
 
 void

src/media/video/video_rtp_session.cpp

@@ -746,6 +746,8 @@ VideoRtpSession::initRecorder(std::shared_ptr<MediaRecorder>& rec)
 void
 VideoRtpSession::deinitRecorder(std::shared_ptr<MediaRecorder>& rec)
 {
+    if (!rec)
+        return;
     if (receiveThread_) {
         if (auto ob = rec->getStream(receiveThread_->getInfo().name)) {
             receiveThread_->detach(ob);

src/sip/sipcall.cpp

@@ -177,20 +177,7 @@ SIPCall::createRtpSession(RtpStream& stream)
     if (not stream.mediaAttribute_)
         throw std::runtime_error("Missing media attribute");
 
-    // Find idx of this stream as we can share several audio/videos
-    auto streamIdx = [&]() {
-        auto idx = 0;
-        for (const auto& st : rtpStreams_) {
-            if (st.mediaAttribute_->label_ == stream.mediaAttribute_->label_)
-                return idx;
-            if (st.mediaAttribute_->type_ == stream.mediaAttribute_->type_)
-                idx++;
-        }
-        return -1;
-    };
-
     // To get audio_0 ; video_0
-    auto idx = streamIdx();
     auto streamId = sip_utils::streamId(id_, stream.mediaAttribute_->label_);
     if (stream.mediaAttribute_->type_ == MediaType::MEDIA_AUDIO) {
         stream.rtpSession_ = std::make_shared<AudioRtpSession>(id_, streamId);
@@ -3406,11 +3393,9 @@ SIPCall::rtpSetupSuccess(MediaType type, bool isRemote)
             mediaReady_.at("v:local") = true;
     }
 
+    isAudioOnly_ = !hasVideo();
 #ifdef ENABLE_VIDEO
-    if (mediaReady_.at("a:local") and mediaReady_.at("a:remote") and mediaReady_.at("v:remote")) {
+    readyToRecord_ = true; // We're ready to record whenever a stream is ready
-        if (Manager::instance().videoPreferences.getRecordPreview() or mediaReady_.at("v:local"))
-            readyToRecord_ = true;
-    }
 #endif
 
     if (pendingRecord_ && readyToRecord_)

test/meson.build

@@ -120,6 +120,16 @@ test('conference', ut_conference,
 )
 
 
+ut_recorder = executable('ut_recorder',
+    sources: files('unitTest/call/recorder.cpp'),
+    include_directories: ut_includedirs,
+    dependencies: ut_dependencies,
+    link_with: ut_library
+)
+test('conference', ut_recorder,
+    workdir: ut_workdir, is_parallel: false, timeout: 1800
+)
+
 ut_connection_manager = executable('ut_connection_manager',
     sources: files('unitTest/connectionManager/connectionManager.cpp'),
     include_directories: ut_includedirs,

test/unitTest/Makefile.am

@@ -130,6 +130,12 @@ ut_audio_frame_resizer_SOURCES = media/audio/test_audio_frame_resizer.cpp common
 check_PROGRAMS += ut_call
 ut_call_SOURCES = call/call.cpp common.cpp
 
+#
+# recorder
+#
+check_PROGRAMS += ut_recorder
+ut_recorder_SOURCES = call/recorder.cpp common.cpp
+
 #
 # conference
 #

test/unitTest/call/recorder.cpp

@@ -0,0 +1,310 @@
+/*
+ *  Copyright (C) 2022 Savoir-faire Linux Inc.
+ *  Author: Sébastien Blin <sebastien.blin@savoirfairelinux.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program. If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#include <cppunit/TestAssert.h>
+#include <cppunit/TestFixture.h>
+#include <cppunit/extensions/HelperMacros.h>
+
+#include <condition_variable>
+#include <filesystem>
+#include <string>
+
+#include "../../test_runner.h"
+#include "account_const.h"
+#include "fileutils.h"
+#include "jami.h"
+#include "jamidht/jamiaccount.h"
+#include "manager.h"
+#include "media_const.h"
+
+#include "common.h"
+
+using namespace DRing::Account;
+using namespace std::literals::chrono_literals;
+
+namespace jami {
+namespace test {
+
+struct CallData
+{
+    std::string callId {};
+    std::string state {};
+    std::string mediaStatus {};
+    std::string device {};
+    std::string hostState {};
+
+    void reset()
+    {
+        callId = "";
+        state = "";
+        mediaStatus = "";
+        device = "";
+        hostState = "";
+    }
+};
+
+class RecorderTest : public CppUnit::TestFixture
+{
+public:
+    RecorderTest()
+    {
+        // Init daemon
+        DRing::init(DRing::InitFlag(DRing::DRING_FLAG_DEBUG | DRing::DRING_FLAG_CONSOLE_LOG));
+        if (not Manager::instance().initialized)
+            CPPUNIT_ASSERT(DRing::start("jami-sample.yml"));
+    }
+    ~RecorderTest() { DRing::fini(); }
+    static std::string name() { return "Recorder"; }
+    void setUp();
+    void tearDown();
+
+    std::string aliceId;
+    std::string bobId;
+    CallData bobCall {};
+
+    std::mutex mtx;
+    std::unique_lock<std::mutex> lk {mtx};
+    std::condition_variable cv;
+
+private:
+    void registerSignalHandlers();
+    void testRecordCall();
+    void testRecordAudioOnlyCall();
+    void testStopCallWhileRecording();
+    void testDaemonPreference();
+
+    CPPUNIT_TEST_SUITE(RecorderTest);
+    CPPUNIT_TEST(testRecordCall);
+    CPPUNIT_TEST(testRecordAudioOnlyCall);
+    CPPUNIT_TEST(testStopCallWhileRecording);
+    CPPUNIT_TEST(testDaemonPreference);
+    CPPUNIT_TEST_SUITE_END();
+};
+
+CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(RecorderTest, RecorderTest::name());
+
+void
+RecorderTest::setUp()
+{
+    auto actors = load_actors_and_wait_for_announcement("actors/alice-bob.yml");
+    aliceId = actors["alice"];
+    bobId = actors["bob"];
+    bobCall.reset();
+
+    fileutils::recursive_mkdir("records");
+    DRing::setRecordPath("records");
+}
+
+void
+RecorderTest::tearDown()
+{
+    fileutils::removeAll("records");
+
+    wait_for_removal_of({aliceId, bobId});
+}
+
+void
+RecorderTest::registerSignalHandlers()
+{
+    auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+    auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+    auto bobUri = bobAccount->getUsername();
+
+    std::map<std::string, std::shared_ptr<DRing::CallbackWrapperBase>> confHandlers;
+    // Watch signals
+    confHandlers.insert(DRing::exportable_callback<DRing::CallSignal::IncomingCallWithMedia>(
+        [=](const std::string& accountId,
+            const std::string& callId,
+            const std::string&,
+            const std::vector<std::map<std::string, std::string>>&) {
+            if (accountId == bobId) {
+                bobCall.callId = callId;
+            }
+            cv.notify_one();
+        }));
+    confHandlers.insert(
+        DRing::exportable_callback<DRing::CallSignal::StateChange>([=](const std::string& accountId,
+                                                                       const std::string& callId,
+                                                                       const std::string& state,
+                                                                       signed) {
+            if (accountId == aliceId) {
+                auto details = DRing::getCallDetails(aliceId, callId);
+                if (details["PEER_NUMBER"].find(bobUri) != std::string::npos)
+                    bobCall.hostState = state;
+            } else if (bobCall.callId == callId)
+                bobCall.state = state;
+            cv.notify_one();
+        }));
+
+    confHandlers.insert(DRing::exportable_callback<DRing::CallSignal::MediaNegotiationStatus>(
+        [&](const std::string& callId,
+            const std::string& event,
+            const std::vector<std::map<std::string, std::string>>&) {
+            if (callId == bobCall.callId)
+                bobCall.mediaStatus = event;
+            cv.notify_one();
+        }));
+
+    DRing::registerSignalHandlers(confHandlers);
+}
+
+void
+RecorderTest::testRecordCall()
+{
+    registerSignalHandlers();
+    auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+    auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+    auto bobUri = bobAccount->getUsername();
+
+    JAMI_INFO("Start call between Alice and Bob");
+    std::vector<std::map<std::string, std::string>> mediaList;
+    auto callId = DRing::placeCallWithMedia(aliceId, bobUri, mediaList);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] { return !bobCall.callId.empty(); }));
+    Manager::instance().answerCall(bobId, bobCall.callId);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] {
+        return bobCall.mediaStatus
+               == DRing::Media::MediaNegotiationStatusEvents::NEGOTIATION_SUCCESS;
+    }));
+
+    // Start recorder
+    CPPUNIT_ASSERT(!DRing::getIsRecording(aliceId, callId));
+    auto files = fileutils::readDirectory("records");
+    CPPUNIT_ASSERT(files.size() == 0);
+    DRing::toggleRecording(aliceId, callId);
+
+    // Stop recorder after a few seconds
+    std::this_thread::sleep_for(5s);
+    CPPUNIT_ASSERT(DRing::getIsRecording(aliceId, callId));
+    DRing::toggleRecording(aliceId, callId);
+    CPPUNIT_ASSERT(!DRing::getIsRecording(aliceId, callId));
+
+    files = fileutils::readDirectory("records");
+    CPPUNIT_ASSERT(files.size() == 1);
+
+    Manager::instance().hangupCall(aliceId, callId);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] { return bobCall.state == "OVER"; }));
+}
+
+void
+RecorderTest::testRecordAudioOnlyCall()
+{
+    registerSignalHandlers();
+    auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+    auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+    auto bobUri = bobAccount->getUsername();
+
+    JAMI_INFO("Start call between Alice and Bob");
+    std::vector<std::map<std::string, std::string>> mediaList;
+    std::map<std::string, std::string> mediaAttribute
+        = {{DRing::Media::MediaAttributeKey::MEDIA_TYPE, DRing::Media::MediaAttributeValue::AUDIO},
+           {DRing::Media::MediaAttributeKey::ENABLED, TRUE_STR},
+           {DRing::Media::MediaAttributeKey::MUTED, FALSE_STR},
+           {DRing::Media::MediaAttributeKey::SOURCE, ""},
+           {DRing::Media::MediaAttributeKey::LABEL, "audio_0"}};
+    mediaList.emplace_back(mediaAttribute);
+    auto callId = DRing::placeCallWithMedia(aliceId, bobUri, mediaList);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] { return !bobCall.callId.empty(); }));
+    Manager::instance().answerCall(bobId, bobCall.callId);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] {
+        return bobCall.mediaStatus
+               == DRing::Media::MediaNegotiationStatusEvents::NEGOTIATION_SUCCESS;
+    }));
+
+    // Start recorder
+    auto files = fileutils::readDirectory("records");
+    CPPUNIT_ASSERT(files.size() == 0);
+    DRing::toggleRecording(aliceId, callId);
+
+    // Stop recorder
+    std::this_thread::sleep_for(5s);
+    CPPUNIT_ASSERT(DRing::getIsRecording(aliceId, callId));
+    DRing::toggleRecording(aliceId, callId);
+    CPPUNIT_ASSERT(!DRing::getIsRecording(aliceId, callId));
+
+    files = fileutils::readDirectory("records");
+    CPPUNIT_ASSERT(files.size() == 1);
+
+    Manager::instance().hangupCall(aliceId, callId);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] { return bobCall.state == "OVER"; }));
+}
+
+void
+RecorderTest::testStopCallWhileRecording()
+{
+    registerSignalHandlers();
+    auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+    auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+    auto bobUri = bobAccount->getUsername();
+
+    JAMI_INFO("Start call between Alice and Bob");
+    std::vector<std::map<std::string, std::string>> mediaList;
+    auto callId = DRing::placeCallWithMedia(aliceId, bobUri, mediaList);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] { return !bobCall.callId.empty(); }));
+    Manager::instance().answerCall(bobId, bobCall.callId);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] {
+        return bobCall.mediaStatus
+               == DRing::Media::MediaNegotiationStatusEvents::NEGOTIATION_SUCCESS;
+    }));
+
+    // Start recorder
+    auto files = fileutils::readDirectory("records");
+    CPPUNIT_ASSERT(files.size() == 0);
+    DRing::toggleRecording(aliceId, callId);
+
+    // Hangup call
+    std::this_thread::sleep_for(5s);
+    Manager::instance().hangupCall(aliceId, callId);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] { return bobCall.state == "OVER"; }));
+
+    files = fileutils::readDirectory("records");
+    CPPUNIT_ASSERT(files.size() == 1);
+}
+
+void
+RecorderTest::testDaemonPreference()
+{
+    registerSignalHandlers();
+    auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+    auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+    auto bobUri = bobAccount->getUsername();
+
+    DRing::setIsAlwaysRecording(true);
+
+    JAMI_INFO("Start call between Alice and Bob");
+    std::vector<std::map<std::string, std::string>> mediaList;
+    auto callId = DRing::placeCallWithMedia(aliceId, bobUri, mediaList);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] { return !bobCall.callId.empty(); }));
+    Manager::instance().answerCall(bobId, bobCall.callId);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] {
+        return bobCall.mediaStatus
+               == DRing::Media::MediaNegotiationStatusEvents::NEGOTIATION_SUCCESS;
+    }));
+
+    // Let record some seconds
+    std::this_thread::sleep_for(5s);
+
+    Manager::instance().hangupCall(aliceId, callId);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 20s, [&] { return bobCall.state == "OVER"; }));
+    auto files = fileutils::readDirectory("records");
+    CPPUNIT_ASSERT(files.size() == 1);
+}
+
+} // namespace test
+} // namespace jami
+
+RING_TEST_RUNNER(jami::test::RecorderTest::name())