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.

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

improve streamservice #170

Merged
merged 5 commits into from May 1, 2019
Merged

Conversation

mediaminister
Copy link
Collaborator

@mediaminister mediaminister commented May 1, 2019

vualto_een, vualto_canvas, vualto_ketnet and vualto_sporza are deprecated video_id's since VRT geoblocks live streams. This was the main cause the previous commit was failing on hls streams.

Other changes:

  • removed direct mpeg_dash option, for two reasons:
    • every VRT stream is available through vualto video aggregator api
    • in the past direct stream urls changed several times a year at unpredictable times
  • separated web scraping function, so it's easier and more failsafe to fix
  • added an error message to Kodi log when web scraping fails
  • centralized api_data manipulations in _get_api_data function

@dagwieers
Copy link
Collaborator

Thanks, looks good and makes it easier to test the web-scraping code in unit tests.
Which is something we will need to test in an automated fashion soon.

The only thing I do not like is the long try-except block, this is unneeded IMO, if the web-scraping fails, it fails early on. The other code should not be throwing an exception (and if it does, we need to know and handle it accordingly).

@mediaminister
Copy link
Collaborator Author

I placed all html data-* attributes in the try-except block, because it's data we scrape from an html page.
We really don't know where on the html page it will fail first. VRT can change attributes while preserving html tags and vice versa.

@dagwieers
Copy link
Collaborator

Well, the data-attributes won't fail (i.e. not return an exception), they will simply return None.
I am proposing a change.

@dagwieers
Copy link
Collaborator

Also, I would like to enable the Sporza stream by default, even if it currently only shows black.
(In fact I wanted to test this during the Cup Final this afternoon, but couldn't make it work, now we know why).

@mediaminister
Copy link
Collaborator Author

Gives an error:

self._kodi_wrapper.log_error('Web scraping api data failed: %s', e)
TypeError: log_error() takes exactly 2 arguments (3 given)

@dagwieers
Copy link
Collaborator

I just found out myself! Great. Everything else looks fine, until it breaks differently ;-)
Let's consider this as good as it gets for now.

PS What is your opinion to Radio support ? I'd like to know whether we can keep it as-is, or start a second project for this.

@dagwieers dagwieers merged commit bf3c63c into add-ons:master May 1, 2019
@mediaminister mediaminister deleted the improvestreamservice branch May 2, 2019 07:26
dagwieers pushed a commit to dagwieers/plugin.video.vrt.nu that referenced this pull request May 2, 2019
* improve streamservice

* Add different fail scenarios

* Fix typos caused by online editing :-(

* fixed log error

* Add Sporza live stream
@dagwieers dagwieers added the bug Something isn't working label May 3, 2019
@dagwieers dagwieers added this to the 1.9.0 milestone May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants