From fbdc6a195759e2123512f6dc4f7e1e01bd37b3b8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Adrien=20B=C3=A9raud?= <adrien.beraud@savoirfairelinux.com>
Date: Mon, 9 May 2022 17:33:32 -0400
Subject: [PATCH] video receive thread: fix race condition on displayMatrix_
Instances of std::shared_ptr are not thread-safe, even if the control block is.
Since AVBufer already has a refcounting system, use a unique_ptr instead,
and add a mutex to protect access during copy.
In practice, the mutex should almost never block since it's locked for
the minimum scope.
Change-Id: I5122e76dadb5da6c3738f8dc711698744b52315b
---
src/media/libav_utils.h | 9 +++++++++
src/media/video/video_receive_thread.cpp | 22 ++++++++++++----------
src/media/video/video_receive_thread.h | 4 +++-
3 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/media/libav_utils.h b/src/media/libav_utils.h
index 942933c95c..6e91a785f8 100644
--- a/src/media/libav_utils.h
+++ b/src/media/libav_utils.h
@@ -24,11 +24,14 @@
#include <vector>
#include <map>
#include <string>
+#include <memory>
extern "C" {
struct AVDictionary;
struct AVFrame;
struct AVPixFmtDescriptor;
+struct AVBufferRef;
+void av_buffer_unref(AVBufferRef **buf);
}
namespace jami {
@@ -51,5 +54,11 @@ void fillWithBlack(AVFrame* frame);
void fillWithSilence(AVFrame* frame);
+struct AVBufferRef_deleter {
+ void operator()(AVBufferRef* buf) const { av_buffer_unref(&buf); }
+};
+
+typedef std::unique_ptr<AVBufferRef, AVBufferRef_deleter> AVBufferPtr;
+
} // namespace libav_utils
} // namespace jami
diff --git a/src/media/video/video_receive_thread.cpp b/src/media/video/video_receive_thread.cpp
index e86c35f86c..42274a8f9e 100644
--- a/src/media/video/video_receive_thread.cpp
+++ b/src/media/video/video_receive_thread.cpp
@@ -92,10 +92,16 @@ VideoReceiveThread::setup()
JAMI_DBG("[%p] Setupping video receiver", this);
videoDecoder_.reset(new MediaDecoder([this](const std::shared_ptr<MediaFrame>& frame) mutable {
- if (auto displayMatrix = displayMatrix_)
+ libav_utils::AVBufferPtr displayMatrix;
+ {
+ std::lock_guard<std::mutex> l(rotationMtx_);
+ if (displayMatrix_)
+ displayMatrix.reset(av_buffer_ref(displayMatrix_.get()));
+ }
+ if (displayMatrix)
av_frame_new_side_data_from_buf(frame->pointer(),
AV_FRAME_DATA_DISPLAYMATRIX,
- av_buffer_ref(displayMatrix.get()));
+ displayMatrix.release());
publishFrame(std::static_pointer_cast<VideoFrame>(frame));
}));
videoDecoder_->setResolutionChangedCallback([this](int width, int height) {
@@ -303,14 +309,10 @@ VideoReceiveThread::getInfo() const
void
VideoReceiveThread::setRotation(int angle)
{
- std::shared_ptr<AVBufferRef> displayMatrix {av_buffer_alloc(sizeof(int32_t) * 9),
- [](AVBufferRef* buf) {
- av_buffer_unref(&buf);
- }};
- if (displayMatrix) {
- av_display_rotation_set(reinterpret_cast<int32_t*>(displayMatrix->data), angle);
- displayMatrix_ = std::move(displayMatrix);
- }
+ libav_utils::AVBufferPtr displayMatrix(av_buffer_alloc(sizeof(int32_t) * 9));
+ av_display_rotation_set(reinterpret_cast<int32_t*>(displayMatrix->data), angle);
+ std::lock_guard<std::mutex> l(rotationMtx_);
+ displayMatrix_ = std::move(displayMatrix);
}
} // namespace video
diff --git a/src/media/video/video_receive_thread.h b/src/media/video/video_receive_thread.h
index c4b156c683..b1b1998a1f 100644
--- a/src/media/video/video_receive_thread.h
+++ b/src/media/video/video_receive_thread.h
@@ -31,6 +31,7 @@
#include "media_stream.h"
#include "threadloop.h"
#include "noncopyable.h"
+#include "libav_utils.h"
#include <functional>
#include <map>
@@ -106,7 +107,8 @@ private:
uint16_t mtu_;
int rotation_ {0};
- std::shared_ptr<AVBufferRef> displayMatrix_;
+ std::mutex rotationMtx_;
+ libav_utils::AVBufferPtr displayMatrix_;
static int interruptCb(void* ctx);
static int readFunction(void* opaque, uint8_t* buf, int buf_size);
--
GitLab