Updating audio sinks - Githubissues

lolodomo commented 1 year ago

Core PR openhab/openhab-core#3461 improved audio feature and allows a simplification for audio sinks and support for more audio streams.

[x] allplay => PR #15201
[x] amplipi => PR #15199
[x] chromecast => PR #15180
[x] doorbird => PR #15189
[x] freebox => PR #15121
[x] freeboxos: => PR #15207
[x] heos => PR #15196
[x] kodi => PR #15192
[x] onkyo => PR #15117
[x] pulseaudio => PR #15179
[x] sonos => PR #15116
[x] squeezebox => PR #15194
[x] upnpcontrol => PR #15122

There is a pending discussion with @dalgwen if URLAudioStream should be proxied or not by the openHAB server. I am not very favorable to that option.

lolodomo commented 1 year ago

@dalgwen : you said in another issue that you will provide a PR for Chromecast, doorbell and pulseaudio. So I do not update these ones. It will be interesting to see if your proposed "pattern" is identical to mine. You are welcome to comment the PRs I have already submitted.

dalgwen commented 1 year ago

@lolodomo I think you will want to kill me, because I have another pending pull request for an audio enhancement in core. When I made the mimic TTS simplification relatives to core-3461, I found that I couldn't make all the simplification I wanted (I wanted to get rid of FixedLengthInputStream but couldn't) because the length() information was important further in the sink process method I tried (pulseaudio). And in mimic I don't want to use FixedLength, because the stream is not cloneable, as it is not a file (which is a huge part of the audio simplification goal...) So as we discussed in one previous comment some time ago, there is a real need to split the length information from the FixedLength class... new PR... The PR is simple, but time is lacking before the 4.0 release...

lolodomo commented 1 year ago

I just reviewed your new PR.

And in mimic I don't want to use FixedLength, because the stream is not cloneable, as it is not a file (which is a huge part of the audio simplification goal...)

In my VoiceRSS PR, when TTS cache is disabled, I simply provide an AudioStream and it seemed to work well with the Sonos sink. So, considering your new PR, I will have to also implement SizeableAudioStream as the service knows the stream length.

dalgwen commented 1 year ago

In my VoiceRSS PR, when TTS cache is disabled, I simply provide an AudioStream and it seemed to work well with the Sonos sink. So, considering your new PR, I will have to also implement SizeableAudioStream as the service knows the stream length.

Not for all sink, but the size information could be valuable for some (the pulseaudio binding tries to get duration information from it). It is not mandatory, but length detection could fail and default to the stream consumption time. Not a big deal, it is still better than before. I also know that, if we have the information, we set the length in the HTTP response header of the http server (I don't know if any binding using the http server really needs it).

lolodomo commented 1 year ago

@dalgwen : your last core PR has just been merged. So we can start again updates of audio sinks and TTS services.

dalgwen commented 1 year ago

Great, it was fast !

I'm taking doorbird and pulseaudio in top priority, as I am the sink's creator for both of them. Maybe today or tomorrow. I have something nearly ready on chromecast too.

If I have time, I could take squeezebox and kodi, as I use them at home (but not for their sink capability)

dalgwen commented 1 year ago

pulseaudio chromecast

lolodomo commented 1 year ago

I'm taking doorbird and pulseaudio in top priority, as I am the sink's creator for both of them. Maybe today or tomorrow. I have something nearly ready on chromecast too.

If I have time, I could take squeezebox and kodi, as I use them at home (but not for their sink capability)

In this case, I will try to provide a PR for the 3 last ones, allplay, amplipi and heos.

lolodomo commented 1 year ago

@kaikreuzer : for amplipi, can you please tell me if playPA is synchronous or asynchronous ? It leads to a HTTP POST request. Does this request returns immediately or only when the announcement is finished ?

dalgwen commented 1 year ago

doorbird

dalgwen commented 1 year ago

kodi

kaikreuzer commented 1 year ago

@kaikreuzer : for amplipi, can you please tell me if playPA is synchronous or asynchronous ? It leads to a HTTP POST request. Does this request returns immediately or only when the announcement is finished ?

@lolodomo Just checked it: It is a synchronous call, i.e. it returns after finishing the announcement.

lolodomo commented 1 year ago

@kaikreuzer : for amplipi, can you please tell me if playPA is synchronous or asynchronous ? It leads to a HTTP POST request. Does this request returns immediately or only when the announcement is finished ?

@lolodomo Just checked it: It is a synchronous call, i.e. it returns after finishing the announcement.

PR is submitted.

lolodomo commented 1 year ago

@dalgwen : testing with the Freebox audio sink in asynchronous mode, I am a little surprised, I noticed a long delay to stop the player at the end of the notification, even with a timeout set to 5s and with a long notification (25s). I will have to add logs in the core audio but I am asking myself if your computation of the audio duration is good or not. Edit: this is with a PCM/WAVE file.

dalgwen commented 1 year ago

I noticed a long delay

Do you have an appproximate number of seconds?

It is clearly far from perfect. If the AudioSink has its own method to guess when the sound ended (as in the Kodi sink monitoring change in the remote player), it is certainly more accurate and should be used.

I will explain it here if you want to debug it.

First, caveats :

for PCM_SIGNED (WAV) : it requires the java sound framework to correctly parse the first bytes of the stream. Or the openhab AudioFormat bitrate property correctly provided.
for MP3 : support is partial (no VBR).
No support for other format (AAC, OGG, PCM other than signed). It could be extended but I am no expert in audio, far from it.

Second, the AudioServlet will work like this :

As you already know, the timeout provided by the call to the sink is the lower limit (the complete method will not be called before, even if the sound is shorter)
Every doGet() call (i.e. every time the device calls the openhab servlet) will update the timeout to now + duration guessed (or let the previous timeout if is already further in time). It means that if the device go on making request to the servlet again and again, the timeout will be pushed further and further in the timeline. (why ?: if there is several devices requesting the same AudioStream, we have to wait for every one of them to finish before completing the Future). It can be an issue : for example the kodi software keeps making this kind of request to the audio servlet, even a long time after the sound starts playing (fortunately kodi is a synchronous sink and we don't use this kind of sound duration guess)
How ?: The doGet() will use the "transferAndAnalyzeLength" utility method that can returns a timestamp if it can computes the sound duration (with the caveats cited before). A Null response otherwise, means that the original timeout will be used.
A periodic call to the cleanup method (removeTimedOutStreams) will call the complete method for streams with timeout in the past (and if and only if they are not still transfering something in a doGet() call). This method is called every 5 seconds. It means that the complete method could be, with a bad timing, called as far as 5 seconds after the timeout occured. We could make this periodic clean more often, but I didn't want to put a load on openHAB. WDYT ?

If I can make a defense of this system, it's that, even with its flaw, it is still better than before (because before, the sound volume restoration occured with no delay at all) 😅

lolodomo commented 1 year ago

@kaikreuzer : as you reviewed the change for the AmpliPi binding and while the changes are very similar in all other audio sinks, could you please review & merge all the other opened PRs listed in this issue (except the kodi PR I am handling myself with @dalgwen) ?

lolodomo commented 1 year ago

Do you have an appproximate number of seconds?

With a long notification, it is around 7/8 seconds.

It is clearly far from perfect. If the AudioSink has its own method to guess when the sound ended (as in the Kodi sink monitoring change in the remote player), it is certainly more accurate and should be used.

Yes, with synchronous audio sinks like Sonos, of course, I do not encounter this problem.

lolodomo commented 1 year ago

for PCM_SIGNED (WAV) : it requires the java sound framework to correctly parse the first bytes of the stream. Or the openhab AudioFormat bitrate property correctly provided.

Here is how is the audio format when received by the sink:

11:05:41.364 [DEBUG] [ebox.internal.FreeboxAirPlayAudioSink] - audioStream AudioStreamFromCache AudioFormat [codec=PCM_SIGNED, container=WAVE, bigEndian=false, bitDepth=16, bitRate=256000, frequency=16000, channels=1]

lolodomo commented 1 year ago

As you already know, the timeout provided by the call to the sink is the lower limit (the complete method will not be called before, even if the sound is shorter)

Yes and that is the reason I tried with a very long notification, expecting the player to stop shortly after the end.

A periodic call to the cleanup method (removeTimedOutStreams) will call the complete method for streams with timeout in the past (and if and only if they are not still transfering something in a doGet() call). This method is called every 5 seconds. It means that the complete method could be, with a bad timing, called as far as 5 seconds after the timeout occured. We could make this periodic clean more often, but I didn't want to put a load on openHAB. WDYT ?

Yes, I believe we could probably reduce it a little.

I will trace to see if with Freebox doGet() is called several times and I will see what duration is computed.

lolodomo commented 1 year ago

A periodic call to the cleanup method (removeTimedOutStreams) will call the complete method for streams with timeout in the past (and if and only if they are not still transfering something in a doGet() call). This method is called every 5 seconds. It means that the complete method could be, with a bad timing, called as far as 5 seconds after the timeout occured. We could make this periodic clean more often, but I didn't want to put a load on openHAB. WDYT ?

Yes, I believe we could probably reduce it a little.

It is only run when an audio stream is currently served by the server, not all the time. So I think we can be more agressive. 2s is probably better.

lolodomo commented 1 year ago

@dalgwen : good news. With the freebox audio sink, I can see that doGet is called only once and the computed new end timestamp looks good (duration of 29,7s). I think the "problem" is only the remove job which is run only every 5 seconds and iI have no chance, it occurs at the wrong time. It is also possible that the Freebox player takes a time to stop the player; So I would only suggest to schedule the job every 2 seconds. I am going to submit a PR.

lolodomo commented 1 year ago

@clinique : I don't know what Freebox player do you own but in case you have one that is not a Freebox Revolution and that supports AirPlay, you are welcome to test the audio sink feature with the freeboxos binding.

lolodomo commented 1 year ago

@jlaur : in case you have some time, can you please help merging all the remaining opened PRs listed in this issue ?

jlaur commented 1 year ago

@jlaur : in case you have some time, can you please help merging all the remaining opened PRs listed in this issue ?

I'm on vacation until Sunday, so can't promise anything. I'm mostly on the phone, and it won't be sufficient for this. I might fire up my backup laptop one evening, but can't promise.

kaikreuzer commented 1 year ago

I don't want that @jlaur spoils his vacation too much - I'll take care of the remaining PRs!

kaikreuzer commented 1 year ago

All merged!

lolodomo commented 1 year ago

Thank you @jlaur and @kaikreuzer

openhab-bot commented 1 year ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/webaudio-not-working-when-trying-to-stream-webradio/148610/14

openhab / openhab-addons

Updating audio sinks #15113