Prefer using unique_ptr<PixelFrame> over shared_ptr<PixelFrame> (#171)

georges-berenger · facebook-github-bot · commit ecb5db325a2d · 2025-01-23T00:28:36.000-08:00
Summary: Pull Request resolved: #171 shared_ptr<PixelFrame> 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<PixelFrame> and as we're seeing apparent race conditions, unique_ptr<PixelFrame> 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
diff --git 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
@@ -22,10 +22,13 @@
 
 #define DEFAULT_LOG_CHANNEL "FramePlayer"
 #include <logging/Log.h>
+#include <logging/Verify.h>
 
 #include <vrs/DiskFile.h>
 #include <vrs/IndexRecord.h>
 
+#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<PixelFrame> frame = getFrame(true);
+  unique_ptr<PixelFrame> 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<PixelFrame>& frame) {
+void FramePlayer::convertFrame(unique_ptr<PixelFrame>& frame) {
   if (blankMode_) {
     makeBlankFrame(frame);
   } else {
-    shared_ptr<PixelFrame> convertedFrame = needsConvertedFrame_ ? getFrame(false) : nullptr;
-    PixelFrame::normalizeFrame(frame, convertedFrame, false, normalizeOptions_);
-    needsConvertedFrame_ = (frame != convertedFrame); // for next time!
+    unique_ptr<PixelFrame> 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<PixelFrame>& frame) {
+void FramePlayer::makeBlankFrame(unique_ptr<PixelFrame>& frame) {
   frame->init(vrs::PixelFormat::GREY8, frame->getWidth(), frame->getHeight());
   frame->blankFrame();
 }
 
-shared_ptr<PixelFrame> FramePlayer::getFrame(bool inputNotConvertedFrame) {
+unique_ptr<PixelFrame> FramePlayer::getFrame(vrs::PixelFormat format) {
   unique_lock<mutex> lock(frameMutex_);
-  vector<shared_ptr<PixelFrame>>& frames = inputNotConvertedFrame ? inputFrames_ : convertedframes_;
-  if (frames.empty()) {
+  if (recycledFrames_.empty()) {
     return nullptr;
   }
-  shared_ptr<PixelFrame> frame = std::move(frames.back());
-  frames.pop_back();
+  if (recycledFrames_.back()->getPixelFormat() == format) {
+    unique_ptr<PixelFrame> frame = std::move(recycledFrames_.back());
+    recycledFrames_.pop_back();
+    return frame;
+  }
+  unique_ptr<PixelFrame> frame = std::move(recycledFrames_.front());
+  recycledFrames_.pop_front();
   return frame;
 }
 
-void FramePlayer::recycle(shared_ptr<PixelFrame>& frame, bool inputNotConvertedFrame) {
+void FramePlayer::recycle(unique_ptr<PixelFrame>& frame) {
   if (frame) {
     {
       unique_lock<mutex> lock(frameMutex_);
-      vector<shared_ptr<PixelFrame>>& 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<PixelFrame>& frame = *job;
+    if (!XR_VERIFY(state_ != FileReaderState::NoMedia)) {
+      continue;
+    }
+    unique_ptr<PixelFrame>& 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
@@ -51,7 +51,7 @@ using ::vrs::StreamPlayer;
 using ::vrs::utils::PixelFrame;
 using ::vrs::utils::VideoRecordFormatStreamPlayer;
 
-using ImageJob = shared_ptr<PixelFrame>;
+using ImageJob = unique_ptr<PixelFrame>;
 
 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<shared_ptr<PixelFrame>> inputFrames_;
-  vector<shared_ptr<PixelFrame>> convertedframes_;
+  std::deque<unique_ptr<PixelFrame>> 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<std::unique_ptr<ImageJob>> imageJobs_;
 
-  void convertFrame(shared_ptr<PixelFrame>& frame);
-  static void makeBlankFrame(shared_ptr<PixelFrame>& frame);
-  shared_ptr<PixelFrame> getFrame(bool inputNotConvertedFrame);
-  void recycle(shared_ptr<PixelFrame>& frame, bool inputNotConvertedFrame);
+  void convertFrame(unique_ptr<PixelFrame>& frame);
+  static void makeBlankFrame(unique_ptr<PixelFrame>& frame);
+  unique_ptr<PixelFrame> getFrame(vrs::PixelFormat format);
+  void recycle(unique_ptr<PixelFrame>& frame);
   bool saveFrame(const CurrentRecord& record, const ContentBlock& contentBlock);
 };
 
diff --git 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<PixelFrame>& image) {
+void FrameWidget::swapImage(unique_ptr<PixelFrame>& image) {
   unique_lock<mutex> 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
@@ -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<PixelFrame>& image);
+  void swapImage(unique_ptr<PixelFrame>& 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<PixelFrame> image_;
+  unique_ptr<PixelFrame> image_;
   string deviceTypeTag_;
   string deviceTypeConfig_;
   QSize imageSize_{640, 480};
diff --git 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<PixelFrame>& convertedFrame,
+    PixelFrame& convertedFrame,
     PixelFormat targetPixelFormat,
     const NormalizeOptions& options) const {
   return false;
