Add named markers to replay recordings as MP4 chapters#289
Conversation
Add a Mark(name) API to the recorder and a POST /recording/mark endpoint that records named, timestamped markers during an active recording. At finalize the markers are written into the output MP4 as chapter markers via an ffmetadata input, so they can be scrubbed to in any tool that reads MP4 chapters (ffprobe, QuickTime, VLC, NLEs). The feature is additive and backwards-compatible: with no markers the remux command is byte-identical to before, and any marker/metadata failure falls back to the plain remux so a recording is never corrupted. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Independent review — replay markers as MP4 chaptersReviewed the full diff, read all changed files, rebuilt, ran unit + integration tests, and verified a few correctness concerns with real ffmpeg/ffprobe. Not blocking — the core mechanism is sound and well-tested. A few minor items below. Verified correct
Minor / nits
Build green, |
buildChapterMetadata always received 0 for the origin shift, so remove the parameter, its drop-when-shifted branch, and the corresponding test. Chapter starts are now computed directly from each marker's offset. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Firetiger deploy monitoring skipped This PR didn't match the auto-monitor filter configured on your GitHub connection:
Reason: PR is in the To monitor this PR anyway, reply with |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Marker limit counts bytes
- Marker validation now enforces the 200-character limit using Unicode rune count instead of UTF-8 byte length, with regression tests for multibyte names.
- ✅ Fixed: OpenAPI omits name length
- The MarkRecordingRequest schema now declares name minLength 1 and maxLength 200 to match server-side validation.
Or push these changes by commenting:
@cursor push 804fa5149b
Preview (804fa5149b)
diff --git a/server/lib/recorder/ffmpeg.go b/server/lib/recorder/ffmpeg.go
--- a/server/lib/recorder/ffmpeg.go
+++ b/server/lib/recorder/ffmpeg.go
@@ -15,6 +15,7 @@
"sync"
"syscall"
"time"
+ "unicode/utf8"
"github.com/kernel/kernel-images/server/lib/logger"
"github.com/kernel/kernel-images/server/lib/scaletozero"
@@ -497,7 +498,7 @@
}
name = strings.TrimSpace(name)
- if name == "" || len(name) > maxMarkerNameLen {
+ if name == "" || utf8.RuneCountInString(name) > maxMarkerNameLen {
return "", 0, ErrInvalidMarkerName
}
diff --git a/server/lib/recorder/markers_test.go b/server/lib/recorder/markers_test.go
--- a/server/lib/recorder/markers_test.go
+++ b/server/lib/recorder/markers_test.go
@@ -156,6 +156,12 @@
_, _, err = rec.Mark(strings.Repeat("x", maxMarkerNameLen+1))
assert.ErrorIs(t, err, ErrInvalidMarkerName)
+
+ _, _, err = rec.Mark(strings.Repeat("界", maxMarkerNameLen))
+ assert.NoError(t, err)
+
+ _, _, err = rec.Mark(strings.Repeat("界", maxMarkerNameLen+1))
+ assert.ErrorIs(t, err, ErrInvalidMarkerName)
}
func TestMark_ConcurrentCallsAreSafe(t *testing.T) {
diff --git a/server/openapi.yaml b/server/openapi.yaml
--- a/server/openapi.yaml
+++ b/server/openapi.yaml
@@ -3484,6 +3484,8 @@
name:
type: string
description: Name of the marker, used as the MP4 chapter title.
+ minLength: 1
+ maxLength: 200
id:
type: string
description: Identifier of the recorder to mark. Alphanumeric or hyphen.You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 81d787d. Configure here.
|
|
||
| name = strings.TrimSpace(name) | ||
| if name == "" || len(name) > maxMarkerNameLen { | ||
| return "", 0, ErrInvalidMarkerName |
There was a problem hiding this comment.
Marker limit counts bytes
Low Severity
Marker name validation uses Go len (UTF-8 byte length) against a 200 limit, while the API error text and caller expectations refer to characters. Non-ASCII titles can be rejected below 200 visible characters, or the limit can diverge from what the OpenAPI contract implies.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 81d787d. Configure here.
| properties: | ||
| name: | ||
| type: string | ||
| description: Name of the marker, used as the MP4 chapter title. |
There was a problem hiding this comment.
OpenAPI omits name length
Low Severity
The MarkRecordingRequest name field has no minLength or maxLength, though the server enforces a non-empty trimmed name and a 200-unit maximum. Clients relying on the spec cannot discover these limits before calling the API.
Triggered by learned rule: OpenAPI request schemas must only expose server-honored fields
Reviewed by Cursor Bugbot for commit 81d787d. Configure here.
1 participant
What
Adds named, timestamped markers to screen recordings. A marker records a point in time during an active recording; at finalize the markers are injected into the output MP4 as chapter markers, so users can scrub/crop to them in post (ffprobe
-show_chapters, QuickTime, VLC, and most NLEs all read MP4 chapters).The feature is purely additive and backwards-compatible.
Why
Recordings are currently opaque blobs — there's no way to flag "the interesting thing happened here" while a session runs. MP4 chapters are the standard, tool-agnostic way to annotate timeline positions, so a marker dropped during recording survives into the downloaded file with no extra sidecar.
API
New endpoint
POST /recording/mark:{ "name": string (required), "id"?: string }201{ "name": string, "offsetMs": integer }on success409when no recording is in progress400for an empty/oversized name or missing bodyoffsetMsis documented as provisional (measured against the recording start time at mark time); the authoritative offset is the chapter start computed at finalize.How
Mark(name)on the recorder appends a marker under the existing mutex and is safe under concurrent calls.TIMEBASE=1/1000chapters, special characters escaped) and passed as a second input with-map 0 -map_chapters 1. A sentinel chapter is prepended atSTART=0because ffmpeg forces the first chapter to start at 0 — without it the first real marker's timestamp would be clamped.lib/oapi/oapi.gowas regenerated viamake oapi-generate(openapi-down-convert + oapi-codegen), not hand-edited.Tests
START=0, integerSTART/ENDtiling, negative-offset markers dropped, ascending ordering, special-character escaping, and a nonzero origin-shift parameter.Mark(): 409 path when not recording, trim/validation, and concurrency (run under-race).-map_chaptersis injected only when markers are present.testsrc, no display needed) that records a synthetic clip, marks at known offsets, finalizes, and asserts viaffprobe -show_chaptersthat chapters land within ~1.5 frame intervals.go vet ./...,go build ./..., and the full non-e2e suite (go test -race) pass locally.Note
Low Risk
Additive API and finalize path with explicit fallback when chapter injection fails; no-marker remux is unchanged and well-tested.
Overview
Adds
POST /recording/markso clients can drop named markers on an in-progress recording. The handler resolves recorderid(default when omitted), callsMark(name)on the recorder, and returns 201 withnameand provisionaloffsetMs; 400 / 409 / 500 cover missing body, bad names, and no active recording.The
Recorderinterface gainsMark.FFmpegRecorderstores markers under the existing mutex (concurrent-safe), validates trimmed names (max 200 chars), and at finalize optionally builds an ffmetadata sidecar and remuxes with-map_chapters 1. A_recording_startsentinel chapter preserves the first real marker offset when ffmpeg clamps chapter 0 to zero. With no markers—or if metadata build fails—the remux args stay the same as before; chapter injection failures only log and fall back.OpenAPI and generated
oapiclient/server stubs are updated for the new route and types. Tests cover API mapping, metadata builder edge cases, remux backward-compat,Markbehavior, and a real-ffmpegffprobechapter integration test.Reviewed by Cursor Bugbot for commit 81d787d. Bugbot is set up for automated code reviews on this repo. Configure here.