Remove use of curl if not needed

[krisgesling]

Description

This removes the use of Curl to download streams locally within the Skill. Instead the url is passed to the audioservice which is responsible for playing that stream in the appropriate manner.

Fixes #99

This came about when investigating an issue with the FOX News stream. The query params on the url are causing an issue with curl, whilst the audioservice handles playing the url without issue. It is also reducing the complexity of the Skill itself in favor of the audioservice handling playback.

We may need to add https as a supported protocol to the Simple Audioservice and/or set VLC as the default. However I'm yet to hit any issues with this change alone.

Type of PR

• [x] Bugfix

Testing

This PR should be manually tested as VK does not actually verify that a stream is playing, only that it intends to play it. Would also be good to verify that it is working as expected across the range of systems that people use to run Mycroft.

CLA

• [x] Yes

[forslund]

will this work with the simple audioservice backend? it uses mpg123 which doesn't support https...

[forslund]

Maybe use self.audioservice.available_backends() to get the info if there is a backend supporting https and if not issue a self.log.warning and fall back to using curl if no backend supports it? Or speak a message that the url can't be played if the protocol isn't supported?

[ken-mycroft]

Just checked the existing news skill. It is a common play skill but it spins up its own curl process to play? Anybody know why that is or have any objections to changing this? I think the best idea is to refactor this stuff a bit so the actual curl fall back code lives in the audio service.

[JarbasAl]

this came up when i tried to add support for catalan news, my suggestions remain the same:

• move this curl stuff into the simple audio backend so it supports https natively without this hacky stuff
• make the default backend vlc

issue for curl #99 catalan PR #102

[ken-mycroft]

Agreed. Note Fox news is also failing because the URL returned is not properly decoded by mpg123 but works fine if vlc is used. Of course the audio service using vlc now opens it up to the issues surrounding qml and screen handling in general. A service that plays audio should not need to be aware of whether it has a screen or not.

[forslund]

:+1: As long as there is a flag to switch of the curl-shenanigans in the simple audioservice if the standard play_mp3 has been replaced by something better.

[krisgesling]

Alright I've reworked this a fair bit. This leaves the existing curl behaviour if the url is https and the default audioservice doesn't support that URI. So if we add support for https into the simple audioservice then it will automatically start using that instead.

Confirming that everything was working has been fairly painful with these tests as we weren't actually confirming the state of the system at each Step. Eg "given nothing is playing" would emit a stop command but not guarantee that "nothing was playing". Hence tests could progress beyond a Step that should have failed, or at least wasn't yet succeeding.

Because of the limitations in our current playback services there doesn't seem to be any good event driven way of determining that playback has actually started, only that the service intends to play something. So rather than leave 5 second sleeps throughout the steps, I've added some loops to check until it succeeds or timeout and fail. These are definitely candidates for improvement when we redo the playback services.

[krisgesling]

I re-read Ake's comment and realised that there's already a mechanism to fallback to another service that supports https, as long as one of them does.

So updating for that now and it's raised an issue that the backends don't have consistent track_info. By default artist will be an empty String but VLC backend only returns an artists list. It seems to be only VLC that has this - at least for audio backends in core itself. So I'm accounting for that but we'll revisit the track_info minimum data in the upcoming music sprint.