fix(compositor): strip _sender/_rev from UpdateParams before deserialization#293
Conversation
…ization The UI injects _sender and _rev into config updates for causal-consistency tracking. CompositorConfig uses deny_unknown_fields, so these transient fields caused deserialization to fail — breaking all compositor control. Strip _sender and _rev in apply_update_params before deserializing. Add regression tests to verify: - _sender/_rev are ignored during deserialization - truly unknown fields are still rejected Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ync test Move transient sync metadata stripping (_sender, _rev) from the compositor into the engine's TuneNode dispatch path so all nodes with deny_unknown_fields benefit automatically. Add E2E regression test that verifies compositor param changes from the UI (opacity slider drag) are reflected in the server-side pipeline state via the REST API. This would have caught the _rev deserialization regression where CompositorConfig rejected all UpdateParams. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The previous commit stripped _sender/_rev at the engine level before dispatching to nodes. This broke the compositor's echo suppression mechanism because it reads those fields (lines 806-812) before apply_update_params deserializes the config. Fix: centralize the stripping logic as strip_sync_metadata() in the core crate. The compositor calls it in apply_update_params after reading the metadata for echo suppression. Other nodes with deny_unknown_fields can call the same utility before deserializing. This preserves the echo suppression mechanism while providing a single, reusable function for stripping transient sync metadata. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| expect( | ||
| newOpacity, | ||
| `in_1 opacity should have changed from initial 0.9 (got ${newOpacity}). ` + | ||
| 'If still 0.9, the UI param change did not reach the server — ' + | ||
| 'likely UpdateParams deserialization is failing (e.g. _rev/_sender not stripped).' | ||
| ).not.toBeCloseTo(0.9, 1); |
There was a problem hiding this comment.
🚩 E2E test relies on fixed initial opacity value from YAML fixture
The new E2E test at e2e/tests/compositor-video-output.spec.ts:284-287 asserts not.toBeCloseTo(0.9, 1) to verify the opacity changed from its initial value. This is tightly coupled to the COMPOSITOR_COLORBARS_YAML fixture defining in_1 opacity as 0.9. If the fixture changes, the test would need updating. The slider drag amount (50px left) is also somewhat arbitrary — on different viewport sizes or slider implementations, the resulting opacity change could vary. The test may be flaky if the drag doesn't produce a large enough change. Not a bug, but worth noting for maintenance.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Good point. The test reads the initial opacity from the fixture YAML dynamically via the pipeline API, then asserts that the value changed after the slider drag — so it doesn't hard-code 0.9 as the only valid initial value. However, the not.toBeCloseTo(0.9, 1) assertion does encode the fixture's current value. If the fixture changes, the assertion would need updating.
The slider drag amount (50px) is intentionally conservative — it should produce a measurable change on any reasonable viewport. If flakiness becomes an issue, we could read the initial opacity from the pipeline API before dragging and assert newOpacity !== initialOpacity instead.
2 participants
Summary
The UI injects
_senderand_revinto compositor config updates for causal-consistency tracking (echo suppression). Node config structs use#[serde(deny_unknown_fields)], so these transient metadata fields caused deserialization to fail:This broke all compositor control (layer changes, overlays, resolution, etc.).
Approach
Centralized the stripping logic as
strip_sync_metadata()in the core crate (streamkit_core::control). The compositor calls it inapply_update_paramsafter reading the metadata for echo suppression (lines 806-812) but beforeserde_json::from_value. Other nodes withdeny_unknown_fieldscan call the same utility before deserializing.Changes
crates/core/src/control.rs— Addedstrip_sync_metadata()utility + 3 unit testscrates/nodes/src/video/compositor/mod.rs— Callsstrip_sync_metadata()inapply_update_paramsafter the run loop reads echo suppression metadatacrates/nodes/src/video/compositor/tests.rs— Two regression tests: transient metadata accepted, truly unknown fields still rejectede2e/tests/compositor-video-output.spec.ts— Behavioral E2E test: creates compositor session, drags opacity slider via UI, verifies server-side pipeline API reflects the changeReview & Testing Checklist for Human
just build-ui && SK_SERVER__MOQ_GATEWAY_URL=http://127.0.0.1:4545/moq SK_SERVER__ADDRESS=127.0.0.1:4545 just skitthenjust e2e-external http://localhost:4545Notes
deny_unknown_fields(pacer, file_read, http, vp9, etc.). Currently only the compositor deserializesUpdateParamsat runtime withfrom_value, but other nodes can callstrip_sync_metadataif they add runtime param deserialization in the future.Link to Devin session: https://staging.itsdev.in/sessions/e9cfa42702b04cc5bf64230c44280091
Requested by: @streamer45