Merge pull request #10416 from jherico/texture_transfer_safety

Fixing crash in texture transfer logic, again

libraries/gpu-gl/CMakeLists.txt

@@ -1,5 +1,5 @@
 set(TARGET_NAME gpu-gl)
-setup_hifi_library()
+setup_hifi_library(Concurrent)
 link_hifi_libraries(shared gl gpu)
 if (UNIX)
     target_link_libraries(${TARGET_NAME} pthread)

libraries/gpu-gl/src/gpu/gl/GLTexture.cpp

@@ -160,8 +160,6 @@ const uvec3 GLVariableAllocationSupport::INITIAL_MIP_TRANSFER_DIMENSIONS { 64, 6
 WorkQueue GLVariableAllocationSupport::_transferQueue;
 WorkQueue GLVariableAllocationSupport::_promoteQueue;
 WorkQueue GLVariableAllocationSupport::_demoteQueue;
-TexturePointer GLVariableAllocationSupport::_currentTransferTexture;
-TransferJobPointer GLVariableAllocationSupport::_currentTransferJob;
 size_t GLVariableAllocationSupport::_frameTexturesCreated { 0 };
 
 #define OVERSUBSCRIBED_PRESSURE_VALUE 0.95f
@@ -176,30 +174,19 @@ const uvec3 GLVariableAllocationSupport::MAX_TRANSFER_DIMENSIONS { 1024, 1024, 1
 const size_t GLVariableAllocationSupport::MAX_TRANSFER_SIZE = GLVariableAllocationSupport::MAX_TRANSFER_DIMENSIONS.x * GLVariableAllocationSupport::MAX_TRANSFER_DIMENSIONS.y * 4;
 
 #if THREADED_TEXTURE_BUFFERING
-std::shared_ptr<std::thread> TransferJob::_bufferThread { nullptr };
-std::atomic<bool> TransferJob::_shutdownBufferingThread { false };
-Mutex TransferJob::_mutex;
-TransferJob::VoidLambdaQueue TransferJob::_bufferLambdaQueue;
 
-void TransferJob::startTransferLoop() {
+TexturePointer GLVariableAllocationSupport::_currentTransferTexture;
-    if (_bufferThread) {
+TransferJobPointer GLVariableAllocationSupport::_currentTransferJob;
-        return;
+QThreadPool* TransferJob::_bufferThreadPool { nullptr };
-    }
+
-    _shutdownBufferingThread = false;
+void TransferJob::startBufferingThread() {
-    _bufferThread = std::make_shared<std::thread>([] {
+    static std::once_flag once;
-        TransferJob::bufferLoop();
+    std::call_once(once, [&] {
+        _bufferThreadPool = new QThreadPool(qApp);
+        _bufferThreadPool->setMaxThreadCount(1);
     });
 }
 
-void TransferJob::stopTransferLoop() {
-    if (!_bufferThread) {
-        return;
-    }
-    _shutdownBufferingThread = true;
-    _bufferThread->join();
-    _bufferThread.reset();
-    _shutdownBufferingThread = false;
-}
 #endif
 
 TransferJob::TransferJob(const GLTexture& parent, uint16_t sourceMip, uint16_t targetMip, uint8_t face, uint32_t lines, uint32_t lineOffset)
@@ -233,7 +220,6 @@ TransferJob::TransferJob(const GLTexture& parent, uint16_t sourceMip, uint16_t t
     // Buffering can invoke disk IO, so it should be off of the main and render threads
     _bufferingLambda = [=] {
         _mipData = _parent._gpuObject.accessStoredMipFace(sourceMip, face)->createView(_transferSize, _transferOffset);
-        _bufferingCompleted = true;
     };
 
     _transferLambda = [=] {
@@ -243,65 +229,66 @@ TransferJob::TransferJob(const GLTexture& parent, uint16_t sourceMip, uint16_t t
 }
 
 TransferJob::TransferJob(const GLTexture& parent, std::function<void()> transferLambda)
-    : _parent(parent), _bufferingCompleted(true), _transferLambda(transferLambda) {
+    : _parent(parent), _bufferingRequired(false), _transferLambda(transferLambda) {
 }
 
 TransferJob::~TransferJob() {
     Backend::updateTextureTransferPendingSize(_transferSize, 0);
 }
 
-
 bool TransferJob::tryTransfer() {
-    // Disable threaded texture transfer for now
 #if THREADED_TEXTURE_BUFFERING
     // Are we ready to transfer
-    if (_bufferingCompleted) {
+    if (!bufferingCompleted()) {
-        _transferLambda();
+        startBuffering();
+        return false;
+    }
+#else
+    if (_bufferingRequired) {
+        _bufferingLambda();
+    }
+#endif
+    _transferLambda();
+    return true;
+}
+
+#if THREADED_TEXTURE_BUFFERING
+bool TransferJob::bufferingRequired() const {
+    if (!_bufferingRequired) {
+        return false;
+    }
+
+    // The default state of a QFuture is with status Canceled | Started  | Finished, 
+    // so we have to check isCancelled before we check the actual state
+    if (_bufferingStatus.isCanceled()) {
         return true;
     }
 
-    startBuffering();
+    return !_bufferingStatus.isStarted();
-    return false;
-#else
-    if (!_bufferingCompleted) {
-        _bufferingLambda();
-        _bufferingCompleted = true;
-    }
-    _transferLambda();
-    return true;
-#endif
 }
 
-#if THREADED_TEXTURE_BUFFERING
+bool TransferJob::bufferingCompleted() const {
+    if (!_bufferingRequired) {
+        return true;
+    }
+
+    // The default state of a QFuture is with status Canceled | Started  | Finished, 
+    // so we have to check isCancelled before we check the actual state
+    if (_bufferingStatus.isCanceled()) {
+        return false;
+    }
+
+    return _bufferingStatus.isFinished();
+}
 
 void TransferJob::startBuffering() {
-    if (_bufferingStarted) {
+    if (bufferingRequired()) {
-        return;
+        assert(_bufferingStatus.isCanceled());
-    }
+        _bufferingStatus = QtConcurrent::run(_bufferThreadPool, [=] {
-    _bufferingStarted = true;
+            _bufferingLambda();
-    {
+        });
-        Lock lock(_mutex);
+        assert(!_bufferingStatus.isCanceled());
-        _bufferLambdaQueue.push(_bufferingLambda);
+        assert(_bufferingStatus.isStarted());
-    }
-}
-
-void TransferJob::bufferLoop() {
-    while (!_shutdownBufferingThread) {
-        VoidLambdaQueue workingQueue;
-        {
-            Lock lock(_mutex);
-            _bufferLambdaQueue.swap(workingQueue);
-        }
-
-        if (workingQueue.empty()) {
-            QThread::msleep(5);
-            continue;
-        }
-
-        while (!workingQueue.empty()) {
-            workingQueue.front()();
-            workingQueue.pop();
-        }
     }
 }
 #endif
@@ -316,7 +303,9 @@ GLVariableAllocationSupport::~GLVariableAllocationSupport() {
 
 void GLVariableAllocationSupport::addMemoryManagedTexture(const TexturePointer& texturePointer) {
     _memoryManagedTextures.push_back(texturePointer);
-    addToWorkQueue(texturePointer);
+    if (MemoryPressureState::Idle != _memoryPressureState) {
+        addToWorkQueue(texturePointer);
+    }
 }
 
 void GLVariableAllocationSupport::addToWorkQueue(const TexturePointer& texturePointer) {
@@ -345,10 +334,8 @@ void GLVariableAllocationSupport::addToWorkQueue(const TexturePointer& texturePo
             break;
 
         case MemoryPressureState::Idle:
-            break;
-
-        default:
             Q_UNREACHABLE();
+            break;
     }
 }
 
@@ -364,10 +351,10 @@ WorkQueue& GLVariableAllocationSupport::getActiveWorkQueue() {
         case MemoryPressureState::Transfer:
             return _transferQueue;
 
-        default:
+        case MemoryPressureState::Idle:
+            Q_UNREACHABLE();
             break;
     }
-    Q_UNREACHABLE();
     return empty;
 }
 
@@ -460,16 +447,11 @@ void GLVariableAllocationSupport::updateMemoryPressure() {
     }
 
     if (newState != _memoryPressureState) {
+        _memoryPressureState = newState;
 #if THREADED_TEXTURE_BUFFERING
         if (MemoryPressureState::Transfer == _memoryPressureState) {
-            TransferJob::stopTransferLoop();
+            TransferJob::startBufferingThread();
         }
-        _memoryPressureState = newState;
-        if (MemoryPressureState::Transfer == _memoryPressureState) {
-            TransferJob::startTransferLoop();
-        }
-#else
-        _memoryPressureState = newState;
 #endif
         // Clear the existing queue
         _transferQueue = WorkQueue();
@@ -487,49 +469,111 @@ void GLVariableAllocationSupport::updateMemoryPressure() {
     }
 }
 
+TexturePointer GLVariableAllocationSupport::getNextWorkQueueItem(WorkQueue& workQueue) {
+    while (!workQueue.empty()) {
+        auto workTarget = workQueue.top();
+
+        auto texture = workTarget.first.lock();
+        if (!texture) {
+            workQueue.pop();
+            continue;
+        }
+
+        // Check whether the resulting texture can actually have work performed
+        GLTexture* gltexture = Backend::getGPUObject<GLTexture>(*texture);
+        GLVariableAllocationSupport* vartexture = dynamic_cast<GLVariableAllocationSupport*>(gltexture);
+        switch (_memoryPressureState) {
+            case MemoryPressureState::Oversubscribed:
+                if (vartexture->canDemote()) {
+                    return texture;
+                }
+                break;
+
+            case MemoryPressureState::Undersubscribed:
+                if (vartexture->canPromote()) {
+                    return texture;
+                }
+                break;
+
+            case MemoryPressureState::Transfer:
+                if (vartexture->hasPendingTransfers()) {
+                    return texture;
+                }
+                break;
+
+            case MemoryPressureState::Idle:
+                Q_UNREACHABLE();
+                break;
+        }
+
+        // If we got here, then the texture has no work to do in the current state, 
+        // so pop it off the queue and continue
+        workQueue.pop();
+    }
+
+    return TexturePointer();
+}
+
+void GLVariableAllocationSupport::processWorkQueue(WorkQueue& workQueue) {
+    if (workQueue.empty()) {
+        return;
+    }
+
+    // Get the front of the work queue to perform work
+    auto texture = getNextWorkQueueItem(workQueue);
+    if (!texture) {
+        return;
+    }
+
+    // Grab the first item off the demote queue
+    PROFILE_RANGE(render_gpu_gl, __FUNCTION__);
+
+    GLTexture* gltexture = Backend::getGPUObject<GLTexture>(*texture);
+    GLVariableAllocationSupport* vartexture = dynamic_cast<GLVariableAllocationSupport*>(gltexture);
+    switch (_memoryPressureState) {
+        case MemoryPressureState::Oversubscribed:
+            vartexture->demote();
+            workQueue.pop();
+            addToWorkQueue(texture);
+            break;
+
+        case MemoryPressureState::Undersubscribed:
+            vartexture->promote();
+            workQueue.pop();
+            addToWorkQueue(texture);
+            break;
+
+        case MemoryPressureState::Transfer:
+            if (vartexture->executeNextTransfer(texture)) {
+                workQueue.pop();
+                addToWorkQueue(texture);
+
+#if THREADED_TEXTURE_BUFFERING
+                // Eagerly start the next buffering job if possible
+                texture = getNextWorkQueueItem(workQueue);
+                if (texture) {
+                    gltexture = Backend::getGPUObject<GLTexture>(*texture);
+                    vartexture = dynamic_cast<GLVariableAllocationSupport*>(gltexture);
+                    vartexture->executeNextBuffer(texture);
+                }
+#endif
+            }
+            break;
+
+        case MemoryPressureState::Idle:
+            Q_UNREACHABLE();
+            break;
+    }
+}
+
 void GLVariableAllocationSupport::processWorkQueues() {
     if (MemoryPressureState::Idle == _memoryPressureState) {
         return;
     }
 
     auto& workQueue = getActiveWorkQueue();
-    PROFILE_RANGE(render_gpu_gl, __FUNCTION__);
+    // Do work on the front of the queue
-    while (!workQueue.empty()) {
+    processWorkQueue(workQueue);
-        auto workTarget = workQueue.top();
-        workQueue.pop();
-        auto texture = workTarget.first.lock();
-        if (!texture) {
-            continue;
-        }
-
-        // Grab the first item off the demote queue
-        GLTexture* gltexture = Backend::getGPUObject<GLTexture>(*texture);
-        GLVariableAllocationSupport* vartexture = dynamic_cast<GLVariableAllocationSupport*>(gltexture);
-        if (MemoryPressureState::Oversubscribed == _memoryPressureState) {
-            if (!vartexture->canDemote()) {
-                continue;
-            }
-            vartexture->demote();
-            _memoryPressureStateStale = true;
-        } else if (MemoryPressureState::Undersubscribed == _memoryPressureState) {
-            if (!vartexture->canPromote()) {
-                continue;
-            }
-            vartexture->promote();
-            _memoryPressureStateStale = true;
-        } else if (MemoryPressureState::Transfer == _memoryPressureState) {
-            if (!vartexture->hasPendingTransfers()) {
-                continue;
-            }
-            vartexture->executeNextTransfer(texture);
-        } else {
-            Q_UNREACHABLE();
-        }
-
-        // Reinject into the queue if more work to be done
-        addToWorkQueue(texture);
-        break;
-    }
 
     if (workQueue.empty()) {
         _memoryPressureState = MemoryPressureState::Idle;
@@ -543,28 +587,83 @@ void GLVariableAllocationSupport::manageMemory() {
     processWorkQueues();
 }
 
+bool GLVariableAllocationSupport::executeNextTransfer(const TexturePointer& currentTexture) {
+#if THREADED_TEXTURE_BUFFERING
+    // If a transfer job is active on the buffering thread, but has not completed it's buffering lambda,
+    // then we need to exit early, since we don't want to have the transfer job leave scope while it's 
+    // being used in another thread -- See https://highfidelity.fogbugz.com/f/cases/4626
+    if (_currentTransferJob && !_currentTransferJob->bufferingCompleted()) {
+        return false;
+    }
+#endif
 
-void GLVariableAllocationSupport::executeNextTransfer(const TexturePointer& currentTexture) {
     if (_populatedMip <= _allocatedMip) {
+#if THREADED_TEXTURE_BUFFERING
+        _currentTransferJob.reset();
+        _currentTransferTexture.reset();
+#endif
+        return true;
+    }
+
+    // If the transfer queue is empty, rebuild it
+    if (_pendingTransfers.empty()) {
+        populateTransferQueue();
+    }
+
+    bool result = false;
+    if (!_pendingTransfers.empty()) {
+#if THREADED_TEXTURE_BUFFERING
+        // If there is a current transfer, but it's not the top of the pending transfer queue, then it's an orphan, so we want to abandon it.
+        if (_currentTransferJob && _currentTransferJob != _pendingTransfers.front()) {
+            _currentTransferJob.reset();
+        }
+
+        if (!_currentTransferJob) {
+            // Keeping hold of a strong pointer to the transfer job ensures that if the pending transfer queue is rebuilt, the transfer job
+            // doesn't leave scope, causing a crash in the buffering thread
+            _currentTransferJob = _pendingTransfers.front();
+
+            // Keeping hold of a strong pointer during the transfer ensures that the transfer thread cannot try to access a destroyed texture
+            _currentTransferTexture = currentTexture;
+        }
+
+        // transfer jobs use asynchronous buffering of the texture data because it may involve disk IO, so we execute a try here to determine if the buffering 
+        // is complete
+        if (_currentTransferJob->tryTransfer()) {
+            _pendingTransfers.pop();
+            // Once a given job is finished, release the shared pointers keeping them alive
+            _currentTransferTexture.reset();
+            _currentTransferJob.reset();
+            result = true;
+        }
+#else
+        if (_pendingTransfers.front()->tryTransfer()) {
+            _pendingTransfers.pop();
+            result = true;
+        }
+#endif
+    }
+    return result;
+}
+
+#if THREADED_TEXTURE_BUFFERING
+void GLVariableAllocationSupport::executeNextBuffer(const TexturePointer& currentTexture) {
+    if (_currentTransferJob && !_currentTransferJob->bufferingCompleted()) {
         return;
     }
 
+    // If the transfer queue is empty, rebuild it
     if (_pendingTransfers.empty()) {
         populateTransferQueue();
     }
 
     if (!_pendingTransfers.empty()) {
-        // Keeping hold of a strong pointer during the transfer ensures that the transfer thread cannot try to access a destroyed texture
+        if (!_currentTransferJob) {
-        _currentTransferTexture = currentTexture;
+            _currentTransferJob = _pendingTransfers.front();
-        // Keeping hold of a strong pointer to the transfer job ensures that if the pending transfer queue is rebuilt, the transfer job
+            _currentTransferTexture = currentTexture;
-        // doesn't leave scope, causing a crash in the buffering thread
-        _currentTransferJob = _pendingTransfers.front();
-        // transfer jobs use asynchronous buffering of the texture data because it may involve disk IO, so we execute a try here to determine if the buffering 
-        // is complete
-        if (_currentTransferJob->tryTransfer()) {
-            _pendingTransfers.pop();
-            _currentTransferTexture.reset();
-            _currentTransferJob.reset();
         }
+
+        _currentTransferJob->startBuffering();
     }
 }
+#endif

libraries/gpu-gl/src/gpu/gl/GLTexture.h

@@ -8,6 +8,9 @@
 #ifndef hifi_gpu_gl_GLTexture_h
 #define hifi_gpu_gl_GLTexture_h
 
+#include <QtCore/QThreadPool>
+#include <QtConcurrent>
+
 #include "GLShared.h"
 #include "GLBackend.h"
 #include "GLTexelFormat.h"
@@ -47,24 +50,19 @@ public:
     class TransferJob {
         using VoidLambda = std::function<void()>;
         using VoidLambdaQueue = std::queue<VoidLambda>;
-        using ThreadPointer = std::shared_ptr<std::thread>;
         const GLTexture& _parent;
         Texture::PixelsPointer _mipData;
         size_t _transferOffset { 0 };
         size_t _transferSize { 0 };
 
-        // Indicates if a transfer from backing storage to interal storage has started
+        bool _bufferingRequired { true };
-        bool _bufferingStarted { false };
-        bool _bufferingCompleted { false };
         VoidLambda _transferLambda;
         VoidLambda _bufferingLambda;
 
 #if THREADED_TEXTURE_BUFFERING
-        static Mutex _mutex;
+        // Indicates if a transfer from backing storage to interal storage has started
-        static VoidLambdaQueue _bufferLambdaQueue;
+        QFuture<void> _bufferingStatus;
-        static ThreadPointer _bufferThread;
+        static QThreadPool* _bufferThreadPool;
-        static std::atomic<bool> _shutdownBufferingThread;
-        static void bufferLoop();
 #endif
 
     public:
@@ -75,14 +73,13 @@ public:
         bool tryTransfer();
 
 #if THREADED_TEXTURE_BUFFERING
-        static void startTransferLoop();
+        void startBuffering();
-        static void stopTransferLoop();
+        bool bufferingRequired() const;
+        bool bufferingCompleted() const;
+        static void startBufferingThread();
 #endif
 
     private:
-#if THREADED_TEXTURE_BUFFERING
-        void startBuffering();
-#endif
         void transfer();
     };
 
@@ -100,8 +97,10 @@ protected:
     static WorkQueue _transferQueue;
     static WorkQueue _promoteQueue;
     static WorkQueue _demoteQueue;
+#if THREADED_TEXTURE_BUFFERING
     static TexturePointer _currentTransferTexture;
     static TransferJobPointer _currentTransferJob;
+#endif
     static const uvec3 INITIAL_MIP_TRANSFER_DIMENSIONS;
     static const uvec3 MAX_TRANSFER_DIMENSIONS;
     static const size_t MAX_TRANSFER_SIZE;
@@ -109,6 +108,8 @@ protected:
 
     static void updateMemoryPressure();
     static void processWorkQueues();
+    static void processWorkQueue(WorkQueue& workQueue);
+    static TexturePointer getNextWorkQueueItem(WorkQueue& workQueue);
     static void addToWorkQueue(const TexturePointer& texture);
     static WorkQueue& getActiveWorkQueue();
 
@@ -118,7 +119,10 @@ protected:
     bool canPromote() const { return _allocatedMip > _minAllocatedMip; }
     bool canDemote() const { return _allocatedMip < _maxAllocatedMip; }
     bool hasPendingTransfers() const { return _populatedMip > _allocatedMip; }
-    void executeNextTransfer(const TexturePointer& currentTexture);
+#if THREADED_TEXTURE_BUFFERING
+    void executeNextBuffer(const TexturePointer& currentTexture);
+#endif
+    bool executeNextTransfer(const TexturePointer& currentTexture);
     virtual void populateTransferQueue() = 0;
     virtual void promote() = 0;
     virtual void demote() = 0;

libraries/gpu-gl/src/gpu/gl45/GL45Backend.h

@@ -17,7 +17,6 @@
 #include <thread>
 
 #define INCREMENTAL_TRANSFER 0
-#define THREADED_TEXTURE_BUFFERING 1
 #define GPU_SSBO_TRANSFORM_OBJECT 1
 
 namespace gpu { namespace gl45 {