fix: Windows stability issues (console encoding, camera enumeration, … - #72
fix: Windows stability issues (console encoding, camera enumeration, …#72ZouzouWP wants to merge 1 commit into
Conversation
…video encoder, error visibility) Four separate crashes that only show up on Windows with real hardware attached. Grouped in one PR because they were all found the same week, not because they're related — happy to split if preferred. ## 1. Console encoding crash (cp1252) **Symptom:** an emoji in a log/print line could raise an unhandled `UnicodeEncodeError` on a Windows console using the legacy cp1252 code page, turning an endpoint into a blank 500 with no useful message reaching the frontend. **Fix:** `stdout`/`stderr` are reconfigured with `errors="replace"` at import time, so unencodable characters degrade instead of crashing the process. ## 2. Camera list sometimes empty **Symptom:** `/available-cameras` would intermittently return an empty or wrong list, depending on which FastAPI worker thread handled the request. **Root cause:** `pygrabber` enumerates cameras via COM, which must be initialized per-thread. FastAPI's thread pool never did that, so enumeration silently failed on threads that hadn't. **Fix:** explicitly initialize/uninitialize COM around the enumeration call. ## 3. Recording crashes with "no status packet" **Symptom:** recording sessions aborted mid-way with a serial `ConnectionError`, non-deterministically, on otherwise-healthy hardware. **Root cause:** the default video encoder (libsvtav1) is CPU-heavy enough that two parallel encode streams could starve the motor-bus read loop of CPU time. **Fix:** default to h264/ultrafast, measured ~11x faster to encode on this hardware. ## 4. Real errors hidden behind "check logs" **Symptom:** any recording failure showed the same generic message to the user — the real exception (exactly what let us diagnose huggingface#2 and huggingface#3 above) never left the server logs. **Fix:** `/recording-status` now includes the actual exception message. ## Testing Each fix corresponds to a crash reproduced and captured in server logs beforehand, then confirmed resolved after the change.
nicolas-rabault
left a comment
There was a problem hiding this comment.
Thanks a lot for this, genuinely useful work. Chasing four separate crashes on real hardware and coming back with a root cause for each one, COM per-thread init and encoder CPU starvation in particular, is exactly the kind of debugging that is hard to do remotely. The write up made the review easy to follow.
Three things before merge:
ruff checkfails with 12 E402 errors from the stdout reconfigure placement, so Quality CI will go red. Moving the block intolelab/scripts/lelab.pyfixes it and is a better home anyway.- Adding
dataset_repo_idto the error dict silently changeshandle_get_dataset_info, so a partially recorded dataset can no longer be read from disk. Gating on success should sort it. - The encoder change is the one I would like to discuss rather than merge as is. lerobot already exposes
vcodec="auto"(hardware encoders with an automatic fallback) andencoder_threads, both of which target the CPU starvation more directly without changing behavior for Mac and Linux users. There is also a hard failure risk if the installed PyAV lacks libx264.
On splitting: yes please. The three Windows scoped fixes are low risk and can land as soon as the E402 and the dataset info regression are addressed. The encoder change deserves its own PR with file size numbers next to the encode speed numbers, since it alters what gets stored and pushed to the Hub.
A few tests would help too, and the suite already covers these exact functions. A test that handle_get_dataset_info still loads a partial dataset after a failed session would have caught point 2.
Really appreciate the effort here, happy to help with the encoder follow up if useful.
| # emoji used in log/print output; an unhandled UnicodeEncodeError inside an | ||
| # endpoint then turns into a 500 and hides the real error from the frontend. | ||
| # Replace unencodable characters instead of crashing. | ||
| for _stream in (sys.stdout, sys.stderr): |
There was a problem hiding this comment.
Blocking: this statement sits between the stdlib imports and the local ones, so every from . import below it trips E402. ruff check reports 12 errors on this branch (base is clean), and quality.yml runs pre-commit on every PR, so CI will fail.
Suggest moving it into main() in lelab/scripts/lelab.py. Process wide stdio setup belongs at the entrypoint rather than as an import side effect, and it sidesteps E402 entirely.
| for _stream in (sys.stdout, sys.stderr): | ||
| if _stream is not None and hasattr(_stream, "reconfigure"): | ||
| with contextlib.suppress(Exception): | ||
| _stream.reconfigure(errors="replace") |
There was a problem hiding this comment.
Minor: errors="replace" turns the emoji into ?. Adding encoding="utf-8" makes them render correctly on Windows Terminal and modern conhost.
Alternatively, dropping the emoji from the log lines in record.py (four 📡 lines around 414-419) removes the failure mode at the source.
| import ctypes | ||
|
|
||
| com_initialized = False | ||
| try: |
There was a problem hiding this comment.
The fix is right, including the hr in (0, 1) guard that correctly skips CoUninitialize on RPC_E_CHANGED_MODE.
Two things though: except Exception: pass swallows silently, and on macOS/Linux ctypes.windll raises AttributeError on every call (the existing tests do call _windows_cameras() off Windows). Suggest guarding on sys.platform == "win32" and logging at debug instead of swallowing.
| # libsvtav1 (lerobot's default) is CPU-heavy enough on this machine to starve | ||
| # the motor bus read loop mid-recording ("no status packet" ConnectionError, | ||
| # see 2026-07-15 crashes). h264/ultrafast measured ~11x faster to encode here. | ||
| rgb_encoder=RGBEncoderConfig(vcodec="h264", preset="ultrafast"), |
There was a problem hiding this comment.
Blocking, and the main discussion point. This changes the default encoder for every platform, not just Windows.
lerobot v0.6.0 already has better levers for this exact problem:
vcodec="auto"probesh264_videotoolbox/h264_nvenc/h264_vaapi/h264_qsvand falls back tolibsvtav1. Hardware encoding is near zero CPU, which is the actual root cause, and it does not degrade Mac and Linux.DatasetRecordConfig.encoder_threadscaps encoder threads directly, which is the surgical fix for "encoder starves the motor bus read loop".
Risk worth checking before merge: RGBEncoderConfig.__post_init__ calls resolve_vcodec(), which raises ValueError: Unsupported video codec: h264 when the installed PyAV has no libx264 encoder. That throws inside create_record_config, so recording would die before starting on machines that work today. Please confirm on Linux and macOS with the pinned lerobot v0.6.0, not only on the Windows box.
Also, crf=30 and g=2 are inherited while preset="ultrafast" disables most compression tools, so datasets get noticeably larger. The PR measures encode speed (~11x) but not file size.
Last, the comment references "this machine" and "2026-07-15 crashes", which a future reader cannot verify. If the setting stays, expose it as a RecordingRequest field (same as video and streaming_encoding already are) or gate it on sys.platform == "win32".
| } | ||
| except Exception as e: | ||
| logger.exception("Recording session failed") | ||
| logger.exception("Recording session failed: %r", e) |
There was a problem hiding this comment.
Nit: logger.exception already emits the traceback, whose last line is the exception repr, so %r is redundant.
| last_recording_info = {"success": False, "error": str(e)} | ||
| last_recording_info = { | ||
| "success": False, | ||
| "error": str(e) or repr(e), |
There was a problem hiding this comment.
str(e) or repr(e) is a good catch for exceptions with an empty message. Keep this.
| last_recording_info = { | ||
| "success": False, | ||
| "error": str(e) or repr(e), | ||
| "dataset_repo_id": request.dataset_repo_id, |
There was a problem hiding this comment.
Blocking: this is an unintended regression. handle_get_dataset_info (line 479) short-circuits on last_recording_info.get("dataset_repo_id") == request.dataset_repo_id. Before this PR failure dicts had no dataset_repo_id, so it never matched and the on-disk dataset was loaded. Now a session that crashed after saving N episodes returns {"success": False, ...} and the partial dataset is never read, which is exactly the scenario this PR targets.
Also, this dict carries error but no message, unlike the other failure return at line 498, so the frontend reads undefined.
Suggest gating on success:
if last_recording_info and last_recording_info.get("success") \
and last_recording_info.get("dataset_repo_id") == request.dataset_repo_id:| and current_phase == "recording", # Only during recording phase | ||
| }, | ||
| "message": "Recording session failed with error - check logs" | ||
| "message": ( |
There was a problem hiding this comment.
The global is read twice, guard then subscript, and handle_start_recording sets it to None at line 242. Narrow window, but the result is a 500 on /recording-status, which is the endpoint whose job is reporting errors. Bind once:
info = last_recording_info
error = info.get("error") if info else NoneThen build the message above the status = {...} literal. The triple nested conditional inside a dict literal is hard to follow as written.
Related issue
Fixes #78 — the reported symptoms match exactly: camera/arm disconnecting mid-session,
Svt[info]: Only a single picture was passed inin the logs, and the session silently marked "failed" with nothing surfaced to the GUI. Fixes 2 (camera enumeration) and 3 (SVT-AV1 encoder starving the motor-bus read loop) address the root cause; fix 4 (surfacing the real exception) closes the "no error shown in GUI" part.Four separate crashes that only show up on Windows with real hardware attached. Grouped in one PR because they were all found the same week, not because they're related — happy to split if preferred.
1. Console encoding crash (cp1252)
Symptom: an emoji in a log/print line could raise an unhandled
UnicodeEncodeErroron a Windows console using the legacy cp1252 code page, turning an endpoint into a blank 500 with no useful message reaching the frontend.Fix:
stdout/stderrare reconfigured witherrors="replace"at import time, so unencodable characters degrade instead of crashing the process.2. Camera list sometimes empty
Symptom:
/available-cameraswould intermittently return an empty or wrong list, depending on which FastAPI worker thread handled the request.Root cause:
pygrabberenumerates cameras via COM, which must be initialized per-thread. FastAPI's thread pool never did that, so enumeration silently failed on threads that hadn't.Fix: explicitly initialize/uninitialize COM around the enumeration call.
3. Recording crashes with "no status packet"
Symptom: recording sessions aborted mid-way with a serial
ConnectionError, non-deterministically, on otherwise-healthy hardware.Root cause: the default video encoder (libsvtav1) is CPU-heavy enough that two parallel encode streams could starve the motor-bus read loop of CPU time.
Fix: default to h264/ultrafast, measured ~11x faster to encode on this hardware.
4. Real errors hidden behind "check logs"
Symptom: any recording failure showed the same generic message to the user — the real exception (exactly what let us diagnose #2 and #3 above) never left the server logs.
Fix:
/recording-statusnow includes the actual exception message.Testing
Each fix corresponds to a crash reproduced and captured in server logs beforehand, then confirmed resolved after the change.