-
Notifications
You must be signed in to change notification settings - Fork 130
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
Add back option to use Frigate-native WebRTC support #784
Conversation
This is what I realized:
@edenhaus, I'd like to kindly ask you to shed some light on 2 and 3, would you? PS: I had asked for your feedback here, but feel free to disregard that one and reply to here only. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #784 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 2048 2080 +32
=========================================
+ Hits 2048 2080 +32 ☔ View full report in Codecov by Sentry. |
Ok, I found these PRs:
I believe they will fix the second bullet. I will test and update the PR once a new HA beta is released with them. EDIT: just found your blog post btw: https://developers.home-assistant.io/blog/2024/11/26/camera-deprecations/ EDIT 2: the second bullet is actually fixed by this exact change: Such commit not only deprecates the attribute but also fixes the method to return the proper This means I'll need to keep setting the attribute until the new version of HA is out. |
@edenhaus, this section of the code is according to the documentation (third bullet): I suppose the HLS player still plays my camera entity because it doesn't take I believe this logic can be improved. Maybe somehow detecting if If you can manage to do so, this PR (of mine) can be reverted: |
e47ccc2
to
5692b61
Compare
326a71e
to
9693b3a
Compare
9693b3a
to
4bcf827
Compare
9ae6665
to
e86bbbe
Compare
Another potential reason why the current approach is inefficient: The Frigate integration will provide an RTSP link with The HA integration will ask HA's go2rtc to transcode back AAC to OPUS: This by itself will consume some CPU %. You can multiply it to the number of cameras streaming at the same time. |
@dermotduffy raised this concern: What if the user doesn't have WebRTC properly configured in the Frigate settings? https://docs.frigate.video/configuration/live/#webrtc-extra-configuration |
we can check for that if we want to |
The ideal would be HA to fallback to HLS in this case. I will try to provoke this situation and see what happens. (despite the fact that HA documentation says camera entities with native WebRTC will not fallback to HLS) |
Presumably if that doesn't work we can just return None in the webrtc functions if webrtc is not enabled, and it will function the same as it does today |
I could not think of a way to detect that. For example, there's no obvious setting to disable webrtc in go2rtc. Do you have any idea? |
Just check if |
Is this correct? I thought it was the presence of those methods that mattered? |
That is what the Frigate UI uses so I would suggest just doing the same
technically we are not supposed to change that value, but I believe for HA 2024.11 you have to set it. |
If we want to go down this path, maybe a new option is the simplest / least surprising and lets people always go back to "HA native" (more likely to be future-proof?) if something breaks:
|
Agreed 👍 |
Unfortunately, it is: if you "tag" a camera as WebRTC-capable, Home Assistant will not fallback to HLS even if WebRTC doesn't work, and even if it provides everything HLS needs ( |
@NickM-27, BTW, this isn't even needed if you are running Frigate through the add-on. Or if you are running with |
I'm working to bring back the option. |
Isn't it? My understanding was when you run via the addon it just sets that value automatically. And running as host is not recommended |
Yes, but it would not be part of the |
Gotcha, we could probably add some field for that, but either way the manual option should work well for now |
1e3e713
to
c9159a5
Compare
1619e16
to
cab49bd
Compare
@dermotduffy, @NickM-27, here is my recommendations for the release notes: Remove:
Add:
Special mention: Frigate-native WebRTCHome Assistant 2024.11 ships with support for streaming any camera entity through WebRTC by relaying it through go2rtc. As you probably know, Frigate also uses go2rtc to support WebRTC streaming. If you have WebRTC configured and working in Frigate, we recommend you tick the Use Frigate-native WebRTC support integration option in the CONFIGURE button. When this option is enabled, Home Assistant will leverage Frigate's go2rtc to provide the WebRTC stream, rather than having to relay the stream into Home Assistant's own go2rtc, which consumes significantly more resources of your server (for example, Home Assistant's go2rtc will transcode AAC audio to OPUS). |
@NickM-27 @dermotduffy this is fully tested in my own Home Assistant instance. Ready to go from my side. |
Thanks for the persistence @felipecrs ! |
This partially reverts fix: Fix live streaming in HA >=
2024.11
#774To be merged after Correctly handle cameras that are not streaming #782
Closes Improve support for HA 2024.11.0 WebRTC streaming #759