Add support for concurrent multisink audio #4653

kgoderis · 2025-03-18T08:07:25Z

Add support for concurrent multisink audio

Allow playing audio concurrently on multiple sinks. In the context of the Sonos binding, this eliminates the need to group or ungroup Sonos players in order to play audio synchronously on all players (e.g. in Rules). Audio is also played concurrently on all sinks using a ThreadPool that is configurable in services.cfg ("threadpool:audio=10"), thereby eliminating the "sequential" output when playing audio on a large group of sinks (e.g. doorbell on Sonos)

Furthermore, it add support for comma-separated lists of sinks or patterns to the various commands, so that arbitrary groupings can be made by the end-user, e.g. "Sonos:CONNECT:*,enhancedjavasound" for usage with playSound() in Rules or the "openhab:audio play" command

kgoderis · 2025-03-18T10:16:43Z

@wborn Can you help me explain why I get a test error whereas locally all runs fine:

[INFO] 
[INFO] --- surefire:3.5.2:test (default-test) @ org.openhab.core.audio ---
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.openhab.core.audio.internal.AudioServletTest
[INFO] Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.450 s -- in org.openhab.core.audio.internal.AudioServletTest
[INFO] Running org.openhab.core.audio.internal.AudioFormatTest
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.003 s -- in org.openhab.core.audio.internal.AudioFormatTest
[INFO] Running org.openhab.core.audio.internal.AudioConsoleTest
[INFO] Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.044 s -- in org.openhab.core.audio.internal.AudioConsoleTest
[INFO] Running org.openhab.core.audio.internal.AudioManagerServletTest
[main] WARN  o.o.c.a.internal.AudioManagerImpl - No audio sink provided for playback.
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.008 s -- in org.openhab.core.audio.internal.AudioManagerServletTest
[INFO] Running org.openhab.core.audio.internal.AudioManagerTest
[main] WARN  o.o.c.a.internal.AudioManagerImpl - No audio sink provided for playback.
[INFO] Tests run: 19, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.022 s -- in org.openhab.core.audio.internal.AudioManagerTest
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 46, Failures: 0, Errors: 0, Skipped: 0

kgoderis · 2025-03-19T11:24:35Z

@wborn @kaikreuzer Something fishy is going on with the build process on Github. As you can see, the build here above fails on a NPE that does not occur when running tests locally with mvn or in the IDE

Copying over one of the /src/test/java files into a new .java file (without change, and keeping the original "faulty" .java as well) suddenly resolves the error, and the build completes. Then removing that copy again, and the build fails.

Signed-off-by: Karel Goderis <[email protected]>

wborn · 2025-03-19T16:12:08Z

I've seen it before but didn't figure it out quickly: #3405. Usually it's a timing issue.

kgoderis · 2025-03-20T11:31:54Z

@wborn Clear. so how to proceed with this PR ?

holgerfriedrich · 2025-03-23T07:59:47Z

bundles/org.openhab.core.audio/src/main/java/org/openhab/core/audio/AudioManager.java

 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.core.library.types.PercentType;

+import io.reactivex.annotations.NonNull;


Hi, I just briefly looked into this and I am wondering why you import an additional annotation framework.
As the main class is annotated with @ NonNullByDefault, you may not need this anyway.
Did I overlook something?

@holgerfriedrich You are right, but it is a bad habit of making things explicit in method signatures that comes into play.

@kgoderis The import uses a different framework, I would expect org.eclipse.jdt.annotation.NonNull instead of io.reactivex.annotations.NonNull....

holgerfriedrich · 2025-04-03T19:46:52Z

@kgoderis could you please rebase?

kgoderis · 2025-04-08T18:29:38Z

@kgoderis could you please rebase?

Yes, will do that, and fix also the NonNull import

Copilot

Pull Request Overview

This PR adds concurrent multisink audio support to allow playing audio simultaneously on multiple sinks instead of sequentially. The key enhancement is adding support for comma-separated sink patterns and leveraging a configurable ThreadPool for parallel audio playback.

Adds new methods to AudioManager and AudioManagerImpl that accept Set sinkIds for multisink operations
Implements parallel audio playback using ThreadPoolManager with configurable thread pool
Updates getSinkIds method to parse comma-separated patterns for flexible sink selection

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
AudioManager.java	Adds interface methods for multisink audio operations
AudioManagerImpl.java	Implements concurrent playback logic and enhanced pattern matching
Audio.java	Updates script actions to use new getSinkIds method
AudioConsoleCommandExtension.java	Refactors console commands to use multisink functionality
AudioSinkFake.java	Adds test implementation of processAndComplete method

Copilot · 2025-07-26T14:36:30Z

...s/org.openhab.core.audio/src/main/java/org/openhab/core/audio/internal/AudioManagerImpl.java

+import io.reactivex.annotations.NonNull;
+


The import io.reactivex.annotations.NonNull is unnecessary since the code already uses org.eclipse.jdt.annotation.NonNullByDefault and standard JDT annotations. This creates confusion about which annotation framework is being used.

Suggested change

import io.reactivex.annotations.NonNull;

Copilot · 2025-07-26T14:36:30Z

bundles/org.openhab.core.audio/src/main/java/org/openhab/core/audio/AudioManager.java

 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.core.library.types.PercentType;

+import io.reactivex.annotations.NonNull;


The import io.reactivex.annotations.NonNull is inconsistent with the existing annotation framework. The interface already uses @NonNullByDefault and should use JDT annotations (@NonNull) for consistency.

Suggested change

import io.reactivex.annotations.NonNull;

// Removed the unused import for io.reactivex.annotations.NonNull

Copilot · 2025-07-26T14:36:31Z

...b.core.audio/src/main/java/org/openhab/core/audio/internal/AudioConsoleCommandExtension.java

 import org.osgi.service.component.annotations.Component;
 import org.osgi.service.component.annotations.Reference;

+import io.reactivex.annotations.NonNull;


The import io.reactivex.annotations.NonNull is inconsistent with the existing annotation framework used in this codebase. Should use JDT annotations for consistency.

Suggested change

import io.reactivex.annotations.NonNull;

import org.eclipse.jdt.annotation.NonNull;

Copilot · 2025-07-26T14:36:31Z

...s/org.openhab.core.audio/src/main/java/org/openhab/core/audio/internal/AudioManagerImpl.java

+        List<CompletableFuture<Object>> futures = sinkIds.stream().map(sinkId -> CompletableFuture.supplyAsync(() -> {
+            AudioSink sink = getSink(sinkId);
+            if (sink == null) {
+                logger.warn("Sink '{}' not found. Skipping.", sinkId);
+                return null; // Return null for missing sinks
+            }
+
+            // Handle volume adjustment for the current sink
+            Runnable restoreVolume = handleVolumeCommand(volume, sink);
+
+            try {
+                // Play the audio stream synchronously on this sink
+                sink.processAndComplete(audioStream);
+                logger.debug("Audio stream '{}' has been played on sink '{}'.", audioStream, sinkId);
+            } catch (Exception e) {
+                logger.error("Error playing '{}' on sink '{}': {}", audioStream, sinkId, e.getMessage(), e);
+            } finally {
+                restoreVolume.run(); // Ensure volume is restored after playback completes
+            }
+            return null;
+        }, pool)).collect(Collectors.toList());


The processAndComplete method returns a CompletableFuture but the return value is ignored. This means the code doesn't wait for completion and volume restoration may happen before playback finishes.

Suggested change

List<CompletableFuture<Object>> futures = sinkIds.stream().map(sinkId -> CompletableFuture.supplyAsync(() -> {

AudioSink sink = getSink(sinkId);

if (sink == null) {

logger.warn("Sink '{}' not found. Skipping.", sinkId);

return null; // Return null for missing sinks

}

// Handle volume adjustment for the current sink

Runnable restoreVolume = handleVolumeCommand(volume, sink);

try {

// Play the audio stream synchronously on this sink

sink.processAndComplete(audioStream);

logger.debug("Audio stream '{}' has been played on sink '{}'.", audioStream, sinkId);

} catch (Exception e) {

logger.error("Error playing '{}' on sink '{}': {}", audioStream, sinkId, e.getMessage(), e);

} finally {

restoreVolume.run(); // Ensure volume is restored after playback completes

}

return null;

}, pool)).collect(Collectors.toList());

List<CompletableFuture<Void>> futures = sinkIds.stream().map(sinkId -> CompletableFuture.supplyAsync(() -> {

AudioSink sink = getSink(sinkId);

if (sink == null) {

logger.warn("Sink '{}' not found. Skipping.", sinkId);

return CompletableFuture.completedFuture(null); // Return a completed future for missing sinks

}

// Handle volume adjustment for the current sink

Runnable restoreVolume = handleVolumeCommand(volume, sink);

// Play the audio stream asynchronously on this sink

return sink.processAndComplete(audioStream)

.exceptionally(exception -> {

logger.error("Error playing '{}' on sink '{}': {}", audioStream, sinkId, exception.getMessage(), exception);

return null; // Handle the exception gracefully

})

.thenRun(() -> {

restoreVolume.run(); // Ensure volume is restored after playback completes

logger.debug("Audio stream '{}' has been played on sink '{}'.", audioStream, sinkId);

});

}, pool).thenCompose(future -> future)).collect(Collectors.toList());

Copilot · 2025-07-26T14:36:31Z

...s/org.openhab.core.audio/src/main/java/org/openhab/core/audio/internal/AudioManagerImpl.java

+    }
+
+    @Override
+    public void play(@Nullable AudioStream audioStream, @NonNull Set<String> sinkIds) {


The @NonNull annotation here uses RxJava imports but should use JDT annotations (@org.eclipse.jdt.annotation.NonNull) for consistency with the rest of the codebase.

Suggested change

public void play(@Nullable AudioStream audioStream, @NonNull Set<String> sinkIds) {

public void play(@Nullable AudioStream audioStream, @org.eclipse.jdt.annotation.NonNull Set<String> sinkIds) {

Copilot · 2025-07-26T14:36:31Z

...s/org.openhab.core.audio/src/main/java/org/openhab/core/audio/internal/AudioManagerImpl.java

+        List<CompletableFuture<Object>> futures = sinkIds.stream().map(sinkId -> CompletableFuture.supplyAsync(() -> {
+            AudioSink sink = getSink(sinkId);
+            if (sink == null) {
+                logger.warn("Sink '{}' not found. Skipping.", sinkId);
+                return null; // Return null for missing sinks


Using CompletableFuture<Object> with supplyAsync that always returns null is misleading. Consider using CompletableFuture<Void> with runAsync for better semantic clarity since no value is being computed.

Suggested change

List<CompletableFuture<Object>> futures = sinkIds.stream().map(sinkId -> CompletableFuture.supplyAsync(() -> {

AudioSink sink = getSink(sinkId);

if (sink == null) {

logger.warn("Sink '{}' not found. Skipping.", sinkId);

return null; // Return null for missing sinks

List<CompletableFuture<Void>> futures = sinkIds.stream().map(sinkId -> CompletableFuture.runAsync(() -> {

AudioSink sink = getSink(sinkId);

if (sink == null) {

logger.warn("Sink '{}' not found. Skipping.", sinkId);

return; // Skip processing for missing sinks

kgoderis requested a review from a team as a code owner March 18, 2025 08:07

kgoderis force-pushed the multisink branch 2 times, most recently from 080f8d9 to 466fa25 Compare March 18, 2025 09:32

kgoderis force-pushed the multisink branch 6 times, most recently from 6162850 to 2f48b3b Compare March 19, 2025 11:12

Add support for concurrent multisink audio

d20819c

Signed-off-by: Karel Goderis <[email protected]>

kgoderis force-pushed the multisink branch from 2f48b3b to d20819c Compare March 19, 2025 11:26

holgerfriedrich reviewed Mar 23, 2025

View reviewed changes

wborn added the enhancement An enhancement or new feature of the Core label Jun 20, 2025

wborn requested a review from Copilot July 26, 2025 14:35

Copilot AI reviewed Jul 26, 2025

View reviewed changes

	import io.reactivex.annotations.NonNull;
	// Removed the unused import for io.reactivex.annotations.NonNull

	import io.reactivex.annotations.NonNull;
	import org.eclipse.jdt.annotation.NonNull;

	public void play(@Nullable AudioStream audioStream, @NonNull Set<String> sinkIds) {
	public void play(@Nullable AudioStream audioStream, @org.eclipse.jdt.annotation.NonNull Set<String> sinkIds) {

Uh oh!

Add support for concurrent multisink audio #4653

Are you sure you want to change the base?

Add support for concurrent multisink audio #4653

Uh oh!

Conversation

kgoderis commented Mar 18, 2025

Add support for concurrent multisink audio

Uh oh!

kgoderis commented Mar 18, 2025

Uh oh!

kgoderis commented Mar 19, 2025

Uh oh!

wborn commented Mar 19, 2025

Uh oh!

kgoderis commented Mar 20, 2025

Uh oh!

holgerfriedrich Mar 23, 2025

Choose a reason for hiding this comment

Uh oh!

kgoderis Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

holgerfriedrich Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

holgerfriedrich commented Apr 3, 2025

Uh oh!

kgoderis commented Apr 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants