core icon indicating copy to clipboard operation
core copied to clipboard

Show WebRTC cameras in the media browser

Open OnFreund opened this issue 1 year ago • 18 comments

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:

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:

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:

OnFreund avatar Jan 24 '24 17:01 OnFreund

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.

home-assistant[bot] avatar Jan 24 '24 17:01 home-assistant[bot]

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

codyc1515 avatar Jan 24 '24 19:01 codyc1515

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:

  1. stream_source can raise
  2. 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.

OnFreund avatar Jan 24 '24 20:01 OnFreund

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:

  1. stream_source can raise
  2. 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.

allenporter avatar Jan 25 '24 02:01 allenporter

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

OnFreund avatar Jan 25 '24 06:01 OnFreund

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

OnFreund avatar Jan 25 '24 06:01 OnFreund

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

allenporter avatar Jan 25 '24 15:01 allenporter

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 avatar Jan 25 '24 18:01 OnFreund

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

OnFreund avatar Feb 10 '24 10:02 OnFreund

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)

allenporter avatar Feb 10 '24 16:02 allenporter

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.

OnFreund avatar Feb 10 '24 18:02 OnFreund

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.

allenporter avatar Feb 10 '24 18:02 allenporter

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

OnFreund avatar Feb 10 '24 18:02 OnFreund

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

allenporter avatar Feb 10 '24 19:02 allenporter

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.

OnFreund avatar Feb 10 '24 19:02 OnFreund

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)

allenporter avatar Feb 10 '24 19:02 allenporter

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

OnFreund avatar Feb 14 '24 16:02 OnFreund

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

Learn more about our pull request process.

home-assistant[bot] avatar Feb 17 '24 04:02 home-assistant[bot]

Thanks @allenporter. I created a follow up PR for the display name change: https://github.com/home-assistant/core/pull/110882

OnFreund avatar Feb 18 '24 18:02 OnFreund