From adc4ea79fc19b0be136623c2b54011e615f61ca3 Mon Sep 17 00:00:00 2001 From: El Date: Wed, 6 May 2026 08:32:12 +0000 Subject: [PATCH] VNC-151 Refactor screenshot handling --- common/network/GetAPI.h | 3 - common/network/GetAPIMessager.cxx | 112 ++++++++++++-------------- common/rfb/VNCServerST.cxx | 34 ++++---- common/rfb/VNCServerST.h | 7 +- common/rfb/util.h | 2 +- unix/xserver/hw/vnc/XserverDesktop.cc | 3 - 6 files changed, 73 insertions(+), 88 deletions(-) diff --git a/common/network/GetAPI.h b/common/network/GetAPI.h index 7f57d85..20d0c76 100644 --- a/common/network/GetAPI.h +++ b/common/network/GetAPI.h @@ -35,8 +35,6 @@ namespace network { // from main thread void mainUpdateScreen(rfb::PixelBuffer *pb); - void lockScreenshots(); - void unlockScreenshots(); void mainUpdateBottleneckStats(const char userid[], const char stats[]); void mainClearBottleneckStats(const char userid[]); void mainUpdateServerFrameStats(uint8_t changedPerc, uint32_t all, @@ -105,7 +103,6 @@ namespace network { const char *passwdfile; pthread_mutex_t screenMutex; - rfb::PixelBuffer *pb; rfb::ManagedPixelBuffer screenPb; uint16_t screenW, screenH; uint64_t screenHash; diff --git a/common/network/GetAPIMessager.cxx b/common/network/GetAPIMessager.cxx index d7dd62a..937c56e 100644 --- a/common/network/GetAPIMessager.cxx +++ b/common/network/GetAPIMessager.cxx @@ -55,7 +55,7 @@ static const struct TightJPEGConfiguration conf[10] = { }; GetAPIMessager::GetAPIMessager(const char *passwdfile_): passwdfile(passwdfile_), - pb(nullptr), screenW(0), screenH(0), screenHash(0), + screenW(0), screenH(0), screenHash(0), cachedW(0), cachedH(0), cachedQ(0), ownerConnected(0), activeUsers(0), sessionsInfo( "{\"users\":[]}"){ @@ -74,21 +74,47 @@ GetAPIMessager::GetAPIMessager(const char *passwdfile_): passwdfile(passwdfile_) } // from main thread -void GetAPIMessager::mainUpdateScreen(rfb::PixelBuffer *pb_) { - pb = pb_; -} +void GetAPIMessager::mainUpdateScreen(rfb::PixelBuffer *pb) { + if (!pb) + return; -void GetAPIMessager::lockScreenshots() { - pthread_mutex_lock(&screenMutex); + if (pthread_mutex_trylock(&screenMutex)) + return; - screenW = screenH = 0; - screenHash = 0; - cachedW = cachedH = cachedQ = 0; - cachedJpeg.clear(); -} + int stride; + TRACE_STOPWATCH(shotstart); -void GetAPIMessager::unlockScreenshots() { - pthread_mutex_unlock(&screenMutex); + const rdr::U8 *const buf = pb->getBuffer(pb->getRect(), &stride); + + if (pb->width() != screenW || pb->height() != screenH) { + screenHash = 0; + screenW = pb->width(); + screenH = pb->height(); + screenPb.setPF(pb->getPF()); + screenPb.setSize(screenW, screenH); + + cachedW = cachedH = cachedQ = 0; + cachedJpeg.clear(); + } + + const uint64_t newHash = XXH64(buf, pb->area() * 4, 0); + if (newHash != screenHash) { + cachedW = cachedH = cachedQ = 0; + cachedJpeg.clear(); + + screenHash = newHash; + rdr::U8 *rw = screenPb.getBufferRW(screenPb.getRect(), &stride); + memcpy(rw, buf, screenW * screenH * 4); + screenPb.commitBufferRW(screenPb.getRect()); + } + + if (!pthread_mutex_lock(&frameStatMutex)) { + serverFrameStats.shot = msSince(&shotstart); + pthread_mutex_unlock(&frameStatMutex); + } + + TRACE_STOPWATCH_PRINT_MS(vlog, shotstart); + pthread_mutex_unlock(&screenMutex); } void GetAPIMessager::mainUpdateBottleneckStats(const char userid[], const char stats[]) { @@ -175,55 +201,24 @@ void GetAPIMessager::mainUpdateSessionsInfo(std::string newSessionsInfo) uint8_t *GetAPIMessager::netGetScreenshot(uint16_t w, uint16_t h, const uint8_t q, const bool dedup, uint32_t &len, uint8_t *staging) { - unsigned shottime = 0; - int stride; - TRACE_STOPWATCH(shotstart); + uint8_t *ret = nullptr; + len = 0; - uint8_t *ret = nullptr; - len = 0; + if (q > 9 || !staging) + return nullptr; - if (pthread_mutex_lock(&screenMutex)) - return nullptr; - - if (!pb) { - pthread_mutex_unlock(&screenMutex); - return nullptr; - } - - const rdr::U8 *buf = pb->getBuffer(pb->getRect(), &stride); - - if (pb->width() != screenW || pb->height() != screenH) { - screenHash = 0; - screenW = pb->width(); - screenH = pb->height(); - screenPb.setPF(pb->getPF()); - screenPb.setSize(screenW, screenH); - - cachedW = cachedH = cachedQ = 0; - cachedJpeg.clear(); - } - - const uint64_t newHash = XXH64(buf, pb->area() * 4, 0); - if (newHash != screenHash) { - cachedW = cachedH = cachedQ = 0; - cachedJpeg.clear(); - - screenHash = newHash; - rdr::U8 *rw = screenPb.getBufferRW(screenPb.getRect(), &stride); - memcpy(rw, buf, screenW * screenH * 4); - screenPb.commitBufferRW(screenPb.getRect()); - } + if (pthread_mutex_lock(&screenMutex)) + return nullptr; if (w > screenW) w = screenW; if (h > screenH) h = screenH; - if (!screenW || !screenH) + if (!w || !h) { vlog.error("Screenshot requested but no screenshot exists (screen hasn't been viewed)"); - - if (!w || !h || q > 9 || !staging) { pthread_mutex_unlock(&screenMutex); + return nullptr; } @@ -245,10 +240,11 @@ uint8_t *GetAPIMessager::netGetScreenshot(uint16_t w, uint16_t h, // Encode the new JPEG, cache it JpegCompressor jc; - const int quality = conf[q].quality; + const int quality = conf[q].quality; const int subsampling = conf[q].subsampling; jc.clear(); + int stride; if (w != screenW || h != screenH) { const float xdiff = w / (float) screenW; @@ -259,7 +255,7 @@ uint8_t *GetAPIMessager::netGetScreenshot(uint16_t w, uint16_t h, const uint16_t newh = screenH * diff; const PixelBuffer *scaled = progressiveBilinearScale(&screenPb, neww, newh, diff); - buf = scaled->getBuffer(scaled->getRect(), &stride); + const rdr::U8 *const buf = scaled->getBuffer(scaled->getRect(), &stride); jc.compress(buf, stride, scaled->getRect(), scaled->getPF(), quality, subsampling); @@ -271,7 +267,7 @@ uint8_t *GetAPIMessager::netGetScreenshot(uint16_t w, uint16_t h, vlog.info("Returning scaled screenshot"); } else { - buf = screenPb.getBuffer(screenPb.getRect(), &stride); + const rdr::U8 *const buf = screenPb.getBuffer(screenPb.getRect(), &stride); jc.compress(buf, stride, screenPb.getRect(), screenPb.getPF(), quality, subsampling); @@ -293,12 +289,6 @@ uint8_t *GetAPIMessager::netGetScreenshot(uint16_t w, uint16_t h, pthread_mutex_unlock(&screenMutex); - shottime = msSince(&shotstart); - if (!pthread_mutex_lock(&frameStatMutex)) { - serverFrameStats.shot = shottime; - pthread_mutex_unlock(&frameStatMutex); - } - return ret; } diff --git a/common/rfb/VNCServerST.cxx b/common/rfb/VNCServerST.cxx index 92eb90e..ce196d6 100644 --- a/common/rfb/VNCServerST.cxx +++ b/common/rfb/VNCServerST.cxx @@ -143,7 +143,7 @@ VNCServerST::VNCServerST(const char* name_, SDesktop* desktop_, const video_enco renderedCursorInvalid(false), queryConnectionHandler(nullptr), keyRemapper(&KeyRemapper::defInstance), lastConnectionTime(0), disableclients(false), - frameTimer(this), apimessager(nullptr), trackingFrameStats(0), + frameTimer(this), screenshotTimer(this), apimessager(nullptr), trackingFrameStats(0), clipboardId(0), sendWatermark(false), encoder_probe(encoder_probe_) { auto to_string = [](const bool value) { @@ -263,6 +263,8 @@ VNCServerST::VNCServerST(const char* name_, SDesktop* desktop_, const video_enco throw std::invalid_argument("Benchmarking video file does not exist"); benchmark(file_name, Server::benchmarkResults.getValueStr()); } + + screenshotTimer.start(SCREENSHOT_INTERVAL_MS); } VNCServerST::~VNCServerST() @@ -507,10 +509,6 @@ void VNCServerST::setPixelBuffer(PixelBuffer* pb_, const ScreenSet& layout) renderedCursorInvalid = true; add_changed(pb->getRect()); - if (apimessager) { - apimessager->mainUpdateScreen(pb); - } - // Make sure that we have at least one screen if (screenLayout.num_screens() == 0) screenLayout.add_screen(Screen(0, 0, 0, pb->width(), pb->height(), 0)); @@ -522,6 +520,8 @@ void VNCServerST::setPixelBuffer(PixelBuffer* pb_, const ScreenSet& layout) // Since the new pixel buffer means an ExtendedDesktopSize needs to // be sent anyway, we don't need to call screenLayoutChange. } + + updateScreenshot = true; } void VNCServerST::setPixelBuffer(PixelBuffer* pb_) @@ -768,6 +768,14 @@ bool VNCServerST::handleTimeout(Timer* t) return true; } + if (t == &screenshotTimer) { + if (apimessager) { + apimessager->mainUpdateScreen(getPixelBuffer()); + } + + return true; + } + return false; } @@ -1109,6 +1117,10 @@ void VNCServerST::writeUpdate() DEBUG_STOPWATCH_PRINT_US(slog, perm_check); if (apimessager) { + if (updateScreenshot) { + apimessager->mainUpdateScreen(pb); + updateScreenshot = false; + } trackingFrameStats = 0; checkAPIMessages(apimessager, trackingFrameStats, trackingClient); } @@ -1371,15 +1383,3 @@ void VNCServerST::notifyUserAction(const VNCSConnectionST* newConnection, std::s logNotification.append( std::to_string(notificationsSent) + " clients"); slog.info("%s", logNotification.c_str()); } - -void VNCServerST::lockScreenBuffer() const { - if (apimessager) { - apimessager->lockScreenshots(); - } -} - -void VNCServerST::unlockScreenBuffer() const { - if (apimessager) { - apimessager->unlockScreenshots(); - } -} diff --git a/common/rfb/VNCServerST.h b/common/rfb/VNCServerST.h index a184c08..3f2b5e3 100644 --- a/common/rfb/VNCServerST.h +++ b/common/rfb/VNCServerST.h @@ -51,6 +51,7 @@ namespace rfb { public Timer::Callback, public network::SocketServer { public: + constexpr static int SCREENSHOT_INTERVAL_MS = 60000; // -=- Constructors // Create a server exporting the supplied desktop. @@ -205,15 +206,13 @@ namespace rfb { enum UserActionType {Join, Leave}; void notifyUserAction(const VNCSConnectionST* newConnection, std::string& user_name, const UserActionType action_type); - void lockScreenBuffer() const; - void unlockScreenBuffer() const; protected: friend class VNCSConnectionST; // Timer callbacks - virtual bool handleTimeout(Timer* t); + bool handleTimeout(Timer* t) override; // - Internal methods @@ -281,6 +280,7 @@ namespace rfb { bool disableclients; Timer frameTimer; + Timer screenshotTimer; int inotify_fd{-1}; @@ -304,6 +304,7 @@ namespace rfb { rdr::U8 &trackingFrameStats, char trackingClient[]); bool sendWatermark; + bool updateScreenshot{false}; const video_encoders::EncoderProbe &encoder_probe; }; diff --git a/common/rfb/util.h b/common/rfb/util.h index d89eefd..1dd2ab3 100644 --- a/common/rfb/util.h +++ b/common/rfb/util.h @@ -206,7 +206,7 @@ namespace rfb { do { \ STOPWATCH_END_MS(name, name##_end); \ log.debug("Time " #name ": %u ms", name##_end); \ - } while (0); + } while (0) #define STOPWATCH_PRINT_MSG_US(log, name, msg) \ do { \ diff --git a/unix/xserver/hw/vnc/XserverDesktop.cc b/unix/xserver/hw/vnc/XserverDesktop.cc index ddc5f38..2d63628 100644 --- a/unix/xserver/hw/vnc/XserverDesktop.cc +++ b/unix/xserver/hw/vnc/XserverDesktop.cc @@ -126,8 +126,6 @@ void XserverDesktop::setFramebuffer(int w, int h, void* fbptr, int stride_) { ScreenSet layout; - server->lockScreenBuffer(); - width_ = w; height_ = h; @@ -149,7 +147,6 @@ void XserverDesktop::setFramebuffer(int w, int h, void* fbptr, int stride_) layout = ::computeScreenLayout(&outputIdMap); server->setPixelBuffer(this, layout); - server->unlockScreenBuffer(); } void XserverDesktop::refreshScreenLayout()