Skip to content

Conversation

tmoida
Copy link
Contributor

@tmoida tmoida commented Aug 8, 2025

  • Updated AudioRecord and AudioPlayback scripts to support both PulseAudio and PipeWire
  • Added AUDIO_BACKEND variable for backend selection
  • Updated README files with usage instructions for both backends
  • Modified LAVA test plan to run tests with both PulseAudio and PipeWire

Copy link
Contributor

@smuppand smuppand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmoida Could you please squash the commits into a single one?

@tmoida
Copy link
Contributor Author

tmoida commented Aug 11, 2025

Hi @smuppand ,
I have squashed the commits into a single one as requested. Kindly review it and let me know if any further changes are needed.

Copy link
Contributor

@smuppand smuppand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few chnages needed like pre-scan dmesg (use common helpers) -> Run playback (possibly loop) -> post-scan dmesg fir any potential audio-related dmesg messages detected.

better to add a daemon check which prevents false failures when the server isn't running

loops + timeout - Lets you simulate longer playback without hanging CI.

@tmoida
Copy link
Contributor Author

tmoida commented Aug 14, 2025

Hi @smuppand,
I've updated the Audio test scripts and documentation as per the feedback. Key improvements include:

- Added daemon checks for both PulseAudio and PipeWire backends
- Replaced eval with safer command execution using sh -c
- Introduced support for environment variables like PLAYBACK_VOLUME, PLAYBACK_TIMEOUT, PLAYBACK_LOOPS, RECORD_TIMEOUT, and RECORD_LOOPS
- Implemented pre/post dmesg scanning using get_kernel_log and scan_dmesg_errors
- Updated both README files to reflect the new features and usage instructions

Please let me know if there is anything else that needs to be addressed.

@smuppand
Copy link
Contributor

@tmoida , please squash your commits.

@tmoida
Copy link
Contributor Author

tmoida commented Aug 20, 2025

Hi @smuppand,

I've squashed the commits and fixed the Shell Check warning by using the loop variable. Please let me know if there is anything else that needs to be addressed.

Also. I have validated these scripts on Yocto builds, and they work fine. However, on Mainline (Nightly sanity) builds, they are being skipped due to a missing timeout binary. I wanted to check: will the timeout binary be included in future builds, or should I update the script to avoid using it?

@smuppand
Copy link
Contributor

Hi @smuppand,

I've squashed the commits and fixed the Shell Check warning by using the loop variable. Please let me know if there is anything else that needs to be addressed.

Also. I have validated these scripts on Yocto builds, and they work fine. However, on Mainline (Nightly sanity) builds, they are being skipped due to a missing timeout binary. I wanted to check: will the timeout binary be included in future builds, or should I update the script to avoid using it?

You can use the library function run_with_timeout() if the build does not include the timeout utility.

@tmoida
Copy link
Contributor Author

tmoida commented Aug 22, 2025

Hi @smuppand,

Thanks for the suggestions!

I've updated the loop logic to handle early exits cleanly and added clearer return handling to distinguish between success and timeout cases. Also, I've replaced timeout with run_with_timeout() as a fallback to ensure compatibility on builds where the timeout binary is missing.

Please let me know if there is anything else that needs to be addressed.

@smuppand
Copy link
Contributor

@tmoida Please update the https://github.com/qualcomm-linux/qcom-linux-testkit/blob/main/Runner/plans/meta-ar-ci-premerge.yaml to include the changes that involve launching or adding any tests.

@tmoida
Copy link
Contributor Author

tmoida commented Aug 28, 2025

Hi @smuppand,
I have tested the standalone scripts on both QLI 1.0 and the QLI mainline (nightly sanity) builds.
Could you please review and merge the PR at your convenience?

Copy link
Contributor

@smuppand smuppand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together. I’ve got a few suggestions to make the Audio test more modular and CI-friendly, consistent.

run.sh only: parse flags → call functions in libaudio.sh.
No direct calls to amixer/aplay/tinymix from run.sh (they go via lib).
All shared helpers should wrap functestlib.sh
Happy to re-review once these land.

@tmoida tmoida force-pushed the patch-1 branch 2 times, most recently from 3996525 to 4a24ae8 Compare September 4, 2025 10:17
@tmoida
Copy link
Contributor Author

tmoida commented Sep 4, 2025

Hi @smuppand,

Thank you for your feedback.

I have implemented the requested modular structure and made the following changes:

1. Moved all core logic for playback and recording into a shared `libaudio.sh`.
2. Created minimal `run.sh` scripts under AudioPlayback and AudioRecord that only parse CLI flags and invoke functions from `libaudio.sh`.
3. Wrapped all binary calls (paplay, pw-play, parec, pw-record) inside `libaudio.sh`, eliminating direct calls in `run.sh`.
4. Added support for both CLI flags and environment variables for configuration.

Why separate run.sh scripts for playback and record?
Regarding the use of separate run.sh scripts for playback and recording: We are utilizing run-test.sh as the main test runner, which expects each test case to be in its own directory and produce a result file named after that directory (e.g., AudioPlayback.res, AudioRecord.res). Using a single run.sh within a common Audio/ directory would prevent run-test.sh from locating the correct .res file, potentially leading to false negatives, even when tests pass.

To address this and ensure compatibility with run-test.sh, we have maintained separate directories and minimal run.sh wrappers for each test case. This approach allows for correct result parsing while centralizing the logic in libaudio.sh.

All usage details, CLI options, and examples are documented in the respective Read_me.md files for easy reference.

Let me know if you’d like any further refinements!

Copy link
Contributor

@smuppand smuppand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also woth adding

  • WAV 16-bit/48k, 44.1k; Ogg Vorbis/Opus (if tools available), AAC.
  • Short (5s), medium (30s), long (120s) smoke tiers.
  • Mixer dumps (pactl list, pw-dump) on failure.

SEARCH=$(dirname "$SEARCH")
done
[ -z "$INIT_ENV" ] && echo "[ERROR] init_env not found" && exit 1
# shellcheck source=/dev/null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using # shellcheck source=/dev/null is a blunt silencer that hides real problems and degrades analysis. It tells ShellCheck “pretend this sourced file exists but is empty, It masks real bugs.

. "$TOOLS/libaudio.sh"

test_path=$(find_test_case_by_name "$TESTNAME")
cd "$test_path" || exit 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach will fail if we run any test from the run-test root, and the .res file will be generated in the directory where the command is executed, rather than in the test directory. Please keep the expected snippet.

test_path=$(find_test_case_by_name "$TESTNAME")
cd "$test_path" || exit 1
# Override with BusyBox-compatible version
check_network_status() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check_network_status override isn’t used—drop it to keep things tight.
Use available built-in fiunctions like get_ip_address() to avoid duplication.

SEARCH=$(dirname "$SEARCH")
done
[ -z "$INIT_ENV" ] && echo "[ERROR] init_env not found" && exit 1
# shellcheck source=/dev/null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT

# shellcheck disable=SC1090
. "$INIT_ENV"
fi
# shellcheck disable=SC1090,SC1091
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT

done

get_kernel_log > "$LOGDIR/dmesg_after.log"
scan_dmesg_errors "audio" "$LOGDIR"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your function signature is scan_dmesg_errors(prefix, module_regex, [exclude]).

Current calls use scan_dmesg_errors "audio" "$LOGDIR" (reversed). This silently weakens the scan. It returns 0 when errors are found, 1 when clean. Runners should only gate FAIL on this if a strict flag is set (to avoid flakiness).


for i in $(seq 1 "$PLAYBACK_LOOPS"); do
log_info "Playback loop $i of $PLAYBACK_LOOPS"
execute_with_timeout "$PLAYBACK_TIMEOUT" sh -c "$PLAY_CMD" >> "$LOGDIR/playback_stdout.log" 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults like 15s are fine for timeout(1) but may break run_with_timeout (usually expects integers). Normalize once, inside execute_with_timeout.


get_kernel_log > "$LOGDIR/dmesg_before.log"
PLAY_CMD=$(setup_playback_command)
PLAY_SUCCESS=0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playback uses PLAY_SUCCESS but sets PLAYBACK_SUCCESS in one branch—typo causes false PASS.

if [ "$AUDIO_BACKEND" = "pipewire" ]; then
select_pipewire_source
fi
check_dependencies "$([ "$AUDIO_BACKEND" = "pulseaudio" ] && echo "parec" || echo "pw-record")" pgrep grep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select_pipewire_source uses wpctl but dependency check doesn’t require it.

Minor: pgrep grep in check_dependencies is unnecessary; keep pgrep, drop grep.

log_fail_exit "$TESTNAME" "Audio playback test failed"
echo "$TESTNAME FAIL" > "$RESULT_FILE"
fi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transferring PASS/FAIL/SKIP ownership to the top-level runner. These shared libraries will be used across the audio files.

@tmoida tmoida force-pushed the patch-1 branch 4 times, most recently from d1cc8fe to da44e9c Compare September 12, 2025 11:38
@tmoida
Copy link
Contributor Author

tmoida commented Sep 12, 2025

Hi @smuppand,
I've updated the changes as per the review comments. Please let me know if any further modifications are required.

@tmoida tmoida force-pushed the patch-1 branch 4 times, most recently from eff1a92 to a8b34f3 Compare September 12, 2025 13:41
@smuppand
Copy link
Contributor

@tmoida Please rebase

@tmoida
Copy link
Contributor Author

tmoida commented Sep 15, 2025

Hi @smuppand, The branch has been rebased and updated as requested. Please let me know if any further changes are needed.

@smuppand smuppand self-requested a review September 15, 2025 10:39
@tmoida
Copy link
Contributor Author

tmoida commented Sep 16, 2025

Hi @smuppand,
I have updated the check_network_status() function in functestlib.sh and added the missing copyright header to audio_common.sh.

Please let me know if any additional changes are needed.

Thank you.

@smuppand smuppand self-requested a review September 16, 2025 06:38
Copy link
Contributor

@smuppand smuppand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a brief commit message, as this PR introduces a new flow and enhancements.

- Modular runner supports both PipeWire and PulseAudio backends
- Validation improved with evidence-based checks for playback and recording
- Automatic handling of audio assets (download and extraction)
- Enhanced diagnostics: dmesg scan, mixer dumps, JUnit XML output for CI
- Added CLI options and environment variable support for configuration
- Utility scripts refactored for better extensibility and maintainability

Signed-off-by: Teja Swaroop Moida <[email protected]>
@tmoida
Copy link
Contributor Author

tmoida commented Sep 16, 2025

Added a brief commit message summarizing the new flow and enhancements.

Copy link
Contributor

@smuppand smuppand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smuppand smuppand merged commit 034b054 into qualcomm-linux:main Sep 16, 2025
8 checks passed
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