Improve A/V synchronization#76
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements format-aware audio sample rates to fix audio-video desynchronization. The C64 Ultimate hardware uses different audio rates for PAL (47,983 Hz) and NTSC (47,940 Hz), but the code was using a compromise value (47,976 Hz) that caused cumulative drift. The fix automatically sets the audio sample rate based on detected video format, eliminating drift for both formats.
Key changes:
- Audio sample rate now dynamically determined from video format detection (PAL: 47,983 Hz, NTSC: 47,940 Hz)
- Added A/V sync diagnostics module for debugging (enabled via C64_AV_SYNC_DEBUG env var)
- Updated documentation and E2E test results showing perfect synchronization
Reviewed changes
Copilot reviewed 23 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_av_sync_format_aware.py | New unit tests verifying correct audio packet intervals for both PAL and NTSC formats |
| tests/e2e/results/pal_default/* | E2E test results showing improved PAL A/V sync performance |
| tests/e2e/results/ntsc_default/* | E2E test results showing improved NTSC A/V sync performance |
| src/c64-video.c | Sets audio_sample_rate when video format is detected |
| src/c64-types.h | Added audio_sample_rate field and av_sync_diagnostics structure |
| src/c64-source.c | Initialize audio rate and diagnostics module |
| src/c64-av-sync-diagnostics.h | Header for new diagnostics module |
| src/c64-av-sync-diagnostics.c | Implementation of A/V sync logging and monitoring |
| src/c64-audio.c | Calculates audio_interval_ns dynamically from format-specific sample rate |
| doc/av-sync-format-aware-fix.md | Comprehensive documentation of the fix and verification |
| doc/c64-stream-spec.md | Updated with correct format-specific audio packet intervals |
| buildspec.json | Version bumped to 1.0.1 |
| CMakeLists.txt | Added c64-av-sync-diagnostics.c to build |
Audio and video streams from C64 Ultimate hardware always use the same format (PAL or NTSC). Audio sample rates are format-specific, derived from the color subcarrier frequencies: - PAL: 47,983 Hz (from 4433618.75 Hz color subcarrier) - NTSC: 47,940 Hz (from 3579545.45 Hz color subcarrier) This results in different audio packet intervals: - PAL: 192 samples ÷ 47983 Hz = 4,001,417 ns/packet - NTSC: 192 samples ÷ 47940 Hz = 4,005,006 ns/packet Implementation: 1. Added C64_PAL_AUDIO_SAMPLE_RATE and C64_NTSC_AUDIO_SAMPLE_RATE constants 2. Audio sample rate set automatically when video format is detected 3. Audio packet interval calculated dynamically from sample rate 4. No independent audio format detection needed Changes: - src/c64-protocol.h: Define audio sample rate constants - src/c64-types.h: Add audio_sample_rate field to context - src/c64-audio.c: Calculate audio_interval_ns dynamically - src/c64-source.c: Initialize with PAL default, set from format hint - src/c64-video.c: Set audio_sample_rate when format detected - doc/c64-stream-spec.md: Correct audio packet interval calculations
4d6f284 to
6611939
Compare
Remove hardcoded magic number 4001417ULL and calculate interval consistently using (192 * 1e9) / sample_rate formula.
…mport Skip directories silently instead of logging warnings when paths ending with separators are passed to palette parser. Only log warnings for actual VPL files that fail to open.
9e0c989 to
6e6a31e
Compare
…mport Use proper filesystem APIs (stat/GetFileAttributes) to detect directories instead of just checking for trailing slashes. Only log warnings for actual VPL files that fail to open.
6e6a31e to
65a3438
Compare
Replace direct POSIX/Windows filesystem calls with OBS wrappers: - os_stat() instead of stat() (c64-file.c, c64-palette.c) - os_opendir/os_readdir/os_closedir instead of opendir/readdir/closedir - Removed Windows-specific #ifdef blocks for directory enumeration - Unified path handling using OBS directory APIs This eliminates platform-specific code and ensures consistent behavior across Windows, Linux, and macOS.
…d A/V synchronization
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.