Skip to content

Comments

ALSA JACK plugin snd_pcm_drain() support#1

Open
twischer-adit wants to merge 5 commits intomasterfrom
adit_devel_SOPL_5833_alsa_jack_plugin_implement_draining
Open

ALSA JACK plugin snd_pcm_drain() support#1
twischer-adit wants to merge 5 commits intomasterfrom
adit_devel_SOPL_5833_alsa_jack_plugin_implement_draining

Conversation

@twischer-adit
Copy link
Owner

No description provided.

@twischer-adit
Copy link
Owner Author

@aditpape please review

if (io->state == SND_PCM_STATE_RUNNING) {
areas = snd_pcm_ioplug_mmap_areas(io);

while (xfer < nframes) {

Choose a reason for hiding this comment

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

where is the improvement? Doesn't this still copy nframes w/o checking availability ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I will change the commit message.
With this commit the JACK buffer will only be filled with silence in case this plugin is not in RUNNING state e.g. in PREPARED or XRUN


if (io->state == SND_PCM_STATE_RUNNING) {
areas = snd_pcm_ioplug_mmap_areas(io);
const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io);

Choose a reason for hiding this comment

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

this change can go into the previous commit.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.


/* split the snd_pcm_area_copy() function into two parts
* if the data to copy passes the buffer wrap around
*/

Choose a reason for hiding this comment

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

nice to add comment, but it does not belong to the commit topic...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Moved to 30b9a26

@twischer-adit twischer-adit force-pushed the adit_devel_SOPL_5833_alsa_jack_plugin_implement_draining branch from f6c39bd to 9d8aba4 Compare January 19, 2018 13:50
@twischer-adit
Copy link
Owner Author

Changes:

  • remove volatile from hw_ptr and state
  • Reorder, merge, move commits
  • Extend commit messages

Diff:

@@ -51,12 +51,12 @@ typedef struct {
 	/* This variable is not always in sync with io->state
 	 * because it will be accessed by multiple threads.
 	 */
-	volatile snd_pcm_state_t state;
+	snd_pcm_state_t state;
 
 	char **port_names;
 	unsigned int num_ports;
 	snd_pcm_uframes_t boundary;
-	volatile snd_pcm_uframes_t hw_ptr;
+	snd_pcm_uframes_t hw_ptr;
 	unsigned int sample_bits;
 	snd_pcm_uframes_t min_avail;

@aditpape Please give me a ping when you are finished with the review and I could try to upstream

@twischer-adit twischer-adit force-pushed the adit_devel_SOPL_5833_alsa_jack_plugin_implement_draining branch from 9d8aba4 to db97a17 Compare January 19, 2018 15:02
@twischer-adit twischer-adit force-pushed the adit_devel_SOPL_5833_alsa_jack_plugin_implement_draining branch from db97a17 to 62d8982 Compare February 23, 2018 14:09
Timo Wischer added 5 commits February 24, 2018 11:42
instead of using buffer_size as wrap around.

This is required to detect Xruns.

It is also required to allow the JACK thread
to processes the whole ALSA audio buffer at once
without calling snd_pcm_avail_update() in between.

For example when the hw_ptr will be updated with
hw_ptr += buffer_size
and it is using the buffer_size as wrap around
hw_ptr %= buffer_size
would result in the same value as before the add operation.

Due to that the user application would not recognize
that the complete audio buffer was copied.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
when the JACK thread is requesting too many audio frames

Playback:
Without this commit the ALSA audio buffer
will be played with endless repeats as long as the user application
has not provided new audio data.
Therefore this garbage will be played as long as the user application
has not called snd_pcm_stop() after an Xrun.
With this fix the rest of the JACK buffer will be filled
with silence.

Capture:
Without this commit the audio data in the ALSA buffer would be
overwritten.
With this commit the new data from the JACK buffer
will not be copied.
Therefore the existing data in the ALSA buffer
will not be overwritten.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
…bles

This variable will be used to exchange the status of the stream between
the ALSA and JACK thread.
In future commits it will also be used
to signal DRAINING state from the ALSA to JACK thread and
to signal XRUN state form the JACK to ALSA thread

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Only increasing the hw_ptr is not sufficient
because it will not be evaluated by the ALSA library
to detect an Xrun.

In addition there is a raise where and Xrun detected by the JACK thread
could not be detected in the ALSA thread.
- In playback use case
- The hw_ptr will be increased by the JACK thread (hw_ptr > appl_ptr =>
over run)
- But the ALSA thread increases the appl_ptr before evaluating the
hw_ptr
- Therefore the hw_ptr < appl_ptr again
- ALSA will not detect the over run which was already detected by the
JACK thread

Therefore an additional variable is required to report an Xrun from the
JACK thread to ALSA.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Block on drain till available samples played

Without this commit the JACK thread will be stopped
before the ALSA buffer was completely forwarded to
the JACK daemon.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
@twischer-adit twischer-adit force-pushed the adit_devel_SOPL_5833_alsa_jack_plugin_implement_draining branch from 62d8982 to b160cfa Compare February 26, 2018 07:44
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