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.

Sign up for GitHub

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

Jump to bottom

Linux PPA package: use of non-optimal SystemCmdSound #3285

Open
leamas opened this issue May 24, 2023 · 10 comments · May be fixed by #3380
Open

Linux PPA package: use of non-optimal SystemCmdSound #3285

leamas opened this issue May 24, 2023 · 10 comments · May be fixed by #3380
Labels
bug done Fixed in master branch
Milestone
OpenCPN 5.10.0

Comments

@leamas
Copy link
Collaborator

leamas commented May 24, 2023 •
edited

Describe the bug
The PPA packages for non-amd64 architectures are configured using SystemCmdSound. Since this means using an external binary to run sounds, it means less control and performance compared to using libraries linked to the main executable.

It also means that the amd64 packages are linked differently than armhf/arm64.

Expected behavior
The PPA package should use the default configuration which uses the portaudio library on all platforms.

Additional context
The official package has been using portaudio as long as it has existed. Furthermore, the build dependencies already contains portaudio19-dev .

BTW: A debian architecture is never aarch64, it is always using arm64. aarch64 is what is used outside of Debian, like the NDC tools, Fedora, the ARM company, etc. Hence the "aarch64" branch in debian/rules is not really useful.

The only change required would be:

iff --git a/debian/rules b/debian/rules
index e2bb60d09..e3bca713b 100755
--- a/debian/rules
+++ b/debian/rules
@@ -12,7 +12,6 @@ override_dh_auto_configure:
 ifeq ($(DEB_BUILD_ARCH_CPU), arm)
        dh_auto_configure -- \
            -DCMAKE_LIBRARY_PATH=$(DEB_HOST_MULTIARCH) \
-            -DOCPN_ENABLE_SYSTEM_CMD_SOUND:BOOL=ON     \
             -DCMAKE_INSTALL_LIBDIR=lib                 \
             -DCMAKE_BUILD_TYPE=RelWithDebInfo          \
             -DDEB_VERSION=$(DEB_VERSION)               \
@@ -21,7 +20,6 @@ ifeq ($(DEB_BUILD_ARCH_CPU), arm)
 else ifeq ($(DEB_BUILD_ARCH_CPU), aarch64)
        dh_auto_configure -- \
            -DCMAKE_LIBRARY_PATH=$(DEB_HOST_MULTIARCH) \
-            -DOCPN_ENABLE_SYSTEM_CMD_SOUND:BOOL=ON     \
             -DCMAKE_INSTALL_LIBDIR=lib                 \
             -DCMAKE_BUILD_TYPE=RelWithDebInfo          \
             -DDEB_VERSION=$(DEB_VERSION)               \
@@ -30,7 +28,6 @@ else ifeq ($(DEB_BUILD_ARCH_CPU), aarch64)
 else ifeq ($(DEB_BUILD_ARCH_CPU), arm64)
        dh_auto_configure -- \
            -DCMAKE_LIBRARY_PATH=$(DEB_HOST_MULTIARCH) \
-            -DOCPN_ENABLE_SYSTEM_CMD_SOUND:BOOL=ON     \
             -DCMAKE_INSTALL_LIBDIR=lib                 \
             -DCMAKE_BUILD_TYPE=RelWithDebInfo          \
             -DDEB_VERSION=$(DEB_VERSION)               \
@leamas leamas added the bug label May 24, 2023
@leamas leamas linked a pull request Feb 7, 2024 that will close this issue
Open
@nohal nohal added this to the OpenCPN 5.10.0 milestone Feb 25, 2024
@leamas
Copy link
Collaborator Author

leamas commented Apr 11, 2024

Note: 801f81d, SystemCmdSound disabled.

@bdbcat
Copy link
Member

bdbcat commented Apr 11, 2024

TODO:
All is not perfect here.
Quick test on linux with multiple overlapping sound requests produces "choppy" output.
To reproduce: Add Hakan's signalk test data feed, enable CPA warnings and audio alert. Sounds are produced, but not very nice.

@leamas
Copy link
Collaborator Author

leamas commented Apr 11, 2024 •
edited

Quick test on linux with multiple overlapping sound requests produces "choppy" output

This was not part of the design. Question is just how this should be handled. Obviously, some kind of mutex. But besides that, should parallel play requests be queued, dropped or what?

EDIT: The design is really that there should be no overlapping sound requests. Calling code should wait for the callback before issuing next request. This is implemented in other places.

@bdbcat
Copy link
Member

bdbcat commented Apr 11, 2024

"Calling code should wait for the callback before issuing next request"
Yes. Should be easy to debug.

bdbcat added a commit that referenced this issue Apr 12, 2024
@bdbcat
Enforce serialized access to sound driver/device #3285
233817d
@bdbcat
Copy link
Member

bdbcat commented Apr 12, 2024

I think we are done here.

@leamas
Copy link
Collaborator Author

leamas commented Apr 12, 2024

Great to hear!

The bug is about the Debian package configuration, we should be able to close it when we abandon the current Debian package build procedures. Marking as fixed for now

@leamas leamas added the done Fixed in master branch label Apr 12, 2024
@Hakansv
Copy link
Contributor

Hakansv commented Apr 13, 2024

Today's master code
One observation on a Rpi Bullseye. The same using any of available sound driver types, hdmi/default)
Using 2 bells sound.
Click any Test AIS sound button:
First click - nothing.
Second click - "Dong". (Only one)
Next clicks - "Click"-"Dong"- "Dong". (The "Click" is a very short distinct sound)

Activated CPA Alert:
"Click"-"Dong"- "Dong". "Click"-"Dong"- "Dong". "Click"-"Dong"- "Dong"

@leamas leamas removed the done Fixed in master branch label Apr 13, 2024
@leamas
Copy link
Collaborator Author

leamas commented Apr 13, 2024

Removing Done label for now.

@leamas
Copy link
Collaborator Author

leamas commented Apr 14, 2024

@bdbcat ^

@bdbcat
Copy link
Member

bdbcat commented Apr 15, 2024

Tested on local rPI5. Could not reproduce Hakan's trouble.
Hakan suspects local configuration problems.
So we declare "OK" for Beta test.
Done now.

@leamas leamas added the done Fixed in master branch label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug done Fixed in master branch
Projects
None yet
Milestone
OpenCPN 5.10.0
Development

Successfully merging a pull request may close this issue.

debian/rules: Use default portaudio sound leamas/OpenCPN
4 participants
@nohal @leamas @bdbcat @Hakansv