Skip to content

Add regular playlist to endpoint data in stream#42130

Closed
uvjustin wants to merge 2 commits intohome-assistant:devfrom
uvjustin:send-master-and-regular-playlist-for-hls
Closed

Add regular playlist to endpoint data in stream#42130
uvjustin wants to merge 2 commits intohome-assistant:devfrom
uvjustin:send-master-and-regular-playlist-for-hls

Conversation

@uvjustin
Copy link
Contributor

Proposed change

In ha-hls-player in the frontend we added a step to skip the master playlist and go straight to the regular playlist after the master playlist is loaded the first time. We used a simple string replace to get the regular playlist there. This PR adds the regular playlist to the endpoint data and allows it to be accessed via sending a "master_playlist=false" in the WS message.

Type of change

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

Example entry for configuration.yaml:

# Example configuration.yaml

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.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @hunterjm, mind taking a look at this pull request as its been labeled with an integration (stream) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@uvjustin
Copy link
Contributor Author

@hunterjm Not sure if this is what you had in mind. I haven't tested it yet, but something like this should work.
BTW, all the calls to request_stream/async_request_stream seem to use fmt="hls". Was this a parameter at first because of plans to eventually provide other stream types like dash?

@hunterjm
Copy link
Member

BTW, all the calls to request_stream/async_request_stream seem to use fmt="hls". Was this a parameter at first because of plans to eventually provide other stream types like dash?

Yeah, it was a bit of a premature addition on my part. I tried to architect the component to be able to easily add additional providers with different formats in the future (like DASH, or some new thing that comes out in the future). It actually worked well when I made the addition of the recorder provider, but I never got around to adding additional formats.

@uvjustin uvjustin marked this pull request as ready for review October 21, 2020 02:07
@uvjustin
Copy link
Contributor Author

Actually, after looking at the frontend, maybe it isn't necessary to do all this. ha-hls-player is opening the master playlist, so we can grab the regular playlist name from the master playlist there which would remove any coupling to the backend (besides assuming we have a master playlist). Otherwise, besides the changes here, we have to fetch and store 2 different urls and pass them along in javascript attributes from ha-camera-stream down to ha-hls-player.
Thoughts?

@uvjustin uvjustin marked this pull request as draft October 21, 2020 02:27
@hunterjm
Copy link
Member

Is the master playlist not playable in the frontend? When I tested it it worked just fine. Or is this an ExoPlayer thing?

@uvjustin
Copy link
Contributor Author

uvjustin commented Oct 21, 2020

The master playlist is playable, nothing to do with ExoPlayer. I just passed along the regular playlist instead of the master playlist in ha-hls-player to avoid having all the players do an extra load of the master playlist. None of the players needs the master playlist (We only added the master playlist for the google cast missing audio problem. Afterwards I also realized we could also use the master playlist to check the codec before resorting to ExoPlayer for H265 only).
Also, since we now do some non-kosher things with the master playlist like capping the AVC level and guessing the bandwidth, it might be a little safer to use the regular playlist vs the master playlist.
I guess there are 2 problems with the string replace (both involve coupling to the backend):

  1. We are assuming that we get a master playlist and not a regular playlist.
  2. We are hard coding the path names of the master vs the regular playlist.

2 is easily solvable by changing the string replace to a proper url substitution from the parsed master playlist.
1 is probably not a big deal since stream is the only provider to ha-hls-player

@uvjustin
Copy link
Contributor Author

Closing this as home-assistant/frontend#7417 seems cleaner.

@uvjustin uvjustin closed this Oct 21, 2020
@uvjustin uvjustin deleted the send-master-and-regular-playlist-for-hls branch November 25, 2020 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants