From 405a480caeebaf4ee2e038ee8199f514f6e0833a Mon Sep 17 00:00:00 2001 From: Martin Hjelmare Date: Wed, 30 Oct 2024 14:11:17 +0100 Subject: [PATCH] Create repair issue for legacy webrtc provider (#129334) * Add repair issue * Add tests * Add option to not use builtin go2rtc provider * Add test * Add domain to new providers * Add learn more url * Update placeholder * Promote the builtin provider * Refactor provider storage * Move check for legacy provider conflict to refresh * Test provider registration race * Add test for registering the same legacy provider twice * Test test_get_not_supported_legacy_provider * Remove blank line between bullets * Call it built-in Co-authored-by: Joost Lekkerkerker * Revert "Add option to not use builtin go2rtc provider" This reverts commit 4e31bad6c0c23d5a1c0935c985351808a46163d6. * Revert "Add test" This reverts commit ddf85fd4db2c78b15c1cdc716804b965f3a1f4e3. * Update issue description * async_close_session is optional * Clean up after rebase * Add required domain property to provider tests --------- Co-authored-by: Joost Lekkerkerker --- homeassistant/components/camera/strings.json | 4 + homeassistant/components/camera/webrtc.py | 111 ++++++++++++------ homeassistant/components/go2rtc/__init__.py | 5 + tests/components/camera/test_init.py | 5 + tests/components/camera/test_webrtc.py | 117 +++++++++++++++++-- 5 files changed, 197 insertions(+), 45 deletions(-) diff --git a/homeassistant/components/camera/strings.json b/homeassistant/components/camera/strings.json index 9176c5ad84ad75..4a7e9aafc6e4ab 100644 --- a/homeassistant/components/camera/strings.json +++ b/homeassistant/components/camera/strings.json @@ -46,6 +46,10 @@ } } } + }, + "legacy_webrtc_provider": { + "title": "Detected use of legacy WebRTC provider registered by {legacy_integration}", + "description": "The {legacy_integration} integration has registered a legacy WebRTC provider. Home Assistant prefers using the built-in modern WebRTC provider registered by the {builtin_integration} integration.\n\nBenefits of the built-in integration are:\n\n- The camera stream is started faster.\n- More camera devices are supported.\n\nTo fix this issue, you can either keep using the built-in modern WebRTC provider and remove the {legacy_integration} integration or remove the {builtin_integration} integration to use the legacy provider, and then restart Home Assistant." } }, "services": { diff --git a/homeassistant/components/camera/webrtc.py b/homeassistant/components/camera/webrtc.py index 74527b43a297ed..aca2b8291f19cc 100644 --- a/homeassistant/components/camera/webrtc.py +++ b/homeassistant/components/camera/webrtc.py @@ -2,6 +2,7 @@ from __future__ import annotations +from abc import ABC, abstractmethod import asyncio from collections.abc import Awaitable, Callable, Iterable from dataclasses import asdict, dataclass, field @@ -15,7 +16,7 @@ from homeassistant.components import websocket_api from homeassistant.core import HomeAssistant, callback from homeassistant.exceptions import HomeAssistantError -from homeassistant.helpers import config_validation as cv +from homeassistant.helpers import config_validation as cv, issue_registry as ir from homeassistant.util.hass_dict import HassKey from homeassistant.util.ulid import ulid @@ -31,7 +32,7 @@ DATA_WEBRTC_PROVIDERS: HassKey[set[CameraWebRTCProvider]] = HassKey( "camera_webrtc_providers" ) -DATA_WEBRTC_LEGACY_PROVIDERS: HassKey[set[CameraWebRTCLegacyProvider]] = HassKey( +DATA_WEBRTC_LEGACY_PROVIDERS: HassKey[dict[str, CameraWebRTCLegacyProvider]] = HassKey( "camera_webrtc_legacy_providers" ) DATA_ICE_SERVERS: HassKey[list[Callable[[], Iterable[RTCIceServer]]]] = HassKey( @@ -113,13 +114,20 @@ def to_frontend_dict(self) -> dict[str, Any]: return data -class CameraWebRTCProvider(Protocol): +class CameraWebRTCProvider(ABC): """WebRTC provider.""" + @property + @abstractmethod + def domain(self) -> str: + """Return the integration domain of the provider.""" + @callback + @abstractmethod def async_is_supported(self, stream_source: str) -> bool: """Determine if the provider supports the stream source.""" + @abstractmethod async def async_handle_async_webrtc_offer( self, camera: Camera, @@ -129,6 +137,7 @@ async def async_handle_async_webrtc_offer( ) -> None: """Handle the WebRTC offer and return the answer via the provided callback.""" + @abstractmethod async def async_on_webrtc_candidate(self, session_id: str, candidate: str) -> None: """Handle the WebRTC candidate.""" @@ -150,10 +159,10 @@ async def async_handle_web_rtc_offer( """Handle the WebRTC offer and return an answer.""" -def _async_register_webrtc_provider[_T]( +@callback +def async_register_webrtc_provider( hass: HomeAssistant, - key: HassKey[set[_T]], - provider: _T, + provider: CameraWebRTCProvider, ) -> Callable[[], None]: """Register a WebRTC provider. @@ -162,7 +171,7 @@ def _async_register_webrtc_provider[_T]( if DOMAIN not in hass.data: raise ValueError("Unexpected state, camera not loaded") - providers = hass.data.setdefault(key, set()) + providers = hass.data.setdefault(DATA_WEBRTC_PROVIDERS, set()) @callback def remove_provider() -> None: @@ -177,20 +186,9 @@ def remove_provider() -> None: return remove_provider -@callback -def async_register_webrtc_provider( - hass: HomeAssistant, - provider: CameraWebRTCProvider, -) -> Callable[[], None]: - """Register a WebRTC provider. - - The first provider to satisfy the offer will be used. - """ - return _async_register_webrtc_provider(hass, DATA_WEBRTC_PROVIDERS, provider) - - async def _async_refresh_providers(hass: HomeAssistant) -> None: """Check all cameras for any state changes for registered providers.""" + _async_check_conflicting_legacy_provider(hass) component = hass.data[DATA_COMPONENT] await asyncio.gather( @@ -334,11 +332,11 @@ def async_register_ws(hass: HomeAssistant) -> None: websocket_api.async_register_command(hass, ws_candidate) -async def _async_get_supported_provider[ - _T: CameraWebRTCLegacyProvider | CameraWebRTCProvider -](hass: HomeAssistant, camera: Camera, key: HassKey[set[_T]]) -> _T | None: +async def async_get_supported_provider( + hass: HomeAssistant, camera: Camera +) -> CameraWebRTCProvider | None: """Return the first supported provider for the camera.""" - providers = hass.data.get(key) + providers = hass.data.get(DATA_WEBRTC_PROVIDERS) if not providers or not (stream_source := await camera.stream_source()): return None @@ -349,20 +347,19 @@ async def _async_get_supported_provider[ return None -async def async_get_supported_provider( - hass: HomeAssistant, camera: Camera -) -> CameraWebRTCProvider | None: - """Return the first supported provider for the camera.""" - return await _async_get_supported_provider(hass, camera, DATA_WEBRTC_PROVIDERS) - - async def async_get_supported_legacy_provider( hass: HomeAssistant, camera: Camera ) -> CameraWebRTCLegacyProvider | None: """Return the first supported provider for the camera.""" - return await _async_get_supported_provider( - hass, camera, DATA_WEBRTC_LEGACY_PROVIDERS - ) + providers = hass.data.get(DATA_WEBRTC_LEGACY_PROVIDERS) + if not providers or not (stream_source := await camera.stream_source()): + return None + + for provider in providers.values(): + if await provider.async_is_supported(stream_source): + return provider + + return None @callback @@ -425,7 +422,49 @@ def async_register_rtsp_to_web_rtc_provider( The first provider to satisfy the offer will be used. """ + if DOMAIN not in hass.data: + raise ValueError("Unexpected state, camera not loaded") + + legacy_providers = hass.data.setdefault(DATA_WEBRTC_LEGACY_PROVIDERS, {}) + + if domain in legacy_providers: + raise ValueError("Provider already registered") + provider_instance = _CameraRtspToWebRTCProvider(provider) - return _async_register_webrtc_provider( - hass, DATA_WEBRTC_LEGACY_PROVIDERS, provider_instance - ) + + @callback + def remove_provider() -> None: + legacy_providers.pop(domain) + hass.async_create_task(_async_refresh_providers(hass)) + + legacy_providers[domain] = provider_instance + hass.async_create_task(_async_refresh_providers(hass)) + + return remove_provider + + +@callback +def _async_check_conflicting_legacy_provider(hass: HomeAssistant) -> None: + """Check if a legacy provider is registered together with the builtin provider.""" + builtin_provider_domain = "go2rtc" + if ( + (legacy_providers := hass.data.get(DATA_WEBRTC_LEGACY_PROVIDERS)) + and (providers := hass.data.get(DATA_WEBRTC_PROVIDERS)) + and any(provider.domain == builtin_provider_domain for provider in providers) + ): + for domain in legacy_providers: + ir.async_create_issue( + hass, + DOMAIN, + f"legacy_webrtc_provider_{domain}", + is_fixable=False, + is_persistent=False, + issue_domain=domain, + learn_more_url="https://www.home-assistant.io/integrations/go2rtc/", + severity=ir.IssueSeverity.WARNING, + translation_key="legacy_webrtc_provider", + translation_placeholders={ + "legacy_integration": domain, + "builtin_integration": builtin_provider_domain, + }, + ) diff --git a/homeassistant/components/go2rtc/__init__.py b/homeassistant/components/go2rtc/__init__.py index 588e403505f921..9501bee776b676 100644 --- a/homeassistant/components/go2rtc/__init__.py +++ b/homeassistant/components/go2rtc/__init__.py @@ -172,6 +172,11 @@ def __init__(self, hass: HomeAssistant, url: str) -> None: self._rest_client = Go2RtcRestClient(self._session, url) self._sessions: dict[str, Go2RtcWsClient] = {} + @property + def domain(self) -> str: + """Return the integration domain of the provider.""" + return DOMAIN + @callback def async_is_supported(self, stream_source: str) -> bool: """Return if this provider is supports the Camera as source.""" diff --git a/tests/components/camera/test_init.py b/tests/components/camera/test_init.py index ae1cce5832d66c..58d87a42572523 100644 --- a/tests/components/camera/test_init.py +++ b/tests/components/camera/test_init.py @@ -938,6 +938,11 @@ async def test(expected_types: set[StreamType]) -> None: class SomeTestProvider(CameraWebRTCProvider): """Test provider.""" + @property + def domain(self) -> str: + """Return domain.""" + return "test" + @callback def async_is_supported(self, stream_source: str) -> bool: """Determine if the provider supports the stream source.""" diff --git a/tests/components/camera/test_webrtc.py b/tests/components/camera/test_webrtc.py index 6b2ca8a7d4c1c4..21d9ccf89f7854 100644 --- a/tests/components/camera/test_webrtc.py +++ b/tests/components/camera/test_webrtc.py @@ -20,6 +20,7 @@ WebRTCError, WebRTCMessage, WebRTCSendMessage, + async_get_supported_legacy_provider, async_register_ice_servers, async_register_rtsp_to_web_rtc_provider, async_register_webrtc_provider, @@ -30,6 +31,7 @@ from homeassistant.core import HomeAssistant, callback from homeassistant.core_config import async_process_ha_core_config from homeassistant.exceptions import HomeAssistantError +from homeassistant.helpers import issue_registry as ir from homeassistant.setup import async_setup_component from .common import STREAM_SOURCE, WEBRTC_ANSWER @@ -49,13 +51,18 @@ TEST_INTEGRATION_DOMAIN = "test" -class TestProvider(CameraWebRTCProvider): +class SomeTestProvider(CameraWebRTCProvider): """Test provider.""" def __init__(self) -> None: """Initialize the provider.""" self._is_supported = True + @property + def domain(self) -> str: + """Return the integration domain of the provider.""" + return "some_test" + @callback def async_is_supported(self, stream_source: str) -> bool: """Determine if the provider supports the stream source.""" @@ -82,6 +89,15 @@ def async_close_session(self, session_id: str) -> None: """Close the session.""" +class Go2RTCProvider(SomeTestProvider): + """go2rtc provider.""" + + @property + def domain(self) -> str: + """Return the integration domain of the provider.""" + return "go2rtc" + + class MockCamera(Camera): """Mock Camera Entity.""" @@ -162,11 +178,13 @@ async def async_unload_entry_init( @pytest.fixture -async def register_test_provider(hass: HomeAssistant) -> AsyncGenerator[TestProvider]: +async def register_test_provider( + hass: HomeAssistant, +) -> AsyncGenerator[SomeTestProvider]: """Add WebRTC test provider.""" await async_setup_component(hass, "camera", {}) - provider = TestProvider() + provider = SomeTestProvider() unsub = async_register_webrtc_provider(hass, provider) await hass.async_block_till_done() yield provider @@ -183,7 +201,7 @@ async def test_async_register_webrtc_provider( camera = get_camera_from_entity_id(hass, "camera.demo_camera") assert camera.frontend_stream_type is StreamType.HLS - provider = TestProvider() + provider = SomeTestProvider() unregister = async_register_webrtc_provider(hass, provider) await hass.async_block_till_done() @@ -211,7 +229,7 @@ async def test_async_register_webrtc_provider( @pytest.mark.usefixtures("mock_camera", "mock_stream", "mock_stream_source") async def test_async_register_webrtc_provider_twice( hass: HomeAssistant, - register_test_provider: TestProvider, + register_test_provider: SomeTestProvider, ) -> None: """Test registering a WebRTC provider twice should raise.""" with pytest.raises(ValueError, match="Provider already registered"): @@ -223,7 +241,7 @@ async def test_async_register_webrtc_provider_camera_not_loaded( ) -> None: """Test registering a WebRTC provider when camera is not loaded.""" with pytest.raises(ValueError, match="Unexpected state, camera not loaded"): - async_register_webrtc_provider(hass, TestProvider()) + async_register_webrtc_provider(hass, SomeTestProvider()) @pytest.mark.usefixtures("mock_camera", "mock_stream", "mock_stream_source") @@ -494,7 +512,7 @@ async def test_websocket_webrtc_offer( async def test_websocket_webrtc_offer_webrtc_provider( hass: HomeAssistant, hass_ws_client: WebSocketGenerator, - register_test_provider: TestProvider, + register_test_provider: SomeTestProvider, message: WebRTCMessage, expected_frontend_message: dict[str, Any], ) -> None: @@ -997,7 +1015,7 @@ async def test_ws_webrtc_candidate_not_supported( async def test_ws_webrtc_candidate_webrtc_provider( hass: HomeAssistant, hass_ws_client: WebSocketGenerator, - register_test_provider: TestProvider, + register_test_provider: SomeTestProvider, ) -> None: """Test ws webrtc candidate command with WebRTC provider.""" with patch.object( @@ -1045,7 +1063,7 @@ async def test_ws_webrtc_candidate_invalid_entity( @pytest.mark.usefixtures("mock_camera_webrtc") -async def test_ws_webrtc_canidate_missing_candidtae( +async def test_ws_webrtc_canidate_missing_candidate( hass: HomeAssistant, hass_ws_client: WebSocketGenerator ) -> None: """Test ws WebRTC candidate command with missing required fields.""" @@ -1094,6 +1112,11 @@ async def test_webrtc_provider_optional_interface(hass: HomeAssistant) -> None: class OnlyRequiredInterfaceProvider(CameraWebRTCProvider): """Test provider.""" + @property + def domain(self) -> str: + """Return the domain of the provider.""" + return "test" + @callback def async_is_supported(self, stream_source: str) -> bool: """Determine if the provider supports the stream source.""" @@ -1125,3 +1148,79 @@ async def async_on_webrtc_candidate( ) await provider.async_on_webrtc_candidate("session_id", "candidate") provider.async_close_session("session_id") + + +@pytest.mark.usefixtures("mock_camera") +async def test_repair_issue_legacy_provider( + hass: HomeAssistant, + issue_registry: ir.IssueRegistry, +) -> None: + """Test repair issue created for legacy provider.""" + # Ensure no issue if no provider is registered + assert not issue_registry.async_get_issue( + "camera", "legacy_webrtc_provider_mock_domain" + ) + + # Register a legacy provider + legacy_provider = Mock(side_effect=provide_webrtc_answer) + unsub_legacy_provider = async_register_rtsp_to_web_rtc_provider( + hass, "mock_domain", legacy_provider + ) + await hass.async_block_till_done() + + # Ensure no issue if only legacy provider is registered + assert not issue_registry.async_get_issue( + "camera", "legacy_webrtc_provider_mock_domain" + ) + + provider = Go2RTCProvider() + unsub_go2rtc_provider = async_register_webrtc_provider(hass, provider) + await hass.async_block_till_done() + + # Ensure issue when legacy and builtin provider are registered + issue = issue_registry.async_get_issue( + "camera", "legacy_webrtc_provider_mock_domain" + ) + assert issue + assert issue.is_fixable is False + assert issue.is_persistent is False + assert issue.issue_domain == "mock_domain" + assert issue.learn_more_url == "https://www.home-assistant.io/integrations/go2rtc/" + assert issue.severity == ir.IssueSeverity.WARNING + assert issue.issue_id == "legacy_webrtc_provider_mock_domain" + assert issue.translation_key == "legacy_webrtc_provider" + assert issue.translation_placeholders == { + "legacy_integration": "mock_domain", + "builtin_integration": "go2rtc", + } + + unsub_legacy_provider() + unsub_go2rtc_provider() + + +@pytest.mark.usefixtures("mock_camera", "register_test_provider", "mock_rtsp_to_webrtc") +async def test_no_repair_issue_without_new_provider( + hass: HomeAssistant, + issue_registry: ir.IssueRegistry, +) -> None: + """Test repair issue not created if no go2rtc provider exists.""" + assert not issue_registry.async_get_issue( + "camera", "legacy_webrtc_provider_mock_domain" + ) + + +@pytest.mark.usefixtures("mock_camera", "mock_rtsp_to_webrtc") +async def test_registering_same_legacy_provider( + hass: HomeAssistant, +) -> None: + """Test registering the same legacy provider twice.""" + legacy_provider = Mock(side_effect=provide_webrtc_answer) + with pytest.raises(ValueError, match="Provider already registered"): + async_register_rtsp_to_web_rtc_provider(hass, "mock_domain", legacy_provider) + + +@pytest.mark.usefixtures("mock_hls_stream_source", "mock_camera", "mock_rtsp_to_webrtc") +async def test_get_not_supported_legacy_provider(hass: HomeAssistant) -> None: + """Test getting a not supported legacy provider.""" + camera = get_camera_from_entity_id(hass, "camera.demo_camera") + assert await async_get_supported_legacy_provider(hass, camera) is None