Skip to content

fix: guard Synchronization construction when no timecode axis is present#214

Open
rocketmark wants to merge 6 commits into
SMPTE:mainfrom
rocketmark:upstream-pr/fix-f4-sync-frequency-none
Open

fix: guard Synchronization construction when no timecode axis is present#214
rocketmark wants to merge 6 commits into
SMPTE:mainfrom
rocketmark:upstream-pr/fix-f4-sync-frequency-none

Conversation

@rocketmark

Copy link
Copy Markdown
Contributor

Summary

get_tracking_frame() in mosys/f4.py unconditionally constructs
Synchronization(frequency=frequency) at the end of every frame. frequency
defaults to None and is only set when a timecode axis (0xF8) appears in
the packet. Synchronization.frequency is typed as StrictlyPositiveRational,
which does not accept None, so the constructor raises a Pydantic validation
error and crashes the parser.

Demonstration

BEFORE: F4PacketParser.get_tracking_frame() on any packet with no timecode
axis raises a Pydantic ValidationError on Synchronization(frequency=None)

AFTER:
returns a valid frame with timing_synchronization unset (None)

Impact

Any F4 packet that omits the timecode axis (FIELD_ID_TIMECODE, 0xF8)
causes get_tracking_frame() to raise, aborting frame parsing. Callers
receive an unhandled exception rather than a partial frame.

Change

mosys/f4.py: wrap the Synchronization construction and
frame.timing_synchronization assignment in a if frequency is not None
guard so they are only executed when a timecode axis was present in the
packet. Four-line change.

test_mosys_reader.py: add test_no_timecode_axis_does_not_crash — a unit
test using a hand-crafted minimal packet with no timecode axis. Verifies the
parser returns a valid frame with timing_synchronization unset.

Found via

Hypothesis-based property testing against a synthetic F4 packet generator.
Packets containing only the minimum required axes triggered the crash
immediately on the first generated example.

@rocketmark
rocketmark force-pushed the upstream-pr/fix-f4-sync-frequency-none branch from 91a6c77 to 81e5a26 Compare May 21, 2026 13:38
@rocketmark

Copy link
Copy Markdown
Contributor Author

Force pushed to resolve the branch conflicts. The change here doesn't conflict with the other changes.

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.

2 participants