core
core copied to clipboard
Show WebRTC cameras in the media browser
Proposed change
As discussed in https://github.com/home-assistant/architecture/discussions/1030, support for resolving WebRTC cameras as media sources was added in https://github.com/home-assistant/core/pull/80646. Those cameras, however, were still excluded from the media browser. This PR makes them available in the browser instead of being filtered out. While it's possible some of these cameras will fail to play, most should succeed. Specifically:
- RTSPtoWebRTC was the impetus for the aforementioned PR.
- Nest cameras can support both WebRTC and HLS(at least for some cameras, I'm not sure if there are exceptions).
- While not a native integration, Frigate is a popular custom integration, and it recently added support for serving both WebRTC and HLS.
Type of change
- [ ] Dependency upgrade
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [X] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
- This PR fixes or closes issue: fixes #
- This PR is related to issue:
- Link to documentation pull request:
Checklist
- [X] The code change is tested and works locally.
- [X] Local tests pass. Your PR cannot be merged unless tests pass
- [X] There is no commented out code in this PR.
- [X] I have followed the development checklist
- [X] I have followed the perfect PR recommendations
- [X] The code has been formatted using Ruff (
ruff format homeassistant tests
) - [X] Tests have been added to verify that the new code works.
If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated for www.home-assistant.io
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 runningpython3 -m script.gen_requirements_all
. - [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [ ] Untested files have been added to
.coveragerc
.
To help with the load of incoming pull requests:
- [ ] I have reviewed two other open pull requests in this repository.
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (camera
) you are listed as a code owner for? Thanks!
Code owner commands
Code owners of camera
can trigger bot actions by commenting:
-
@home-assistant close
Closes the pull request. -
@home-assistant rename Awesome new title
Renames the pull request. -
@home-assistant reopen
Reopen the pull request. -
@home-assistant unassign camera
Removes the current integration label and assignees on the pull request, add the integration domain after the command. -
@home-assistant add-label needs-more-information
Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request. -
@home-assistant remove-label needs-more-information
Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.
Which cameras are expected to fail? If we know them, could we exclude them?
I don't know of specific cameras that would fail (and if such exist), but the Nest tests seem to suggest there are cameras that support both WebRTC and HLS and cameras that only support WebRTC.
Perhaps testing stream_source
against None
could be a better filter, but I'm not sure whether:
-
stream_source
can raise -
stream_source
can take long or run IO (it'sasync
, which suggests it can)
In the arch discussion I proposed that WebRTC cameras explicitly opt in to an HLS fallback, but it got some push back. It was also decided against when WebRTC cameras were introduced (albeit that was before the introduction of camera media sources). Last but not least, it seems like failures should be rare, and have an appropriate error.
I don't know of specific cameras that would fail (and if such exist), but the Nest tests seem to suggest there are cameras that support both WebRTC and HLS and cameras that only support WebRTC.
Perhaps testing
stream_source
againstNone
could be a better filter, but I'm not sure whether:
stream_source
can raisestream_source
can take long or run IO (it'sasync
, which suggests it can)In the arch discussion I proposed that WebRTC cameras explicitly opt in to an HLS fallback, but it got some push back. It was also decided against when WebRTC cameras were introduced (albeit that was before the introduction of camera media sources). Last but not least, it seems like failures should be rare, and have an appropriate error.
I agree that testing it against None is more accurate, but yes it can also raise and do I/O. (example)
This is kind of why i was waffling a bit on the architecture discussion. The right way to check this is by checking if the stream is none, but that causes I/O that generates a url in the nest case, so, meh. This is what lead me to ask the question about media player and webrtc support.
I agree that real support WebRTC media sources is preferable. In the meantime, I think this PR is a good compromise.
@allenporter are there Nest cameras that only support WebRTC but can't produce a stream for HLS? If so, how common are they?
@allenporter are there Nest cameras that only support WebRTC but can't produce a stream for HLS? If so, how common are they?
There is a new wave of cameras being converted to webrtc and I'm not yet sure if that means they still also support hls. I have not yet converted mine but will at some point in the next few months.
The other case is rtsp_to_webrtc component which allows an rtsp camera to support both.
Yeah, rstp_to_webrtc supports both, and so does Frigate after the relevant PR. The only unknown is some Nest cameras. Since the error message is a friendly one, I think it should be fine.
@allenporter Do you think we can get this merged and continue the discussion on real WebRTC support separately?
I realize i misunderstood your question: Every new camera from the last few years only supports WebRTC. (I misunderstood and thought you were asking if they supports both webrtc and hls, and so my answer was about old cameras moving to google home)
As I mentioned above, this is not the correct use of the API and I'm not sure even what checking of stream type is None
is even doing, if anything at all. Also, I the unrelated name change topic should be a separate PR. (I don't know if it is correct)
Do you mean that new Nest cameras do not expose a stream that can be used over HLS?
As for the name change - this change kind of exposed it, because some of the newly added cameras weren't named correctly or did not have a name.
Do you mean that new Nest cameras do not expose a stream that can be used over HLS?
Nest cameras made for the last few years only support webrtc, and do not support rtsp and therefore don't use HLS in the frontend.
OK, that definitely changes the picture. I guess the question is - are we ok with making more cameras more browsable, at the expense of some of them failing (but with a friendly error message)?
I would prefer to only show cameras that will work. I personally think it makes sense to just check the source...
How would you handle cameras that raise in stream_source
? One camera shouldn't prevent browsing, but I also wouldn't feel comfortable with a catch-all.
I would suggest:
- always show HLS cameras
- check the source for webrtc cameras
- get the stream source checks in parallel
- omit cameras with failed stream sources
FWIW the thumbnail shown in the browser also uses the rtsp stream source so it'll probably be fetched again immediately anyway (and be cached if there is a performance issue)
Thanks @allenporter, I updated the code with your suggestions.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:
Thanks @allenporter. I created a follow up PR for the display name change: https://github.com/home-assistant/core/pull/110882