feat: add WebRTC live stream support for Netatmo Indoor Camera Advance (NPC) #548
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideAdds WebRTC live stream support for the Netatmo Indoor Camera Advance by introducing WebRTC-specific camera mixins and data structures, refactoring shared camera functionality into a base mixin, wiring device classes to HLS/WebRTC protocols, and extending tests and fixtures for live snapshots and WebRTC offer/terminate flows. Sequence diagram for WebRTC start and stop stream flowsequenceDiagram
actor Client
participant NPC as NPC_WebRTCCameraMixin
participant Home as Home
participant Auth as Auth
participant NetatmoAPI as Netatmo_API
Client->>NPC: async_start_stream(session_id, sdp_offer)
NPC->>Home: get home
Home->>Auth: async_post_api_request(WEBRTC_OFFER_ENDPOINT, params)
Auth->>NetatmoAPI: POST /api/webrtc/offer
NetatmoAPI-->>Auth: JSON response
Auth-->>Home: Response
Home-->>NPC: Response
NPC->>NPC: _parse_webrtc_response_body(json_resp)
NPC->>NPC: validate session_id, extract tag_id, sdpAnswer
NPC-->>Client: WebRTCAnswer(WebRTCStream(session_id, tag_id), sdp)
Client->>NPC: async_stop_stream(stream)
NPC->>Home: get home
Home->>Auth: async_post_api_request(WEBRTC_TERMINATE_ENDPOINT, params)
Auth->>NetatmoAPI: POST /api/webrtc/terminate
NetatmoAPI-->>Auth: JSON response
Auth-->>Home: Response
Home-->>NPC: Response
NPC->>NPC: _parse_webrtc_response_body(json_resp)
NPC-->>Client: None
Class diagram for updated camera mixins and WebRTC data structuresclassDiagram
direction TB
class EntityBase
class Home {
+str entity_id
+Auth auth
}
class Auth {
+async_get_image(base_url, endpoint) bytes
+async_post_api_request(endpoint, params) Response
}
class CameraMixinBase {
<<mixin>>
-Home home
-ModuleT module
-str~None local_url
-str~None vpn_url
-bool~None is_local
-int~None alim_status
-DeviceType device_type
-bool _force_vpn_url
+camera_url() str~None
+async_get_live_snapshot() bytes~None
+async_update_camera_urls() None
-_async_check_url(url str) str~None
}
class HLSCameraMixin {
<<mixin>>
}
class WebRTCCameraMixin {
<<mixin>>
-bool _force_vpn_url
+async_start_stream(session_id str, sdp_offer str) WebRTCAnswer
+async_stop_stream(stream WebRTCStream) None
-_parse_webrtc_response_body(json_resp dict~str,Any~) dict~str,Any~
}
class Camera {
}
class FloodlightMixin {
<<mixin>>
}
class NACamera {
}
class NPC {
}
class NOC {
}
class NDB {
}
class WebRTCStream {
<<dataclass>>
+str session_id
+str tag_id
}
class WebRTCAnswer {
<<dataclass>>
+WebRTCStream stream
+str sdp
}
class ApiError {
}
%% Inheritance and mixin relationships
CameraMixinBase --|> EntityBase
Camera --|> CameraMixinBase
HLSCameraMixin --|> CameraMixinBase
WebRTCCameraMixin --|> CameraMixinBase
FloodlightMixin --|> EntityBase
NACamera --|> Camera
NACamera --|> HLSCameraMixin
NPC --|> Camera
NPC --|> WebRTCCameraMixin
NOC --|> Camera
NOC --|> HLSCameraMixin
NOC --|> FloodlightMixin
NDB --|> Camera
NDB --|> WebRTCCameraMixin
WebRTCCameraMixin --> Home
Home --> Auth
WebRTCCameraMixin --> WebRTCAnswer
WebRTCAnswer --> WebRTCStream
WebRTCCameraMixin ..> ApiError
CameraMixinBase ..> ApiError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
WebRTCCameraMixin._parse_webrtc_response_body, it may be helpful to include the endpoint or operation context in the raisedApiErrormessages so that callers (and logs) can distinguish which WebRTC call failed when debugging. - The
HLSCameraMixincurrently doesn’t add any behavior beyondCameraMixinBase; if it’s intended only for future extension, consider at least documenting its purpose or moving the shared HLS-specific logic into it now to make the separation between HLS and WebRTC clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `WebRTCCameraMixin._parse_webrtc_response_body`, it may be helpful to include the endpoint or operation context in the raised `ApiError` messages so that callers (and logs) can distinguish which WebRTC call failed when debugging.
- The `HLSCameraMixin` currently doesn’t add any behavior beyond `CameraMixinBase`; if it’s intended only for future extension, consider at least documenting its purpose or moving the shared HLS-specific logic into it now to make the separation between HLS and WebRTC clearer.
## Individual Comments
### Comment 1
<location> `tests/test_camera.py:39` </location>
<code_context>
assert module.device_type == DeviceType.NPC
assert module.is_local
- assert module.local_url == "http://192.168.0.123/678460a0d47e5618699fb31169e2b47d"
+ assert module.local_url is None
person_id = "91827374-7e04-5298-83ad-a0cb8372dff1"
assert person_id in module.home.persons
</code_context>
<issue_to_address>
**suggestion (testing):** Add targeted tests for `camera_url` and `_force_vpn_url` behavior, especially for WebRTC cameras.
This assertion now indirectly exercises `_force_vpn_url` for WebRTC cameras by requiring `local_url is None`. To make the new `CameraMixinBase.camera_url` and `_force_vpn_url` behavior explicit and resilient to future refactors, please add focused tests that:
- Cover `camera_url` preferring `local_url` over `vpn_url` for HLS cameras (and the fallback when one is missing).
- Cover WebRTC cameras (`NPC`/`NDB`) where `async_update_camera_urls` always clears `local_url` and `camera_url` resolves only to the VPN URL.
- Optionally, for WebRTC cameras, assert that `async_get_live_snapshot` uses only the VPN URL.
This will document the intended HLS vs WebRTC URL resolution and guard against regressions.
Suggested implementation:
```python
async def test_async_camera_NACamera(async_home):
await module.async_update_camera_urls()
# WebRTC NPC camera should always resolve camera_url via VPN URL
assert module.device_type == DeviceType.NPC
assert module.is_local
# async_update_camera_urls() should always clear local_url for WebRTC cameras
assert module.local_url is None
assert module.vpn_url is not None
# camera_url must resolve to vpn_url when local_url is not available
assert module.camera_url == module.vpn_url
# _force_vpn_url should be set for WebRTC cameras to document intent
assert module._force_vpn_url is True
person_id = "91827374-7e04-5298-83ad-a0cb8372dff1"
assert person_id in module.home.persons
person = module.home.persons[person_id]
```
To fully implement your review comment, additional tests should be added elsewhere in `tests/test_camera.py`:
1. **HLS cameras (`CameraMixinBase.camera_url` resolution)**
Add a new test (or parametrized tests) using an HLS camera fixture (e.g. NOC/NACamera HLS variant, depending on your existing fixtures) that:
- Sets up `module.local_url` and `module.vpn_url` and runs `await module.async_update_camera_urls()` if required.
- Asserts that `camera_url` prefers `local_url` when both `local_url` and `vpn_url` are available.
- Asserts that when `local_url` is missing/`None` but `vpn_url` is set, `camera_url == vpn_url`.
- Asserts that `_force_vpn_url` remains `False` (or the default) for HLS cameras.
2. **WebRTC cameras (`async_get_live_snapshot` using VPN URL only)**
If you have mocking in place for HTTP calls (e.g. via `MockResponse` or a requests/aiohttp mock), add a WebRTC-focused test such as:
- Arrange a WebRTC camera (`DeviceType.NPC`/`DeviceType.NDB`) with `local_url` cleared and `vpn_url` set after `async_update_camera_urls`.
- Monkeypatch or assert via the mock that `async_get_live_snapshot` performs the request against `vpn_url` and never uses `local_url`.
- This test should explicitly assert that the constructed snapshot URL starts with `module.vpn_url` and/or that the mocked call is made to `vpn_url` only.
You will need to adapt fixture names (e.g. the HLS camera fixture) and any mocking utilities to match the rest of your existing test suite.
</issue_to_address>
### Comment 2
<location> `tests/test_camera.py:205-207` </location>
<code_context>
+ ("webrtc_terminate_no_session.json", pytest.raises(ApiError)),
+ ],
+)
+async def test_async_webrtc_stream_stop(async_home, response_fixture, exception):
+ """Test starting a WebRTC stream."""
+ module_id = "12:34:56:00:f1:63"
+ assert module_id in async_home.modules
</code_context>
<issue_to_address>
**nitpick (typo):** Docstring for `test_async_webrtc_stream_stop` should describe stopping, not starting, the stream.
The docstring still says "Test starting a WebRTC stream," while this test covers stopping/terminating it. Please update the wording to reflect that it tests stopping the stream to avoid confusion when reading the tests.
```suggestion
async def test_async_webrtc_stream_stop(async_home, response_fixture, exception):
"""Test stopping a WebRTC stream."""
module_id = "12:34:56:00:f1:63"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
taltenbach
force-pushed
the
feature/npc_stream_support
branch
from
6618c17 to
bf36053
Compare
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new mixin hierarchy introduces a diamond around
CameraMixinBase(Cameraalready inherits it whileHLSCameraMixin/WebRTCCameraMixinalso subclass it and are then mixed intoCamera), which works today but is fragile; consider removingCameraMixinBasefromCameraand letting only the protocol-specific mixins provide the camera behavior to simplify the MRO. HLSCameraMixinis currently an empty subclass ofCameraMixinBasebut is inserted beforeCamerain the MRO; if you later add an__init__or overrides there, ensure it callssuper()correctly or consider documenting that expectation now to avoid subtle initialization bugs.- In
CameraMixinBase.async_update_camera_urls, setting_force_vpn_urlonly clearslocal_urlbut leavesis_localunchanged; if any callers useis_localas a signal that the local URL is usable, you may want to explicitly normalize or document this behavior to avoid inconsistent state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new mixin hierarchy introduces a diamond around `CameraMixinBase` (`Camera` already inherits it while `HLSCameraMixin`/`WebRTCCameraMixin` also subclass it and are then mixed into `Camera`), which works today but is fragile; consider removing `CameraMixinBase` from `Camera` and letting only the protocol-specific mixins provide the camera behavior to simplify the MRO.
- `HLSCameraMixin` is currently an empty subclass of `CameraMixinBase` but is inserted before `Camera` in the MRO; if you later add an `__init__` or overrides there, ensure it calls `super()` correctly or consider documenting that expectation now to avoid subtle initialization bugs.
- In `CameraMixinBase.async_update_camera_urls`, setting `_force_vpn_url` only clears `local_url` but leaves `is_local` unchanged; if any callers use `is_local` as a signal that the local URL is usable, you may want to explicitly normalize or document this behavior to avoid inconsistent state.
## Individual Comments
### Comment 1
<location> `src/pyatmo/modules/module.py:497-507` </location>
<code_context>
self.alim_status: int | None = None
self.device_type: DeviceType
+ self._force_vpn_url = False
+
+ @property
+ def camera_url(self) -> str | None:
+ """Return the camera URL, if available.
+
+ Depending on the camera streaming protocol, this URL can be used to retrieve:
+ - The live snapshot (HLS and WebRTC).
+ - The live stream (HLS only).
+ """
+ return self.local_url or self.vpn_url
+
async def async_get_live_snapshot(self) -> bytes | None:
</code_context>
<issue_to_address>
**suggestion:** Consider making `camera_url` respect `_force_vpn_url` directly rather than via side effects on `local_url`.
Currently `_force_vpn_url` only takes effect via `async_update_camera_urls` setting `local_url` to `None`, so `camera_url` assumes `local_url` is already sanitized and is sensitive to call order. It would be safer for `camera_url` to apply the flag directly, e.g. `return self.vpn_url if self._force_vpn_url else (self.local_url or self.vpn_url)`, so callers aren’t relying on `async_update_camera_urls` having been run first.
```suggestion
self._force_vpn_url = False
@property
def camera_url(self) -> str | None:
"""Return the camera URL, if available.
Depending on the camera streaming protocol, this URL can be used to retrieve:
- The live snapshot (HLS and WebRTC).
- The live stream (HLS only).
"""
if self._force_vpn_url:
return self.vpn_url
return self.local_url or self.vpn_url
```
</issue_to_address>
### Comment 2
<location> `tests/test_camera.py:206` </location>
<code_context>
+ ("webrtc_offer_unreachable.json", pytest.raises(ApiError)),
+ ],
+)
+async def test_async_webrtc_stream_start(async_home, response_fixture, exception):
+ """Test starting a WebRTC stream."""
+ module_id = "12:34:56:00:f1:63"
</code_context>
<issue_to_address>
**suggestion (testing):** Extend WebRTC start-stream tests to cover session ID mismatch and malformed response bodies
The current parametrized test only covers the happy path and a non-OK status, but not the other branches in `_parse_webrtc_response_body` or the session ID validation in `async_start_stream`. Please add cases where the response has `status: "ok"` but a mismatched `session_id`, and several malformed payloads (e.g., missing `body`, `status`, or `error.code` / `error.message`) and assert they all raise `ApiError`. You can stub `async_post_api_request` with inline dicts to keep these tests focused on error handling and fully cover the WebRTC answer parsing and validation logic.
Suggested implementation:
```python
async with await anyio.open_file(
f"fixtures/{response_fixture}",
encoding="utf-8",
) as json_file:
response = json.loads(await json_file.read())
@pytest.mark.parametrize(
("api_response", "exception"),
[
# status: ok but mismatched session_id should fail validation
(
{
"status": "ok",
"body": {
"session_id": "different-session-id",
"sdp_answer": "v=0\r\n...",
},
},
pytest.raises(ApiError),
),
# Completely empty / malformed payload
({}, pytest.raises(ApiError)),
# Missing body
({"status": "ok"}, pytest.raises(ApiError)),
# status: error without error details
({"status": "error"}, pytest.raises(ApiError)),
# status: error with empty error object
({"status": "error", "error": {}}, pytest.raises(ApiError)),
# status: error with only code
({"status": "error", "error": {"code": 42}}, pytest.raises(ApiError)),
# status: error with only message
({"status": "error", "error": {"message": "Boom"}}, pytest.raises(ApiError)),
],
)
async def test_async_webrtc_stream_start_invalid_payloads(
async_home, api_response, exception, monkeypatch
):
"""Test WebRTC stream start with malformed / invalid response bodies."""
module_id = "12:34:56:00:f1:63"
assert module_id in async_home.modules
module = async_home.modules[module_id]
assert module.device_type == DeviceType.NPC
async def _fake_async_post_api_request(*args, **kwargs): # type: ignore[unused-arg]
return api_response
# Monkeypatch the API call used by async_start_stream to return our test payloads.
# Adjust the attribute path if async_post_api_request lives on a client object.
monkeypatch.setattr(
module,
"async_post_api_request",
_fake_async_post_api_request,
raising=False,
)
# All parametrized cases should raise ApiError
with exception:
await module.async_start_stream()
```
Depending on the actual implementation, you may need to adjust:
1. The monkeypatch target: if `async_start_stream` calls something like `module._client.async_post_api_request(...)` instead of `module.async_post_api_request(...)`, change the `monkeypatch.setattr` call to target the correct object (e.g. `monkeypatch.setattr(module._client, "async_post_api_request", _fake_async_post_api_request)`).
2. The method name invoked at the end of the test: if the public API is `async_home.async_webrtc_stream_start(module_id=module_id, ...)` or similar instead of `module.async_start_stream()`, update the `await module.async_start_stream()` call accordingly, and pass any required parameters (e.g. offer SDP).
3. If `_parse_webrtc_response_body` requires additional fields (e.g. `type`, `headers`), extend the inline `api_response` dicts with minimal dummy values so that the code reaches the intended branches before failing on the specific missing/mismatched fields.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+497
to
+507
| self._force_vpn_url = False | ||
|
|
||
| @property | ||
| def camera_url(self) -> str | None: | ||
| """Return the camera URL, if available. | ||
| Depending on the camera streaming protocol, this URL can be used to retrieve: | ||
| - The live snapshot (HLS and WebRTC). | ||
| - The live stream (HLS only). | ||
| """ | ||
| return self.local_url or self.vpn_url |
Contributor
There was a problem hiding this comment.
suggestion: Consider making camera_url respect _force_vpn_url directly rather than via side effects on local_url.
Currently _force_vpn_url only takes effect via async_update_camera_urls setting local_url to None, so camera_url assumes local_url is already sanitized and is sensitive to call order. It would be safer for camera_url to apply the flag directly, e.g. return self.vpn_url if self._force_vpn_url else (self.local_url or self.vpn_url), so callers aren’t relying on async_update_camera_urls having been run first.
Suggested change
| self._force_vpn_url = False | |
| @property | |
| def camera_url(self) -> str | None: | |
| """Return the camera URL, if available. | |
| Depending on the camera streaming protocol, this URL can be used to retrieve: | |
| - The live snapshot (HLS and WebRTC). | |
| - The live stream (HLS only). | |
| """ | |
| return self.local_url or self.vpn_url | |
| self._force_vpn_url = False | |
| @property | |
| def camera_url(self) -> str | None: | |
| """Return the camera URL, if available. | |
| Depending on the camera streaming protocol, this URL can be used to retrieve: | |
| - The live snapshot (HLS and WebRTC). | |
| - The live stream (HLS only). | |
| """ | |
| if self._force_vpn_url: | |
| return self.vpn_url | |
| return self.local_url or self.vpn_url |
| ("webrtc_offer_unreachable.json", pytest.raises(ApiError)), | ||
| ], | ||
| ) | ||
| async def test_async_webrtc_stream_start(async_home, response_fixture, exception): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend WebRTC start-stream tests to cover session ID mismatch and malformed response bodies
The current parametrized test only covers the happy path and a non-OK status, but not the other branches in _parse_webrtc_response_body or the session ID validation in async_start_stream. Please add cases where the response has status: "ok" but a mismatched session_id, and several malformed payloads (e.g., missing body, status, or error.code / error.message) and assert they all raise ApiError. You can stub async_post_api_request with inline dicts to keep these tests focused on error handling and fully cover the WebRTC answer parsing and validation logic.
Suggested implementation:
async with await anyio.open_file(
f"fixtures/{response_fixture}",
encoding="utf-8",
) as json_file:
response = json.loads(await json_file.read())
@pytest.mark.parametrize(
("api_response", "exception"),
[
# status: ok but mismatched session_id should fail validation
(
{
"status": "ok",
"body": {
"session_id": "different-session-id",
"sdp_answer": "v=0\r\n...",
},
},
pytest.raises(ApiError),
),
# Completely empty / malformed payload
({}, pytest.raises(ApiError)),
# Missing body
({"status": "ok"}, pytest.raises(ApiError)),
# status: error without error details
({"status": "error"}, pytest.raises(ApiError)),
# status: error with empty error object
({"status": "error", "error": {}}, pytest.raises(ApiError)),
# status: error with only code
({"status": "error", "error": {"code": 42}}, pytest.raises(ApiError)),
# status: error with only message
({"status": "error", "error": {"message": "Boom"}}, pytest.raises(ApiError)),
],
)
async def test_async_webrtc_stream_start_invalid_payloads(
async_home, api_response, exception, monkeypatch
):
"""Test WebRTC stream start with malformed / invalid response bodies."""
module_id = "12:34:56:00:f1:63"
assert module_id in async_home.modules
module = async_home.modules[module_id]
assert module.device_type == DeviceType.NPC
async def _fake_async_post_api_request(*args, **kwargs): # type: ignore[unused-arg]
return api_response
# Monkeypatch the API call used by async_start_stream to return our test payloads.
# Adjust the attribute path if async_post_api_request lives on a client object.
monkeypatch.setattr(
module,
"async_post_api_request",
_fake_async_post_api_request,
raising=False,
)
# All parametrized cases should raise ApiError
with exception:
await module.async_start_stream()Depending on the actual implementation, you may need to adjust:
- The monkeypatch target: if
async_start_streamcalls something likemodule._client.async_post_api_request(...)instead ofmodule.async_post_api_request(...), change themonkeypatch.setattrcall to target the correct object (e.g.monkeypatch.setattr(module._client, "async_post_api_request", _fake_async_post_api_request)). - The method name invoked at the end of the test: if the public API is
async_home.async_webrtc_stream_start(module_id=module_id, ...)or similar instead ofmodule.async_start_stream(), update theawait module.async_start_stream()call accordingly, and pass any required parameters (e.g. offer SDP). - If
_parse_webrtc_response_bodyrequires additional fields (e.g.type,headers), extend the inlineapi_responsedicts with minimal dummy values so that the code reaches the intended branches before failing on the specific missing/mismatched fields.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
The public cloud API provided by Netatmo now allows access to the live stream of the Netatmo Indoor Camera Advance (NPC). Unlike the other Netatmo cameras currently supported by
pyatmo, which use HLS, the NPC provides access to its live stream via WebRTC.To access a WebRTC live stream from a third-party application, the application must be reviewed and approved by Netatmo for privacy reasons. Unlike previous cameras, this requirement also applies when accessing your own cameras through your personal developer account. To test these changes, I used a privileged account provided by Netatmo specifically for this development.
Netatmo will reach out to the Open Home Foundation to grant access for Home Assistant. However, before this can happen, Netatmo needs to perform a small change to the NPC’s internal firmware, which means this process will unfortunately take some time.
In the meantime, I am working on adding live stream support for the NPC camera in Home Assistant, so that the feature will be ready to merge once Netatmo releases the required update. Implementing this support requires several changes in
pyatmo, which is the purpose of this PR.Note: The Netatmo Smart Video Doorbell (NDB) already uses WebRTC, but access to its live stream is not available via the public API. Therefore, while the NPC is not Netatmo’s first WebRTC-based camera, it is the first one to allow live stream access for third-party applications.
Main changes
This PR mainly:
WebRTCCameraMixinto implement WebRTC-specific functionality. Logic shared between WebRTC and HLS cameras is moved to a newCameraMixinBase.Documentation
Summary by Sourcery
Add WebRTC streaming support and API integration for Netatmo Indoor Camera Advance and other WebRTC-capable cameras while unifying shared camera behavior.
New Features:
Enhancements:
Tests: