[audio] Added getSupportedStreams() and UnsupportedAudioStreamException #3764

cweitkamp · 2017-06-28T12:36:04Z

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

cweitkamp · 2017-06-28T12:47:13Z

CAUTION: This is a change breaking PR for bindings using an AudioSink. It itroduces a new abstract method getSupportedStreams().

I will post (or push) an example implementation later.

sjsf · 2017-06-29T06:47:37Z

Could you please also adapt the voice bundle?

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.0.0:compile (default-compile) on project org.eclipse.smarthome.core.voice: Compilation failure: Compilation failure:
[ERROR] /home/travis/build/eclipse/smarthome/bundles/core/org.eclipse.smarthome.core.voice/src/main/java/org/eclipse/smarthome/core/voice/internal/DialogProcessor.java:[190]
[ERROR] sink.process(audioStream);
[ERROR] ^^^^^^^^^^^^^^^^^^^^^^^^^
[ERROR] Unhandled exception type UnsupportedAudioStreamException
[ERROR] /home/travis/build/eclipse/smarthome/bundles/core/org.eclipse.smarthome.core.voice/src/main/java/org/eclipse/smarthome/core/voice/internal/VoiceManagerImpl.java:[169]
[ERROR] sink.process(audioStream);
[ERROR] ^^^^^^^^^^^^^^^^^^^^^^^^^
[ERROR] Unhandled exception type UnsupportedAudioStreamException
[ERROR] 2 problems (2 errors)

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

cweitkamp · 2017-06-29T09:51:04Z

@SJKA Done.

I also added potential examples for getSupportedFormats() and getSupportedStreams() to the unit test class AudioSinkFake.

sjsf · 2017-06-29T11:07:46Z

Thanks!

Now, as you already predicted above, Travis has got another wish:

public class SonosAudioSink implements AudioSink {
[ERROR] ^^^^^^^^^^^^^^
[ERROR] The type SonosAudioSink must implement the inherited abstract method AudioSink.getSupportedStreams()
[ERROR] 1 problem (1 error)

cweitkamp · 2017-06-30T08:59:48Z

@SJKA Done.

I applied those changes to binding.sonos, io. javasound and io.webaudio to prevent good boy Travis to fail again.

sjsf · 2017-06-30T09:21:25Z

Thanks!
(btw, this time I made the build fail - will be fixed in a few minutes and I will trigger it again)

kaikreuzer

Is it by intention that the newly introduced method getSupportedStreams is actually not used anywhere? I thought the idea of it was to allow the framework to check it upfront, instead of having exceptions being thrown at runtime...

kaikreuzer · 2017-06-30T09:11:42Z

...re.audio/src/main/java/org/eclipse/smarthome/core/audio/UnsupportedAudioStreamException.java

+ * Thrown when a requested {@link AudioStream} is not supported by an {@link AudioSource} or {@link AudioSink}
+ * implementation
+ * 
+ * @author Christoph Weitkamp - Added UnsupportedAudioStreamException


Put "Initial contribution and API" here.

kaikreuzer · 2017-06-30T09:13:49Z

...home.core.voice/src/main/java/org/eclipse/smarthome/core/voice/internal/DialogProcessor.java

@@ -184,12 +186,17 @@ protected void say(String text) {
                }
            }
            if (null == voice) {
-                throw new TTSException("Unable to find a suitable voice");
+                throw new TTSException("Unable to find a voice");


why did you change this? It seems unrelated to this PR and the message is not correct anymore as the system probably was able to find a voice, but not one of the right locale.

kaikreuzer · 2017-06-30T09:16:56Z

...ome.core.voice/src/main/java/org/eclipse/smarthome/core/voice/internal/VoiceManagerImpl.java

@@ -139,20 +141,24 @@ public void say(String text, String voiceId, String sinkId) {
            } else if (voiceId.contains(":")) {
                // it is a fully qualified unique id
                String[] segments = voiceId.split(":");
-                tts = ttsServices.get(segments[0]);
-                voice = getVoice(tts.getAvailableVoices(), segments[1]);
+                tts = getTTS(segments[0]);


Why did you change this? This potentially returns you the default tts instead of the one specified in segments[0], so it unnecessarily complicates things here.

I changed it for readability and to prevent NPE (in case of segment[0] contains a not suitable value). Have a look at https://github.com/eclipse/smarthome/pull/3764/files#diff-2bdbf6176fb6bd9b1af9a6cb230b6293R472. The getTTS(String) method returns exaclty the same like before (ttsServices.get(id)).

kaikreuzer · 2017-06-30T09:20:27Z

...binding.sonos/src/main/java/org/eclipse/smarthome/binding/sonos/internal/SonosAudioSink.java

+            IOUtils.closeQuietly(audioStream);
+            throw new UnsupportedAudioStreamException(
+                    "Sonos can only handle FixedLengthAudioStreams and URLAudioStreams.", audioStream.getClass());
+            // TODO: Instead of throwing an exception, we could ourselves try to wrap it into a


Is this todo still relevant? If I understand @maggu2810 right, it should now be rather the framework, which decides to do the conversion to a FixedLengthAudioStream, in case it sees that the sink does not support anything else.

kaikreuzer · 2017-06-30T09:21:17Z

....javasound/src/main/java/org/eclipse/smarthome/io/javasound/internal/JavaSoundAudioSink.java

@@ -71,6 +75,11 @@ public void process(AudioStream audioStream) throws UnsupportedAudioFormatExcept
    }

    @Override
+    public Set<Class<? extends AudioStream>> getSupportedStreams() {
+        return Collections.emptySet();


Why empty? It supports ALL streams!

cweitkamp · 2017-06-30T10:26:27Z

Is it by intention that the newly introduced method getSupportedStreams is actually not used anywhere? I thought the idea of it was to allow the framework to check it upfront, instead of having exceptions being thrown at runtime...

@kaikreuzer thanks for review. No, of course not. My intention was to change it in a follow-up PR. I can do that in this PR if you preferr.

kaikreuzer · 2017-06-30T11:37:38Z

Many thanks @cweitkamp! Ok, all fine for me to make use of this feature in an upcoming PR.

kaikreuzer · 2017-06-30T11:39:01Z

We need to remember that once the next ESH stable is pushed out, quite some bindings in openhab2-addons will break and need to be adapted.

lolodomo · 2017-07-01T09:14:35Z

The timing looks not very good. Such a breaking API will prevent any update of OH2 bindings relying on it for any user running OH 2.1 during around 5 months. We have to pray that no urgent patch is needed for all impacted bindings.
What will happen for users using the online distribution ?

kaikreuzer · 2017-07-01T09:44:57Z

We are talking about changes on unstable development branches, so there should be no need for praying or anything.

lolodomo · 2017-07-08T16:46:47Z

@cweitkamp: may we ask you to provide a fix for all broken OH2 bindings ?

cweitkamp · 2017-07-08T17:15:21Z

@lolodomo Yes of course. I am working on it.

cweitkamp · 2017-07-13T06:04:46Z

PR https://github.com/openhab/openhab2-addons/pull/2457

cweitkamp mentioned this pull request Jun 28, 2017

[audio] New exception type if audio sink doesn't support the served AudioStream. #3566

Closed

maggu2810 approved these changes Jun 28, 2017

View reviewed changes

cweitkamp added 3 commits June 29, 2017 11:25

Added getSupportedStreams() and UnsupportedAudioStreamException

7870952

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

Added example for getSupportedStreams() and getSupportedFormats()

322ff11

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

Applied AudioSink changes to Voice bundle

2ec1516

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

Applied changes to binding.sonos, io.javasound and io.webaudio

c2ba048

kaikreuzer suggested changes Jun 30, 2017

View reviewed changes

Addressed comments from review

85e7ab3

kaikreuzer approved these changes Jun 30, 2017

View reviewed changes

kaikreuzer merged commit 1bada87 into eclipse-archived:master Jun 30, 2017

kaikreuzer added the APIBreaking label Jun 30, 2017

cweitkamp deleted the feature-3566-audio-stream-exception branch June 30, 2017 12:03

cweitkamp mentioned this pull request Jul 13, 2017

[audio] Added support for new AudioSink API openhab/openhab-addons#2457

Merged

lolodomo mentioned this pull request Jul 24, 2017

Sonos notification broken in snapshot 988 #3877

Closed

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

cweitkamp mentioned this pull request Jan 22, 2018

[audio] Improvements in Audio and Voice API #4963

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[audio] Added getSupportedStreams() and UnsupportedAudioStreamException #3764

[audio] Added getSupportedStreams() and UnsupportedAudioStreamException #3764

cweitkamp commented Jun 28, 2017

cweitkamp commented Jun 28, 2017

sjsf commented Jun 29, 2017

cweitkamp commented Jun 29, 2017

sjsf commented Jun 29, 2017

cweitkamp commented Jun 30, 2017

sjsf commented Jun 30, 2017

kaikreuzer left a comment

kaikreuzer Jun 30, 2017

kaikreuzer Jun 30, 2017

kaikreuzer Jun 30, 2017

cweitkamp Jun 30, 2017

kaikreuzer Jun 30, 2017

kaikreuzer Jun 30, 2017

cweitkamp commented Jun 30, 2017

kaikreuzer commented Jun 30, 2017

kaikreuzer commented Jun 30, 2017

lolodomo commented Jul 1, 2017

kaikreuzer commented Jul 1, 2017

lolodomo commented Jul 8, 2017

cweitkamp commented Jul 8, 2017

cweitkamp commented Jul 13, 2017

[audio] Added getSupportedStreams() and UnsupportedAudioStreamException #3764

[audio] Added getSupportedStreams() and UnsupportedAudioStreamException #3764

Conversation

cweitkamp commented Jun 28, 2017

cweitkamp commented Jun 28, 2017

sjsf commented Jun 29, 2017

cweitkamp commented Jun 29, 2017

sjsf commented Jun 29, 2017

cweitkamp commented Jun 30, 2017

sjsf commented Jun 30, 2017

kaikreuzer left a comment

Choose a reason for hiding this comment

kaikreuzer Jun 30, 2017

Choose a reason for hiding this comment

kaikreuzer Jun 30, 2017

Choose a reason for hiding this comment

kaikreuzer Jun 30, 2017

Choose a reason for hiding this comment

cweitkamp Jun 30, 2017

Choose a reason for hiding this comment

kaikreuzer Jun 30, 2017

Choose a reason for hiding this comment

kaikreuzer Jun 30, 2017

Choose a reason for hiding this comment

cweitkamp commented Jun 30, 2017

kaikreuzer commented Jun 30, 2017

kaikreuzer commented Jun 30, 2017

lolodomo commented Jul 1, 2017

kaikreuzer commented Jul 1, 2017

lolodomo commented Jul 8, 2017

cweitkamp commented Jul 8, 2017

cweitkamp commented Jul 13, 2017