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

Hard latency restriction for the PulseAudio backend. #170

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

szlop
Copy link
Contributor

@szlop szlop commented Mar 21, 2023

The changes add two optional arguments to the player and recorder functions:

  • maxlatencyis translated to the PulseAudiobufattr.maxlength`` parameter, which sets a limit for the internal audio buffer and such restricts latency.
  • report_under_overflow is an optional boolean debug argument, which enables the reporting of buffer underflows and overflows to the terminal.

The changes are meant for use-cases, which prioritize a defined latency over interruption-free playback/recording.

szlop and others added 3 commits March 21, 2023 13:55
…player, record and recorder. The arguments takes an integeger value and restricts the latency of the PulseAudio buffering to the given number of sample frames. If maxlatency is set, buffer underflows or overflows will occur, when the processing cannot keep up.
…member functions of the PulseAudio back end. If set to True, debug information will be printed to the terminal, whenever a buffer underflow or overflow occurs during playback.
Added short explanation of maxlatency and report_under_overflow parameters.
Copy link
Owner

@bastibe bastibe left a comment

Choose a reason for hiding this comment

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

Very nice! I like your additions.

Two small requests:

  • reformat the docstrings to the common line length
  • use a warning instead of print, same as on the Windows side

Speaking of the Windows side, we don't have a report_under_overflow over there, but simply always report errors when they occur. I think we should do a similar thing here, as I can't think of a situation where I wouldn't want to know about an under/overflow. (And if we're using warnings, the user can still opt to install a warning-filter)

might be necessary for short block lengths or high
sample rates or optimal performance. Default is ``False``.
maxlatency : int
Linux only: restrict latency to maxlatency sample frames. If set, buffer underflows or overflows will occur when
Copy link
Owner

Choose a reason for hiding this comment

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

please restrict line length of docstrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do that. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess docstrings should be limited to 72 characters, right?

Pycharm is complaining about some format errors in the pulseaudio.py file, e.g., PEP 8: E302 expected 2 blank lines, found 1 above the function headers. Is there a reason for the existing format or could we just hit "reformat" and be done with this? :)

Copy link
Owner

Choose a reason for hiding this comment

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

So long as it's a few changes, I'm fine with that. Just make sure it's in a separate commit so it's easy to revert if necessary.

I guess docstrings should be limited to 72 characters, right?

Yes. Don't worry if it's a few characters too much, though, I don't follow these rules religiously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave some love to the docstrings and the overall format. It's just a proposal, I won't be insulted, if you reject the changes. :)

Copy link
Owner

Choose a reason for hiding this comment

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

Looks good overall. There are a few details I'd do differently, but not enough to warrant a redaction. Thank you!

if self._report_under_overflow:
@_ffi.callback("pa_stream_notify_cb_t")
def overflow_callback(stream, userdata):
print('Overflow detected.')
Copy link
Owner

Choose a reason for hiding this comment

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

On the Windows side, we used a custom SoundcardRuntimeWarning instead of print: https://github.com/bastibe/SoundCard/blob/master/soundcard/mediafoundation.py#L772

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint. I was wondering how to report the event properly.

szlop added 3 commits April 15, 2023 11:57
- Replaced all occurrences of pulseaudio in comments by upper case Pulseaudio.
- Replaced all occurrences of numpy in comments by upper case Numpy.
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