Skip to content

renderer_adjustment() uses wrong formula for negative-offset renderers; only positive offsets tested #380

Description

@forkwright

Finding

renderer_adjustment() returns how much extra delay a renderer needs relative to the zone-wide playout timestamp (server_time + abs(max_offset) + margin). A renderer with signed offset r must delay by abs(max_offset) - r, so that behind-clock renderers (negative r) wait longer. The implementation subtracts renderer_offset.abs() instead of the signed renderer_offset, collapsing the sign and returning a wrong adjustment for any behind-clock renderer. The existing test only feeds non-negative offsets, the exact half of the domain where .abs() happens to be a no-op, so the defect is invisible to the suite.

Worked example: with max_offset = 3000 and a renderer at -3000 µs (behind the server), the code computes 3000 - abs(-3000) = 0; the correct value is 3000 - (-3000) = 6000. A fast renderer (+3000) and a slow renderer (-3000) both receive a zero adjustment despite being 6000 µs apart.

Evidence

crates/syndesis/src/clock/coordinator.rs:143

max_offset - renderer_offset.abs()

The regression test that should pin this behaviour, renderer_adjustment_compensates_offset_difference at crates/syndesis/src/clock/coordinator.rs:283, supplies only fast = +3000 and slow = 0 — both non-negative — so the signed-subtraction path is never exercised and the bug passes CI unnoticed.

Why this matters

renderer_adjustment() is the public API that drives per-renderer playback delay for zone synchronisation. Returning zero for every behind-clock renderer defeats the synchronisation it exists to provide: renderers whose clocks lag the server play frames too early, producing audible desync between zone members. Because the test suite covers only the trivially correct positive-offset half of the input domain, the defect ships green and any future refactor inherits a false sense of coverage.

Desired correction

Replace renderer_offset.abs() with the signed renderer_offset on line 143 so the result is max_abs_offset - signed_renderer_offset. Add a mixed-sign test: two renderers at offsets +3000 and -3000; assert the behind renderer's adjustment is 6000 and the ahead renderer's is 0.

Done when: renderer_adjustment returns max_abs_offset - signed_renderer_offset, and a test covering both a positive and a negative renderer offset asserts the 6000-vs-0 split and passes.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions