From 12ec77a21c10c8bfefd0e60eea3ce7e486027bc8 Mon Sep 17 00:00:00 2001
From: Alexandre Lision <alexandre.lision@savoirfairelinux.com>
Date: Fri, 9 Oct 2015 14:10:08 -0400
Subject: [PATCH] video: fix concurrent access in sink direct rendering
This patchset prevents client from reading a frame that is being written
by the daemon using a push/pull mechanism.
**WARNING** This breaks the direct rendering API
Issue: #81542
Change-Id: I55d4ee9b986ac682903b326fd86c32d8f5f68825
Signed-off-by: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
---
src/client/videomanager.cpp | 25 +++++--------------------
src/dring/videomanager_interface.h | 24 ++++++++++++++++++++++--
src/media/media_buffer.cpp | 3 ++-
src/media/media_buffer.h | 6 +++++-
src/media/video/shm_header.h | 3 ++-
src/media/video/sinkclient.cpp | 18 +++++++++++-------
src/media/video/sinkclient.h | 12 ++++++------
7 files changed, 53 insertions(+), 38 deletions(-)
diff --git a/src/client/videomanager.cpp b/src/client/videomanager.cpp
index d07bd917fb..009e8130d5 100644
--- a/src/client/videomanager.cpp
+++ b/src/client/videomanager.cpp
@@ -138,28 +138,13 @@ hasCameraStarted()
return videoManager.started;
}
-template <class T>
-static void
-registerSinkTarget_(const std::string& sinkId, std::vector<unsigned char>& frameBuffer, T&& cb)
-{
- if (auto sink = ring::Manager::instance().getSinkClient(sinkId))
- sink->registerTarget(std::forward<T>(cb), frameBuffer);
- else
- RING_WARN("No sink found for id '%s'", sinkId.c_str());
-}
-
-void
-registerSinkTarget(const std::string& sinkId, std::vector<unsigned char>& frameBuffer,
- const std::function<void(int, int)>& cb)
-{
- registerSinkTarget_(sinkId, frameBuffer, cb);
-}
-
void
-registerSinkTarget(const std::string& sinkId, std::vector<unsigned char>& frameBuffer,
- std::function<void(int, int)>&& cb)
+registerSinkTarget(const std::string& sinkId, const SinkTarget& target)
{
- registerSinkTarget_(sinkId, frameBuffer, cb);
+ if (auto sink = ring::Manager::instance().getSinkClient(sinkId))
+ sink->registerTarget(target);
+ else
+ RING_WARN("No sink found for id '%s'", sinkId.c_str());
}
#ifdef __ANDROID__
diff --git a/src/dring/videomanager_interface.h b/src/dring/videomanager_interface.h
index 1827f33b84..13eef20cfd 100644
--- a/src/dring/videomanager_interface.h
+++ b/src/dring/videomanager_interface.h
@@ -1,6 +1,8 @@
/*
* Copyright (C) 2012-2015 Savoir-faire Linux Inc.
*
+ * Author: Guillaume Roguez <guillaume.roguez@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
@@ -24,11 +26,29 @@
#include <map>
#include <string>
#include <functional>
+#include <cstdint>
+#include <cstdlib>
#include "dring.h"
namespace DRing {
+/* FrameBuffer is a generic video frame container */
+struct FrameBuffer {
+ uint8_t* ptr {nullptr}; // data as a plain raw pointer
+ std::size_t ptrSize {0}; // size in byte of ptr array
+ int format {0}; // as listed by AVPixelFormat (avutils/pixfmt.h)
+ int width {0}; // frame width
+ int height {0}; // frame height
+ std::vector<uint8_t> storage;
+};
+
+struct SinkTarget {
+ using FrameBufferPtr = std::unique_ptr<FrameBuffer>;
+ std::function<FrameBufferPtr(std::size_t bytes)> pull;
+ std::function<void(FrameBufferPtr)> push;
+};
+
using VideoCapabilities = std::map<std::string, std::map<std::string, std::vector<std::string>>>;
void registerVideoHandlers(const std::map<std::string, std::shared_ptr<CallbackWrapperBase>>&);
@@ -48,8 +68,8 @@ void stopCamera();
bool hasCameraStarted();
bool switchInput(const std::string& resource);
bool switchToCamera();
-void registerSinkTarget(const std::string& sinkId, std::vector<unsigned char>& frameBuffer, const std::function<void(int, int)>& cb);
-void registerSinkTarget(const std::string& sinkId, std::vector<unsigned char>& frameBuffer, std::function<void(int, int)>&& cb);
+void registerSinkTarget(const std::string& sinkId, const SinkTarget& target);
+
#ifdef __ANDROID__
void addVideoDevice(const std::string &node);
void removeVideoDevice(const std::string &node);
diff --git a/src/media/media_buffer.cpp b/src/media/media_buffer.cpp
index 589ed96e4c..ac8ded9301 100644
--- a/src/media/media_buffer.cpp
+++ b/src/media/media_buffer.cpp
@@ -20,6 +20,7 @@
#include "libav_deps.h" // MUST BE INCLUDED FIRST
#include "media_buffer.h"
+#include "dring/videomanager_interface.h"
#include <new>
#include <cstdlib>
@@ -107,7 +108,7 @@ VideoFrame::reserve(int format, int width, int height)
}
void
-VideoFrame::setFromMemory(void* ptr, int format, int width, int height) noexcept
+VideoFrame::setFromMemory(uint8_t* ptr, int format, int width, int height)
{
reset();
setGeometry(format, width, height);
diff --git a/src/media/media_buffer.h b/src/media/media_buffer.h
index 359cd2583f..106597cc00 100644
--- a/src/media/media_buffer.h
+++ b/src/media/media_buffer.h
@@ -26,6 +26,10 @@
class AVFrame;
+namespace DRing {
+class FrameBuffer; // from dring/videomanager_interface.h
+}
+
namespace ring {
class MediaFrame {
@@ -74,7 +78,7 @@ class VideoFrame: public MediaFrame {
// Set internal pixel buffers on given memory buffer
// This buffer must follow given specifications.
- void setFromMemory(void* ptr, int format, int width, int height) noexcept;
+ void setFromMemory(uint8_t* data, int format, int width, int height);
void noise();
diff --git a/src/media/video/shm_header.h b/src/media/video/shm_header.h
index 594913d1fe..9811a17a17 100644
--- a/src/media/video/shm_header.h
+++ b/src/media/video/shm_header.h
@@ -21,6 +21,7 @@
#ifndef SHM_HEADER_H_
#define SHM_HEADER_H_
+#include <cstdint>
#include <semaphore.h>
// Implementation note: double-buffering
@@ -37,7 +38,7 @@ struct SHMHeader {
unsigned mapSize; // size to map if you need to see all data
unsigned readOffset; // offset of readable frame in data
unsigned writeOffset; // offset of writable frame in data
- char data[]; // the whole shared memory
+ uint8_t data[]; // the whole shared memory
};
#endif
diff --git a/src/media/video/sinkclient.cpp b/src/media/video/sinkclient.cpp
index 84d13c78a7..0b9e584290 100644
--- a/src/media/video/sinkclient.cpp
+++ b/src/media/video/sinkclient.cpp
@@ -3,6 +3,7 @@
*
* Author: Tristan Matthews <tristan.matthews@savoirfairelinux.com>
* Author: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
+ * Author: Alexandre Lision <alexandre.lision@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
@@ -35,6 +36,7 @@
#include "noncopyable.h"
#include "client/ring_signal.h"
#include "dring/videomanager_interface.h"
+#include "libav_utils.h"
#ifndef _WIN32
#include <sys/mman.h>
@@ -323,7 +325,7 @@ SinkClient::update(Observable<std::shared_ptr<VideoFrame>>* /*obs*/,
shm_->renderFrame(*f.get());
#endif
- if (targetData_) {
+ if (target_.pull) {
VideoFrame dst;
VideoScaler scaler;
const int width = f->width();
@@ -336,12 +338,14 @@ SinkClient::update(Observable<std::shared_ptr<VideoFrame>>* /*obs*/,
const auto bytes = videoFrameSize(format, width, height);
if (bytes > 0) {
- targetData_->resize(bytes);
- auto data = targetData_->data();
-
- dst.setFromMemory(data, format, width, height);
- scaler.scale(*f, dst);
- target_(width, height);
+ if (auto buffer_ptr = target_.pull(bytes)) {
+ buffer_ptr->format = libav_utils::libav_pixel_format(format);
+ buffer_ptr->width = width;
+ buffer_ptr->height = height;
+ dst.setFromMemory(buffer_ptr->ptr, format, width, height);
+ scaler.scale(*f, dst);
+ target_.push(std::move(buffer_ptr));
+ }
}
}
}
diff --git a/src/media/video/sinkclient.h b/src/media/video/sinkclient.h
index b446422fa9..91168a238c 100644
--- a/src/media/video/sinkclient.h
+++ b/src/media/video/sinkclient.h
@@ -2,6 +2,8 @@
* Copyright (C) 2012-2015 Savoir-faire Linux Inc.
*
* Author: Tristan Matthews <tristan.matthews@savoirfairelinux.com>
+ * Author: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
+ * Author: Alexandre Lision <alexandre.lision@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
@@ -25,6 +27,7 @@
#endif
#include "video_base.h"
+#include <videomanager_interface.h>
#include <string>
#include <vector>
@@ -56,18 +59,15 @@ class SinkClient : public VideoFramePassiveReader
void setFrameSize(int width, int height);
- template <class T>
- void registerTarget(T&& cb, std::vector<unsigned char>& frameBuffer) noexcept {
- target_ = std::forward<T>(cb);
- targetData_ = &frameBuffer;
+ void registerTarget(const DRing::SinkTarget& target) noexcept {
+ target_ = target;
}
private:
const std::string id_;
const bool mixer_;
bool started_ {false}; // used to arbitrate client's stop signal.
- std::function<void(int, int)> target_;
- std::vector<unsigned char>* targetData_ {nullptr}; // filled by registerTarget, user owned
+ DRing::SinkTarget target_;
#ifdef DEBUG_FPS
unsigned frameCount_;
--
GitLab