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

fix(main/cmus): ensure the AAudio buffer is at least 80ms #22418

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pgaskin
Copy link
Contributor

@pgaskin pgaskin commented Nov 29, 2024

This fixes buffer underruns on the Pixel 9 due to the default buffer capacity and burst size being much lower than other devices (e.g., 2886/962 frames at 48000 Hz on the P9 vs 6734/3367 on the P8).

I am one of the upstream maintainers and the author of the AAudio output plugin.

See cmus/cmus#1386. This PR is a cherry-pick of the first commit (the bugfix), but not the second (a feature to allow overriding the requested buffer capacity).

Copy link
Member

@TomJo2000 TomJo2000 left a comment

Choose a reason for hiding this comment

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

This PR will presumably cause the next auto-update of cmus to fail due to this patch being upstreamed by then.

Edit: Whoops, did not realize you're a frequent contributor to the upstream cmus project.
Guess I'll take a look at the patch itself now then.

Edit 2: Yep patch itself should be fine from a cursory look.

56f21a455bc611d879ad5384ca26c154bd59a34d # ensure the aaudio buffer is at least 80ms (cmus/cmus#1386)
)
for sha in "${commits[@]}"; do
termux_download "https://github.com/cmus/cmus/commit/$sha.patch" "$sha.patch" "SKIP_CHECKSUM" # skip checksum since we're already referencing an exact commit hash
Copy link
Member

Choose a reason for hiding this comment

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

Where are we downloading this patch to anyway?
Please make sure it goes in the temp directory so it will be cleaned up properly.

Suggested change
termux_download "https://github.com/cmus/cmus/commit/$sha.patch" "$sha.patch" "SKIP_CHECKSUM" # skip checksum since we're already referencing an exact commit hash
termux_download "https://github.com/cmus/cmus/commit/$sha.patch" "${TERMUX_PKG_TMPDIR}/${sha}.patch" "SKIP_CHECKSUM" # skip checksum since we're already referencing an exact commit hash

)
for sha in "${commits[@]}"; do
termux_download "https://github.com/cmus/cmus/commit/$sha.patch" "$sha.patch" "SKIP_CHECKSUM" # skip checksum since we're already referencing an exact commit hash
git apply "$sha.patch"
Copy link
Member

Choose a reason for hiding this comment

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

This one will also need to be changed accordingly.

Suggested change
git apply "$sha.patch"
git apply "${TERMUX_PKG_TMPDIR}/${sha}.patch"

TERMUX_PKG_DEPENDS="ffmpeg, libandroid-support, libflac, libiconv, libmad, libmodplug, libvorbis, libwavpack, ncurses, opusfile, pulseaudio"
TERMUX_PKG_SRCURL=https://github.com/cmus/cmus/archive/refs/tags/v${TERMUX_PKG_VERSION}.tar.gz
TERMUX_PKG_SHA256=44b96cd5f84b0d84c33097c48454232d5e6a19cd33b9b6503ba9c13b6686bfc7
TERMUX_PKG_AUTO_UPDATE=true
TERMUX_PKG_BUILD_IN_SRC=true

termux_step_pre_configure() {
# cherry-pick patches
local sha commits=(
56f21a455bc611d879ad5384ca26c154bd59a34d # ensure the aaudio buffer is at least 80ms (cmus/cmus#1386)
Copy link
Member

Choose a reason for hiding this comment

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

This is 40 characters long, so it's obviously not the full SHA256 hash.
If you're just using the short-hash for the commit the first 7 chars should be sufficient.
Either use the full commit hash or use the proper short-hash.

Please also link to the PR with a full link.

Suggested change
56f21a455bc611d879ad5384ca26c154bd59a34d # ensure the aaudio buffer is at least 80ms (cmus/cmus#1386)
56f21a455bc611d879ad5384ca26c154bd59a34d # ensure the aaudio buffer is at least 80ms (https://github.com/cmus/cmus/pull/1386)

I also can't help but notice that this is your own upstream PR, from about an hour ago.
I'd like to hear back from upstream about their opinion on this before making any decision regarding this PR.

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