Skip to content

Use snd_pcm_pause if available #2496

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

Open
Mikachu wants to merge 2 commits into
base: master
Choose a base branch
from
+84 −7
Open

Use snd_pcm_pause if available #2496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ccce638
Use snd_pcm_pause if available
Mikachu May 20, 2026
b7f4893
review comments
Mikachu May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/lib/alsa/HwSetup.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,8 @@ SetupHw(snd_pcm_t *pcm,
if (err < 0)
throw Alsa::MakeError(err, "snd_pcm_hw_params_get_period_size() failed");

result.can_pause = snd_pcm_hw_params_can_pause(hwparams);

return result;
}

Expand Down
1 change: 1 addition & 0 deletions src/lib/alsa/HwSetup.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace Alsa {
struct HwResult {
snd_pcm_format_t format;
snd_pcm_uframes_t buffer_size, period_size;
bool can_pause;
};

/**
Expand Down
3 changes: 2 additions & 1 deletion src/output/Filtered.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ FilteredAudioOutput::Cancel() noexcept
void
FilteredAudioOutput::BeginPause() noexcept
{
Cancel();
if (!output->SupportsHardwarePause())
Cancel();
}

bool
Expand Down
14 changes: 14 additions & 0 deletions src/output/Interface.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,20 @@ public:
return flags & FLAG_NEED_FULLY_DEFINED_AUDIO_FORMAT;
}

/**
* Returns true if this plugin can pause playback without first
* discarding buffered audio via Cancel(). When this returns true,
* #FilteredAudioOutput::BeginPause() will skip the Cancel() call,
* allowing the plugin to preserve its hardware buffer across a pause
* and resume it intact when Play() is next called.
*
* Plugins that return true must handle buffer cleanup if
* Cancel() is called while paused (e.g. on stop or seek).
*/
virtual bool SupportsHardwarePause() const noexcept {
return false;
}

/**
* Returns a map of runtime attributes.
*
Expand Down
4 changes: 4 additions & 0 deletions src/output/Thread.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,10 @@ AudioOutputControl::Task() noexcept
the actual playback */
if (source_state == SourceState::OPEN)
source.Cancel();
{

@MaxKellermann MaxKellermann May 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can only guess why this is necessary - this is worth a comment explaining it.
But it's only really necessary if the output was paused-without-cancel? Can we skip the call on outputs that don't do that? And only do it if the output is really paused?

@Mikachu Mikachu May 21, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, this is needed to drop output if we paused, and then for example decide to stop and switch to another track. We could skip it, but then we need to add some extra boilerplate, something like the below, so that we can check if this is supported on the output. The double Cancel() call is a no-op but it is an extra blocking call, idk how much that matters? I can go either way.

diff --git i/src/output/Filtered.cxx w/src/output/Filtered.cxx
index b5e41e98e7..693783bded 100644
--- i/src/output/Filtered.cxx
+++ w/src/output/Filtered.cxx
@@ -33,12 +33,6 @@ FilteredAudioOutput::SupportsPause() const noexcept
        return output->SupportsPause();
 }

+bool
+FilteredAudioOutput::SupportsPauseWithoutCancel() const noexcept
+{
+       return output->SupportsPauseWithoutCancel();
+}
+
 std::map<std::string, std::string, std::less<>>
 FilteredAudioOutput::GetAttributes() const noexcept
 {
diff --git i/src/output/Filtered.hxx w/src/output/Filtered.hxx
index b9e3bb68f7..e9c4b27806 100644
--- i/src/output/Filtered.hxx
+++ w/src/output/Filtered.hxx
@@ -155,9 +155,6 @@ public:
        [[gnu::pure]]
        bool SupportsPause() const noexcept;

+       [[gnu::pure]]
+       bool SupportsPauseWithoutCancel() const noexcept;
+
        std::map<std::string, std::string, std::less<>> GetAttributes() const noexcept;
        void SetAttribute(std::string &&name, std::string &&value);

@MaxKellermann MaxKellermann Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Even if it's effectively a no-op, it adds overhead, so we shouldn't do any calls when we already know it's a no-op.

const ScopeUnlock unlock(mutex);
output->Cancel();
}
InternalPause(lock);
} else {
InternalClose(false);
Expand Down
67 changes: 61 additions & 6 deletions src/output/plugins/AlsaOutputPlugin.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ class AlsaOutput final
const bool close_on_pause;

std::atomic_bool paused;
bool hw_can_pause = false;

@MaxKellermann MaxKellermann May 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How is cross-thread access to this field protected?

@Mikachu Mikachu May 21, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

(see other reply about return Pause()).


public:
AlsaOutput(EventLoop &loop, const ConfigBlock &block);
Expand Down Expand Up @@ -398,6 +399,11 @@ class AlsaOutput final
/* virtual methods from class MultiSocketMonitor */
Event::Duration PrepareSockets() noexcept override;
void DispatchSockets() noexcept override;

bool SupportsHardwarePause() const noexcept override {
return !close_on_pause && hw_can_pause;
}

};

static constexpr Domain alsa_output_domain("alsa_output");
Expand Down Expand Up @@ -563,6 +569,8 @@ AlsaOutput::Setup(AudioFormat &audio_format,
AlsaSetupSw(pcm, hw_result.buffer_size - hw_result.period_size,
hw_result.period_size);

hw_can_pause = hw_result.can_pause;

auto alsa_period_size = hw_result.period_size;
if (alsa_period_size == 0)
/* this works around a SIGFPE bug that occurred when
Expand Down Expand Up @@ -1139,13 +1147,46 @@ AlsaOutput::Cancel() noexcept
bool
AlsaOutput::Pause() noexcept
{
std::lock_guard lock{mutex};
interrupted = false;
{
std::lock_guard lock{mutex};
interrupted = false;

if (close_on_pause)
return false;
if (close_on_pause)
return false;

// TODO use snd_pcm_pause()?
if (!hw_can_pause) {
paused = true;
return true;
}
}

if (LockIsActive()) {
BlockingCall(GetEventLoop(), [this](){
/* only pause if actually running; if the PCM is
still in PREPARED state, there is nothing in
the hardware buffer worth preserving */
if (snd_pcm_state(pcm) == SND_PCM_STATE_RUNNING) {
int err = snd_pcm_pause(pcm, /* enable */ 1);
if (err < 0) {
LogError(alsa_output_domain,
"snd_pcm_pause() failed");
hw_can_pause = false;
return;
}
}

UnregisterSockets();
silence_timer.Cancel();

/* set waiting=true so that Activate() inside
Play() will re-schedule the event loop on
resume; without this, Activate() sees
active=true/waiting=false and returns early */
waiting = true;
});
}
if (!hw_can_pause)
return Pause(); // try again without hw pause

@MaxKellermann MaxKellermann May 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please avoid recursive self-calls. This smells badly.
What's the point of this call, anyway?

@Mikachu Mikachu May 21, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The idea here was that if somehow the snd_pcm_pause call above fails even though the driver said yes to can_pause(), we'll disable hw_can_pause and pause the old fashioned way. This recursive call would only ever happen once per session, but maybe it's a bit overly paranoid? Plus as you mentioned in the other comment, it might require protecting hw_can_pause with some lock, when we otherwise don't need it (it stays constant after the hw init, and I'm assuming Setup() is not run in parallel with playback :) ). I can remove this whole fallback thing if it's too silly?

@MaxKellermann MaxKellermann Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you believe there needs to be a fallback, then there needs to be a fallback. Don't remove it. But implement it without Pause() calling itself recursively.
If you're brave, you can update hw_can_pause without a lock, if you believe it's safe (enough). You can do anything if you know what you're doing. I'm not going to lawyer about 100% strict thread-safety; sometimes we can cut corners. But that consideration should be documented.


paused = true;
return true;
Expand Down Expand Up @@ -1215,7 +1256,12 @@ AlsaOutput::Play(std::span<const std::byte> src)
assert(!src.empty());
assert(src.size() % in_frame_size == 0);

paused = false;
const bool was_paused = paused.exchange(false);

if (was_paused) {
std::lock_guard lock{mutex};
Activate();
}

const size_t max_frames = LockWaitWriteAvailable();
const size_t max_size = max_frames * in_frame_size;
Expand All @@ -1242,6 +1288,15 @@ AlsaOutput::PrepareSockets() noexcept
}

try {
/* if the PCM was paused via snd_pcm_pause() (i.e. Pause()
was called with hw_can_pause), resume it now that Play()
has delivered new data and re-activated the I/O thread */
if (snd_pcm_state(pcm) == SND_PCM_STATE_PAUSED) {
int err = snd_pcm_pause(pcm, /* disable */ 0);
if (err < 0)
throw Alsa::MakeError(err, "snd_pcm_pause() failed");
}

return non_block.PrepareSockets(*this, pcm);
} catch (...) {
ClearSocketList();
Expand Down