Avoid calling alsa::PCM::new() multiple times.

[alexmoon]

Some low-level ALSA devices seem to not react well to having snd_pcm_open() called immediately after snd_pcm_close() on another handle for the same device. See for example this StackOverflow answer.

Currently enumerating ALSA devices will cause each device to be opened and immediately closed, as will calling supported_configs(). Finally, the device is only opened for good when build_*_stream_[raw]() is called.

This patch ensures that alsa::PCM::new() is called only once for a given Device by saving each PCM object in the Device instance the first time it is opened and then handing ownership off to the Stream when it is created. A parking_lot::Mutex is used to provide interior mutability and Sync for the Device.

This also ensures that a Device that isn't shared can't be "stolen" by another process between when it is enumerated and when the stream is opened.

[mitchmindtree]

Thanks a lot for the PR!

This looks fine to me, but I should cc @diwic as they're more intimate with the alsa API than I.

If we don't hear back I'm happy to merge though :)

[diwic]

Hi and thanks for the PR!

Some low-level ALSA devices seem to not react well to having snd_pcm_open() called immediately after snd_pcm_close() on another handle for the same device.

I would argue that this would be a bug in the driver if this is the case. But anyhow...

This patch ensures that alsa::PCM::new() is called only once for a given Device by saving each PCM object in the Device instance the first time it is opened and then handing ownership off to the Stream when it is created.

...this sounds like a good idea. Opening and closing devices can cause clicks sometimes, and can also take some milliseconds to complete, and if we can minimize that we should definitely do it.

A parking_lot::Mutex is used to provide interior mutability and Sync for the Device.

Is there any reason std::sync::Mutex does not work here? It seems overkill to add another dependency just for this use case.

That a Mutex is needed at all is a bit unfortunate (Rust can hand over ownership just fine) but we have just a &self rather than a &mut self here.
I think this is a question for the other @RustAudio/cpal-maintainers - we actually do change the logic here of when devices are opened and when they are not:

• The first call to the stream gets the PCM device, what if somebody closes the Stream and builds another Stream again later, as the stream is not returned to the Device. Is this a use case we want to support?
• And in general - if devices are kept open where they previously were not, can this cause problems for applications expecting the previous behavior? What do other backends do?

[alexmoon]

Hi and thanks for the PR!

Some low-level ALSA devices seem to not react well to having snd_pcm_open() called immediately after snd_pcm_close() on another handle for the same device.

I would argue that this would be a bug in the driver if this is the case. But anyhow...

I agree, but there's not much cpal can do about that.

A parking_lot::Mutex is used to provide interior mutability and Sync for the Device.

Is there any reason std::sync::Mutex does not work here? It seems overkill to add another dependency just for this use case.

That a Mutex is needed at all is a bit unfortunate (Rust can hand over ownership just fine) but we have just a &self rather than a &mut self here.

std::sync::Mutex would work fine. I used parking_lot because it is already used in the Windows host and doesn't have the poisoning behavior of the std Mutex. I'd be happy to switch to the std Mutex if that is preferred.

I think this is a question for the other @RustAudio/cpal-maintainers - we actually do change the logic here of when devices are opened and when they are not:

* The first call to the stream gets the PCM device, what if somebody closes the `Stream` and builds another `Stream` again later, as the stream is not returned to the `Device`. Is this a use case we want to support?

Just for clarity, this use case is supported by the patch. When the first stream is opened it takes the alsa::PCM out of the Option in the Device struct. If you then try to open a second stream with the same Device it will see the Option is None and try to open a new alsa::PCM. If the first stream is still open and the Device doesn't support sharing that second open could fail, but that's no different than the previous behavior.

* And in general - if devices are kept open where they previously were not, can this cause problems for applications expecting the previous behavior? What do other backends do?

This behavior is different, but don't think it's likely to cause a problem. The only way to get a non-default Device is out of the Devices iterator, which is already opening the device temporarily in the current version. Default devices don't open until you create the stream even with this patch. So to observe different behavior you'd have to take ownership of a Device from the Devices iterator and keep it alive without starting a stream. Even then to observe different behavior within cpal I think you'd have to iterate over Devices again at which point if the Device you kept was non-shareable it would not appear in the second iteration.

[diwic]

@alexmoon Thanks for the answers! So I have not been involved in cpal's design, I just know ALSA from having worked with it previously. So if other maintainers prefer parking_lot's mutex over std's I don't mind either.

 Just for clarity, this use case is supported by the patch. When the first stream is opened it takes the alsa::PCM out of the Option in the Device struct. If you then try to open a second stream with the same Device it will see the Option is None and try to open a new alsa::PCM. If the first stream is still open and the Device doesn't support sharing that second open could fail, but that's no different than the previous behavior.

Ah, now I see it. And something similar goes for supported_configs.

From a code readability perspective I wonder if it would be good to refactor the creation of PCM's into a separate function, perhaps if the new DeviceHandles struct had fn get_mut(&mut self, &name, direction) -> Result<&mut alsa::PCM> and fn take(&mut self, &name, direction) -> Result<alsa::PCM> that would return the existing handle if there is one, or try to create one if there isn't? Do you think that would make sense?

[alexmoon]

From a code readability perspective I wonder if it would be good to refactor the creation of PCM's into a separate function, perhaps if the new DeviceHandles struct had fn get_mut(&mut self, &name, direction) -> Result<&mut alsa::PCM> and fn take(&mut self, &name, direction) -> Result<alsa::PCM> that would return the existing handle if there is one, or try to create one if there isn't? Do you think that would make sense?

Yes, that's a good improvement. I've updated the PR.

[diwic]

Thanks for the update! I've done more suggestions; they are just my opinions though so if you disagree then I'm fine with this PR merged as it is.

[diwic]

I think this looks good now, so I merged it. Thanks for the work!