core icon indicating copy to clipboard operation
core copied to clipboard

Add async webrtc offer support

Open edenhaus opened this issue 1 year ago • 6 comments

Breaking change

Proposed change

Until now, the WebRTC was synced, which means the front end needs to get all candidates upfront. This can take up to 40 seconds. Therefore, this PR adds the async approach.

The Camera Entity will be extended with the following functions, which integrations can overwrite it:

  • async_handle_webrtc_offer: Async webrtc offer with offer, session id and send_message callback
  • async_on_webrtc_candidate: with session ID and candidate. The frontend will call it with any candidate coming in after the offer is sent.
  • close_webrtc_session: The frontend will call it when the stream is closed. Can be used to clean up things.

The synced approach is not deprecated and will be deprecated in a follow-up PR. As the frontend needs to know if it needs to get candidates upfront or not, the WebRTCClientConfiguration was extended to have a boolean for it.

The CameraWebRTCProvider was refactored to support only the async approach. For integrations that are using the async_register_rtsp_to_web_rtc_provider automatically, a synced (legacy) Provider is created, and they will continue to work without any issues.

The first integration, go2rtc, is implementing the async offer approach and can be used as a guide for other integrations. For the async approach, a dependency bump was required, which cannot be split out due to breaking changes. Changes: https://github.com/home-assistant-libs/python-go2rtc-client/compare/0.0.1b0...0.0.1b1

Type of change

  • [x] 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: https://github.com/home-assistant/developers.home-assistant/pull/2409

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:

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.

To help with the load of incoming pull requests:

edenhaus avatar Oct 09 '24 08:10 edenhaus

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 Oct 09 '24 08:10 home-assistant[bot]

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (websocket_api) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of websocket_api 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 websocket_api 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 Oct 09 '24 08:10 home-assistant[bot]

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (go2rtc) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of go2rtc 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 go2rtc 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 Oct 09 '24 08:10 home-assistant[bot]

Test failure unrelated

edenhaus avatar Oct 19 '24 10:10 edenhaus

Until there is a frontend linked, please use the frontend branch async-webrtc

edenhaus avatar Oct 19 '24 15:10 edenhaus

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 Oct 19 '24 18:10 home-assistant[bot]

Some tests need updating.

MartinHjelmare avatar Oct 22 '24 14:10 MartinHjelmare

It would be nice to allow entering a go2rtc URL even when running through docker, for Frigate users, for example.

felipecrs avatar Oct 22 '24 22:10 felipecrs

@felipecrs This PR does not change anything in the go2rtc config flow, and therefore, your comment is offtopic. Anyways, the config flow will be removed in https://github.com/home-assistant/core/pull/129020

edenhaus avatar Oct 23 '24 11:10 edenhaus

Indeed my comment is off-topic, but the PRs where this conversation would be relevant were locked. Anyway, thanks for pointing to https://github.com/home-assistant/core/pull/129020.

felipecrs avatar Oct 23 '24 11:10 felipecrs

@felipecrs Please don't comment on PRs with off-topic topics only because the relevant PRs are locked in the future

edenhaus avatar Oct 23 '24 19:10 edenhaus

frontend pr is approved

bramkragten avatar Oct 23 '24 19:10 bramkragten

Waiting on this architecture discussion: https://github.com/home-assistant/architecture/discussions/1165

We also need to rebase here.

MartinHjelmare avatar Oct 24 '24 09:10 MartinHjelmare

We need to rebase again.

MartinHjelmare avatar Oct 25 '24 09:10 MartinHjelmare

We need to rebase again.

🎉

emontnemery avatar Oct 25 '24 11:10 emontnemery

Waiting on this architecture discussion: home-assistant/architecture#1165

We also need to rebase here.

It's approved and we can merge it :D

edenhaus avatar Oct 28 '24 14:10 edenhaus

package_constraints.txt was missed to be updated. Ok, see a PR has been made to fix it.

gjohansson-ST avatar Oct 28 '24 15:10 gjohansson-ST