[voicerss] Add support for WAV audio format #11916
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,7 +106,7 @@ private void generateCacheForMessage(String apiKey, String cacheDir, String loca | |
| return; | ||
| } | ||
| CachedVoiceRSSCloudImpl impl = new CachedVoiceRSSCloudImpl(cacheDir); | ||
| File cachedFile = impl.getTextToSpeechAsFile(apiKey, trimmedMsg, locale, voice, "MP3", null); | ||
|
There was a problem hiding this comment. I am a little lost. Why is MP3 hardcoded here ? More generally, using VoiceRSS with your changes, what will be the default codec used ? Will it be MP3 if the sink supports MP3 ? There was a problem hiding this comment.
"MP3" was already hardcoded before and not parameterized in the commit to "add support for OGG and AAC audio formats" because it's probably unused. So I also didn't touch it. This file is meant to be called from the CLI, see Caching in README.md. Perhaps it should be deleted as calling the
No risk of a NPE:
Hm. Good question. I think so, if the sink does not support WAV. That's decided in So I think that's not really an implementation concern of the VoiceRSS binding but the openhab-core classes. There was a problem hiding this comment. I see that it was previously hardcoded with MP3 but I did not remember why. As I was probably the one who added OGG/AAC, if I left MP3 hardcoded here, there is certainly a good reason, I hope :) But I will check that again, considering your comments about the CLI command to build the cache. I would like to be sure that these changes will not lead to users "loosing" their cache in practice because the binding is now using another codec and other file names in the cache. Let me the time to analyse more in details the code and to test your changes. There was a problem hiding this comment. I made sure not to change the filename for previous uses of the default 44khz_16bit_mono format: Thanks for your review so far. There was a problem hiding this comment. A lot of audio sinks support MP3 and WAV: javasound, webaudio, chromecast, kodi, squeezebox, ...
But it would be somewhat unfitting to have special "prefer MP3" code only in this binding. Better to implement something in openhab-core. There's code duplication between There was a problem hiding this comment. Maybe we should just add an advanced parameter to let the user force an audio format. There was a problem hiding this comment. A config option to opt-in to WAV support could be implemented. The return value of The code that decides what audio format to use is in openhab-core. I can't say if a sink can prefer a format by returning it first in the of list of supported formats. Oh, I see that it's a Another possibility is to mention how to batch convert the cache in the release notes: There was a problem hiding this comment.
Listing the preferred audio formats to use as a config option in openhab-core would be a good idea. But would not solve our problem here, because -core preferred wav and should continue to do so when such an option is introduced, I guess. So if you do not think a remark in the release notes is enough we should start to implement an option for the voicerss binding and maybe later on do something about it in -core. Perhaps even changing There was a problem hiding this comment. What I was thinking of is clearly an option in voicerss. My concern is users who created cached files for voicerss using the provided tool. Currently all these files are MP3 files. My idea was a binding parameter that would limit the audio formats handled by the binding to a particular audio format, MP3 as an example. But now I think the best option is just to enhance the tool to let the user select the output audio format for cached files to be created. Like that, to switch to WAV, files, the user will have only to rerurn his command but asking WAV format. Can you enhance the tool with one additional optional command line argument that will be the audio codec to be used? The call will then use this argument rather than hardcoded string "MP3". |
||
| System.out.println( | ||
| "Created cached audio for locale='" + locale + "', msg='" + trimmedMsg + "' to file=" + cachedFile); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /** | ||
| * Copyright (c) 2010-2022 Contributors to the openHAB project | ||
| * | ||
| * See the NOTICE file(s) distributed with this work for additional | ||
| * information. | ||
| * | ||
| * This program and the accompanying materials are made available under the | ||
| * terms of the Eclipse Public License 2.0 which is available at | ||
| * http://www.eclipse.org/legal/epl-2.0 | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| */ | ||
| package org.openhab.voice.voicerss.internal; | ||
|
|
||
| import org.hamcrest.Description; | ||
| import org.hamcrest.Matcher; | ||
| import org.hamcrest.TypeSafeMatcher; | ||
| import org.openhab.core.audio.AudioFormat; | ||
|
|
||
| /** | ||
| * Hamcrest {@link Matcher} to assert a compatible {@link AudioFormat}. | ||
| * | ||
| * @author Andreas Brenk - Initial contribution | ||
| */ | ||
| public class CompatibleAudioFormatMatcher extends TypeSafeMatcher<AudioFormat> { | ||
|
|
||
| private final AudioFormat audioFormat; | ||
|
|
||
| public CompatibleAudioFormatMatcher(AudioFormat audioFormat) { | ||
| this.audioFormat = audioFormat; | ||
| } | ||
|
|
||
| @Override | ||
| protected boolean matchesSafely(AudioFormat actual) { | ||
| return audioFormat.isCompatible(actual); | ||
| } | ||
|
|
||
| @Override | ||
| public void describeTo(Description description) { | ||
| description.appendText("an audio format compatible to ").appendValue(audioFormat); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a matcher that matches when the examined object is | ||
| * compatible to the specified <code>audioFormat</code>. | ||
| * | ||
| * @param audioFormat the audio format which must be compatible | ||
| */ | ||
| public static Matcher<AudioFormat> compatibleAudioFormat(AudioFormat audioFormat) { | ||
| return new CompatibleAudioFormatMatcher(audioFormat); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check isn't needed here any longer because the methods getApiAudioCodec() and getApiAudioFormat() called later on throw a TTSException if a format isn't supported.