Show WebRTC cameras that also support HLS in the media browser by OnFreund · Pull Request #108796 · home-assistant/core

OnFreund · 2024-01-24T17:53:18Z

Proposed change

As discussed in home-assistant/architecture#1030, support for resolving WebRTC cameras as media sources was added in #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!)
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

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
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
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 running python3 -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.

home-assistant · 2024-01-24T17:53:25Z

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.

OnFreund · 2024-01-24T17:54:37Z

                    media_class=MediaClass.VIDEO,
                    media_content_type=content_type,
-                    title=camera.name,
+                    title=cast(


The previous implementation left an empty title for cameras that took their name from the device. This seems to be a better choice, but if there's a better way to get the name I'll change it.

codyc1515 · 2024-01-24T19:23:27Z

Which cameras are expected to fail? If we know them, could we exclude them?

OnFreund · 2024-01-24T20:03:02Z

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's async, 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.

allenporter · 2024-01-25T02:39:44Z

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's async, 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.

OnFreund · 2024-01-25T06:25:41Z

I agree that real support WebRTC media sources is preferable. In the meantime, I think this PR is a good compromise.

OnFreund · 2024-01-25T06:53:23Z

@allenporter are there Nest cameras that only support WebRTC but can't produce a stream for HLS? If so, how common are they?

allenporter · 2024-01-25T15:21:35Z

@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.

OnFreund · 2024-01-25T18:14:33Z

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.

OnFreund · 2024-02-10T10:49:49Z

@allenporter Do you think we can get this merged and continue the discussion on real WebRTC support separately?

allenporter · 2024-02-10T16:02:18Z

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)

OnFreund · 2024-02-10T18:45:10Z

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.

allenporter · 2024-02-10T18:54:28Z

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.

OnFreund · 2024-02-10T18:57:45Z

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)?

allenporter · 2024-02-10T19:03:41Z

I would prefer to only show cameras that will work. I personally think it makes sense to just check the source...

OnFreund · 2024-02-10T19:06:35Z

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.

allenporter · 2024-02-10T19:10:15Z

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)

OnFreund · 2024-02-14T16:51:55Z

Thanks @allenporter, I updated the code with your suggestions.

allenporter · 2024-02-17T03:45:19Z

+                media_class=MediaClass.VIDEO,
+                media_content_type=content_type,
+                title=cast(
+                    State, self.hass.states.get(camera.entity_id)


I still want to see this change in a separate PR with its own description, etc. https://developers.home-assistant.io/docs/review-process/#creating-the-perfect-pr as i mentioned before.

allenporter · 2024-02-17T04:01:16Z

-    assert len(item.children) == 0
-    assert item.not_shown == 3
+async def test_browsing_web_rtc(hass: HomeAssistant, mock_camera_web_rtc) -> None:
+    """Test browsing camera media source."""


Update test description

allenporter · 2024-02-17T04:05:48Z

+    """Test browsing camera media source."""
+    component: EntityComponent[Camera] = hass.data[DOMAIN]
+    cameras = list(component.entities)
+    with patch.object(cameras[0], "stream_source", return_value="test"), patch.object(


Tests do not interact with hass.data and instead should by string name as is done in other parts of the tests in this file..

In this case i think the reason this approach is chosen is because there are different return values for the different cameras?

Instead, patch once and have either the callable or side effect return the different values for each call.

Also a small comment might go a long way here, something like: "One camera supports HLS and WebRTC, one cameras supports only WebRTC, and one camera fails." etc

home-assistant · 2024-02-17T04:08:05Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

allenporter · 2024-02-17T17:42:57Z

-        children = []
-        not_shown = 0
-        for camera in component.entities:
+        async def _media_source_for_camera(camera: Camera) -> BrowseMediaSource | None:


I think everything looks good to go here, but now i'm thinking about one more idea to further streamline this, but not positive it would work out: Could the scope of this function be to just filter out the not relevant cameras and leave the browse media source building all together how it was? It could just get the eligible cameras in one pass then build the sources in a simple loop together with building the root.

We could return the camera instead of BrowseMediaSource, and then iterate over the results to build the sources, but I'm not sure I see why it would be better?

Its possibly a style preference, but my thinking is: in general, reducing # of verical lines of inline functions or reducing the scope as much as possible seems positive. Having the BrowseMediaSource building closer together together seems logical as well. One exception may be if there is shared logic on both parts but i think the existing stream type filtering looks more complex than it needs to be

It's a bit tricky because we also have to recalculate the content type (or return a tuple).
Arguably refactoring the creation of the BrowseMediaSource to a separate function would achieve a similar result (see my latest commit)

Thanks, that looks good. What do you think about gutting all the existing vertical whitespace? It seems like it makes this harder to follow given it has so many branches and is in an inline function. Collapsing vertically will also help make this simpler to grok quickly.

allenporter

Looks great, i think i just have one more quick suggestion to further shorten

allenporter · 2024-02-17T19:29:03Z

+                if stream_type != StreamType.HLS:
+                    source = await camera.stream_source()
+                    if not source:
+                        return None


Does it work for this to be a single statement? (Given this is 3 levels of nested ifs in an inline function)

Suggested change

if stream_type != StreamType.HLS:

source = await camera.stream_source()

if not source:

return None

if stream_type != StreamType.HLS and not (source := await camera.stream_source())

return None

I took advantage of the ability to return early in a successful case to refactor a bit differently. Let me know what you think.

allenporter · 2024-02-17T19:30:19Z

-        children = []
-        not_shown = 0
-        for camera in component.entities:
+        async def _media_source_for_camera(camera: Camera) -> BrowseMediaSource | None:


Thanks, that looks good. What do you think about gutting all the existing vertical whitespace? It seems like it makes this harder to follow given it has so many branches and is in an inline function. Collapsing vertically will also help make this simpler to grok quickly.

allenporter

Minor nits otherwise looks good

allenporter · 2024-02-17T23:57:06Z

+            if stream_type != StreamType.HLS:
+                source = await camera.stream_source()
+                if not source:


This seems like it can either be 1 or 2 lines rather than 3

allenporter · 2024-02-17T23:59:36Z

+
+            return _media_source_for_camera(camera, content_type)
+
+        # Root. List cameras.


Maybe just drop this while we're here

Suggested change

# Root. List cameras.

OnFreund · 2024-02-18T18:25:07Z

Thanks @allenporter. I created a follow up PR for the display name change: #110882

OnFreund requested a review from a team as a code owner January 24, 2024 17:53

home-assistant Bot added cla-signed core has-tests integration: camera new-feature small-pr PRs with less than 30 lines. Quality Scale: internal labels Jan 24, 2024

OnFreund commented Jan 24, 2024

View reviewed changes

OnFreund force-pushed the webrtc_media_source branch from 9a68e09 to 0febce4 Compare February 14, 2024 16:51

allenporter requested changes Feb 17, 2024

View reviewed changes

home-assistant Bot marked this pull request as draft February 17, 2024 04:08

OnFreund added 3 commits February 17, 2024 11:52

Show WebRTC cameras in the media browser

4c61d80

Only show webrtc cameras with source in the browser

8f69dff

Address code review

7b72172

OnFreund force-pushed the webrtc_media_source branch from 0febce4 to 7b72172 Compare February 17, 2024 10:18

OnFreund marked this pull request as ready for review February 17, 2024 10:19

home-assistant Bot requested a review from allenporter February 17, 2024 10:19

allenporter reviewed Feb 17, 2024

View reviewed changes

Refactor BrowseMediaSource creation

c9d93a0

allenporter requested changes Feb 17, 2024

View reviewed changes

home-assistant Bot marked this pull request as draft February 17, 2024 19:32

Refactor

27929e1

OnFreund marked this pull request as ready for review February 17, 2024 19:51

home-assistant Bot requested a review from allenporter February 17, 2024 19:51

allenporter reviewed Feb 18, 2024

View reviewed changes

Address code review

ea791fe

OnFreund requested a review from allenporter February 18, 2024 16:42

allenporter approved these changes Feb 18, 2024

View reviewed changes

allenporter changed the title ~~Show WebRTC cameras in the media browser~~ Show WebRTC cameras that also support HLS in the media browser Feb 18, 2024

allenporter merged commit e879ab0 into home-assistant:dev Feb 18, 2024

OnFreund deleted the webrtc_media_source branch February 18, 2024 18:15

github-actions Bot locked and limited conversation to collaborators Feb 19, 2024


		return _media_source_for_camera(camera, content_type)

		# Root. List cameras.

Uh oh!

Conversation

OnFreund commented Jan 24, 2024

Proposed change

Type of change

Additional information

Checklist

Uh oh!

home-assistant Bot commented Jan 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codyc1515 commented Jan 24, 2024

Uh oh!

OnFreund commented Jan 24, 2024

Uh oh!

allenporter commented Jan 25, 2024

Uh oh!

OnFreund commented Jan 25, 2024

Uh oh!

OnFreund commented Jan 25, 2024

Uh oh!

allenporter commented Jan 25, 2024

Uh oh!

OnFreund commented Jan 25, 2024

Uh oh!

OnFreund commented Feb 10, 2024

Uh oh!

allenporter commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OnFreund commented Feb 10, 2024

Uh oh!

allenporter commented Feb 10, 2024

Uh oh!

OnFreund commented Feb 10, 2024

Uh oh!

allenporter commented Feb 10, 2024

Uh oh!

OnFreund commented Feb 10, 2024

Uh oh!

allenporter commented Feb 10, 2024

Uh oh!

OnFreund commented Feb 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

home-assistant Bot commented Feb 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allenporter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allenporter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OnFreund commented Feb 18, 2024

allenporter commented Feb 10, 2024 •

edited

Loading