From ecb5db325a2da8568a85a24d098b57702231b4f3 Mon Sep 17 00:00:00 2001 From: Georges Berenger Date: Thu, 23 Jan 2025 00:28:36 -0800 Subject: [PATCH] Prefer using unique_ptr over shared_ptr (#171) Summary: Pull Request resolved: https://github.com/facebookresearch/vrs/pull/171 shared_ptr can look convenient, but it's heavier and riskier, in particular when multhreading is involved, as potential shared ownership (in particular accidental shared ownership) can make using/modifying objects unsafe. We don't have a good reason to use shared_ptr and as we're seeing apparent race conditions, unique_ptr is a better approach. Note: this doesn't resolve the issue we're hunting down, but it closes one possible explanation. Differential Revision: D68521262 fbshipit-source-id: 692e6de7da2367de7786bd68fa739344e6a559d4 --- tools/vrsplayer/FileReader.cpp | 1 + tools/vrsplayer/FramePlayer.cpp | 60 ++++++++++++++++++++++----------- tools/vrsplayer/FramePlayer.h | 13 ++++--- tools/vrsplayer/FrameWidget.cpp | 18 +++++----- tools/vrsplayer/FrameWidget.h | 7 ++-- vrs/utils/PixelFrame.cpp | 2 +- 6 files changed, 61 insertions(+), 40 deletions(-) diff --git a/tools/vrsplayer/FileReader.cpp b/tools/vrsplayer/FileReader.cpp index 40284d54..6b915997 100644 --- a/tools/vrsplayer/FileReader.cpp +++ b/tools/vrsplayer/FileReader.cpp @@ -144,6 +144,7 @@ bool FileReader::isAtEnd() const { void FileReader::closeFile() { stop(); + setState(FileReaderState::NoMedia); if (fileConfig_) { saveConfiguration(); fileConfig_.reset(); diff --git a/tools/vrsplayer/FramePlayer.cpp b/tools/vrsplayer/FramePlayer.cpp index 38fad014..d0dcdd25 100644 --- a/tools/vrsplayer/FramePlayer.cpp +++ b/tools/vrsplayer/FramePlayer.cpp @@ -22,10 +22,13 @@ #define DEFAULT_LOG_CHANNEL "FramePlayer" #include +#include #include #include +#include "FileReader.h" + namespace vrsp { using namespace std; @@ -44,6 +47,9 @@ bool FramePlayer::onDataLayoutRead( const CurrentRecord& record, size_t blockIndex, DataLayout& layout) { + if (!XR_VERIFY(state_ != FileReaderState::NoMedia)) { + return false; + } ostringstream buffer; layout.printLayoutCompact(buffer); string text = buffer.str(); @@ -62,12 +68,15 @@ bool FramePlayer::onImageRead( const CurrentRecord& record, size_t /*blockIndex*/, const ContentBlock& contentBlock) { + if (!XR_VERIFY(state_ != FileReaderState::NoMedia)) { + return false; + } if (!saveNextFramePath_.empty()) { return saveFrame(record, contentBlock); } widget_->setDataFps(dataFps_.newFrame()); // fps counter for images read from file const auto& spec = contentBlock.image(); - shared_ptr frame = getFrame(true); + unique_ptr frame = getFrame(spec.getPixelFormat()); bool frameValid = false; imageFormat_ = spec.getImageFormat(); if (imageFormat_ == vrs::ImageFormat::VIDEO) { @@ -132,7 +141,7 @@ bool FramePlayer::onImageRead( widget_->swapImage(frame); } } - recycle(frame, !needsConvertedFrame_); + recycle(frame); return true; // read next blocks, if any } @@ -141,44 +150,52 @@ void FramePlayer::setVisible(bool visible) { widget_->setVisible(visible_); } -void FramePlayer::convertFrame(shared_ptr& frame) { +void FramePlayer::convertFrame(unique_ptr& frame) { if (blankMode_) { makeBlankFrame(frame); } else { - shared_ptr convertedFrame = needsConvertedFrame_ ? getFrame(false) : nullptr; - PixelFrame::normalizeFrame(frame, convertedFrame, false, normalizeOptions_); - needsConvertedFrame_ = (frame != convertedFrame); // for next time! + unique_ptr convertedFrame = + needsConvertedFrame_ ? getFrame(frame->getPixelFormat()) : nullptr; + needsConvertedFrame_ = + frame->normalizeFrame(PixelFrame::make(convertedFrame), false, normalizeOptions_); if (needsConvertedFrame_) { - recycle(frame, true); + recycle(frame); frame = std::move(convertedFrame); } } } -void FramePlayer::makeBlankFrame(shared_ptr& frame) { +void FramePlayer::makeBlankFrame(unique_ptr& frame) { frame->init(vrs::PixelFormat::GREY8, frame->getWidth(), frame->getHeight()); frame->blankFrame(); } -shared_ptr FramePlayer::getFrame(bool inputNotConvertedFrame) { +unique_ptr FramePlayer::getFrame(vrs::PixelFormat format) { unique_lock lock(frameMutex_); - vector>& frames = inputNotConvertedFrame ? inputFrames_ : convertedframes_; - if (frames.empty()) { + if (recycledFrames_.empty()) { return nullptr; } - shared_ptr frame = std::move(frames.back()); - frames.pop_back(); + if (recycledFrames_.back()->getPixelFormat() == format) { + unique_ptr frame = std::move(recycledFrames_.back()); + recycledFrames_.pop_back(); + return frame; + } + unique_ptr frame = std::move(recycledFrames_.front()); + recycledFrames_.pop_front(); return frame; } -void FramePlayer::recycle(shared_ptr& frame, bool inputNotConvertedFrame) { +void FramePlayer::recycle(unique_ptr& frame) { if (frame) { { unique_lock lock(frameMutex_); - vector>& frames = - inputNotConvertedFrame ? inputFrames_ : convertedframes_; - if (frames.size() < 10) { - frames.emplace_back(std::move(frame)); + if (recycledFrames_.size() < 10) { + if (recycledFrames_.empty() || + recycledFrames_.back()->getPixelFormat() == frame->getPixelFormat()) { + recycledFrames_.emplace_back(std::move(frame)); + } else { + recycledFrames_.emplace_front(std::move(frame)); + } } } frame.reset(); @@ -267,7 +284,10 @@ void FramePlayer::imageJobsThreadActivity() { while (imageJobs_.getJob(job)) { ; // just skip! } - shared_ptr& frame = *job; + if (!XR_VERIFY(state_ != FileReaderState::NoMedia)) { + continue; + } + unique_ptr& frame = *job; bool frameValid = false; vrs::ImageFormat imageFormat = frame->getImageFormat(); if (imageFormat == vrs::ImageFormat::RAW) { @@ -283,7 +303,7 @@ void FramePlayer::imageJobsThreadActivity() { widget_->swapImage(frame); } if (imageFormat != vrs::ImageFormat::VIDEO) { - recycle(frame, !frameValid || !needsConvertedFrame_); + recycle(frame); } } } diff --git a/tools/vrsplayer/FramePlayer.h b/tools/vrsplayer/FramePlayer.h index bb1781bd..a405b351 100644 --- a/tools/vrsplayer/FramePlayer.h +++ b/tools/vrsplayer/FramePlayer.h @@ -51,7 +51,7 @@ using ::vrs::StreamPlayer; using ::vrs::utils::PixelFrame; using ::vrs::utils::VideoRecordFormatStreamPlayer; -using ImageJob = shared_ptr; +using ImageJob = unique_ptr; class FramePlayer : public QObject, public VideoRecordFormatStreamPlayer { Q_OBJECT @@ -91,8 +91,7 @@ class FramePlayer : public QObject, public VideoRecordFormatStreamPlayer { private: std::mutex videoDecodingMutex_; std::mutex frameMutex_; - vector> inputFrames_; - vector> convertedframes_; + std::deque> recycledFrames_; vrs::utils::NormalizeOptions normalizeOptions_; bool needsConvertedFrame_{false}; vrs::ImageFormat imageFormat_{vrs::ImageFormat::UNDEFINED}; @@ -110,10 +109,10 @@ class FramePlayer : public QObject, public VideoRecordFormatStreamPlayer { vrs::JobQueueWithThread> imageJobs_; - void convertFrame(shared_ptr& frame); - static void makeBlankFrame(shared_ptr& frame); - shared_ptr getFrame(bool inputNotConvertedFrame); - void recycle(shared_ptr& frame, bool inputNotConvertedFrame); + void convertFrame(unique_ptr& frame); + static void makeBlankFrame(unique_ptr& frame); + unique_ptr getFrame(vrs::PixelFormat format); + void recycle(unique_ptr& frame); bool saveFrame(const CurrentRecord& record, const ContentBlock& contentBlock); }; diff --git a/tools/vrsplayer/FrameWidget.cpp b/tools/vrsplayer/FrameWidget.cpp index 4b046e50..7d972537 100644 --- a/tools/vrsplayer/FrameWidget.cpp +++ b/tools/vrsplayer/FrameWidget.cpp @@ -71,6 +71,8 @@ FrameWidget::FrameWidget(QWidget* parent) { connect(this, &FrameWidget::customContextMenuRequested, this, &FrameWidget::ShowContextMenu); } +FrameWidget::~FrameWidget() = default; + void FrameWidget::paintEvent(QPaintEvent* evt) { QPainter painter(this); painter.setRenderHint(QPainter::Antialiasing); @@ -176,7 +178,7 @@ int FrameWidget::heightForWidth(int w) const { return (w * size.height()) / size.width(); } -void FrameWidget::swapImage(shared_ptr& image) { +void FrameWidget::swapImage(unique_ptr& image) { unique_lock lock; imageFps_.newFrame(); bool resize = image && image->qsize() != imageSize_; @@ -198,17 +200,15 @@ int FrameWidget::saveImage(const std::string& path) { } void FrameWidget::updateMinMaxSize() { - if (image_) { - QSize size = getImageSize(); - setMinimumSize(size.scaled(100, 100, Qt::KeepAspectRatio)); - setBaseSize(size); + QSize size = getImageSize(); + setMinimumSize(size.scaled(100, 100, Qt::KeepAspectRatio)); + setBaseSize(size); #if QT_VERSION < QT_VERSION_CHECK(5, 14, 0) - QSize screenSize = QApplication::desktop()->screenGeometry(this).size().operator*=(0.95); + QSize screenSize = QApplication::desktop()->screenGeometry(this).size().operator*=(0.95); #else - QSize screenSize = screen()->geometry().size() * 0.95; + QSize screenSize = screen()->geometry().size() * 0.95; #endif - setMaximumSize(size.scaled(screenSize, Qt::KeepAspectRatio)); - } + setMaximumSize(size.scaled(screenSize, Qt::KeepAspectRatio)); } void FrameWidget::ShowContextMenu(const QPoint& pos) { diff --git a/tools/vrsplayer/FrameWidget.h b/tools/vrsplayer/FrameWidget.h index e417707c..89255b9d 100644 --- a/tools/vrsplayer/FrameWidget.h +++ b/tools/vrsplayer/FrameWidget.h @@ -37,8 +37,8 @@ class PixelFrame; namespace vrsp { using std::map; -using std::shared_ptr; using std::string; +using std::unique_ptr; using vrs::utils::PixelFrame; // Frame Per Second estimator class @@ -78,6 +78,7 @@ class FrameWidget : public QWidget { Q_OBJECT public: explicit FrameWidget(QWidget* parent = nullptr); + ~FrameWidget() override; void paintEvent(QPaintEvent* evt) override; QSize sizeHint() const override; @@ -116,7 +117,7 @@ class FrameWidget : public QWidget { return flipped_; } - void swapImage(shared_ptr& image); + void swapImage(unique_ptr& image); int saveImage(const string& path); void setTypeToShow(Record::Type type) { @@ -154,7 +155,7 @@ class FrameWidget : public QWidget { private: std::mutex mutex_; - shared_ptr image_; + unique_ptr image_; string deviceTypeTag_; string deviceTypeConfig_; QSize imageSize_{640, 480}; diff --git a/vrs/utils/PixelFrame.cpp b/vrs/utils/PixelFrame.cpp index e782bb71..f97ae894 100644 --- a/vrs/utils/PixelFrame.cpp +++ b/vrs/utils/PixelFrame.cpp @@ -1035,7 +1035,7 @@ PixelFrame::getStreamNormalizeOptions(RecordFileReader& reader, StreamId id, Pix #if IS_VRS_OSS_CODE() bool PixelFrame::normalizeToPixelFormat( - shared_ptr& convertedFrame, + PixelFrame& convertedFrame, PixelFormat targetPixelFormat, const NormalizeOptions& options) const { return false;