Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pulseaudio direct connection #852

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Conversation

josa41
Copy link
Contributor

@josa41 josa41 commented Oct 17, 2024

Set Ghaf audio apps to directly connect to audiovm with pulseaudio

Description of changes

Makes app vms use audiovm pulseaudio directly without application audio tunneling
With this change the pulseaudio don't move patch is not needed

Checklist for things done

  • Summary of the proposed changes in the PR description
  • More detailed description in the commit message(s)
  • Commits are squashed into relevant entities - avoid a lot of minimal dev time commits in the PR
  • Contribution guidelines followed
  • Ghaf documentation updated with the commit - https://tiiuae.github.io/ghaf/
  • PR linked to architecture documentation and requirement(s) (ticket id)
  • Test procedure described (or includes tests). Select one or more:
    • Tested on Lenovo X1 x86_64
    • Tested on Jetson Orin NX or AGX aarch64
    • Tested on Polarfire riscv64
  • Author has run make-checks and it passes
  • All automatic Github Action checks pass - see actions
  • Author has added reviewers and removed PR draft status
  • Change requires full re-installation
  • Change can be updated with nixos-rebuild ... switch

Appvm audio routing has changed, audio should work same as before

Instructions for Testing

  • List all targets that this applies to: Lenovo X1, Dell-Latitude
  • Is this a new feature
    • List the test steps to verify:
  • If it is an improvement how does it impact existing functionality? no changes

@josa41 josa41 requested a review from nesteroff October 17, 2024 13:19
@josa41 josa41 temporarily deployed to internal-build-workflow October 17, 2024 13:19 — with GitHub Actions Inactive
@josa41 josa41 added the Needs Testing CI Team to pre-verify label Oct 17, 2024
@josa41 josa41 mentioned this pull request Oct 18, 2024
15 tasks
@johannarautanen
Copy link

johannarautanen commented Oct 21, 2024

Tested on Lenovo-X1 (flashed to the SSD) (commit: 32d481e)

Working:
-internal, usb, 3,5mm and bluetooth audio works with Chromium, Microsoft(365,outlook,Teams), slack, video editor.
-ghafaudiocontrolstandalone: playback-line disappeared after app closed
-all other ghaf apps can be launched
-ci-test automation cases ok (older auitomation tests before 18th oct)

Notes: when 3,5mm audio is plugin during reboot operation, audio don't heard from 3.5 mm headset or from internal speaker. Need to plug out and plugin the 3,5 mm headset and after that audio heard via headset. Same issue with main x1 build.

@johannarautanen johannarautanen added the Tested on Lenovo X1 Carbon This PR has been tested on Lenovo X1 Carbon label Oct 21, 2024
@johannarautanen
Copy link

johannarautanen commented Oct 21, 2024

The Dell-Latitude needed to be tested also?

no need to test with Dell, cause change is not actually device dependent.

@josa41
Copy link
Contributor Author

josa41 commented Oct 21, 2024

Tested on Lenovo-X1 (flashed to the SSD) (commit: 32d481e)

Working: -internal, usb, 3,5mm and bluetooth audio works with Chromium, Microsoft(365,outlook,Teams), slack, video editor. -all other ghaf apps can be launched -ci-test automation cases ok

Notes: when 3,5mm audio is plugin during reboot operation, audio don't heard from 3.5 mm headset or from internal speaker. Need to plug out and plugin the 3,5 mm headset and after that audio heard via headset. Same issue with main x1 build.
This is a good find and i have separate ticket for figuring out how to fix that. The reason for this is that we had to statically assign the audio profile that is being used at audiovm startup. You found the loophole which escaped us. Great work.
Edit: This bug is not caused by this commit but it result of earlier efforts to get audio microphone to work.

@josa41 josa41 mentioned this pull request Oct 21, 2024
19 tasks
@johannarautanen johannarautanen removed the Needs Testing CI Team to pre-verify label Oct 21, 2024
@brianmcgillion
Copy link
Collaborator

@josa41 is the open conversation going to be resolved in this PR? it needs to be resolved before merging is possible

@josa41
Copy link
Contributor Author

josa41 commented Oct 21, 2024

@josa41 is the open conversation going to be resolved in this PR? it needs to be resolved before merging is possible

This PR would fix 3 issues and cause one (some apps may not respect export PULSE_SERVER env variable).
The fixed issues are:

  • Removing our pulseaudio patch
  • Apps are removed from volume control apps list when apps are closed (but appvm remains open)
  • Only one TCP tunnel between appvm and audiovm (instead of separate tunnels for sink/source)
    Also this makes direct connection from apps to audiovm pulseaudio (less latency)

I think the main question is do we have an app which would not respect the PULSE_SERVER variable?

@brianmcgillion
Copy link
Collaborator

@josa41 is the open conversation going to be resolved in this PR? it needs to be resolved before merging is possible

This PR would fix 3 issues and cause one (some apps may not respect export PULSE_SERVER env variable). The fixed issues are:

  • Removing our pulseaudio patch
  • Apps are removed from volume control apps list when apps are closed (but appvm remains open)
  • Only one TCP tunnel between appvm and audiovm (instead of separate tunnels for sink/source)
    Also this makes direct connection from apps to audiovm pulseaudio (less latency)

I think the main question is do we have an app which would not respect the PULSE_SERVER variable?

that i am not sure. and by the comment needs to be resolved. there is a button "resolve conversation". even if that is to say "not possible", "will be done later" etc. that will resolve the conversation nd it can be merged

@humaidq-tii
Copy link
Member

I think the main question is do we have an app which would not respect the PULSE_SERVER variable?

This has to be tested, but I seen some applications that don't read the pulse environment variable and pass it to pulse connect function: https://docs.rs/libpulse-binding/latest/src/libpulse_binding/context/mod.rs.html#316-342

GitHub search would reveal many projects. One case I faced is ironbar: https://github.com/JakeStanger/ironbar/blob/6240b4b4fd2b42898860b656b5fe3f946d11db2a/src/clients/volume/mod.rs#L118

I had to create a patch for it to connect to audio-vm when I was testing it.


This PR is a good step, I agree with this change, but I think it is a good idea to allow an option to still keep running a local pulseaudio server (default to false). This way this issue wouldn't cause breaking changes to applications, and there wouldn't be a need to revert this change.

@josa41
Copy link
Contributor Author

josa41 commented Oct 21, 2024

I think the main question is do we have an app which would not respect the PULSE_SERVER variable?

This has to be tested, but I seen some applications that don't read the pulse environment variable and pass it to pulse connect function: https://docs.rs/libpulse-binding/latest/src/libpulse_binding/context/mod.rs.html#316-342

GitHub search would reveal many projects. One case I faced is ironbar: https://github.com/JakeStanger/ironbar/blob/6240b4b4fd2b42898860b656b5fe3f946d11db2a/src/clients/volume/mod.rs#L118

I had to create a patch for it to connect to audio-vm when I was testing it.

This PR is a good step, I agree with this change, but I think it is a good idea to allow an option to still keep running a local pulseaudio server (default to false). This way this issue wouldn't cause breaking changes to applications, and there wouldn't be a need to revert this change.

Now I like your idea. I did not like to have both env and pulse service running at the same time on all app vms but with the variable its good middle ground. I'll update the PR. Thanks.

Set Ghaf audio apps to directly connect to audiovm with pulseaudio
With this change we can also remove the pulseaudio don't move patch

Signed-off-by: Jon Sahlberg <[email protected]>
@brianmcgillion brianmcgillion merged commit 5d5ae6d into tiiuae:main Oct 23, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tested on Lenovo X1 Carbon This PR has been tested on Lenovo X1 Carbon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants