From 15e133301eae6fff7354490d453b2446fd16a7a7 Mon Sep 17 00:00:00 2001
From: Andreas Traczyk <andreas.traczyk@savoirfairelinux.com>
Date: Mon, 26 Sep 2022 21:01:45 -0400
Subject: [PATCH] avmodel: use synchronous callbacks for
DecodingStarted/Stopped
This is an older issue that has resurfaced where mobile device rotation
at the beginning of a call cause a frame-copy to a stale buffer because
the DecodingStarted event is handled asynchronously.
Noticed on Windows but I believe any non-dbus build should have it.
So we make all the connections blocking and adjust some parameters.
This commit also removes the DecodingStarted handler in CallModel which
was causing the client's target video frame to be reallocated for each
account present.
Change-Id: I23ac4e0bd4b446e7a532f0d362f7ecd209d3c790
GitLab: #536
---
src/app/avadapter.cpp | 3 +-
src/app/avadapter.h | 2 +-
src/app/videoprovider.cpp | 32 ++++++++-----
src/app/videoprovider.h | 2 +-
src/libclient/api/avmodel.h | 3 +-
src/libclient/avmodel.cpp | 76 +++++++++++++++---------------
src/libclient/callbackshandler.cpp | 4 +-
src/libclient/callmodel.cpp | 37 +++++----------
src/libclient/directrenderer.cpp | 2 +-
src/libclient/renderer.h | 2 +-
src/libclient/shmrenderer.cpp | 2 +-
11 files changed, 79 insertions(+), 86 deletions(-)
diff --git a/src/app/avadapter.cpp b/src/app/avadapter.cpp
index 8716decc3..c33da4723 100644
--- a/src/app/avadapter.cpp
+++ b/src/app/avadapter.cpp
@@ -298,8 +298,9 @@ AvAdapter::onAudioDeviceEvent()
}
void
-AvAdapter::onRendererStarted(const QString& id)
+AvAdapter::onRendererStarted(const QString& id, const QSize& size)
{
+ Q_UNUSED(size)
auto callId = lrcInstance_->getCurrentCallId();
auto callModel = lrcInstance_->getCurrentCallModel();
auto renderDevice = callModel->getCurrentRenderedDevice(callId);
diff --git a/src/app/avadapter.h b/src/app/avadapter.h
index 793a6f438..f565ce59a 100644
--- a/src/app/avadapter.h
+++ b/src/app/avadapter.h
@@ -108,7 +108,7 @@ protected:
private Q_SLOTS:
void onAudioDeviceEvent();
- void onRendererStarted(const QString& id);
+ void onRendererStarted(const QString& id, const QSize& size);
private:
// Get screens arrangement rect relative to primary screen.
diff --git a/src/app/videoprovider.cpp b/src/app/videoprovider.cpp
index 127a89662..ac9ccde79 100644
--- a/src/app/videoprovider.cpp
+++ b/src/app/videoprovider.cpp
@@ -64,9 +64,17 @@ VideoProvider::registerSink(const QString& id, QVideoSink* obj)
if (it == framesObjects_.end()) {
auto fo = std::make_unique<FrameObject>();
fo->subscribers.insert(obj);
- framesObjects_.emplace(id, std::move(fo));
- return;
+ qDebug() << "Creating new FrameObject for id:" << id;
+ auto emplaced = framesObjects_.emplace(id, std::move(fo));
+ if (!emplaced.second) {
+ qWarning() << "Couldn't create FrameObject for id:" << id;
+ return;
+ }
+ it = emplaced.first;
}
+ qDebug().noquote() << QString("Adding sink: 0x%1 to subscribers for id: %2")
+ .arg((quintptr) obj, QT_POINTER_SIZE * 2, 16, QChar('0'))
+ .arg(id);
it->second->subscribers.insert(obj);
}
@@ -78,7 +86,11 @@ VideoProvider::unregisterSink(QVideoSink* obj)
auto& subs = frameObjIt.second->subscribers;
auto it = subs.constFind(obj);
if (it != subs.cend()) {
+ qDebug().noquote() << QString("Removing sink: 0x%1 from subscribers for id: %2")
+ .arg((quintptr) obj, QT_POINTER_SIZE * 2, 16, QChar('0'))
+ .arg(frameObjIt.first);
subs.erase(it);
+ return;
}
}
}
@@ -121,15 +133,11 @@ VideoProvider::captureVideoFrame(const QString& id)
}
void
-VideoProvider::onRendererStarted(const QString& id)
+VideoProvider::onRendererStarted(const QString& id, const QSize& size)
{
- auto size = avModel_.getRendererSize(id);
- // This slot is queued, the renderer may have been destroyed.
- if (size.width() == 0 || size.height() == 0 || activeRenderers_[id] == size) {
- return;
- }
- auto pixelFormat = avModel_.useDirectRenderer() ? QVideoFrameFormat::Format_RGBA8888
- : QVideoFrameFormat::Format_BGRA8888;
+ static const auto pixelFormat = avModel_.useDirectRenderer()
+ ? QVideoFrameFormat::Format_RGBA8888
+ : QVideoFrameFormat::Format_BGRA8888;
auto frameFormat = QVideoFrameFormat(size, pixelFormat);
{
QMutexLocker lk(&framesObjsMutex_);
@@ -137,11 +145,11 @@ VideoProvider::onRendererStarted(const QString& id)
if (it == framesObjects_.end()) {
auto fo = std::make_unique<FrameObject>();
fo->videoFrame = std::make_unique<QVideoFrame>(frameFormat);
- qDebug() << "Create new QVideoFrame " << frameFormat.frameSize();
+ qDebug() << "Create new QVideoFrame" << frameFormat.frameSize();
framesObjects_.emplace(id, std::move(fo));
} else {
it->second->videoFrame.reset(new QVideoFrame(frameFormat));
- qDebug() << "QVideoFrame reset to " << frameFormat.frameSize();
+ qDebug() << "QVideoFrame reset to" << frameFormat.frameSize();
}
}
diff --git a/src/app/videoprovider.h b/src/app/videoprovider.h
index c2358a80a..775e42500 100644
--- a/src/app/videoprovider.h
+++ b/src/app/videoprovider.h
@@ -48,7 +48,7 @@ public:
Q_INVOKABLE QString captureVideoFrame(const QString& id);
private Q_SLOTS:
- void onRendererStarted(const QString& id);
+ void onRendererStarted(const QString& id, const QSize& size);
void onFrameBufferRequested(const QString& id, AVFrame* avframe);
void onFrameUpdated(const QString& id);
void onRendererStopped(const QString& id);
diff --git a/src/libclient/api/avmodel.h b/src/libclient/api/avmodel.h
index 5bcdc5dc4..d0ab442e2 100644
--- a/src/libclient/api/avmodel.h
+++ b/src/libclient/api/avmodel.h
@@ -281,8 +281,9 @@ Q_SIGNALS:
/**
* Emitted when a renderer is started
* @param id of the renderer
+ * @param size of the renderer
*/
- void rendererStarted(const QString& id);
+ void rendererStarted(const QString& id, const QSize& size);
/**
* Emitted when a renderer is stopped
* @param id of the renderer
diff --git a/src/libclient/avmodel.cpp b/src/libclient/avmodel.cpp
index 02b8e3f13..dacaf1e07 100644
--- a/src/libclient/avmodel.cpp
+++ b/src/libclient/avmodel.cpp
@@ -694,11 +694,13 @@ AVModelPimpl::AVModelPimpl(AVModel& linked, const CallbacksHandler& callbacksHan
connect(&callbacksHandler,
&CallbacksHandler::decodingStarted,
this,
- &AVModelPimpl::onDecodingStarted);
+ &AVModelPimpl::onDecodingStarted,
+ Qt::DirectConnection);
connect(&callbacksHandler,
&CallbacksHandler::decodingStopped,
this,
- &AVModelPimpl::onDecodingStopped);
+ &AVModelPimpl::onDecodingStopped,
+ Qt::DirectConnection);
auto startedPreview = false;
auto restartRenderers = [&](const QStringList& callList) {
@@ -750,8 +752,6 @@ AVModelPimpl::getRecordingPath() const
void
AVModelPimpl::onDecodingStarted(const QString& id, const QString& shmPath, int width, int height)
{
- if (id != "" && id != "local" && !id.contains("://")) // Else managed by callmodel
- return;
addRenderer(id, QSize(width, height), shmPath);
}
@@ -824,41 +824,39 @@ createRenderer(const QString& id, const QSize& res, const QString& shmPath = {})
void
AVModelPimpl::addRenderer(const QString& id, const QSize& res, const QString& shmPath)
{
- {
- auto connectRenderer = [this](Renderer* renderer, const QString& id) {
- connect(
- renderer,
- &Renderer::started,
- this,
- [this, id] { Q_EMIT linked_.rendererStarted(id); },
- Qt::QueuedConnection);
- connect(
- renderer,
- &Renderer::frameBufferRequested,
- this,
- [this, id](AVFrame* frame) { Q_EMIT linked_.frameBufferRequested(id, frame); },
- Qt::DirectConnection);
- connect(
- renderer,
- &Renderer::frameUpdated,
- this,
- [this, id] { Q_EMIT linked_.frameUpdated(id); },
- Qt::DirectConnection);
- connect(
- renderer,
- &Renderer::stopped,
- this,
- [this, id] { Q_EMIT linked_.rendererStopped(id); },
- Qt::DirectConnection);
- };
- std::lock_guard<std::mutex> lk(renderers_mtx_);
- renderers_.erase(id); // Because it should be done before creating the renderer
- auto renderer = createRenderer(id, res, shmPath);
- auto& r = renderers_[id];
- r = std::move(renderer);
- connectRenderer(r.get(), id);
- r->startRendering();
- }
+ auto connectRenderer = [this](Renderer* renderer, const QString& id) {
+ connect(
+ renderer,
+ &Renderer::started,
+ this,
+ [this, id](const QSize& size) { Q_EMIT linked_.rendererStarted(id, size); },
+ Qt::DirectConnection);
+ connect(
+ renderer,
+ &Renderer::frameBufferRequested,
+ this,
+ [this, id](AVFrame* frame) { Q_EMIT linked_.frameBufferRequested(id, frame); },
+ Qt::DirectConnection);
+ connect(
+ renderer,
+ &Renderer::frameUpdated,
+ this,
+ [this, id] { Q_EMIT linked_.frameUpdated(id); },
+ Qt::DirectConnection);
+ connect(
+ renderer,
+ &Renderer::stopped,
+ this,
+ [this, id] { Q_EMIT linked_.rendererStopped(id); },
+ Qt::DirectConnection);
+ };
+ std::lock_guard<std::mutex> lk(renderers_mtx_);
+ renderers_.erase(id); // Because it should be done before creating the renderer
+ auto renderer = createRenderer(id, res, shmPath);
+ auto& r = renderers_[id];
+ r = std::move(renderer);
+ connectRenderer(r.get(), id);
+ r->startRendering();
}
void
diff --git a/src/libclient/callbackshandler.cpp b/src/libclient/callbackshandler.cpp
index 6a93d5124..b0e085d14 100644
--- a/src/libclient/callbackshandler.cpp
+++ b/src/libclient/callbackshandler.cpp
@@ -275,13 +275,13 @@ CallbacksHandler::CallbacksHandler(const Lrc& parent)
&VideoManagerInterface::decodingStarted,
this,
&CallbacksHandler::decodingStarted,
- Qt::QueuedConnection);
+ Qt::DirectConnection);
connect(&VideoManager::instance(),
&VideoManagerInterface::decodingStopped,
this,
&CallbacksHandler::decodingStopped,
- Qt::QueuedConnection);
+ Qt::DirectConnection);
connect(&VideoManager::instance(),
&VideoManagerInterface::deviceEvent,
diff --git a/src/libclient/callmodel.cpp b/src/libclient/callmodel.cpp
index 93afd5419..baf1a19aa 100644
--- a/src/libclient/callmodel.cpp
+++ b/src/libclient/callmodel.cpp
@@ -253,14 +253,6 @@ public Q_SLOTS:
* @param state the new state
*/
void remoteRecordingChanged(const QString& callId, const QString& peerNumber, bool state);
- /**
- * Listen from CallbacksHandler when a renderer starts
- * @param id
- * @param shmPath
- * @param width
- * @param height
- */
- void onDecodingStarted(const QString& id, const QString& shmPath, int width, int height);
};
CallModel::CallModel(const account::Info& owner,
@@ -525,7 +517,7 @@ CallModel::addMedia(const QString& callId, const QString& source, MediaRequestTy
CallManager::instance().requestMediaChange(owner.id, callId, proposedList);
callInfo->mediaList = proposedList;
if (callInfo->status == call::Status::IN_PROGRESS)
- Q_EMIT callInfosChanged(owner.id, callId);
+ Q_EMIT callInfosChanged(owner.id, callId);
}
void
@@ -578,12 +570,15 @@ CallModel::removeMedia(const QString& callId,
return;
} else if (!hasVideo) {
// To receive the remote video, we need a muted camera
- proposedList.push_back(
- MapStringString {{MediaAttributeKey::MEDIA_TYPE, MediaAttributeValue::VIDEO},
- {MediaAttributeKey::ENABLED, TRUE_STR},
- {MediaAttributeKey::MUTED, TRUE_STR},
- {MediaAttributeKey::SOURCE, pimpl_->lrc.getAVModel().getCurrentVideoCaptureDevice()}, // not needed to set the source. Daemon should be able to check it
- {MediaAttributeKey::LABEL, label.isEmpty() ? "video_0" : label}});
+ proposedList.push_back(MapStringString {
+ {MediaAttributeKey::MEDIA_TYPE, MediaAttributeValue::VIDEO},
+ {MediaAttributeKey::ENABLED, TRUE_STR},
+ {MediaAttributeKey::MUTED, TRUE_STR},
+ {MediaAttributeKey::SOURCE,
+ pimpl_->lrc.getAVModel()
+ .getCurrentVideoCaptureDevice()}, // not needed to set the source. Daemon should be
+ // able to check it
+ {MediaAttributeKey::LABEL, label.isEmpty() ? "video_0" : label}});
}
if (isVideo && !label.isEmpty())
@@ -592,7 +587,7 @@ CallModel::removeMedia(const QString& callId,
CallManager::instance().requestMediaChange(owner.id, callId, proposedList);
callInfo->mediaList = proposedList;
if (callInfo->status == call::Status::IN_PROGRESS)
- Q_EMIT callInfosChanged(owner.id, callId);
+ Q_EMIT callInfosChanged(owner.id, callId);
}
void
@@ -945,10 +940,6 @@ CallModelPimpl::CallModelPimpl(const CallModel& linked,
&CallbacksHandler::remoteRecordingChanged,
this,
&CallModelPimpl::remoteRecordingChanged);
- connect(&callbacksHandler,
- &CallbacksHandler::decodingStarted,
- this,
- &CallModelPimpl::onDecodingStarted);
#ifndef ENABLE_LIBWRAP
// Only necessary with dbus since the daemon runs separately
@@ -1644,12 +1635,6 @@ CallModelPimpl::remoteRecordingChanged(const QString& callId, const QString& pee
Q_EMIT linked.remoteRecordingChanged(callId, it->second->peerRec, state);
}
-void
-CallModelPimpl::onDecodingStarted(const QString& id, const QString& shmPath, int width, int height)
-{
- lrc.getAVModel().addRenderer(id, QSize(width, height), shmPath);
-}
-
} // namespace lrc
#include "api/moc_callmodel.cpp"
diff --git a/src/libclient/directrenderer.cpp b/src/libclient/directrenderer.cpp
index 51209d9b3..6af6d3ba1 100644
--- a/src/libclient/directrenderer.cpp
+++ b/src/libclient/directrenderer.cpp
@@ -104,7 +104,7 @@ DirectRenderer::~DirectRenderer() {}
void
DirectRenderer::startRendering()
{
- Q_EMIT started();
+ Q_EMIT started(size());
}
void
diff --git a/src/libclient/renderer.h b/src/libclient/renderer.h
index 7cba0f9ef..df7419768 100644
--- a/src/libclient/renderer.h
+++ b/src/libclient/renderer.h
@@ -61,7 +61,7 @@ public Q_SLOTS:
Q_SIGNALS:
void frameUpdated();
void stopped();
- void started();
+ void started(const QSize& size);
void frameBufferRequested(AVFrame* avFrame);
private:
diff --git a/src/libclient/shmrenderer.cpp b/src/libclient/shmrenderer.cpp
index bdd4eee25..0a2f9b892 100644
--- a/src/libclient/shmrenderer.cpp
+++ b/src/libclient/shmrenderer.cpp
@@ -328,7 +328,7 @@ ShmRenderer::startRendering()
pimpl_->timer->start();
- Q_EMIT started();
+ Q_EMIT started(size());
}
// Done on destroy instead
--
GitLab