Skip to content

Add raw-frame streaming endpoint to BaseVideo alongside MJPEG - #766

Merged
thusser merged 3 commits into
developfrom
feature/basevideo-raw-frame-streaming
Aug 16, 2026
Merged

Add raw-frame streaming endpoint to BaseVideo alongside MJPEG#766
thusser merged 3 commits into
developfrom
feature/basevideo-raw-frame-streaming

Conversation

@thusser

@thusser thusser commented Aug 16, 2026

Copy link
Copy Markdown
Member

Implements the basevideo-raw-frame-streaming plan/design.

What

  • New /video.raw multipart endpoint: JSON FITS-keyed meta header (X-Pyobs-Frame-Meta) + raw little-endian bytes, event-driven via a new asyncio.Event with latest-frame-wins backpressure (coalesces bursts into a single wake).
  • VideoCapabilities.video renamed to mjpeg, plus a new raw field (both str | None). Collapsed live_view + video_path into a single video_path: str | None, added raw_path: str | None. /, /video.mjpg, and /video.raw routes are now registered only when their path is configured.
  • Split add_fits_headers() into add_local_fits_headers() (no VFS I/O) plus the persistent FRAMENUM VFS step, so the raw path builds headers without per-frame VFS writes.
  • Added Image.from_ndarray() reconstruction helper (consumer side of the wire contract).
  • Fixed video_handler's hardcoded 1 fps interval (now uses self._interval).
  • Lowered sleep_time default 600s -> 60s.
  • Also guarded the once-per-frame DET-CPX warning (it fired every frame when centre is unset).

Breaking changes

  • VideoCapabilities.video removed in favor of mjpeg/raw.
  • BaseVideo(live_view=...) removed; use video_path=None to disable the MJPEG live view.

Both acceptable per the plan (project is 2.0.0.dev).

Tests

Added coverage for: capability round-trip (XML), route gating, _new_frame set + coalescing, raw meta/little-endian framing, raw handler wake/write + activity, add_local_fits_headers VFS-free, and Image.from_ndarray.

- New /video.raw multipart endpoint: JSON FITS-keyed meta header + raw
  little-endian bytes, event-driven with latest-frame-wins backpressure.
- VideoCapabilities.video -> mjpeg, add raw (both str | None); collapse
  live_view into video_path, add raw_path.
- Split add_fits_headers() into add_local_fits_headers() (no VFS I/O)
  plus the persistent FRAMENUM step; guard the per-frame centre warning.
- Add Image.from_ndarray() reconstruction helper.
- Fix video_handler's hardcoded 1 fps interval; lower sleep_time default
  to 60s.
@thusser

thusser commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Code review

Found 6 issues, most-severe first.

  1. raw_handler can let the camera sleep out from under it forever. Activity is only refreshed on frame arrival (await self.activate_camera() runs right after _new_frame.wait() returns). If the producer gates frame generation behind _active (as DummyVideo._frame_task does — the only in-repo BaseVideo subclass), and the frame interval approaches/exceeds sleep_time (now 60s, was 600s), _active_update() deactivates the camera, no more frames get produced, and the handler blocks on _new_frame.wait() forever — never reactivates, and can't even detect client disconnect since it never reaches response.write(). This directly contradicts specs/design/basevideo-raw-frame-streaming.md §5, which requires the raw handler to keep touching _active_time continuously (mirroring video_handler's poll loop) specifically to prevent a raw-only session from letting the camera sleep out from under it.

# wait for a new frame; the event coalesces multiple frames that
# arrive while a slow consumer is still writing into a single wake
await self._new_frame.wait()
self._new_frame.clear()
# keep activity fresh for the duration of this connection

  1. DATE-OBS is stamped at send time, not capture time. _raw_frame() only receives the bare last.data ndarray — no capture timestamp travels with it — so DATE-OBS reflects when the handler happened to process the frame, not when it was acquired. Under the coalescing this code explicitly expects (multiple _set_image() calls collapsing into one wake) or event-loop delay, this can drift arbitrarily from true acquisition time. The FITS path (_create_image/grab_data()) avoids this by threading NextImage.date_obs through.

# path (no VFS I/O, no cross-module comm) -- see design doc §3
image = Image(data)
image.header["DATE-OBS"] = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S.%f")
image.header["IMAGETYP"] = self._image_type

  1. Redundant per-client work. _raw_frame() runs inside each connection's own raw_handler task rather than once per frame and shared. With N simultaneous raw clients (explicitly anticipated by the design doc — "guiding, a recorder, a second viewer"), the header build, JSON serialization, and ascontiguousarray+tobytes() copy all happen N times per frame for identical output.

# path (no VFS I/O, no cross-module comm) -- see design doc §3
image = Image(data)
image.header["DATE-OBS"] = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S.%f")
image.header["IMAGETYP"] = self._image_type

  1. Building the meta dict by iterating image.header collapses duplicate FITS keys. for key in image.header: meta[key] = ... writes into a plain dict, so duplicate/commentary cards (COMMENT/HISTORY, or a configured fits_headers entry colliding with a computed key) silently lose all but the last value on the wire. Not covered by any test.

meta: dict[str, Any] = {}
for key in image.header:
meta[key] = self._json_safe(image.header[key])
meta["DTYPE"] = data.dtype.newbyteorder("<").str

  1. _json_safe() duplicates existing numpy-scalar normalization. pyobs/comm/xmpp/serializer.py:73-74 already has if isinstance(value, np.generic): value = value.item() for the same reason. This adds a second local copy of the same idiom instead of reusing/extracting a shared helper.

@staticmethod
def _json_safe(value: Any) -> Any:
"""Convert a FITS header value to a JSON-serializable Python scalar."""
if isinstance(value, np.generic):
return value.item()
return str(value) if isinstance(value, StrEnum) else value

  1. Plan checklist claims a trade-off comment that isn't there. The checklist item is marked [x] and requires flagging "in a code comment that it trades off against _activate_camera()/_deactivate_camera() cost" — but sleep_time: int = 60 has no such comment anywhere in the file.

— see design doc §3 for why. This is producer/wire-contract scope (`pyobs-core`), not a
guiding consumer module.
- [x] Lower `sleep_time`'s default from 600s to **60s** (`basevideo.py`, param TBD exact location) —
a raw-only consumer session that dies previously left hardware streaming for up to 10 minutes
with nothing consuming it. 60s is a starting point, not measured — flag in a code comment that
it trades off against `_activate_camera()`/`_deactivate_camera()` cost (driver-specific,
mostly unknown today) and risks flapping if that cost is high relative to 60s. Stays a single

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@thusser

thusser commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Thanks, useful review. I agree with 1, 2, and 6 and will fix those. Pushing back on 4 and 5, and 3 is real but has a tradeoff.

  1. Valid, real liveness bug. Will keep _active_time fresh independent of frame arrival.

  2. Valid. Will capture the timestamp in _set_image() and carry it in LastImage instead of stamping at send time.

  3. Real, but precomputing in _set_image() (the obvious fix) contradicts the design's "costs nothing when no raw client is connected". Would need a lazy cache keyed by frame_num. Happy to do it if multiple simultaneous raw consumers is a near-term reality, otherwise I'd leave it.

  4. Not as stated. astropy stores COMMENT/HISTORY as a _HeaderCommentaryCards list, so meta['COMMENT'] = header['COMMENT'] preserves all cards; iteration yields the key twice but writes the same full list both times. And non-commentary duplicates can't happen here because the mixin uses header[key] = ... assignment (overwrite), never append. Nothing is lost.

  5. Technically true but it's a two-line idiom; extracting a shared helper would couple the camera module to the XMPP serializer. The StrEnum branch isn't in the serializer's path either. I'd leave it.

  6. Valid, sloppy of me. Will add the comment.

@thusser

thusser commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Filed #769 to track the lazy per-frame-num cache for #3 (deferred until multiple simultaneous raw consumers are a near-term reality). Sounds good on the rest — thanks.

@thusser

thusser commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Agreed, that's the right split. One consequence worth stating up front for #1: refreshing _active_time independent of frame arrival means a connected-but-idle raw client keeps the hardware awake indefinitely (producer paused/dead but connection still held). That's correct per design §5 and matches what video_handler already does, just noting it so it's a deliberate behavior, not an accident.

…ocument sleep_time trade-off

Addresses review findings on #766:
- raw_handler now bounds its wait on a new frame with a timeout, re-touching
  _active_time even when no frame arrives, so a connected-but-idle raw client
  can no longer let the camera sleep out from under it.
- DATE-OBS is now captured in _set_image() at acquisition time instead of at
  send time in _raw_frame(), avoiding drift under frame coalescing or
  scheduling delay.
- Documents the sleep_time default's cost trade-off in a code comment, as the
  plan required.
# Conflicts:
#	pyobs/modules/camera/basevideo.py
@thusser
thusser merged commit 1c20552 into develop Aug 16, 2026
3 checks passed
@thusser
thusser deleted the feature/basevideo-raw-frame-streaming branch August 16, 2026 19:57
thusser added a commit that referenced this pull request Aug 16, 2026
PR #766 merged. Records the three bugs found and fixed during review
(raw_handler liveness, DATE-OBS send-time stamping, missing sleep_time
trade-off comment) and the follow-up filed as issue #769 for the
per-client redundant frame-build work.
thusser added a commit that referenced this pull request Sep 2, 2026
* Add raw-frame streaming endpoint to BaseVideo alongside MJPEG

- New /video.raw multipart endpoint: JSON FITS-keyed meta header + raw
  little-endian bytes, event-driven with latest-frame-wins backpressure.
- VideoCapabilities.video -> mjpeg, add raw (both str | None); collapse
  live_view into video_path, add raw_path.
- Split add_fits_headers() into add_local_fits_headers() (no VFS I/O)
  plus the persistent FRAMENUM step; guard the per-frame centre warning.
- Add Image.from_ndarray() reconstruction helper.
- Fix video_handler's hardcoded 1 fps interval; lower sleep_time default
  to 60s.

* Fix raw-handler activity liveness, capture DATE-OBS at acquisition, document sleep_time trade-off

Addresses review findings on #766:
- raw_handler now bounds its wait on a new frame with a timeout, re-touching
  _active_time even when no frame arrives, so a connected-but-idle raw client
  can no longer let the camera sleep out from under it.
- DATE-OBS is now captured in _set_image() at acquisition time instead of at
  send time in _raw_frame(), avoiding drift under frame coalescing or
  scheduling delay.
- Documents the sleep_time default's cost trade-off in a code comment, as the
  plan required.
thusser added a commit that referenced this pull request Sep 2, 2026
PR #766 merged. Records the three bugs found and fixed during review
(raw_handler liveness, DATE-OBS send-time stamping, missing sleep_time
trade-off comment) and the follow-up filed as issue #769 for the
per-client redundant frame-build work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant