From 7de0f46028e964bd7b7979835ef0a80cf8cb01b7 Mon Sep 17 00:00:00 2001 From: Stanislaw Halik Date: Fri, 22 Feb 2019 15:20:52 +0100 Subject: video/widget: fix mutex starvation Get rid of contention in `preview_size' and `set_image'. After switching the Qt mutex to non-recursive, turns out the writer thread preempts the UI thread to the point of freezing the entire thing. Mutex fairness is an implementation detail and we must assume unfair mutexes in the worst case. --- cv/video-widget.cpp | 38 ++++++++++-------------- cv/video-widget.hpp | 2 +- tracker-pt/ftnoir_tracker_pt.cpp | 3 +- video/video-widget.cpp | 64 +++++++++++++++++++++++----------------- video/video-widget.hpp | 31 ++++++++++--------- 5 files changed, 72 insertions(+), 66 deletions(-) diff --git a/cv/video-widget.cpp b/cv/video-widget.cpp index 7240814f..8ff252c9 100644 --- a/cv/video-widget.cpp +++ b/cv/video-widget.cpp @@ -4,15 +4,15 @@ void cv_video_widget::update_image(const cv::Mat& frame) { - QMutexLocker l(&mtx); - - if (freshp) + if (fresh()) return; + auto [ W, H ] = preview_size(); + if (W < 1 || H < 1 || frame.rows < 1 || frame.cols < 1) return; - cv::Mat const* __restrict frame_scaled = nullptr; + cv::Mat const* __restrict scaled = nullptr; if (frame3.cols != W || frame3.rows != H) { @@ -26,22 +26,20 @@ void cv_video_widget::update_image(const cv::Mat& frame) if (frame.cols != W || frame.rows != H) { cv::resize(frame, frame3, { W, H }, 0, 0, cv::INTER_NEAREST); - frame_scaled = &frame3; + scaled = &frame3; } else if (!frame.isContinuous()) { frame.copyTo(frame3); - frame_scaled = &frame3; + scaled = &frame3; } else - frame_scaled = &frame; - - freshp = true; + scaled = &frame; int color_cvt = 0; constexpr int nchannels = 4; - switch (frame_scaled->channels()) + switch (scaled->channels()) { case 1: color_cvt = cv::COLOR_GRAY2BGRA; @@ -56,23 +54,19 @@ void cv_video_widget::update_image(const cv::Mat& frame) break; } - cv::Mat const* frame_color; + cv::Mat const* color; if (color_cvt != cv::COLOR_COLORCVT_MAX) { - cv::cvtColor(*frame_scaled, frame2, color_cvt); - frame_color = &frame2; + cv::cvtColor(*scaled, frame2, color_cvt); + color = &frame2; } else - frame_color = frame_scaled; + color = scaled; - int stride = frame_color->step.p[0], rows = frame_color->rows; - unsigned nbytes = (unsigned)(rows * stride); - vec.resize(nbytes); vec.shrink_to_fit(); - std::memcpy(vec.data(), frame_color->data, nbytes); + int width = color->cols, height = color->rows; + unsigned stride = color->step.p[0]; + set_image(color->data, width, height, stride, QImage::Format_ARGB32); - texture = QImage((const unsigned char*) vec.data(), W, H, stride, QImage::Format_ARGB32); - texture.setDevicePixelRatio(devicePixelRatioF()); + set_fresh(true); } - -cv_video_widget::cv_video_widget(QWidget* parent) : video_widget(parent) {} diff --git a/cv/video-widget.hpp b/cv/video-widget.hpp index 9d62f19e..54316d32 100644 --- a/cv/video-widget.hpp +++ b/cv/video-widget.hpp @@ -12,7 +12,7 @@ struct cv_video_widget final : video_widget { - cv_video_widget(QWidget* parent = nullptr); + using video_widget::video_widget; void update_image(const cv::Mat& frame); private: diff --git a/tracker-pt/ftnoir_tracker_pt.cpp b/tracker-pt/ftnoir_tracker_pt.cpp index a1a0dc1c..a2272d1c 100644 --- a/tracker-pt/ftnoir_tracker_pt.cpp +++ b/tracker-pt/ftnoir_tracker_pt.cpp @@ -101,8 +101,7 @@ void Tracker_PT::run() widget->update_image(preview_frame->get_bitmap()); { - int w = -1, h = -1; - widget->get_preview_size(w, h); + auto [ w, h ] = widget->preview_size(); if (w != preview_width || h != preview_height) { preview_width = w; preview_height = h; diff --git a/video/video-widget.cpp b/video/video-widget.cpp index fb380fc4..4394fec7 100644 --- a/video/video-widget.cpp +++ b/video/video-widget.cpp @@ -3,76 +3,86 @@ #include "compat/check-visible.hpp" #include "compat/math.hpp" -#include #include #include +#include +#include void video_widget::init_image_nolock() { - texture = QImage(W, H, QImage::Format_ARGB32); - texture.setDevicePixelRatio(devicePixelRatioF()); + double dpr = devicePixelRatioF(); + size_.store({ iround(width() * dpr), iround(height() * dpr) }, + std::memory_order_release); } video_widget::video_widget(QWidget* parent) : QWidget(parent) { - W = width(); H = height(); - init_image_nolock(); texture.fill(Qt::gray); - - connect(&timer, &QTimer::timeout, this, &video_widget::update_and_repaint, Qt::DirectConnection); + init_image_nolock(); + connect(&timer, &QTimer::timeout, this, &video_widget::draw_image, Qt::DirectConnection); timer.start(65); } void video_widget::update_image(const QImage& img) { - if (freshp.load(std::memory_order_relaxed)) + if (fresh()) return; + set_image(img.constBits(), img.width(), img.height(), + img.bytesPerLine(), img.format()); + set_fresh(true); +} + +void video_widget::set_image(const unsigned char* src, int width, int height, + unsigned stride, QImage::Format fmt) +{ QMutexLocker l(&mtx); - unsigned nbytes = (unsigned)(img.bytesPerLine() * img.height()); + texture = QImage(); + unsigned nbytes = stride * height; vec.resize(nbytes); vec.shrink_to_fit(); - std::memcpy(vec.data(), img.constBits(), nbytes); - - texture = QImage((const unsigned char*) vec.data(), img.width(), img.height(), img.bytesPerLine(), img.format()); - texture.setDevicePixelRatio(devicePixelRatioF()); - - freshp.store(true, std::memory_order_relaxed); + std::memcpy(vec.data(), src, nbytes); + texture = QImage((const unsigned char*)vec.data(), width, height, stride, fmt); } void video_widget::paintEvent(QPaintEvent*) { - QMutexLocker foo(&mtx); - QPainter painter(this); + + QMutexLocker l(&mtx); painter.drawImage(rect(), texture); } -void video_widget::update_and_repaint() +void video_widget::draw_image() { - if (!freshp.load(std::memory_order_relaxed)) + if (!fresh()) return; if (!check_is_visible()) return; - QMutexLocker l(&mtx); repaint(); - freshp.store(false, std::memory_order_relaxed); + set_fresh(false); } void video_widget::resizeEvent(QResizeEvent*) { QMutexLocker l(&mtx); - double dpr = devicePixelRatioF(); - W = iround(width() * dpr); - H = iround(height() * dpr); init_image_nolock(); } -void video_widget::get_preview_size(int& w, int& h) +std::tuple video_widget::preview_size() const { - QMutexLocker l(&mtx); - w = W; h = H; + QSize sz = size_.load(std::memory_order_acquire); + return { sz.width(), sz.height() }; } +bool video_widget::fresh() const +{ + return fresh_.load(std::memory_order_acquire); +} + +void video_widget::set_fresh(bool x) +{ + fresh_.store(x, std::memory_order_release); +} diff --git a/video/video-widget.hpp b/video/video-widget.hpp index 563f468c..ab5d42d2 100644 --- a/video/video-widget.hpp +++ b/video/video-widget.hpp @@ -11,36 +11,39 @@ #include "export.hpp" #include +#include +#include #include +#include #include + #include -class OTR_VIDEO_EXPORT video_widget : public QWidget +struct OTR_VIDEO_EXPORT video_widget : QWidget { - Q_OBJECT - -public: video_widget(QWidget* parent = nullptr); void update_image(const QImage& image); - void get_preview_size(int& w, int& h); + std::tuple preview_size() const; void resizeEvent(QResizeEvent*) override; -protected slots: void paintEvent(QPaintEvent*) override; - void update_and_repaint(); -private: - QTimer timer; + void draw_image(); protected: - QMutex mtx { QMutex::Recursive }; + mutable QMutex mtx { QMutex::NonRecursive }; QImage texture; std::vector vec; + bool fresh() const; + void set_fresh(bool x); + void set_image(const unsigned char* src, int width, int height, unsigned stride, QImage::Format fmt); - bool freshp = false; +private: + void init_image_nolock(); + QTimer timer; - int W = iround(QWidget::width() * devicePixelRatioF()); - int H = iround(QWidget::height() * devicePixelRatioF()); + std::atomic size_ = QSize(320, 240); + std::atomic fresh_ { false }; - void init_image_nolock(); + static_assert(decltype(fresh_)::is_always_lock_free); }; -- cgit v1.2.3