Add config flow stream preview to generic camera #122563
Conversation
davet2001
changed the title
davet2001
changed the title
|
This is a bit more tricky than normal to get full pytest coverage because of the websocket connection. Would value an initial review from someone before I invest time adding the tests. |
davet2001
requested review from
allenporter and
uvjustin
and removed request for
allenporter
| entity_namespace=None, | ||
| ) | ||
|
|
||
| stream = await cam.async_create_stream() |
There was a problem hiding this comment.
Perhaps camera._async_stream_endpoint_url should be exposed for this purpose since a lot of this setup is shared? I realize this is specific to generic so nice to not polute the apis, but also all the stream APIs are fairly low level so it may be worth having fewer special interactions with them.
Alternatively, could consider moving the preview webview into camera but not sure that other integrations should reuse it... But given the deep integration with creating an entity platform and all that, it seems very much tied to camera integration details.
Did you consider a direct stream integration here rather than a camera integration?
There was a problem hiding this comment.
I just re-read how async_test_stream works and that seems to me to argue for a direct stream integration since one already exists.
- Whats the motivation for multiple ways to do that?
- Whats going to happen to the image preview? seems like its a parallel path, but can it be combined?
There was a problem hiding this comment.
Yes, I agree on the camera._async_stream_endpoint_url, exposing this would avoid some duplication.
In general my starting approach was to simulate the creation of the final camera entity as much as possible, with all of its settings, so that the user gets real information on whether that configuration will actually work. This seemed the most reliable & future proof way of doing it. So I see the preview (still and stream) equally about testing the behaviour of the entity that will be created as showing an image/stream.
There are two phases:
-
Verify that the settings are valid
This happens when the user clicks submit on the first config flow screen. -
Show a preview
Create something with those settings and show it. This happens when the websocket callback is triggered on the second screen.
The issue I found was that during 1) I'm not ready to create the preview. The settings could be wrong, in which case I need to bounce the user back to the first screen with an error next to the wrong parameter.
But via the websocket method, the notice to generate the preview only comes when we are already at step 2). If I find an error at that step, I need to bounce them back to step 1) again, but I couldn't see a way of doing that since the step 2) dialog is already showing.
So we end up with two hits to the external URLs, one for checking and one for the preview. Not a big issue in the case of the still image, but 2x the stream delay is very noticeable. I don't really like this and I'm open to ideas for a better approach. Possibly I could create the stream during the check and keep hold of it if the check was successful.
Regarding the image preview, yes you are right, I was planning to integrate that back into the same approach in a separate PR.
Side note: I did try for a while with a live preview at the bottom of the settings screen in the same way that time_date does it. But I stopped down that route because:
- the dialog became incredibly tall, so you couldn't actually see the preview without scrolling
- the external URLs were being hit and streams generated for every character typed, which felt likely to cause problems (e.g. authentication lockouts)
There might be a potential architecture improvement here, because I imagine most integrations will face similar issues, and will want to run a preview without interfering with the real hass object (also during options flow).
There was a problem hiding this comment.
The line of thought around making an entity is a good one to just do an end to end test, though I have a couple doubts. My thinking now is (1) i'm not that familiar with making fake entities we don't actually use anywhere, and implications of doing that e.g. entity may write its state as there are status updates on the stream (discussed below) and (2) we already have decided when doing validation to create the stream object directly anyway, so it may be easiest to just reuse that (though i didn't look at all the ways it can diverge based on a setting in camera that isn't just passed 1:1 to stream)
Maybe there are two things i might tease apart here:
- code reuse: i think it makes sense to have one code path for creating the stream and that code path could be reused for testing settings and making the preview
- object reuse: there is a possible performance win from reusing the same actual stream object
Perhaps we can tease these apart, and there may be an incremental path for first having reusable code, then second iteration on having reusable state.
I realize it may not be that simple so happy to keep discussing if that path isn't as simple as i'm making it seem.
There was a problem hiding this comment.
@allenporter Ok, I've progressed it quite a bit further now. The stream test and the preview are now combined, this makes it a lot faster too.
Left over streams are now auto-stopped after 10minutes
I've tidied up the rest of the code and removed several things that were unnecessary.
Since I've 'sort of' subclassed Stream anyway now to do the timeout, that could possibly open up the option for blocking async_write_ha_state.
Either way I would like your thoughts on this before going further. Thanks in advance!
There was a problem hiding this comment.
I really like this approach a lot and it seems very simple and easy to read/understand. I really like that we don't need to use the entity anymore here and can just have one way to use the stream.
Any other specific input now or review needed? It generally looks good and i wanted to give quick feedback, but happy to do the last round of review when you are ready.
(The part where it loads the platform to ensure translations are loaded is something i'll want to give a closer look at for prior art/ alternatives)
There was a problem hiding this comment.
Thanks @allenporter I'll get the tests implemented, then go for final review.
I will put the options flow on a separate PR.
There was a problem hiding this comment.
I may have missed before that the image preview works by using the camera entity, so now the image preview and stream preview take different strategies.
| await prefs.async_load() | ||
| hass.data[DATA_CAMERA_PREFS] = prefs | ||
|
|
||
| # Create a camera but don't add it to the hass object. |
There was a problem hiding this comment.
Is it a problem that stream may call async_write_ha_state on this if its not really added to hass?
There was a problem hiding this comment.
If it gets called, then I think it will write something to the hass data object. But is stream likely to do that?
Either way I don't see it causing a problem other than a data entry for something that will no longer exist in the future. I have looked at a few ways but think it could be hard to decouple this.
Possibly could do something like
self._platform_state == EntityPlatformState.REMOVED
to block the write but that is a bit of a hack. Maybe there is a better way of doing this?
Possibly subclass it and modify async_write_ha_state? Maybe there is a general case for this type of throw away temporary entity to handle previews in config/option flows?
There was a problem hiding this comment.
@allenporter I've done some experimentation, and haven't seen the stream writing _async_write_ha_state so far in my logs. I couldn't see a mechanism where this would happen other than one being added via set_update_callback(), and nothing seems to do that.
Please let me know if I'm missing something!
I think this is working ok now from the core side.
| entity_namespace=None, | ||
| ) | ||
|
|
||
| stream = await cam.async_create_stream() |
There was a problem hiding this comment.
I just re-read how async_test_stream works and that seems to me to argue for a direct stream integration since one already exists.
- Whats the motivation for multiple ways to do that?
- Whats going to happen to the image preview? seems like its a parallel path, but can it be combined?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Makes sense, gave my initial thoughts and happy to brainstorm / discuss more here. I could see this also being upleveled to an architecture discussion if it seems worthwhile to try to centralize some of the preview stuff a bit more into streamlined APIs |
davet2001
force-pushed
the
generic_camera_stream_preview_3
branch
from
ee52452 to
acbec89
Compare
| ) -> dict[str, str] | PreviewStream: | ||
| """Verify that the stream is valid before we create an entity. | ||
|
|
||
| Returns a dict with errors if any, or the stream object if valid. |
There was a problem hiding this comment.
I think it's time to switch to raising a specific exception here
| entity_namespace=None, | ||
| ) | ||
|
|
||
| stream = await cam.async_create_stream() |
There was a problem hiding this comment.
I really like this approach a lot and it seems very simple and easy to read/understand. I really like that we don't need to use the entity anymore here and can just have one way to use the stream.
Any other specific input now or review needed? It generally looks good and i wanted to give quick feedback, but happy to do the last round of review when you are ready.
(The part where it loads the platform to ensure translations are loaded is something i'll want to give a closer look at for prior art/ alternatives)
davet2001
force-pushed
the
generic_camera_stream_preview_3
branch
from
acbec89 to
67a141f
Compare
davet2001
force-pushed
the
generic_camera_stream_preview_3
branch
2 times, most recently
from
640ee52 to
ca044fd
Compare
| if not await hls_provider.part_recv(timeout=SOURCE_TIMEOUT): | ||
| hass.async_create_task(stream.stop()) | ||
| return {CONF_STREAM_SOURCE: "timeout"} | ||
| await stream.stop() | ||
| except StreamWorkerError as err: |
There was a problem hiding this comment.
I very much like the goal of giving helpful error messages and I think it is a good idea. However, I'm just wondering if its even possible for these errors to be thrown. I'm not sure it is possible for a StreamWorkerError, PermissionError, OSError etc to get raised here. My impression is none of these exceptions can be raised directly here at the moment.
|
|
||
| async def _timeout() -> None: | ||
| _LOGGER.debug("Starting preview stream with timeout %ss", timeout) | ||
| await asyncio.sleep(timeout) |
There was a problem hiding this comment.
OK, sounds good.
Regarding the delay between steps, i'm trying to say that calling stream.add_provider is idempotent and it can be called before the stream is shown to add a new idle timer if the stream already timed out because the delay between steps was too long.
I think its reasonable to not have a test verify this and trust the stream component will handle the timeout, so that sounds fine.
Yes, the test needed changing to reflect the removed WS parameters. |
|
@allenporter I think this is done now from my point of view. Not sure how catch 22s like this are solved. Any ideas? |
The labels will be removed, when both PRs are approved |
|
I have a question on your video: Can we disable the checkbox until the stream is loaded? |
|
|
||
| async def _timeout() -> None: | ||
| _LOGGER.debug("Starting preview stream with timeout %ss", timeout) | ||
| await asyncio.sleep(timeout) |
There was a problem hiding this comment.
I was still expecting this PR would remove this timeout behavior and rely on the existing stream timeout.
I thought about that, and I would prefer to do it that way but it is not at all easy. The checkbox comes from the conventional config_flow.py defined in python. Everything below the word 'Preview' comes from javascript. So I could add a checkbox there but it would have to communicate via WebSocket to tell the config flow what to do next, and if anything went wrong with making the websocket connection, it would not be possible to complete the config flow. In reality the checkbox is a bit of a workaround for not having the option of a back button in config flow. Happy to put in place another solution if you can see a good way of doing it. |
|
There is a bug in There is a "%" missing at the end so Lokalise flagged this as a syntax error when it came in an hour ago. In addition does not exist. I assume this should be instead. |
|
Hi @NoRi2909, you are probably correct, I will take a look later today. |
Proposed change
Add preview of video stream during config flow to allow user to verify stream settings when configuring.
stream_preview_2.mp4
The generic camera config flow was updated some time ago to show a preview of the still image before the config flow was complete, the aim was to help users get username/passwords correct without having to add a camera, then delete it if it didn't work. But so far that was only implemented on still images.
The real problem and most of the remaining questions that still occur on the forums are from users trying to set up cameras, and confusing rtsp settings, passwords, or simply not knowing how to find a valid stream URL for their camera.
This PR uses a custom preview element to create a view of the stream before the end of the config flow.
A corresponding PR for the frontend adds the preview element.
[ ] Todo: update pytests to 100% coverage
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: