From 86905c5a294a384802110a992fbe032dc09d8728 Mon Sep 17 00:00:00 2001 From: Raphael Dumusc Date: Mon, 24 Sep 2018 16:39:22 +0200 Subject: [PATCH] Fix segfault affecting resizing of large streams with small tiles Concurrent getTileImage() calls for the same tileIndex can occur unexpectedly when resizing a stream window. Adding a per-tile mutex is the simplest way to fix the problem and should not penalize performance too much as those events are rare. Returning an empty ImagePtr does not work (the stream blocks). --- doc/Changelog.md | 2 + tide/wall/datasources/PixelStreamUpdater.cpp | 42 ++++++++++++------- tide/wall/datasources/PixelStreamUpdater.h | 2 + tide/wall/tools/PixelStreamAssembler.cpp | 12 +++++- tide/wall/tools/PixelStreamAssembler.h | 3 ++ .../tools/PixelStreamChannelAssembler.cpp | 2 +- tide/wall/tools/PixelStreamChannelAssembler.h | 4 +- tide/wall/tools/PixelStreamPassthrough.cpp | 7 +++- tide/wall/tools/PixelStreamPassthrough.h | 3 ++ tide/wall/tools/PixelStreamProcessor.h | 3 ++ 10 files changed, 60 insertions(+), 20 deletions(-) diff --git a/doc/Changelog.md b/doc/Changelog.md index a9444a29..1fdb2a4d 100644 --- a/doc/Changelog.md +++ b/doc/Changelog.md @@ -3,6 +3,8 @@ Changelog {#changelog} # Release 1.5 (git master) +* [275](https://github.com/BlueBrain/Tide/pull/275): + Fix a crash that could occur when resizing large pixel streams. * [272](https://github.com/BlueBrain/Tide/pull/272): - Smooth transitions between resolution levels for opaque contents (PDFs, regular TIFF pyramids). diff --git a/tide/wall/datasources/PixelStreamUpdater.cpp b/tide/wall/datasources/PixelStreamUpdater.cpp index 47597496..19150b6a 100644 --- a/tide/wall/datasources/PixelStreamUpdater.cpp +++ b/tide/wall/datasources/PixelStreamUpdater.cpp @@ -91,24 +91,28 @@ const QString& PixelStreamUpdater::getUri() const ImagePtr PixelStreamUpdater::getTileImage(const uint tileIndex, const deflect::View view) const { - if (!_frameLeftOrMono) - { - print_log(LOG_ERROR, LOG_STREAM, "No frames yet"); - return ImagePtr(); - } - // guard against frame swap during asynchronous readings const QReadLocker frameLock(&_frameMutex); - const bool rightEye = view == deflect::View::right_eye; - const bool rightFrame = rightEye && !_frameRight->tiles.empty(); - const auto& processor = rightFrame ? _processRight : _processorLeft; - - // turbojpeg handles need to be per thread, and this function may be - // called from multiple threads - static QThreadStorage tileDecoders; try { + if (!_frameLeftOrMono) + throw std::runtime_error("No frames yet"); + + if (tileIndex >= _perTileLock->size()) + throw std::runtime_error("Tile index is invalid"); + + // prevent double-decoding of a tile that could occur unexpectedly when + // resizing the stream window + std::lock_guard lock{_perTileLock->at(tileIndex)}; + + const bool rightEye = view == deflect::View::right_eye; + const bool rightFrame = rightEye && !_frameRight->tiles.empty(); + const auto& processor = rightFrame ? _processRight : _processorLeft; + + // turbojpeg handles need to be per thread, and this function may be + // called from multiple threads + static QThreadStorage tileDecoders; return processor->getTileImage(tileIndex, tileDecoders.localData()); } catch (const std::runtime_error& e) @@ -140,10 +144,8 @@ Indices PixelStreamUpdater::computeVisibleSet(const QRectF& visibleTilesArea, const uint channel) const { Q_UNUSED(lod); - if (!_frameLeftOrMono || visibleTilesArea.isEmpty()) return Indices{}; - return _processorLeft->computeVisibleSet(visibleTilesArea, channel); } @@ -190,6 +192,7 @@ void PixelStreamUpdater::_onFrameSwapped(deflect::server::FramePtr frame) _frameLeftOrMono = std::move(leftOrMono); _frameRight = std::move(right); _createFrameProcessors(); + _createPerTileMutexes(); } emit pictureUpdated(); @@ -216,3 +219,12 @@ void PixelStreamUpdater::_createFrameProcessors() _processRight.reset(new PixelStreamPassthrough(_frameRight)); } } + +void PixelStreamUpdater::_createPerTileMutexes() +{ + const auto tiles = (_processorLeft ? _processorLeft->getTilesCount() : 0) + + (_processRight ? _processRight->getTilesCount() : 0); + + if (!_perTileLock || _perTileLock->size() < tiles) + _perTileLock = std::make_unique>(tiles); +} diff --git a/tide/wall/datasources/PixelStreamUpdater.h b/tide/wall/datasources/PixelStreamUpdater.h index ca08e964..225d6799 100644 --- a/tide/wall/datasources/PixelStreamUpdater.h +++ b/tide/wall/datasources/PixelStreamUpdater.h @@ -113,10 +113,12 @@ class PixelStreamUpdater : public QObject, public DataSource std::unique_ptr _processorLeft; std::unique_ptr _processRight; mutable QReadWriteLock _frameMutex; + mutable std::unique_ptr> _perTileLock; bool _readyToSwap = true; void _onFrameSwapped(deflect::server::FramePtr frame); void _createFrameProcessors(); + void _createPerTileMutexes(); }; #endif diff --git a/tide/wall/tools/PixelStreamAssembler.cpp b/tide/wall/tools/PixelStreamAssembler.cpp index 8f20e591..d53d922c 100644 --- a/tide/wall/tools/PixelStreamAssembler.cpp +++ b/tide/wall/tools/PixelStreamAssembler.cpp @@ -39,6 +39,8 @@ #include "PixelStreamAssembler.h" +#include // std::accumulate + namespace { Indices _mapToGlobalIndices(const Indices& perChannelIndices, @@ -58,7 +60,7 @@ PixelStreamAssembler::PixelStreamAssembler(deflect::server::FramePtr frame) } ImagePtr PixelStreamAssembler::getTileImage( - uint tileIndex, deflect::server::TileDecoder& decoder) + const uint tileIndex, deflect::server::TileDecoder& decoder) { const auto& channel = _getChannel(tileIndex); return channel.assembler.getTileImage(tileIndex - channel.offset, decoder); @@ -79,6 +81,14 @@ Indices PixelStreamAssembler::computeVisibleSet(const QRectF& visibleArea, return _mapToGlobalIndices(indices, channel.offset); } +size_t PixelStreamAssembler::getTilesCount() const +{ + return std::accumulate(_channels.begin(), _channels.end(), 0u, + [](const auto count, const Channel& channel) { + return count + channel.tilesCount; + }); +} + bool PixelStreamAssembler::_parseChannels(deflect::server::FramePtr frame) { if (frame->tiles.empty()) diff --git a/tide/wall/tools/PixelStreamAssembler.h b/tide/wall/tools/PixelStreamAssembler.h index 16ce6e91..135ca5c7 100644 --- a/tide/wall/tools/PixelStreamAssembler.h +++ b/tide/wall/tools/PixelStreamAssembler.h @@ -77,6 +77,9 @@ class PixelStreamAssembler : public PixelStreamProcessor Indices computeVisibleSet(const QRectF& visibleArea, uint channel) const final; + /** @copydoc PixelStreamProcessor::getTilesCount */ + size_t getTilesCount() const final; + private: struct Channel { diff --git a/tide/wall/tools/PixelStreamChannelAssembler.cpp b/tide/wall/tools/PixelStreamChannelAssembler.cpp index b3772f5e..6e2a0251 100644 --- a/tide/wall/tools/PixelStreamChannelAssembler.cpp +++ b/tide/wall/tools/PixelStreamChannelAssembler.cpp @@ -130,7 +130,7 @@ Indices PixelStreamChannelAssembler::computeVisibleSet( return visibleSet; } -uint PixelStreamChannelAssembler::getTilesCount() const +size_t PixelStreamChannelAssembler::getTilesCount() const { return _getTilesX() * _getTilesY(); } diff --git a/tide/wall/tools/PixelStreamChannelAssembler.h b/tide/wall/tools/PixelStreamChannelAssembler.h index f525379c..1ab2c6f2 100644 --- a/tide/wall/tools/PixelStreamChannelAssembler.h +++ b/tide/wall/tools/PixelStreamChannelAssembler.h @@ -78,8 +78,8 @@ class PixelStreamChannelAssembler : public PixelStreamProcessor Indices computeVisibleSet(const QRectF& visibleArea, uint channel) const final; - /** @return the number of tiles in the assembled image. */ - uint getTilesCount() const; + /** @copydoc PixelStreamProcessor::getTilesCount */ + size_t getTilesCount() const final; private: deflect::server::FramePtr _frame; diff --git a/tide/wall/tools/PixelStreamPassthrough.cpp b/tide/wall/tools/PixelStreamPassthrough.cpp index 3a4926ef..017e1700 100644 --- a/tide/wall/tools/PixelStreamPassthrough.cpp +++ b/tide/wall/tools/PixelStreamPassthrough.cpp @@ -45,7 +45,7 @@ #include PixelStreamPassthrough::PixelStreamPassthrough(deflect::server::FramePtr frame) - : _frame(frame) + : _frame{std::move(frame)} { } @@ -81,3 +81,8 @@ Indices PixelStreamPassthrough::computeVisibleSet(const QRectF& visibleArea, } return visibleSet; } + +size_t PixelStreamPassthrough::getTilesCount() const +{ + return _frame->tiles.size(); +} diff --git a/tide/wall/tools/PixelStreamPassthrough.h b/tide/wall/tools/PixelStreamPassthrough.h index e4b4f7bf..c3157c63 100644 --- a/tide/wall/tools/PixelStreamPassthrough.h +++ b/tide/wall/tools/PixelStreamPassthrough.h @@ -65,6 +65,9 @@ class PixelStreamPassthrough : public PixelStreamProcessor Indices computeVisibleSet(const QRectF& visibleArea, uint channel) const final; + /** @copydoc PixelStreamProcessor::getTilesCount */ + size_t getTilesCount() const final; + private: deflect::server::FramePtr _frame; }; diff --git a/tide/wall/tools/PixelStreamProcessor.h b/tide/wall/tools/PixelStreamProcessor.h index 2c7ec948..8d96e7e4 100644 --- a/tide/wall/tools/PixelStreamProcessor.h +++ b/tide/wall/tools/PixelStreamProcessor.h @@ -69,6 +69,9 @@ class PixelStreamProcessor virtual Indices computeVisibleSet(const QRectF& visibleArea, uint channel) const = 0; + /** @return the total number of assembled tiles. */ + virtual size_t getTilesCount() const = 0; + protected: /** @return the coordinates of the tile as a QRect. */ QRect toRect(const deflect::server::Tile& tile) const;