Add virtual audio support to CI/CD timesync-stimuli test #238
Add virtual audio support to CI/CD timesync-stimuli test #238
timesync-stimuli test #238Conversation
…ng unit tests and CI/CD audio support
vmdocua
added
enhancement
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CI/CD pipeline for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a task list for the timesync-stimuli project, outlining unit tests, CI/CD virtual audio support, and future work. It also implements virtual audio support for CI/CD by setting up a PulseAudio null sink in the xvfb environment and passing the PulseAudio server details to containers. This enables audio recording during CI tests. Feedback includes addressing a hardcoded video duration in the ffmpeg command and reconsidering the suppression of pulseaudio stderr output to prevent hidden errors.
| echo "ffmpeg -video_size \"${FRAME_WIDTH}x${FRAME_HEIGHT}\" -framerate \"${FRAME_RATE}\" -f x11grab -i \"$DISPLAY_ID\" -t \"$VIDEO_DURATION_SEC\" -c:v libx264 -pix_fmt yuv420p \"$REPROSTIM_SCREENSHOT_PATH\"" | ||
| ffmpeg -video_size "${FRAME_WIDTH}x${FRAME_HEIGHT}" -framerate "${FRAME_RATE}" -f x11grab -i "$DISPLAY_ID" -t 45 -c:v libx264 -pix_fmt yuv420p "$REPROSTIM_SCREENSHOT_PATH" | ||
| echo "ffmpeg -video_size \"${FRAME_WIDTH}x${FRAME_HEIGHT}\" -framerate \"${FRAME_RATE}\" -f x11grab -i \"$DISPLAY_ID\" ${FFMPEG_AUDIO_ARGS[*]} -t \"$VIDEO_DURATION_SEC\" -c:v libx264 -pix_fmt yuv420p \"$REPROSTIM_SCREENSHOT_PATH\"" | ||
| ffmpeg -video_size "${FRAME_WIDTH}x${FRAME_HEIGHT}" -framerate "${FRAME_RATE}" -f x11grab -i "$DISPLAY_ID" "${FFMPEG_AUDIO_ARGS[@]}" -t 45 -c:v libx264 -pix_fmt yuv420p "$REPROSTIM_SCREENSHOT_PATH" |
There was a problem hiding this comment.
The video recording duration is hardcoded to 45 seconds, but it should use the $VIDEO_DURATION_SEC variable defined at the top of the script for consistency and maintainability. The echo command on the preceding line correctly uses the variable, but the ffmpeg command itself does not.
| ffmpeg -video_size "${FRAME_WIDTH}x${FRAME_HEIGHT}" -framerate "${FRAME_RATE}" -f x11grab -i "$DISPLAY_ID" "${FFMPEG_AUDIO_ARGS[@]}" -t 45 -c:v libx264 -pix_fmt yuv420p "$REPROSTIM_SCREENSHOT_PATH" | |
| ffmpeg -video_size "${FRAME_WIDTH}x${FRAME_HEIGHT}" -framerate "${FRAME_RATE}" -f x11grab -i "$DISPLAY_ID" "${FFMPEG_AUDIO_ARGS[@]}" -t "$VIDEO_DURATION_SEC" -c:v libx264 -pix_fmt yuv420p "$REPROSTIM_SCREENSHOT_PATH" |
| pulseaudio --start --exit-idle-time=-1 \ | ||
| -n \ | ||
| --load="module-null-sink sink_name=reprostim_sink" \ | ||
| --load="module-native-protocol-unix auth-anonymous=1 socket=/tmp/reprostim_pulse.sock" \ | ||
| 2>/dev/null || true |
There was a problem hiding this comment.
Suppressing all stderr output with 2>/dev/null can hide important errors if pulseaudio fails to start for reasons other than it already running. This could lead to silent failures where audio is not recorded, and debugging would be difficult. Consider removing 2>/dev/null or redirecting stderr to a log file. A subsequent check to ensure the PulseAudio server is running and the sink is available would make the script more robust.
1 participant
Previously the CI/CD
timesync-stimulitest ran with--mutemode, which masked real audio issues. This was exposed during thePsychoPy2024→2025migration where audio library breakage went undetected in CI until it failed in a real environment:This PR wires up a virtual PulseAudio null sink in xvfb mode so the test now runs with
-a psychopy_sounddeviceand captures actual audio output alongside the screen recording. Future audio regressions (codec changes, library incompatibilities, device initialization failures) will be caught in CI before reaching a real capture environment.