Encode one framebuffer update at a single negotiated pixel format

client/vnc/server/session_cursor.go

@@ -11,13 +11,17 @@ import (
 // when the client negotiated the encoding and the platform exposes a
 // cursor source whose serial has changed since the last emission. A nil
 // return means "do not include a cursor rect in this FramebufferUpdate".
-func (s *session) pendingCursorRect() []byte {
+// pf is the format the rest of this update is being encoded in, passed in
+// rather than read again here: a SetPixelFormat landing mid-update would
+// otherwise pack the cursor at the new shifts and the framebuffer at the old
+// ones, and the client would paint a correctly coloured desktop under a cursor
+// with its channels swapped.
+func (s *session) pendingCursorRect(pf clientPixelFormat) []byte {
 	s.encMu.RLock()
 	supported := s.clientSupportsCursor
 	failed := s.cursorSourceFailed
 	composite := s.showRemoteCursor
 	lastSerial := s.lastCursorSerial
-	pf := s.pf
 	s.encMu.RUnlock()
 	if !supported || failed || composite {
 		return nil

client/vnc/server/session_cursor_test.go

@@ -47,13 +47,13 @@ func TestPendingCursorRect_SwitchingBackToALowerSerial(t *testing.T) {
 	s := newCursorSession(src)
 
 	// The arrow, then an I-beam the X server happens to number higher.
-	require.NotNil(t, s.pendingCursorRect(), "first cursor must be sent")
+	require.NotNil(t, s.pendingCursorRect(defaultClientPixelFormat()), "first cursor must be sent")
 	src.serial = 250
-	require.NotNil(t, s.pendingCursorRect(), "a different cursor must be sent")
+	require.NotNil(t, s.pendingCursorRect(defaultClientPixelFormat()), "a different cursor must be sent")
 
 	// Back to the arrow: a lower serial, and still a real change.
 	src.serial = 100
-	assert.NotNil(t, s.pendingCursorRect(), "returning to an earlier cursor must be sent, not dropped as stale")
+	assert.NotNil(t, s.pendingCursorRect(defaultClientPixelFormat()), "returning to an earlier cursor must be sent, not dropped as stale")
 }
 
 // An unchanged serial is still the one case that must not produce a rect,
@@ -63,6 +63,6 @@ func TestPendingCursorRect_UnchangedSerialIsSkipped(t *testing.T) {
 	src := &stubCursorSource{img: sprite, serial: 7}
 	s := newCursorSession(src)
 
-	require.NotNil(t, s.pendingCursorRect())
+	require.NotNil(t, s.pendingCursorRect(defaultClientPixelFormat()))
-	assert.Nil(t, s.pendingCursorRect(), "the same cursor must not be re-sent")
+	assert.Nil(t, s.pendingCursorRect(defaultClientPixelFormat()), "the same cursor must not be re-sent")
 }

client/vnc/server/session_encode.go

@@ -440,7 +440,11 @@ func (s *session) swapPrevCur() {
 // pseudo-rect into the same message so a shape change (e.g. hovering onto
 // a resize handle) reaches the client without waiting for a dirty frame.
 func (s *session) sendEmptyUpdate() error {
-	cursorRect := s.pendingCursorRect()
+	s.encMu.RLock()
+	pf := s.pf
+	s.encMu.RUnlock()
+
+	cursorRect := s.pendingCursorRect(pf)
 	if cursorRect == nil {
 		var buf [4]byte
 		buf[0] = serverFramebufferUpdate
@@ -464,7 +468,7 @@ func (s *session) sendFullUpdate(img *image.RGBA) error {
 	zlib := s.zlib
 	s.encMu.RUnlock()
 
-	cursorRect := s.pendingCursorRect()
+	cursorRect := s.pendingCursorRect(pf)
 	rectCount := uint16(1)
 	if cursorRect != nil {
 		rectCount++
@@ -534,7 +538,14 @@ func (s *session) writeFramed(buf []byte) error {
 // their source tiles are read from the client's pre-update framebuffer state,
 // before any subsequent rect overwrites them.
 func (s *session) sendDirtyAndMoves(img *image.RGBA, moves []copyRectMove, rects [][4]int) error {
-	cursorRect := s.pendingCursorRect()
+	// One snapshot for the whole message: every tile below and the cursor rect
+	// are packed at these shifts, so a SetPixelFormat arriving mid-update
+	// cannot leave one rectangle in a different format from its neighbours.
+	s.encMu.RLock()
+	pf := s.pf
+	s.encMu.RUnlock()
+
+	cursorRect := s.pendingCursorRect(pf)
 	if len(moves) == 0 && len(rects) == 0 && cursorRect == nil {
 		return nil
 	}
@@ -570,7 +581,7 @@ func (s *session) sendDirtyAndMoves(img *image.RGBA, moves []copyRectMove, rects
 
 	for _, r := range rects {
 		x, y, w, h := r[0], r[1], r[2], r[3]
-		rectBuf := s.encodeTile(img, x, y, w, h)
+		rectBuf := s.encodeTile(img, x, y, w, h, pf)
 		if _, err := s.conn.Write(rectBuf); err != nil {
 			return err
 		}
@@ -587,9 +598,10 @@ func (s *session) sendDirtyAndMoves(img *image.RGBA, moves []copyRectMove, rects
 //
 // Output omits the 4-byte FramebufferUpdate header; callers combine multiple
 // tiles into one message.
-func (s *session) encodeTile(img *image.RGBA, x, y, w, h int) []byte {
+// pf is the caller's snapshot, so every tile of one update packs at the same
+// shifts even if the client renegotiates the format while it is being built.
+func (s *session) encodeTile(img *image.RGBA, x, y, w, h int, pf clientPixelFormat) []byte {
 	s.encMu.RLock()
-	pf := s.pf
 	useHextile := s.useHextile
 	useTight := s.useTight
 	tight := s.tight