Skip to content

Get regular playlist url properly in ha-hls-player#7417

Merged
bramkragten merged 6 commits intohome-assistant:devfrom
uvjustin:use-relative-url-for-hls-playlist
Nov 10, 2020
Merged

Get regular playlist url properly in ha-hls-player#7417
bramkragten merged 6 commits intohome-assistant:devfrom
uvjustin:use-relative-url-for-hls-playlist

Conversation

@uvjustin
Copy link
Contributor

@uvjustin uvjustin commented Oct 21, 2020

Proposed change

The existing way of getting the regular playlist from the master playlist used a simple string replace. This couples the frontend to the naming convention of the playlists in the backend. This PR gets the regular playlist url properly by parsing the url from the master playlist.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@uvjustin
Copy link
Contributor Author

uvjustin commented Oct 21, 2020

@hunterjm How about just doing this? It will skip one extra load if possible, and it will fall back to the master playlist if it doesn't match. The only potential concern is it would only grab the first stream of an adaptive master playlist (or a playlist with separated tracks), but I don't think we will be using those. Actually, if that is a concern we could easily test for those too as those would have more matches to the regexp.
Edit: Added a commit that checks for only one match. There's no need to grab the regular playlist from the backend, so we don't need to make any changes there. Just to summarize, this whole "regular playlist" thing just ends up just being a frontend optimization that avoids loading the master playlist a second time so long as there is only one variant, otherwise the master playlist is sent.

@uvjustin uvjustin marked this pull request as ready for review October 22, 2020 14:51
@uvjustin uvjustin marked this pull request as draft October 22, 2020 23:51
@uvjustin uvjustin marked this pull request as ready for review October 23, 2020 01:51
Comment on lines +132 to +135
const playlistRegexp = /#EXT-X-STREAM-INF:.*?(?:CODECS=".*?(hev1|hvc1)?\..*?".*?)?(?:\n|\r\n)(.+)/g;
const match = playlistRegexp.exec(masterPlaylist);
if (match !== null && playlistRegexp.exec(masterPlaylist) === null) {
playlist_url = new URL(match[2], this.url).href;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain to me, and in a comment in the code, what we are doing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I added comments to the code which should help clarify things.
HLS has two types of playlists, a master playlist and regular playlists. The master playlist usually contains multiple variant streams and metadata which describes them (including bandwidth and resolution). The players can then choose between the variants depending on available resources.
The stream component originally just used a single regular playlist since we only have one stream variant. In 0.115, we made several changes to stream, including not including an audio stream with the video stream when there is no audio. However, this made chromecast devices not play the stream. We realized that we could fix around this problem by making a master playlist with only our single variant regular playlist with metadata which indicated that the stream had only video and no audio.
After this change, the stream component now sends a master playlist instead of a regular playlist. This is slightly less efficient since a master playlist must be loaded and parsed before the regular playlist can be loaded and played. Using a master playlist did help us by giving the ability to check the codec of the stream so we could restrict ExoPlayer use to only videos using the H.265 (HEVC) codec. But this actually resulted in 2 loads of the master playlist before the regular playlist load - once by ha-hls-player to check the codec, then once by the player itself. So we effectively went from one load (the player loading the regular playlist) to three loads (ha-hls-player loading the master playlist, the player loading the master playlist, then the player loading the regular playlist).
We previously reduced these 3 loads to 2 loads by passing the regular playlist directly to the player, helping the player avoid loading the master playlist. However, we did this by a simple string replace. @hunterjm noted that this might be undesirable because makes the frontend rely on the path of the variant stream coded in the backend.
This PR just gets the regular playlist correctly by parsing the regular playlist url from the master playlist.

// Get the regular playlist url from the input (master) playlist, falling back to the input playlist if necessary
// This avoids the player having to load and parse the master playlist again before loading the regular playlist
let playlist_url: string;
if (match !== null && playlistRegexp.exec(masterPlaylist) === null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still is not clear what we are doing here, why do we do the regex a second time and why should it not match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second regex is to test whether there's another #EXT-X-STREAM-INF line which would mean that our master playlist has more than one stream. We should actually not encounter this case currently as we are currently only include one stream in our master playlist, but in case we change things, this safely sends the master_playlist instead of skipping to the regular playlist when there is more than one stream available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comment for that, can we also give the various match part logical variable names? like:
const isHevc = match && match[1];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a change, does that look better? I haven't had a chance to test it yet

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that looks better. Let me know when you tested it :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested it and seems to be working fine

@bramkragten bramkragten merged commit e4ce611 into home-assistant:dev Nov 10, 2020
@bramkragten bramkragten mentioned this pull request Nov 11, 2020
@uvjustin uvjustin deleted the use-relative-url-for-hls-playlist branch November 25, 2020 16:29
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants