Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Chromecast] fix audio sink (when requesting multiple times) #1799

Merged
merged 1 commit into from
Jan 30, 2017
Merged

[Chromecast] fix audio sink (when requesting multiple times) #1799

merged 1 commit into from
Jan 30, 2017

Conversation

lolodomo
Copy link
Contributor

Signed-off-by: Laurent Garnier lg.hc@free.fr

@mention-bot
Copy link

@lolodomo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kaikreuzer, @tavalin and @BClark09 to be potential reviewers.

@tavalin
Copy link

tavalin commented Jan 29, 2017

What happens when the audio stream is longer than 30 seconds?

@lolodomo
Copy link
Contributor Author

Note that it concerns only audio files provided by openHAB and so notifications. The playback will very probably stop after the 30 seconds. This can be checked by downloading a big MP3 in conf/sounds and then play it.

@lolodomo
Copy link
Contributor Author

Tested and oh surprise, the playback is finally not stopped :)
To be honest, I think it is due a bug in the audio servlet that is cleaning streams only when handling a new request. If no request arrives, the audio file is still delivered even after the 30 seconds ;)

@tavalin
Copy link

tavalin commented Jan 29, 2017 •
edited
Loading

OK, if that is how it is supposed to work (30 seconds to receive any new requests) then in theory I am OK with the change.

@lolodomo
Copy link
Contributor Author

I updated my code (same code as the other PR for the Onkyo binding).

@tavalin : note that for a radio stream, there will be no change (no duration limit), it will not be handled by the audio servlet, just a direct link.
PS: I have not yet tested direct streams...

@tavalin
Copy link

tavalin commented Jan 29, 2017

Yes I see that bit now.

LGTM then.

@lolodomo
Copy link
Contributor Author

@tavalin : I tested the stream console command using different WEB radios as URL and it is working well. My PR is not impacting this part.

@lolodomo lolodomo changed the title Chromecast binding: fix #1784 Chromecast binding: fix audio sink (fix #1784) Jan 29, 2017
@kaikreuzer
Copy link
Member

The playback will very probably stop after the 30 seconds.

It should not. Existing streams should not be killed by the servlet. If this is what this issue is about, we should indeed rather look at fixing the AudioServlet.
Did you do any detailed analysis on the exact behavior?

@lolodomo
Copy link
Contributor Author

It is not killed, confirmed by a test with a big MP3. In this case, the duration has apparently no impact which is finally a very good thing. I am just not sure that is what you expect ?
But the reason of the change is that without the change Android TV is not able to play the audio file.

@kaikreuzer
Copy link
Member

which is finally a very good thing. I am just not sure that is what you expect ?

Yes, this is what I expect - you can access the stream and you fully receive it.

Android TV is not able to play the audio file.

So we would have to figure out why. Could you enable debug logging for the audio servlet:

log:set DEBUG org.eclipse.smarthome.core.audio

We should then see what kind of requests the Android TV is doing.

@lolodomo
Copy link
Contributor Author

I will do and let you know the result.

@lolodomo
Copy link
Contributor Author

@kaikreuzer @tavalin : reading again the code from the audio servlet, I understood that the provided duration is only used to define a delay in which the file can be requested (with no restriction) to the servlet, and not a delay after which the servlet will break an already handled request. So it looks good and I understand now why I can cast a MP3 with a long duration without any interruption.

@lolodomo
Copy link
Contributor Author

That now understood, reducing the delay from 20s to 10s should probably be enough.

@lolodomo
Copy link
Contributor Author

I can now confirm that Android TV is requesting the audio file twice:

19:07:35.505 [DEBUG] [ome.core.audio.internal.AudioServlet] - Removed timed out stream 92640c90-dfe8-45e8-a8c5-1824c75d1b8f
19:07:35.509 [DEBUG] [ome.core.audio.internal.AudioServlet] - Stream to serve is 2e740657-b8e5-4ca8-9629-6b0c758d6105
19:07:35.576 [DEBUG] [ome.core.audio.internal.AudioServlet] - Stream to serve is 2e740657-b8e5-4ca8-9629-6b0c758d6105

@lolodomo
Copy link
Contributor Author

Trying a TTS command, I even noticed 3 requests:


19:12:09.414 [DEBUG] [ome.core.audio.internal.AudioServlet] - Removed timed out stream 2e740657-b8e5-4ca8-9629-6b0c758d6105
19:12:09.420 [DEBUG] [ome.core.audio.internal.AudioServlet] - Stream to serve is 366b8ffb-9a6d-4730-83a6-ded1e4caaa85
19:12:09.456 [DEBUG] [ome.core.audio.internal.AudioServlet] - Stream to serve is 366b8ffb-9a6d-4730-83a6-ded1e4caaa85
19:12:12.298 [DEBUG] [ome.core.audio.internal.AudioServlet] - Stream to serve is 366b8ffb-9a6d-4730-83a6-ded1e4caaa85

@lolodomo
Copy link
Contributor Author

lolodomo commented Jan 30, 2017 •
edited
Loading

After several tests, it seems to be generally 3 requests to the audio file. Even with my long duration MP3 audio file (several minutes), I only got 3 requests at the beginning of the playback

All that seems to confirm that my PR is totally appropriate ;)

@kaikreuzer
Copy link
Member

Thanks, that is proof enough :-)
But didn't you want to reduce the time from 20 to 10 now?

@lolodomo
Chromecast binding: fix audio sink (fix #1784)
747ac36 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

Done.

@kaikreuzer kaikreuzer merged commit 03776b0 into openhab:master Jan 30, 2017
@lolodomo lolodomo deleted the chromecast branch January 30, 2017 21:47
ibaton pushed a commit to ibaton/openhab2-addons that referenced this pull request Jan 31, 2017
@lolodomo @ibaton
Chromecast binding: fix audio sink (fix openhab#1784) (openhab#1799)
96d9949 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Markinus pushed a commit to Markinus/openhab2-addons that referenced this pull request Feb 8, 2017
@lolodomo @Markinus
Chromecast binding: fix audio sink (fix openhab#1784) (openhab#1799)
ac33715 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
fharni pushed a commit to fharni/openhab2-addons that referenced this pull request Feb 10, 2017
@lolodomo @fharni
Chromecast binding: fix audio sink (fix openhab#1784) (openhab#1799)
ded01e9 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
jarlebh pushed a commit to jarlebh/openhab2-addons that referenced this pull request Mar 4, 2017
@lolodomo @jarlebh
Chromecast binding: fix audio sink (fix openhab#1784) (openhab#1799)
28c65c6 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
tratho pushed a commit to tratho/openhab2-addons that referenced this pull request May 5, 2017
@lolodomo @tratho
Chromecast binding: fix audio sink (fix openhab#1784) (openhab#1799)
fad8085 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@martinvw martinvw changed the title Chromecast binding: fix audio sink (fix #1784) Chromecast binding: fix audio sink (when requesting multiple times) Jun 25, 2017
@martinvw martinvw added the enhancement An enhancement or new feature for an existing add-on label Jun 25, 2017
@martinvw martinvw added this to the 2.1 milestone Jun 25, 2017
@martinvw martinvw changed the title Chromecast binding: fix audio sink (when requesting multiple times) [Chromecast] fix audio sink (when requesting multiple times) Jun 25, 2017
@martinvw martinvw added bug An unexpected problem or unintended behavior of an add-on and removed enhancement An enhancement or new feature for an existing add-on labels Jun 25, 2017
Flole998 pushed a commit to Flole998/openhab-addons that referenced this pull request Dec 30, 2021
@ghys
[automation] Create Nashorn script engines with the proper class load…
4e04520 
…er (openhab#1799)

This should fix the issue reported here:
https://community.openhab.org/t/openhab-3-0-milestone-2-discussion/107564/8

where the Nashorn script engine would be created with the
current thread's class loader, causing JS code like this:
```
var Log = Java.type("org.openhab.core.model.script.actions.Log");
Log.logError("Experiments", "This is an OH error log");
Log.logWarn("Experiments", "This is an OH warn log");
Log.logInfo("Experiments", "This is an OH info log");
Log.logDebug("Experiments", "This is an OH debug log");
```
to run fine when the rule was triggered but fail to find the Log
class when run from the REST API's `/rest/rules/{ruleUID}/runnow`,
because in that case the generic createScriptEngine implementation
would return script engines using the JAX-RS class loader as the
"app" class loader.

Note:
We also have an opportunity to restrict which classes are exposed
to the script with a ClassFilter to a specific set:
https://docs.oracle.com/javase/8/docs/jdk/api/nashorn/jdk/nashorn/api/scripting/NashornScriptEngineFactory.html#getScriptEngine-java.lang.String:A-java.lang.ClassLoader-jdk.nashorn.api.scripting.ClassFilter-
This could prove useful to mitigate code execution vulnerabilities,
as the script code is modifiable remotely.

Signed-off-by: Yannick Schaus <github@schaus.net>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Aug 12, 2023
@dilyanpalauzov
building-pages: typography (openhab#1799)
bf286ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.
5 participants
@lolodomo @mention-bot @tavalin @kaikreuzer @martinvw