Use snd_pcm_pause if available#2496
Conversation
MaxKellermann
left a comment
MaxKellermann
left a comment
There was a problem hiding this comment.
Thanks for the work, at a first glance, I like it. I need to take some more time to verify it and of course test it.
I have a few questions and comments.
| return flags & FLAG_PAUSE; | ||
| } | ||
|
|
||
| virtual bool SupportsPauseWithoutCancel() const noexcept { |
There was a problem hiding this comment.
That's a clunky method name, but lacking an idea for a better name, that's okay.
But please add some API documentation explaining what it means.
There was a problem hiding this comment.
Another idea is maybe SupportsHardwarePause() but I'm not sure how applicable that might be to other plugins.
/**
* 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).
*/
does this make sense for an api doc comment? (I've literally only looked at the alsa output plugin, so i'm not exactly sure in which directions things generalize in mpd).
| the actual playback */ | ||
| if (source_state == SourceState::OPEN) | ||
| source.Cancel(); | ||
| { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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);| const bool close_on_pause; | ||
|
|
||
| std::atomic_bool paused; | ||
| bool hw_can_pause = false; |
There was a problem hiding this comment.
How is cross-thread access to this field protected?
There was a problem hiding this comment.
(see other reply about return Pause()).
| ring_buffer.Clear(); | ||
|
|
||
| active = false; | ||
| paused = false; |
There was a problem hiding this comment.
I think this was just me being a bit confused about things at one point, and then didn't realize it wasn't actually needed. I'll remove it.
| }); | ||
| } | ||
| if (!hw_can_pause) | ||
| return Pause(); // try again without hw pause |
There was a problem hiding this comment.
Please avoid recursive self-calls. This smells badly.
What's the point of this call, anyway?
There was a problem hiding this comment.
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?
2 participants
This should fix #1136 but note I have only tried this on hardware that does support pause (quite old pci soundblaster live). It is also quite possible I do silly things in the patch, I'm not very familiar with the codebase, but pausing/unpausing works, stopping/playing while paused or not paused works, with and without the always_on and/or close_on_pause options (the latter should fully disable hardware pausing and revert to the old behaviour, and it's on by default). It didn't seem worth adding another option explicitly to disable hardware pause since this option already exists, but I don't have whatever DSD is, so maybe I do break that usecase if that needs silence streamed actively?