Skip to content

Commit

Permalink
Fix segfault affecting resizing of large streams with small tiles
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
Raphael Dumusc committed Sep 26, 2018
1 parent 13f4282 commit 03086b3
Show file tree
Hide file tree
Showing 10 changed files with 60 additions and 20 deletions.
2 changes: 2 additions & 0 deletions doc/Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
42 changes: 27 additions & 15 deletions tide/wall/datasources/PixelStreamUpdater.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<deflect::server::TileDecoder> 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<std::mutex> 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<deflect::server::TileDecoder> tileDecoders;
return processor->getTileImage(tileIndex, tileDecoders.localData());
}
catch (const std::runtime_error& e)
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -190,6 +192,7 @@ void PixelStreamUpdater::_onFrameSwapped(deflect::server::FramePtr frame)
_frameLeftOrMono = std::move(leftOrMono);
_frameRight = std::move(right);
_createFrameProcessors();
_createPerTileMutexes();
}

emit pictureUpdated();
Expand All @@ -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<std::vector<std::mutex>>(tiles);
}
2 changes: 2 additions & 0 deletions tide/wall/datasources/PixelStreamUpdater.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,12 @@ class PixelStreamUpdater : public QObject, public DataSource
std::unique_ptr<PixelStreamProcessor> _processorLeft;
std::unique_ptr<PixelStreamProcessor> _processRight;
mutable QReadWriteLock _frameMutex;
mutable std::unique_ptr<std::vector<std::mutex>> _perTileLock;
bool _readyToSwap = true;

void _onFrameSwapped(deflect::server::FramePtr frame);
void _createFrameProcessors();
void _createPerTileMutexes();
};

#endif
12 changes: 11 additions & 1 deletion tide/wall/tools/PixelStreamAssembler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@

#include "PixelStreamAssembler.h"

#include <numeric> // std::accumulate

namespace
{
Indices _mapToGlobalIndices(const Indices& perChannelIndices,
Expand All @@ -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);
Expand All @@ -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())
Expand Down
3 changes: 3 additions & 0 deletions tide/wall/tools/PixelStreamAssembler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
2 changes: 1 addition & 1 deletion tide/wall/tools/PixelStreamChannelAssembler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ Indices PixelStreamChannelAssembler::computeVisibleSet(
return visibleSet;
}

uint PixelStreamChannelAssembler::getTilesCount() const
size_t PixelStreamChannelAssembler::getTilesCount() const
{
return _getTilesX() * _getTilesY();
}
Expand Down
4 changes: 2 additions & 2 deletions tide/wall/tools/PixelStreamChannelAssembler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 6 additions & 1 deletion tide/wall/tools/PixelStreamPassthrough.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
#include <deflect/server/TileDecoder.h>

PixelStreamPassthrough::PixelStreamPassthrough(deflect::server::FramePtr frame)
: _frame(frame)
: _frame{std::move(frame)}
{
}

Expand Down Expand Up @@ -81,3 +81,8 @@ Indices PixelStreamPassthrough::computeVisibleSet(const QRectF& visibleArea,
}
return visibleSet;
}

size_t PixelStreamPassthrough::getTilesCount() const
{
return _frame->tiles.size();
}
3 changes: 3 additions & 0 deletions tide/wall/tools/PixelStreamPassthrough.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
3 changes: 3 additions & 0 deletions tide/wall/tools/PixelStreamProcessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 03086b3

Please sign in to comment.