Skip to content

Conversation

@davidandreoletti
Copy link
Contributor

@davidandreoletti davidandreoletti commented Dec 20, 2023

Maintainer: @ejurgensen

Compile tested:

  • arch: armv8
  • model: NanoPI R4S
  • OpenWrt version: 23.05

Run tested:

  • arch: armv8
  • model: NanoPI R4S
  • OpenWrt version: OpenWRT version: OpenWrt 23.05.2 r23630-842932a63d / LuCI openwrt-23.05 branch git-23.306.39416-c86c256
  • test done:
  • owntone-pulseaudio 28.5-2 installed on NanoPi R4S
    • I compiled + tested the changes above to this repo's master. Why ? master uses a new version of libsodium incompatible with the lib sodium version installed on 23.05 openwrt releases.
  • setup shairport-sync to write audio to named pipe
  • setup owntone read audio named pipe and stream to pulseaudio's default bluetooth sink (thanks for the write up on the owntone doc)
    • 🔊 People can hear music sent from an iPhone to OpenWRT (shairport-sync[0] -> named pipe -> owntone -> pulseaudio -> bluetooth stack) to Bluetooth Speaker !

[0] #22923

Description:

  • Goal: Create package variant with pulseaudio support
  • Packages size difference: + 4076 bytes
    • owntone_28.8-2_aarch64_generic.ipk: 734443 bytes
    • owntone-pulseaudio_28.8-2_aarch64_generic.ipk: 738519 bytes
    • Size reported with ls -ls --block-size 1 /tmp/shairport-sync-openssl_4.3.2-0_aarch64_generic.ipk

Irrelevant content now:

  • @ejurgensen Building the package "owntone-pulseaudio" fails with this error.
    • I am hoping you can provide some guidance as to how to resolve the issues mark "FIXME".

@davidandreoletti davidandreoletti force-pushed the owntone-enable-pulseaudio-with-package-variant branch 2 times, most recently from 207591c to 6edaa8d Compare December 20, 2023 15:32
@neheb
Copy link
Contributor

neheb commented Dec 20, 2023

If I had to guess, rpath-link hack as used in mpd is needed.

@davidandreoletti
Copy link
Contributor Author

If I had to guess, rpath-link hack as used in mpd is needed.

@neheb Good guess! Thank you.

@davidandreoletti davidandreoletti force-pushed the owntone-enable-pulseaudio-with-package-variant branch 2 times, most recently from 029ee89 to 33114df Compare December 21, 2023 07:11
@davidandreoletti
Copy link
Contributor Author

davidandreoletti commented Dec 21, 2023

owntone-pulseaudio is defined with DEPENDS+= +AUDIO_SUPPORT:pulseaudio.

  • pulseaudio is a virtual package, declared in:
    • pulseaudio-daemon
    • pulseaudio-daemon-avahi

On my router:
0. pulseaudio-daemon-avahi is installed
1 opkg install /tmp/owntone-pulseaudio_28.8-2_aarch64_generic.ipk fails:

Installing owntone-pulseaudio (28.8-2) to root...
Installing pulseaudio-daemon (14.2-10) to root...
Collected errors:
 * check_conflicts_for: The following packages conflict with pulseaudio-daemon:
 * check_conflicts_for:         pulseaudio-daemon-avahi *
 * opkg_install_cmd: Cannot install package owntone-pulseaudio.

@neheb / @ejurgensen How to specify owntone-pulseaudio is fine with either dependency pulseaudio-daemon or pulseaudio-daemon-avahi installed (as long as one of them installed) ?

@davidandreoletti davidandreoletti force-pushed the owntone-enable-pulseaudio-with-package-variant branch 3 times, most recently from 4ab53b0 to 5a3be37 Compare January 3, 2024 12:10
@davidandreoletti
Copy link
Contributor Author

davidandreoletti commented Jan 4, 2024

Moving the discussion to https://forum.openwrt.org/t/owntone-support-pulseaudio/182872?u=dandreolett to get help from the wider community
Problem solved. Discussion closed.

@davidandreoletti davidandreoletti force-pushed the owntone-enable-pulseaudio-with-package-variant branch 3 times, most recently from 08c7d5e to 76cca92 Compare January 5, 2024 05:00
@davidandreoletti davidandreoletti changed the title owntone: Add new owntone package variant: owntone-pulseaudio owntone: New package variant: owntone-pulseaudio Jan 5, 2024
@davidandreoletti davidandreoletti force-pushed the owntone-enable-pulseaudio-with-package-variant branch from 76cca92 to 8991b99 Compare January 5, 2024 16:15
@davidandreoletti davidandreoletti marked this pull request as ready for review January 5, 2024 16:15
@davidandreoletti
Copy link
Contributor Author

@ejurgensen

@ejurgensen
Copy link
Contributor

ejurgensen commented Jan 5, 2024

Should this PR be merged in https://github.com/owntone/owntone-openwrt/tree/master instead ?

I think here is the right place, the owntone repo is for testing/CI. You are welcome to make a PR to that repo too, but otherwise I will pull it myself from here.

Let me know if further work needed on the PR itself.

Looks good to me, but I should say that I'm not an OpenWrt packaging expert.

People can hear music sent from an iPhone to OpenWRT (shairport-sync[0] -> named pipe -> owntone -> pulseaudio -> bluetooth stack) to Bluetooth Speaker !

Super cool!

@BKPepe
Copy link
Member

BKPepe commented Jan 5, 2024

  1. Commit description is missing.

Ad) the size difference. Does it make sense to create variant in the first place instead of enabling it by default? Doing that for +4076 bytes. Hmmm. I would rather see that enabled it by default. You know. Every year requirements are higher and higher. Software is using more storage, etc.

@davidandreoletti
Copy link
Contributor Author

I will pull it myself from here.

Ok

@davidandreoletti
Copy link
Contributor Author

davidandreoletti commented Jan 6, 2024

Commit description is missing.

done.

@davidandreoletti
Copy link
Contributor Author

davidandreoletti commented Jan 7, 2024

Does it make sense to create variant in the first place instead of enabling it by default? Doing that for +4076 bytes. Hmmm. I would rather see that enabled it by default.

I agree: Enabled by default. With DEPENDS+= +AUDIO_SUPPORT:pulseaudio, targets not supporting audio support will gracefully compile without pulseaudio support, ie current owntone package.

The PR has 2 commits:

  • (a) b46a8fe: new owntone-pulseaudio package variant
  • (b) 194d4c8: original owntone package with conditional pulseaudio support added

@BKPepe / @ejurgensen
Once a decision is taken, I will remove the irrelevant commit.

I prefer (b).

@davidandreoletti davidandreoletti force-pushed the owntone-enable-pulseaudio-with-package-variant branch 3 times, most recently from 211c509 to d568764 Compare January 9, 2024 11:11
@davidandreoletti
Copy link
Contributor Author

Does it make sense to create variant in the first place instead of enabling it by default? Doing that for +4076 bytes. Hmmm. I would rather see that enabled it by default.

I agree: Enabled by default. With DEPENDS+= +AUDIO_SUPPORT:pulseaudio, targets not supporting audio support will gracefully compile without pulseaudio support, ie current owntone package.

The PR has 2 commits:

  • (a) b46a8fe: new owntone-pulseaudio package variant
  • (b) 194d4c8: original owntone package with conditional pulseaudio support added

@BKPepe / @ejurgensen Once a decision is taken, I will remove the irrelevant commit.

I prefer (b).

@BKPepe / @ejurgensen (ping)

@ejurgensen
Copy link
Contributor

I agree, just having it enabled makes sense if the difference is that small.

With DEPENDS+= +AUDIO_SUPPORT:pulseaudio, targets not supporting audio support will gracefully compile without pulseaudio support

Not sure I understand this - for package building, the "target" is the buildbot, right? Shouldn't they all build the same type of package?

@davidandreoletti
Copy link
Contributor Author

davidandreoletti commented Jan 13, 2024

This is how I understood how it works.

  • A target is a hardware device.
  • Each hardware device has different audio capabilities, some supported by OpenWRT.
  • AUDIO_SUPPORT enables theowntone package to build against pulseaudio when the kernel / .config file support/enable it.

For my particular case, I don't think AUDIO_SUPPORT is required (audio over bluetooth stack). Yet I think it might be relevant for other device targets.

@BKPepe Would mind amending gaps in my understanding, if any please ?

@davidandreoletti
Copy link
Contributor Author

@BKPepe Would mind amending gaps in my understanding, if any please ?

(Ping @BKPepe)

@BKPepe
Copy link
Member

BKPepe commented Jan 26, 2024

I will look at it. It looks like we dont have enough reviewers and I am trying to do it as much as I can in my free time, which I dont have much these days.

Packages size difference: +4076 bytes
- owntone_28.8-2_aarch64_generic.ipk: 734443 bytes
- owntone-pulseaudio_28.8-2_aarch64_generic.ipk: 738519 bytes

Size reported with 'ls -ls --block-size 1 /tmp/shairport-sync-openssl_4.3.2-0_aarch64_generic.ipk'

Signed-off-by: David Andreoletti <[email protected]>
Packages size difference: +4076 bytes
- owntone_28.8-2_aarch64_generic.ipk: 734443 bytes
- owntone-pulseaudio_28.8-2_aarch64_generic.ipk: 738519 bytes

PS: Size reported with 'ls -ls --block-size 1 /tmp/shairport-sync-openssl_4.3.2-0_aarch64_generic.ipk'
Signed-off-by: David Andreoletti <[email protected]>
@davidandreoletti davidandreoletti force-pushed the owntone-enable-pulseaudio-with-package-variant branch from d568764 to fb8f114 Compare February 6, 2024 10:39
@davidandreoletti
Copy link
Contributor Author

I will look at it. It looks like we dont have enough reviewers and I am trying to do it as much as I can in my free time, which I dont have much these days.

@BKPepe

I would like to merge this (once ready) before the next OpenWRT stable release.

So far the next release (timeframe seems not defined in any new meetings) and no release target set either.

Does that give you more or less time to review + answer this question ?

@davidandreoletti
Copy link
Contributor Author

@BKPepe This PR needs your input: #22942 (comment)

@GeorgeSapkin
Copy link
Member

@davidandreoletti still relevant? There are merge conflicts.

@davidandreoletti
Copy link
Contributor Author

@davidandreoletti still relevant? There are merge conflicts.

Not relevant for me at the moment yet PR updated if anyone else needs it.

@GeorgeSapkin
Copy link
Member

I think if you're the only one who has been interested in this and there has been no interest since, we can close this PR for now until something changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants