From 03034cf0895489b09f30c2e4ec96b395ce68e6c4 Mon Sep 17 00:00:00 2001 From: masnwilliams <43387599+masnwilliams@users.noreply.github.com> Date: Thu, 28 May 2026 02:07:48 +0000 Subject: [PATCH 1/2] Skip resize and chromium restart when display already matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PatchDisplay and the multipart Configure both unconditionally proceeded through xrandr/xvfb resize plus a chromium restart even when the requested width/height/refresh-rate already matched the current display state. Detect the no-op case and return success immediately — skipping the idle check, recording stop, resize, and restart. Extract the merge logic into resolveDisplayParams so both handlers share one definition of "the request changes nothing." Co-Authored-By: Claude Opus 4.7 --- server/cmd/api/api/chromium_configure.go | 17 +++--- server/cmd/api/api/display.go | 48 +++++++++++----- server/cmd/api/api/display_test.go | 70 ++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 23 deletions(-) diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go index 167ac529..a3505367 100644 --- a/server/cmd/api/api/chromium_configure.go +++ b/server/cmd/api/api/chromium_configure.go @@ -632,21 +632,18 @@ func chromiumPrepareDisplay(ctx context.Context, s *ApiService, displayJSON *str logger.FromContext(ctx).Error("failed to get current resolution", "error", err) return nil, cfg500ConfigureStep(chromiumConfigureStepDisplay, "failed to get current display resolution") } - width, height, refreshRate := currentWidth, currentHeight, currentRefreshRate - if body.Width != nil { - width = *body.Width - } - if body.Height != nil { - height = *body.Height - } - if body.RefreshRate != nil { - refreshRate = int(*body.RefreshRate) - } + width, height, refreshRate, changed := resolveDisplayParams(body, currentWidth, currentHeight, currentRefreshRate) if width <= 0 || height <= 0 { return nil, cfg400("invalid width/height") } + // Display already matches — return a nil plan so the caller skips the apply step. + if !changed { + logger.FromContext(ctx).Info("display already at requested resolution, skipping resize", "width", width, "height", height, "refreshRate", refreshRate) + return nil, nil + } + requireIdle := true if body.RequireIdle != nil { requireIdle = *body.RequireIdle diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 5dd29ff5..1b45429e 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -41,24 +41,24 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ log.Error("failed to get current resolution", "error", err) return oapi.PatchDisplay500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get current display resolution"}}, nil } - width := currentWidth - height := currentHeight - refreshRate := currentRefreshRate - - if req.Body.Width != nil { - width = *req.Body.Width - } - if req.Body.Height != nil { - height = *req.Body.Height - } - if req.Body.RefreshRate != nil { - refreshRate = int(*req.Body.RefreshRate) - } + width, height, refreshRate, changed := resolveDisplayParams(req.Body, currentWidth, currentHeight, currentRefreshRate) if width <= 0 || height <= 0 { return oapi.PatchDisplay400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: "invalid width/height"}}, nil } + // Display already matches the requested state. Skip the idle check, + // recording stop, resize, and chromium restart — all of which would be + // no-ops at the existing resolution. + if !changed { + log.Info("display already at requested resolution, skipping resize", "width", width, "height", height, "refreshRate", refreshRate) + return oapi.PatchDisplay200JSONResponse{ + Width: &width, + Height: &height, + RefreshRate: &refreshRate, + }, nil + } + log.Info(fmt.Sprintf("resolution change requested from %dx%d@%d to %dx%d@%d", currentWidth, currentHeight, currentRefreshRate, width, height, refreshRate)) // Parse requireIdle flag (default true) @@ -177,6 +177,28 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ }, nil } +// resolveDisplayParams merges the request body with the current display +// state, returning the final width, height, and refresh rate plus whether +// any field would actually change. Callers use the changed flag to skip +// the resize path when the request is a no-op. +func resolveDisplayParams(body *oapi.PatchDisplayJSONRequestBody, currentWidth, currentHeight, currentRefreshRate int) (width, height, refreshRate int, changed bool) { + width, height, refreshRate = currentWidth, currentHeight, currentRefreshRate + if body == nil { + return + } + if body.Width != nil { + width = *body.Width + } + if body.Height != nil { + height = *body.Height + } + if body.RefreshRate != nil { + refreshRate = int(*body.RefreshRate) + } + changed = width != currentWidth || height != currentHeight || refreshRate != currentRefreshRate + return +} + // detectDisplayMode detects whether we're running Xorg (headful) or Xvfb // (headless). The result is cached because the display server type does not // change during the container's lifetime, and querying supervisorctl during diff --git a/server/cmd/api/api/display_test.go b/server/cmd/api/api/display_test.go index 3a9d919c..24576f25 100644 --- a/server/cmd/api/api/display_test.go +++ b/server/cmd/api/api/display_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/kernel/kernel-images/server/lib/logger" + oapi "github.com/kernel/kernel-images/server/lib/oapi" "github.com/kernel/kernel-images/server/lib/recorder" "github.com/kernel/kernel-images/server/lib/scaletozero" "github.com/stretchr/testify/assert" @@ -253,6 +254,75 @@ func TestStopAndStartNewSegment_RoundTrip(t *testing.T) { _ = newRec.Stop(ctx) } +func TestResolveDisplayParams(t *testing.T) { + intPtr := func(v int) *int { return &v } + rrPtr := func(v int) *oapi.PatchDisplayRequestRefreshRate { + x := oapi.PatchDisplayRequestRefreshRate(v) + return &x + } + + cases := []struct { + name string + body *oapi.PatchDisplayJSONRequestBody + curW, curH, curRR int + wantW, wantH, wantRR int + wantChanged bool + }{ + { + name: "matching dims, no refresh rate", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080)}, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1920, wantH: 1080, wantRR: 25, + wantChanged: false, + }, + { + name: "matching dims and matching refresh rate", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080), RefreshRate: rrPtr(60)}, + curW: 1920, curH: 1080, curRR: 60, + wantW: 1920, wantH: 1080, wantRR: 60, + wantChanged: false, + }, + { + name: "different width", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1280), Height: intPtr(1080)}, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1280, wantH: 1080, wantRR: 25, + wantChanged: true, + }, + { + name: "different refresh rate, matching dims", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080), RefreshRate: rrPtr(60)}, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1920, wantH: 1080, wantRR: 60, + wantChanged: true, + }, + { + name: "only width supplied, matches current", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920)}, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1920, wantH: 1080, wantRR: 25, + wantChanged: false, + }, + { + name: "nil body", + body: nil, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1920, wantH: 1080, wantRR: 25, + wantChanged: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + w, h, rr, changed := resolveDisplayParams(tc.body, tc.curW, tc.curH, tc.curRR) + assert.Equal(t, tc.wantW, w) + assert.Equal(t, tc.wantH, h) + assert.Equal(t, tc.wantRR, rr) + assert.Equal(t, tc.wantChanged, changed) + }) + } +} + func TestProbeDisplayMode(t *testing.T) { t.Run("detects xvfb when config exists", func(t *testing.T) { svc := &ApiService{} From b35386a80c3988c0194539561ba678cea60a71bb Mon Sep 17 00:00:00 2001 From: masnwilliams <43387599+masnwilliams@users.noreply.github.com> Date: Fri, 29 May 2026 15:39:34 +0000 Subject: [PATCH 2/2] Persist last-applied refresh rate for headless display Xvfb does not surface a refresh rate via xrandr, so getCurrentResolutionFromXrandr always falls back to 60. After the background Xvfb resize cleared the viewport override, the next identical request at a non-default rate (e.g. 1280x720@25) was not detected as a no-op because the comparison saw 60. Keep lastHeadlessRefreshRate as a sticky field that survives override clears, and have getCurrentResolution prefer it whenever xrandr did not surface a real rate. Set it from every headless apply path (setViewportOverride, the synchronous PatchDisplay xvfb branch, and the chromium-configure xvfb branch). --- server/cmd/api/api/api.go | 10 ++- server/cmd/api/api/chromium_configure.go | 1 + server/cmd/api/api/display.go | 58 ++++++++++++---- server/cmd/api/api/display_test.go | 84 ++++++++++++++++-------- 4 files changed, 111 insertions(+), 42 deletions(-) diff --git a/server/cmd/api/api/api.go b/server/cmd/api/api/api.go index 887d6613..08857a86 100644 --- a/server/cmd/api/api/api.go +++ b/server/cmd/api/api/api.go @@ -68,9 +68,13 @@ type ApiService struct { // viewportOverride stores the last viewport dimensions set via CDP so // that getCurrentResolution can return consistent values even while - // Xvfb is restarting in the background. - viewportMu sync.RWMutex - viewportOverride *[3]int // [width, height, refreshRate] or nil + // Xvfb is restarting in the background. lastHeadlessRefreshRate + // persists across override clears because Xvfb does not surface the + // refresh rate via xrandr — without it, repeat requests at the same + // non-default rate would not be detected as no-ops. + viewportMu sync.RWMutex + viewportOverride *[3]int // [width, height, refreshRate] or nil + lastHeadlessRefreshRate int // cachedDisplayMode caches the result of detectDisplayMode since the // display server type (xorg vs xvfb) does not change at runtime. diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go index a3505367..d4cbab72 100644 --- a/server/cmd/api/api/chromium_configure.go +++ b/server/cmd/api/api/chromium_configure.go @@ -680,6 +680,7 @@ func chromiumDisplayApplyWhileStopped(ctx context.Context, s *ApiService, plan * err := s.resizeXvfb(ctx, w, h) if err == nil { s.clearViewportOverride() + s.recordHeadlessRefreshRate(rr) } s.xvfbResizeMu.Unlock() if err != nil { diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 92715374..955320e3 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -137,6 +137,7 @@ func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequ err = s.resizeXvfb(ctx, width, height) if err == nil { s.clearViewportOverride() + s.recordHeadlessRefreshRate(refreshRate) } s.xvfbResizeMu.Unlock() if err == nil { @@ -446,37 +447,70 @@ func (s *ApiService) resolveDisplayFromEnv() string { // setViewportOverride stores the last-known viewport dimensions so // getCurrentResolution can return them even while Xvfb is restarting. +// It also records the refresh rate as the sticky headless value. func (s *ApiService) setViewportOverride(width, height, refreshRate int) { s.viewportMu.Lock() s.viewportOverride = &[3]int{width, height, refreshRate} + if refreshRate > 0 { + s.lastHeadlessRefreshRate = refreshRate + } s.viewportMu.Unlock() } // clearViewportOverride removes the viewport override (e.g. after Xvfb -// finishes restarting and xrandr is reliable again). +// finishes restarting and xrandr is reliable again). The sticky +// lastHeadlessRefreshRate is intentionally preserved so that +// getCurrentResolution can still return the requested refresh rate after +// xrandr stops surfacing it. func (s *ApiService) clearViewportOverride() { s.viewportMu.Lock() s.viewportOverride = nil s.viewportMu.Unlock() } +// recordHeadlessRefreshRate stores the last refresh rate applied via a +// headless path so identical follow-up requests are detected as no-ops. +// Zero values are ignored — they signal "rate not specified". +func (s *ApiService) recordHeadlessRefreshRate(refreshRate int) { + if refreshRate <= 0 { + return + } + s.viewportMu.Lock() + s.lastHeadlessRefreshRate = refreshRate + s.viewportMu.Unlock() +} + // getCurrentResolution returns the current display resolution and refresh // rate. If a viewport override is set (from a recent CDP resize while Xvfb // restarts in the background), it returns the override instead of querying -// xrandr, which may fail during Xvfb restarts. +// xrandr, which may fail during Xvfb restarts. When xrandr does not surface +// a refresh rate (Xvfb), the last-applied headless rate is used so repeat +// requests at the same rate are detected as no-ops. func (s *ApiService) getCurrentResolution(ctx context.Context) (int, int, int, error) { s.viewportMu.RLock() override := s.viewportOverride + stickyRate := s.lastHeadlessRefreshRate s.viewportMu.RUnlock() if override != nil { return override[0], override[1], override[2], nil } - return s.getCurrentResolutionFromXrandr(ctx) + w, h, rr, rateFromXrandr, err := s.getCurrentResolutionFromXrandr(ctx) + if err != nil { + return 0, 0, 0, err + } + if !rateFromXrandr && stickyRate > 0 { + rr = stickyRate + } + return w, h, rr, nil } -// getCurrentResolutionFromXrandr queries xrandr for the current display resolution. -func (s *ApiService) getCurrentResolutionFromXrandr(ctx context.Context) (int, int, int, error) { +// getCurrentResolutionFromXrandr queries xrandr for the current display +// resolution. The fourth return value reports whether the refresh rate was +// parsed from xrandr output (true) or is the synthesized fallback (false); +// callers can use it to prefer a previously-recorded rate when xrandr is +// silent, as Xvfb is. +func (s *ApiService) getCurrentResolutionFromXrandr(ctx context.Context) (int, int, int, bool, error) { log := logger.FromContext(ctx) display := s.resolveDisplayFromEnv() @@ -488,41 +522,43 @@ func (s *ApiService) getCurrentResolutionFromXrandr(ctx context.Context) (int, i out, err := cmd.Output() if err != nil { log.Error("failed to get current resolution", "error", err) - return 0, 0, 0, fmt.Errorf("failed to execute xrandr command: %w", err) + return 0, 0, 0, false, fmt.Errorf("failed to execute xrandr command: %w", err) } resStr := strings.TrimSpace(string(out)) parts := strings.Split(resStr, "x") if len(parts) != 2 { log.Error("unexpected xrandr output format", "output", resStr) - return 0, 0, 0, fmt.Errorf("unexpected xrandr output format: %s", resStr) + return 0, 0, 0, false, fmt.Errorf("unexpected xrandr output format: %s", resStr) } width, err := strconv.Atoi(parts[0]) if err != nil { log.Error("failed to parse width", "error", err, "value", parts[0]) - return 0, 0, 0, fmt.Errorf("failed to parse width '%s': %w", parts[0], err) + return 0, 0, 0, false, fmt.Errorf("failed to parse width '%s': %w", parts[0], err) } // Parse height and refresh rate (e.g., "1080_60.00" -> height=1080, rate=60) heightStr := parts[1] - refreshRate := 60 // default + refreshRate := 60 // default when xrandr omits the _rate suffix (e.g. Xvfb) + rateFromXrandr := false if idx := strings.Index(heightStr, "_"); idx != -1 { rateStr := heightStr[idx+1:] heightStr = heightStr[:idx] // Parse the refresh rate (e.g., "60.00" -> 60) if rateFloat, err := strconv.ParseFloat(rateStr, 64); err == nil { refreshRate = int(rateFloat) + rateFromXrandr = true } } height, err := strconv.Atoi(heightStr) if err != nil { log.Error("failed to parse height", "error", err, "value", heightStr) - return 0, 0, 0, fmt.Errorf("failed to parse height '%s': %w", heightStr, err) + return 0, 0, 0, false, fmt.Errorf("failed to parse height '%s': %w", heightStr, err) } - return width, height, refreshRate, nil + return width, height, refreshRate, rateFromXrandr, nil } // stoppedRecordingInfo holds state captured from a recording that was stopped diff --git a/server/cmd/api/api/display_test.go b/server/cmd/api/api/display_test.go index 24576f25..154b6359 100644 --- a/server/cmd/api/api/display_test.go +++ b/server/cmd/api/api/display_test.go @@ -262,52 +262,52 @@ func TestResolveDisplayParams(t *testing.T) { } cases := []struct { - name string - body *oapi.PatchDisplayJSONRequestBody - curW, curH, curRR int + name string + body *oapi.PatchDisplayJSONRequestBody + curW, curH, curRR int wantW, wantH, wantRR int - wantChanged bool + wantChanged bool }{ { - name: "matching dims, no refresh rate", - body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080)}, - curW: 1920, curH: 1080, curRR: 25, - wantW: 1920, wantH: 1080, wantRR: 25, + name: "matching dims, no refresh rate", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080)}, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1920, wantH: 1080, wantRR: 25, wantChanged: false, }, { - name: "matching dims and matching refresh rate", - body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080), RefreshRate: rrPtr(60)}, - curW: 1920, curH: 1080, curRR: 60, - wantW: 1920, wantH: 1080, wantRR: 60, + name: "matching dims and matching refresh rate", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080), RefreshRate: rrPtr(60)}, + curW: 1920, curH: 1080, curRR: 60, + wantW: 1920, wantH: 1080, wantRR: 60, wantChanged: false, }, { - name: "different width", - body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1280), Height: intPtr(1080)}, - curW: 1920, curH: 1080, curRR: 25, - wantW: 1280, wantH: 1080, wantRR: 25, + name: "different width", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1280), Height: intPtr(1080)}, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1280, wantH: 1080, wantRR: 25, wantChanged: true, }, { - name: "different refresh rate, matching dims", - body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080), RefreshRate: rrPtr(60)}, - curW: 1920, curH: 1080, curRR: 25, - wantW: 1920, wantH: 1080, wantRR: 60, + name: "different refresh rate, matching dims", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920), Height: intPtr(1080), RefreshRate: rrPtr(60)}, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1920, wantH: 1080, wantRR: 60, wantChanged: true, }, { - name: "only width supplied, matches current", - body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920)}, - curW: 1920, curH: 1080, curRR: 25, - wantW: 1920, wantH: 1080, wantRR: 25, + name: "only width supplied, matches current", + body: &oapi.PatchDisplayJSONRequestBody{Width: intPtr(1920)}, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1920, wantH: 1080, wantRR: 25, wantChanged: false, }, { - name: "nil body", - body: nil, - curW: 1920, curH: 1080, curRR: 25, - wantW: 1920, wantH: 1080, wantRR: 25, + name: "nil body", + body: nil, + curW: 1920, curH: 1080, curRR: 25, + wantW: 1920, wantH: 1080, wantRR: 25, wantChanged: false, }, } @@ -323,6 +323,34 @@ func TestResolveDisplayParams(t *testing.T) { } } +func TestHeadlessRefreshRateSticky(t *testing.T) { + t.Run("setViewportOverride records sticky rate", func(t *testing.T) { + svc := &ApiService{} + svc.setViewportOverride(1280, 720, 25) + assert.Equal(t, 25, svc.lastHeadlessRefreshRate) + }) + + t.Run("clearViewportOverride preserves sticky rate", func(t *testing.T) { + svc := &ApiService{} + svc.setViewportOverride(1280, 720, 25) + svc.clearViewportOverride() + assert.Nil(t, svc.viewportOverride) + assert.Equal(t, 25, svc.lastHeadlessRefreshRate) + }) + + t.Run("recordHeadlessRefreshRate ignores zero", func(t *testing.T) { + svc := &ApiService{lastHeadlessRefreshRate: 30} + svc.recordHeadlessRefreshRate(0) + assert.Equal(t, 30, svc.lastHeadlessRefreshRate) + }) + + t.Run("recordHeadlessRefreshRate updates sticky rate", func(t *testing.T) { + svc := &ApiService{lastHeadlessRefreshRate: 30} + svc.recordHeadlessRefreshRate(75) + assert.Equal(t, 75, svc.lastHeadlessRefreshRate) + }) +} + func TestProbeDisplayMode(t *testing.T) { t.Run("detects xvfb when config exists", func(t *testing.T) { svc := &ApiService{}