From 7281e6b97bbb247bdbc50d80a64148f848601a54 Mon Sep 17 00:00:00 2001 From: Michael Adler Date: Wed, 5 Aug 2026 11:48:27 -0400 Subject: [PATCH] VNCCanvas: fix faint ghost edges left by a moving cursor Solid-color content (e.g. a block text cursor) advancing across the screen left faint stale borders behind at its previous positions, worst in 1:1 view and still present when scaled. Not a compression artifact -- reproduced regardless of encoding/quality settings. Two separate bugs were stacked here: 1. onPaint drew each dirty rect as its own small, independently composited bitmap. Every such tile leaves a visible seam at its own boundary (blending of a scaled edge, or HiDPI content-scale interpolation even at 1:1). Fix: composite a single whole-canvas bitmap in onPaint instead of many per-rect tiles, removing any internal edge that could leave a seam. 2. Even after (1), stale content remained in some spots, and a full forced repaint (e.g. from a resize) always cleared it -- meaning the pixel data was correct, but some Refresh(false, &rect) calls for just the changed sub-rect just never resulted in that area being repainted on this wx port. Tried coalescing all of a tick's changes into one bounding-box Refresh() call instead of one call per rect, in case it was multiple overlapping calls confusing the OS's dirty tracking -- still lost updates. So partial-rect invalidation itself isn't reliable here, not just how it's split into calls. Fix: always invalidate the whole canvas on any change, removing the dependency on that tracking entirely. Invalidating and redrawing everything on every update would normally be wasteful, so VNCCanvas now keeps its own native-resolution copy of the framebuffer (fb_cache), updated incrementally: only the changed rect is re-fetched from the connection (the expensive per-pixel copy, unchanged in cost from before this fix), while onPaint just blits the already-current cache in one cheap, GPU-composited call. The always-redraw-everything requirement from (2) ends up nearly free. Root cause of the wx-level partial-invalidation unreliability itself is still unknown. Signed-off-by: Michael Adler --- src/gui/ViewerWindow.cpp | 135 ++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 79 deletions(-) diff --git a/src/gui/ViewerWindow.cpp b/src/gui/ViewerWindow.cpp index accb8cbb..e4ea1bd8 100644 --- a/src/gui/ViewerWindow.cpp +++ b/src/gui/ViewerWindow.cpp @@ -1,5 +1,6 @@ +#include #include #include #include @@ -8,6 +9,7 @@ #include #include #include +#include #include #include "keyboardgrab/KeyboardGrabber.h" #include "res/vnccursor.xbm" @@ -54,9 +56,16 @@ class VNCCanvas: public wxPanel VNCCanvas(wxWindow* parent, VNCConn* c); void grab_keyboard(); void ungrab_keyboard(); + // called on the GUI thread as update notifications for `conn` arrive + void updateCache(const wxRect& rect); VNCConn* conn; - wxRegion updated_area; + + // Native-resolution framebuffer copy, kept current by updateCache() and + // always blitted whole in onPaint() -- see onUpdateTimer for why. + wxBitmap fb_cache; + bool dirty = false; + double scale_factor = 1.0; bool do_keyboard_grab; std::vector sync_targets; @@ -137,39 +146,51 @@ void VNCCanvas::ungrab_keyboard() -void VNCCanvas::onUpdateTimer(wxTimerEvent& event) +void VNCCanvas::updateCache(const wxRect& rect) { - // get the update rect list - wxRegionIterator upd(updated_area); - while(upd) +#ifndef NDEBUG + wxLongLong t0 = wxGetLocalTimeMillis(); +#endif + + wxSize fb_size(conn->getFrameBufferWidth(), conn->getFrameBufferHeight()); + if(fb_size.GetWidth() <= 0 || fb_size.GetHeight() <= 0) + return; + + // (re)allocate on first use or framebuffer resize + if(!fb_cache.IsOk() || fb_cache.GetSize() != fb_size) + fb_cache = wxBitmap(fb_size, 32); + + // scope the expensive per-pixel copy to just what changed + const wxBitmap& changed = conn->getFrameBufferRegion(rect); + if(changed.IsOk()) { - wxRect update_rect(upd.GetRect()); - - if(scale_factor != 1.0) { - update_rect.x *= scale_factor; - update_rect.y *= scale_factor; - update_rect.width *= scale_factor; - update_rect.height *= scale_factor; - - // fixes artifacts. +2 because double->int cuttofs can happen for x,y _and_ w,h scaling - update_rect.width += 2; - update_rect.height += 2; - } - - wxLogDebug(wxT("VNCCanvas %p: invalidating updated rect: (%i,%i,%i,%i)"), - this, - update_rect.x, - update_rect.y, - update_rect.width, - update_rect.height); - - // triggers onPaint() - Refresh(false, &update_rect); - ++upd; + wxMemoryDC dc(fb_cache); + dc.DrawBitmap(changed, rect.x, rect.y); } - updated_area.Clear(); + dirty = true; + +#ifndef NDEBUG + wxLongLong t1 = wxGetLocalTimeMillis(); + wxLogDebug(wxT("VNCCanvas %p: updateCache rect (%i,%i,%i,%i), %f megapixels, took %lld ms"), + this, + rect.x, rect.y, rect.width, rect.height, + (double)(rect.width * rect.height)/(1024.0*1024.0), + (t1-t0).GetValue()); +#endif +} + +void VNCCanvas::onUpdateTimer(wxTimerEvent& event) +{ + // Partial-rect invalidation is unreliable on this wx port -- some + // changed rects never get repainted. Always invalidate the whole + // canvas instead; fb_cache keeps that cheap. + if(dirty) + { + Refresh(false); + dirty = false; + } } @@ -177,8 +198,6 @@ void VNCCanvas::onPaint(wxPaintEvent &WXUNUSED(event)) { #ifndef NDEBUG wxLongLong t0 = wxGetLocalTimeMillis(); - size_t nr_rects = 0; - size_t nr_pixels = 0; #endif // this happens on GTK even if our size is (0,0) @@ -188,56 +207,14 @@ void VNCCanvas::onPaint(wxPaintEvent &WXUNUSED(event)) wxPaintDC dc(this); dc.SetUserScale(scale_factor, scale_factor); - // get the update rect list - wxRegionIterator upd(GetUpdateRegion()); - while(upd) - { - wxRect update_rect(upd.GetRect()); - - if(scale_factor != 1.0) { - update_rect.x /= scale_factor; - update_rect.y /= scale_factor; - update_rect.width /= scale_factor; - update_rect.height /= scale_factor; - - // fixes artifacts. +2 because double->int cuttofs can happen for x,y _and_ w,h scaling - update_rect.width += 2; - update_rect.height += 2; - - // make sure this is always within the framebuffer boudaries; - // might not always be due to the artifact fix above, would not be drawn then - update_rect.Intersect(wxRect(0, - 0, - conn->getFrameBufferWidth(), - conn->getFrameBufferHeight())); - } - - wxLogDebug(wxT("VNCCanvas %p: got repaint event: (%i,%i,%i,%i)"), - this, - update_rect.x, - update_rect.y, - update_rect.width, - update_rect.height); - -#ifndef NDEBUG - ++nr_rects; - nr_pixels += update_rect.width * update_rect.height; -#endif - - const wxBitmap& region = conn->getFrameBufferRegion(update_rect); - if(region.IsOk()) - dc.DrawBitmap(region, update_rect.x, update_rect.y); - - ++upd; - } + // fb_cache is already fully up to date -- just blit it, no per-pixel + // fetch here, and no per-rect tiling that could leave seams + if(fb_cache.IsOk()) + dc.DrawBitmap(fb_cache, 0, 0); #ifndef NDEBUG wxLongLong t1 = wxGetLocalTimeMillis(); - wxLogDebug(wxT("VNCCanvas %p: updating %zu rects (%f megapixels) took %lld ms"), - this, - nr_rects, - (double)nr_pixels/(1024.0*1024.0), - (t1-t0).GetValue()); + wxLogDebug(wxT("VNCCanvas %p: paint blit took %lld ms"), this, (t1-t0).GetValue()); #endif } @@ -502,7 +479,7 @@ void ViewerWindow::onVNCConnUpdateNotify(VNCConnUpdateNotifyEvent& event) // only do something if this is our VNCConn if(sending_conn == canvas->conn) - canvas->updated_area.Union(event.rect); + canvas->updateCache(event.rect); }