Allow generic camera conf without still_image_url by uvjustin · Pull Request #62611 · home-assistant/core

uvjustin · 2021-12-22T17:05:36Z

Proposed change

This PR updates generic to allow getting the still image from Stream.get_image() which was added in #61918. To enable this the config schema was updated to allow CONF_STILL_IMAGE_URL to be omitted when CONF_STREAM_SOURCE is present.
GenericCamera.camera_image() was removed as it was unused and unnecessary.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
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

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
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

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.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

homeassistant/components/generic/camera.py

uvjustin · 2021-12-23T05:57:29Z

I didn't test this live yet but this should avoid the sync->async->sync problem in generic. Basically if the sync GenericCamera.camera_image gets called it should call the sync Stream.get_image directly without going through the main loop again. Just relying on a wrapper to implement camera_image using async_camera_image would cause a problem.

uvjustin · 2021-12-23T05:59:53Z

Also renamed Camera.create_stream to Camera.async_create_stream to go along with the convention that async_ prefixed methods should be called from the main loop (allowing the asyncio.Lock to work properly).

homeassistant/components/camera/__init__.py

uvjustin · 2021-12-23T23:56:01Z

@allenporter I realize this whole sync business is probably unnecessary. I think you are right that the wrappers are there just for backwards compatibility.
A camera platform only has to implement one of the camera_image/async_camera_image and at the end only async_camera_image is used. For some reason generic had camera_image defined which is unnecessary and threw me off.
If this makes sense to you we can keep Stream.get_image as async which keeps a little complexity over in stream but makes it easier to call from an async_camera_image function. We can also note that it should not be called from sync (I suppose we can rename it async_get_image as well).

uvjustin · 2021-12-24T06:11:49Z

CI seems to be broken from elsewhere again

allenporter · 2021-12-24T06:28:24Z

homeassistant/components/generic/camera.py

+        if not self._still_image_url:
+            if self.stream:
+                return await self.stream.async_get_image(width, height)
+            await self.async_create_stream()


What does this call do? It seems like it will create the stream, but not start it, is that right? It seems like the next call to async_get_image will be safe, but will just return None, unless something else starts the stream, if i understand right.

Yes, you are right, we should start the stream there too

This is a reasonably different behavior change, so i think this could also come in a follow up PR to evaluate on its own, but for now using the image if already preloaded since like a nice win.

I already pushed the relevant change in b5b26bf

Rebased to try and fix the CI problem.
I'm curious as to what you think of the stream.start() in here. It might make more sense to move that into Stream.async_get_image

How about we give the stream a 15 second timeout when we start it this way? Since the normal cards grab images ~10 seconds apart, if we don't get 2 consecutive requests then we can stop the stream. Or maybe that's too quick, as the user might switch between views but still want to come back. Maybe a 2-3 minute timeout makes sense.
The current IdleTimer/Provider framework is based on StreamOutput. I'll have to see if we can expand it to work with KeyFrameConverter. Alternatively, maybe we actually want to start up a HLS provider, since a natural use case is to use an "auto" picture entity card, and the regular picture load will help start the HLS provider before the user clicks in to the individual camera.
I don't have any numbers, but my own instance has the rough CPU power of a RPi, and I haven't seen any issues with 10+ cameras refreshing every ~30 secs or so (I use very long keyframe intervals so 2 out of 3 requests get back the old image, so only one is actually doing work).
The CPU usage of the open stream is very low (as we know from preload stream - and if we don't use a HLS provider it's even lower), so the concern would be decoding and encoding. Although we're decoding and encoding using different codecs, encoding is usually a lot more resource intensive than decoding. On this front libjpeg-turbo seems to be very fast - better than PIL which is used in some other integrations. Also, I'm not sure how jpeg scaling works, but the naive assumption is that if any cameras are rescaling to use the new width and height parameters, they'd have to do a decode/reencode as well, so the CPU load would actually be similar to this.

I like your 2-3 minute timeout since we expect the frontend to be polling fairly often.

I see what you mean about IdleTimer + StreamOutput. I can see that how Stream interacts w/ StreamOutput is very similar for this case (e.g. checking for idle timeout, shutting down, etc)..

But the side where the worker pushes data to it is different. I could see how KeyframeConverter could be a StreamOutput, then the StreamMuxer would beed to know about different types of stream outputs (some that want just keyframes pushed in, or some that want Segments) but they could also just ignore the parts they don't care about, or an optimization could be to only do segmentation when the outputs are actually listening for segments, which could come later.

I am not sure if generalizing the idle time out side or stream output side is easier, but may be worth thinking through both to decide.

On performance, thanks for talking it through for me 👍🏼

Can this go in as is?

Did you look at the most recent push? I ended up just starting up the HLS output as that at least will start the IdleTimer to time everything out. I think it can go in as is now, and we can continue the IdleTimer/StreamOutput discussion.

I see, i missed that part. Yes, that looks like it covers it fine, handling the error conditions.

homeassistant/components/camera/__init__.py

allenporter · 2021-12-24T18:05:29Z

homeassistant/components/generic/camera.py

+        if not self._still_image_url:
+            if self.stream:
+                return await self.stream.async_get_image(width, height)
+            await self.async_create_stream()


I have few cases I am thinking about here:
(1) If the stream fails and is not restarted, this will show the last image. The current ffmpeg call will get a fresh frame every time.
(2) Creating the stream without starting it will just get back None, so the fallback to ffmpeg would not longer happen
(3) Not yet sure how I feel about always starting the stream, but it would fix those issues at the cost of having the stream always on. That seems like a larger topic that may need more considerations, data, etc. (e.g. maybe we collect data about the impact of such a change to argue whether or not its ok? or maybe showing that the ffmpeg frame thing is super expensive, etc)

Another alternative that is more incremental could be to only use the stream if its created /and/ active (or only use the stream image with X minutes before it is considered stale if a stream failed) before falling back to ffmpeg. This is extra code, but preserves status quo behavior.

Curious to hear your thoughts on this. I definitely like the direction of trying to replace ffmpeg. This also makes me think it would be really cool to be able to collect stats w.r.t. stream bandwidth usage.

allenporter · 2021-12-24T23:45:21Z

Overall, i think i'm good with this given you're more familiar with how this integration is used than I am. Probably worth now also referencing a documentation PR for this new feature/behavior

allenporter · 2021-12-25T18:34:53Z

Also make sure to send a documentation PR

probot-home-assistant bot added integration: generic new-feature small-pr PRs with less than 30 lines. labels Dec 22, 2021

homeassistant added the cla-signed label Dec 22, 2021

uvjustin commented Dec 22, 2021

View reviewed changes

homeassistant/components/generic/camera.py Outdated Show resolved Hide resolved

uvjustin marked this pull request as draft December 23, 2021 03:53

uvjustin force-pushed the use-stream-get-image-generic branch from ce0abd8 to cdd7a6d Compare December 23, 2021 05:51

uvjustin force-pushed the use-stream-get-image-generic branch 2 times, most recently from e345697 to 1d6ff21 Compare December 23, 2021 16:30

uvjustin commented Dec 23, 2021

View reviewed changes

homeassistant/components/camera/__init__.py Outdated Show resolved Hide resolved

uvjustin force-pushed the use-stream-get-image-generic branch from 5d90542 to e213220 Compare December 24, 2021 05:16

allenporter reviewed Dec 24, 2021

View reviewed changes

uvjustin marked this pull request as ready for review December 24, 2021 07:21

uvjustin requested a review from hunterjm as a code owner December 24, 2021 07:21

uvjustin force-pushed the use-stream-get-image-generic branch from b5b26bf to 82239ca Compare December 24, 2021 09:45

allenporter reviewed Dec 24, 2021

View reviewed changes

uvjustin mentioned this pull request Dec 24, 2021

Use lock in Camera.create_stream #62757

Merged

22 tasks

uvjustin added 4 commits December 24, 2021 20:18

Allow generic config with no CONF_STILL_IMAGE_URL

51b24e0

Use Stream.async_get_image when no still image url

06d0410

Remove GenericCamera.camera_image

deb3c61

Add test

e6f7cd2

uvjustin force-pushed the use-stream-get-image-generic branch from 085aa31 to 4f907a6 Compare December 24, 2021 20:25

Add HLS output and start stream in Stream.async_get_image

3c9a4af

uvjustin force-pushed the use-stream-get-image-generic branch from 4f907a6 to 3c9a4af Compare December 25, 2021 17:04

allenporter approved these changes Dec 25, 2021

View reviewed changes

allenporter mentioned this pull request Dec 25, 2021

Update nest to use stream thumbnail when it exists #62653

Closed

22 tasks

uvjustin merged commit 08a3140 into home-assistant:dev Dec 26, 2021

uvjustin deleted the use-stream-get-image-generic branch December 26, 2021 07:53

uvjustin mentioned this pull request Dec 26, 2021

Update generic with no still_image_url option home-assistant/home-assistant.io#20862

Merged

8 tasks

github-actions bot locked and limited conversation to collaborators Dec 27, 2021

Uh oh!

Conversation

uvjustin commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

Uh oh!

uvjustin commented Dec 23, 2021

Uh oh!

uvjustin commented Dec 23, 2021

Uh oh!

Uh oh!

uvjustin commented Dec 23, 2021

Uh oh!

uvjustin commented Dec 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allenporter commented Dec 24, 2021

Uh oh!

allenporter commented Dec 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

uvjustin commented Dec 22, 2021 •

edited

Loading