core: stabilize label updates and revision locking in the asyncResourceGatherer (#384)

* core: handle rerendering when frameCallback is pending

* core: log when skipping label updates

* asyncResourceGatherer: remove busy and use loopMutex

Makes getAssetById fail less often and thus labels get more stable
updates

* asyncResourceGatherer: revision locking

`assetsMutex` was not needed, since `apply` only gets called from the
main thread and resources are also only aquired via the main thread.

`preloadTargets`, previously kinda guarded by the `busy` flag are now
locked as suggested in #367 (but via a copy of `peloadTargets`).

`apply` now returns a boolean so that the locking of preloadTargets in
combination with checking `preloadTargets.empty()` is a bit nicer.

* asyncResourceGatherer: remove explicit template arg for unique lock

src/core/LockSurface.cpp

@@ -129,8 +129,10 @@ static const wl_callback_listener callbackListener = {
 void CSessionLockSurface::render() {
     Debug::log(TRACE, "render lock");
 
-    if (frameCallback || !readyForFrame)
+    if (frameCallback || !readyForFrame) {
+        needsFrame = true;
         return;
+    }
 
     const auto FEEDBACK = g_pRenderer->renderLock(*this);
     frameCallback       = wl_surface_frame(surface);

src/renderer/AsyncResourceGatherer.cpp

@@ -11,8 +11,6 @@
 #include "../helpers/Jpeg.hpp"
 #include "../helpers/Webp.hpp"
 
-std::mutex cvmtx;
-
 CAsyncResourceGatherer::CAsyncResourceGatherer() {
     asyncLoopThread = std::thread([this]() {
         this->gather(); /* inital gather */
@@ -83,23 +81,17 @@ void CAsyncResourceGatherer::recheckDMAFramesFor(COutput* output) {
 }
 
 SPreloadedAsset* CAsyncResourceGatherer::getAssetByID(const std::string& id) {
-    if (asyncLoopState.busy)
-        return nullptr;
-
-    std::lock_guard<std::mutex> lg(asyncLoopState.loopMutex);
-
     for (auto& a : assets) {
         if (a.first == id)
             return &a.second;
     }
 
-    if (!preloadTargets.empty()) {
+    if (apply()) {
-        apply();
         for (auto& a : assets) {
             if (a.first == id)
                 return &a.second;
         }
-    }
+    };
 
     for (auto& dma : dmas) {
         if (id == "dma:" + dma->name)
@@ -210,11 +202,20 @@ void CAsyncResourceGatherer::gather() {
     ready = true;
 }
 
-void CAsyncResourceGatherer::apply() {
+bool CAsyncResourceGatherer::apply() {
+    preloadTargetsMutex.lock();
 
-    for (auto& t : preloadTargets) {
+    if (preloadTargets.empty()) {
+        preloadTargetsMutex.unlock();
+        return false;
+    }
+
+    auto currentPreloadTargets = preloadTargets;
+    preloadTargets.clear();
+    preloadTargetsMutex.unlock();
+
+    for (auto& t : currentPreloadTargets) {
         if (t.type == TARGET_IMAGE) {
-            std::lock_guard<std::mutex> lg(asyncLoopState.assetsMutex);
             const auto  ASSET = &assets[t.id];
 
             const auto  SURFACESTATUS = cairo_surface_status((cairo_surface_t*)t.cairosurface);
@@ -248,9 +249,8 @@ void CAsyncResourceGatherer::apply() {
         }
     }
 
-    preloadTargets.clear();
-
     applied = true;
+    return true;
 }
 
 void CAsyncResourceGatherer::renderImage(const SPreloadRequest& rq) {
@@ -269,6 +269,7 @@ void CAsyncResourceGatherer::renderImage(const SPreloadRequest& rq) {
     target.data         = cairo_image_surface_get_data(CAIROISURFACE);
     target.size         = {(double)cairo_image_surface_get_width(CAIROISURFACE), (double)cairo_image_surface_get_height(CAIROISURFACE)};
 
+    std::lock_guard lg{preloadTargetsMutex};
     preloadTargets.push_back(target);
 }
 
@@ -363,6 +364,7 @@ void CAsyncResourceGatherer::renderText(const SPreloadRequest& rq) {
     target.data         = cairo_image_surface_get_data(CAIROSURFACE);
     target.size         = {layoutWidth / PANGO_SCALE, layoutHeight / PANGO_SCALE};
 
+    std::lock_guard lg{preloadTargetsMutex};
     preloadTargets.push_back(target);
 }
 
@@ -380,27 +382,23 @@ static void timerCallback(std::shared_ptr<CTimer> self, void* data_) {
 void CAsyncResourceGatherer::asyncAssetSpinLock() {
     while (!g_pHyprlock->m_bTerminate) {
 
-        std::unique_lock lk(cvmtx);
+        std::unique_lock lk(asyncLoopState.requestsMutex);
         if (asyncLoopState.pending == false) // avoid a lock if a thread managed to request something already since we .unlock()ed
-            asyncLoopState.loopGuard.wait_for(lk, std::chrono::seconds(5), [this] { return asyncLoopState.pending; }); // wait for events
+            asyncLoopState.requestsCV.wait_for(lk, std::chrono::seconds(5), [this] { return asyncLoopState.pending; }); // wait for events
-
-        asyncLoopState.requestMutex.lock();
 
         asyncLoopState.pending = false;
 
         if (asyncLoopState.requests.empty()) {
-            asyncLoopState.requestMutex.unlock();
+            lk.unlock();
             continue;
         }
 
         auto requests = asyncLoopState.requests;
         asyncLoopState.requests.clear();
 
-        asyncLoopState.requestMutex.unlock();
+        lk.unlock();
 
         // process requests
-
-        asyncLoopState.busy = true;
         for (auto& r : requests) {
             if (r.type == TARGET_TEXT) {
                 renderText(r);
@@ -415,29 +413,26 @@ void CAsyncResourceGatherer::asyncAssetSpinLock() {
             if (r.callback)
                 g_pHyprlock->addTimer(std::chrono::milliseconds(0), timerCallback, new STimerCallbackData{r.callback, r.callbackData});
         }
-
-        asyncLoopState.busy = false;
     }
 
     dmas.clear();
 }
 
 void CAsyncResourceGatherer::requestAsyncAssetPreload(const SPreloadRequest& request) {
-    std::lock_guard<std::mutex> lg(asyncLoopState.requestMutex);
+    std::lock_guard<std::mutex> lg(asyncLoopState.requestsMutex);
     asyncLoopState.requests.push_back(request);
     asyncLoopState.pending = true;
-    asyncLoopState.loopGuard.notify_all();
+    asyncLoopState.requestsCV.notify_all();
 }
 
 void CAsyncResourceGatherer::unloadAsset(SPreloadedAsset* asset) {
-    std::lock_guard<std::mutex> lg(asyncLoopState.assetsMutex);
-
     std::erase_if(assets, [asset](const auto& a) { return &a.second == asset; });
 }
 
 void CAsyncResourceGatherer::notify() {
+    std::lock_guard<std::mutex> lg(asyncLoopState.requestsMutex);
     asyncLoopState.pending = true;
-    asyncLoopState.loopGuard.notify_all();
+    asyncLoopState.requestsCV.notify_all();
 }
 
 void CAsyncResourceGatherer::await() {

src/renderer/AsyncResourceGatherer.hpp

@@ -24,7 +24,7 @@ class CAsyncResourceGatherer {
     /* only call from ogl thread */
     SPreloadedAsset* getAssetByID(const std::string& id);
 
-    void             apply();
+    bool             apply();
 
     enum eTargetType {
         TARGET_IMAGE = 0,
@@ -59,12 +59,8 @@ class CAsyncResourceGatherer {
     void        renderImage(const SPreloadRequest& rq);
 
     struct {
-        std::condition_variable      loopGuard;
+        std::condition_variable      requestsCV;
-        std::mutex                   loopMutex;
+        std::mutex                   requestsMutex;
-
-        std::mutex                   requestMutex;
-
-        std::mutex                   assetsMutex;
 
         std::vector<SPreloadRequest> requests;
         bool                         pending = false;
@@ -86,6 +82,8 @@ class CAsyncResourceGatherer {
     std::vector<std::unique_ptr<CDMAFrame>>          dmas;
 
     std::vector<SPreloadTarget>                      preloadTargets;
+    std::mutex                                       preloadTargetsMutex;
+
     std::unordered_map<std::string, SPreloadedAsset> assets;
 
     void                                             gather();

src/renderer/widgets/Label.cpp

@@ -39,8 +39,10 @@ void CLabel::onTimerUpdate() {
     if (label.formatted == oldFormatted && !label.alwaysUpdate)
         return;
 
-    if (!pendingResourceID.empty())
+    if (!pendingResourceID.empty()) {
-        return; // too many updates, we'll miss some. Shouldn't happen tbh
+        Debug::log(WARN, "Trying to update label, but resource {} is still pending! Skipping update.", pendingResourceID);
+        return;
+    }
 
     // request new
     request.id        = getUniqueResourceId();
@@ -141,7 +143,4 @@ static void onAssetCallbackTimer(std::shared_ptr<CTimer> self, void* data) {
 
 void CLabel::renderSuper() {
     g_pHyprlock->renderOutput(outputStringPort);
-
-    if (!pendingResourceID.empty()) /* did not consume the pending resource */
-        g_pHyprlock->addTimer(std::chrono::milliseconds(100), onAssetCallbackTimer, this);
 }