plugins.mbcmini: New Plugin for http://mini.imbc.com #3224
Conversation
|
I presume the reason you have added the Edit: Actually two plugins if you count schoolism, although that seems to work partly on URL as well as a lesson part parameter. |
|
Hi, thanks for the review. I modified it to work without parameters as you said. |
|
I'm sure they'll be happier with that. 👍 |
There was a problem hiding this comment.
Thank you for the pull request. Is this a radio station only without any live video content?
Before writing a plugin and submitting a PR, it's usually best to open a plugin request or ask first whether it'll be accepted, because otherwise, you may waste the time you've spent on it if it gets rejected. I'm just mentioning it, because I don't know what the current policy is for audio-only streams (even though it's currently not stated in the contribution guidelines). I personally don't have a problem with this, but let's hear from the other maintainers as well.
src/streamlink/plugins/mbcmini.py
Outdated
| json_data = json.loads(json_text) | ||
|
|
||
| if json_url_key in json_data: | ||
| return self.session.streams(json_data[json_url_key]) |
There was a problem hiding this comment.
You are recursively calling the plugin's _get_streams method by calling self.session.streams (1, 2).
In Plugin._get_streams, you are supposed to return an iterator or a list of objects which implement the Stream class, eg. HLSStream, DashStream, HTTPStream, RTMPStream, etc., depending on what kind of streams the website you're writing the plugin for provides.
There was a problem hiding this comment.
Hi, thanks for the review. I modified it to use HTTPStream.
src/streamlink/plugins/mbcmini.py
Outdated
| json_text = '{' + ''.join(res.text.split()[1:-1]) | ||
| json_data = json.loads(json_text) |
There was a problem hiding this comment.
Plugins should use Streamlink's validator API to return results from network requests, if possible. The API response appears to be a JSONP call, which means you should extract the JSON payload via a regex instead of splitting the response text by newline characters.
There was a problem hiding this comment.
Modified to use Streamlink's validation API.
|
This radio station only transmits audio content. |
There was a problem hiding this comment.
I've added a commit to the PR branch to fix up the plugin.
Whether it's good for merging depends on whether radio stations (audio only streams) are acceptable.
$ streamlink -l debug 'https://miniplay.imbc.com/WebLiveURL.ashx?channel=sfm'
[cli][debug] OS: Linux-5.9.0-rc8-1-git-x86_64-with-glibc2.2.5
[cli][debug] Python: 3.8.5
[cli][debug] Streamlink: 1.6.0+23.g49f009f
[cli][debug] Requests(2.24.0), Socks(1.7.1), Websocket(0.56.0)
[cli][info] Found matching plugin mbcmini for URL https://miniplay.imbc.com/WebLiveURL.ashx?channel=sfm
[utils.l10n][debug] Language code: en_US
Available streams: 81k (worst, best)
|
@bastimeyer My concern is that allowing a radio plugin opens up a pretty big can of worms. There are a million online radio stations, are we going to accept all those because "well you accepted mbcmini". One of the big reasons for putting a site through Streamlink is to make it less resource intensive (Twitch is always my go to example because the site is so awful) or terrible to use in the event of a mediocre video player. Radio stations don't suffer from that issue, it's just streaming audio and presumably isn't that resource intensive though I can't confirm because the site is geo-blocked. I'm leaning towards no on this, Streamlink is designed specifically for video content, and live content at that. I don't think a radio station really counts any more than a playlist of audio files in my local audio player does. I'm open to discussing it but these are my thoughts. It's not a video, it's not especially resource intensive, so what need does Streamlink serve in this case? |
|
I understand. Then I'll close this PR. |
mbcmini is South Korea radio station.