Add camera proxy #12006

PhracturedBlue · 2018-01-28T18:29:31Z

Description:

As per discussion in #11577, this is an implementation of a camera proxy. Its primary purpose is to reduce the bandwidth utilization of streaming images to the frontend.

Note that this does not implement dynamic sizing as requested by the frontend as that part of #11577 was more controversial. I may try to implement that capability in a separate patch in the future.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for `configuration.yaml` (if applicable):

camera:
  - platform: foscam
    ip: xx.xx.xx.xx
  - platform: proxy
    entity_id: camera.proxy
    max_stream_width: 360
    max_image_width: 480
    stream_quality: 60
    image_refresh_rate: 10

Checklist:

The code change is tested and works locally.

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

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

pvizeli · 2018-01-30T09:37:47Z

Maybe render is a better name?

PhracturedBlue · 2018-01-30T14:24:59Z

I don't really think that give the proper connotation. How about 'transform' or 'munge'? While the current code can only do resizing/quality adjustments, I was planning to add stuff like flipping and rotating for the cases where the camera doesn't support those features.

dgomes

Hi! Good work submit this component 👍

I'm not an official reviewer, but an interested party in this component :)

Just giving you some feedback to get your PR approved ASAP:

You should check voluptuous documentation, default parameters would greatly improve this component.

dgomes · 2018-02-01T21:31:09Z

homeassistant/components/camera/proxy.py

+    # Read in HTTP headers:
+    stream = req.content
+    # multipart/x-mixed-replace; boundary=--frameboundary
+    __mimetype, options = cgi.parse_header(req.headers['content-type'])


Why the double underscore ?

Double underscore is used to mangle the attribute names of a class to avoid conflicts of attribute names between classes. That see this as the case.

Just leave let it be a "_" (don't care)

dgomes · 2018-02-01T21:33:34Z

homeassistant/components/camera/proxy.py

+    __mimetype, options = cgi.parse_header(req.headers['content-type'])
+    boundary = options.get('boundary').encode('utf-8')
+    if not boundary:
+        raise Exception("Can't find content-type")


I would advise a _LOGGER.error() message before the raise. So it can become easier to debug

dgomes · 2018-02-01T21:36:42Z

homeassistant/components/camera/proxy.py

+    old_size = len(image)
+    if old_width <= new_width:
+        if opts.quality is None:
+            _LOGGER.debug("Image is smaller than requested width")


according to your code, it can also be the same size...

dgomes · 2018-02-01T21:37:46Z

homeassistant/components/camera/proxy.py

+    newimage = imgbuf.getvalue()
+    if not opts.force_resize and len(newimage) >= old_size:
+        _LOGGER.debug("Using original image(%d bytes) "
+                      "because resized image (%d bytes) is larger",


or equal...

dgomes · 2018-02-01T21:40:16Z

homeassistant/components/camera/proxy.py

+        self._proxied_camera = config.get(CONF_ENTITY_ID)
+        self._name = config.get(CONF_NAME)
+        if not self._name:
+            self._name = "{} - {}".format(DEFAULT_BASENAME,


This makes no sense... just pass along this string through the default parameter of the vol.Optional() method.

I do not believe that voluptuous can evaluate a default parameter based on another parameter. The point here is that the camera name needs to include the original proxied name unless the user overrode it

OK

Still, some room for improvement:

self._name = config.get(CONF_NAME) or "{} - {}".format(DEFAULT_BASENAME, self._proxied_camera)

4 lines can be resumed in this short one as long as config.get(CONF_NAME) is None by default

dgomes · 2018-02-01T21:41:15Z

homeassistant/components/camera/proxy.py

+        self.force_resize = force_resize
+
+    def __bool__(self):
+        """Bool evaution rules."""


dgomes · 2018-02-01T21:42:50Z

homeassistant/components/camera/proxy.py

+        self._image_refresh_rate = config.get(CONF_IMAGE_REFRESH_RATE)
+        self._cache_images = (
+            True
+            if (config.get(CONF_IMAGE_REFRESH_RATE)


why does CONF_IMAGE_REFRESH_RATE trigger cache ?

Because the frontend makes a request at an arbitrary rate so you must cache the results when the refresh rate is specified (strictly speaking only when slower than the frontend rate, but there is no way to know what that is)

Multilines if statements are unreadable. Also, in this case you just want True or False, so just convert the if-condition to a bool.

PhracturedBlue · 2018-02-02T02:44:24Z

As far as further default values in voluptous, I don't see any places where it will help. The only defualt parameter that would make any sense would be image quality, but I want to distinguish between a user specifying a value vs not, and using a default value would mean I just need a different set of logic to get the same result.

balloob · 2018-02-28T20:18:21Z

homeassistant/components/camera/proxy.py

+    """Set up the Proxy camera platform."""
+    async_add_devices([ProxyCamera(hass, config)])
+
+    return True


Platforms have no return value

balloob · 2018-02-28T20:18:46Z

homeassistant/components/camera/proxy.py

+
+
+@asyncio.coroutine
+def async_setup_platform(hass, config, async_add_devices, discovery_info=None):


We've switched to async/await syntax so you can drop @asyncio.coroutine and make it async def async_setup_platform(…)

balloob · 2018-02-28T20:20:55Z

homeassistant/components/camera/proxy.py

+
+
+@asyncio.coroutine
+def _read_frame(req):


We already have extract_image_from_jpeg

That function is synchronous. I don't know how to feed async data to it. Is there another way to feed asynchronous data to that function? Also, I don't think this function is suitable for my needs. It reads data by chunks, and if I understand it correctly, it will throw away unused data. I want to capture every image in an mjpeg into an image without dropping any.

balloob · 2018-02-28T20:21:53Z

homeassistant/components/camera/proxy.py

+    if not opts:
+        return image
+
+    quality = opts.quality if opts.quality else DEFAULT_QUALITY


quality = opts.quality or DEFAULT_QUALITY

balloob · 2018-02-28T20:34:39Z

homeassistant/components/camera/proxy.py

+
+    def __bool__(self):
+        """Bool evalution rules."""
+        return True if self.max_width or self.quality else False


return bool(self.max_width or self.quality)

balloob · 2018-02-28T20:35:12Z

homeassistant/components/camera/proxy.py

+    return newimage
+
+
+class ImageOpts():


You can optionally use the attrs package which is installed as part of Home Assistant core

Unless you are going to mandate it, I'd rather not. As far as I can tell, it won't reduce any code for my use case, and would make the code harder to read.

… fixes from code review

* Add camera proxy * Fix additional tox linting issues * Trivial cleanup * update to new async/await methods rather than decorators. Other minor fixes from code review

homeassistant added cla-signed new-platform platform: camera.proxy labels Jan 28, 2018

PhracturedBlue force-pushed the camera2 branch from f2588c1 to 45a8914 Compare January 28, 2018 18:31

PhracturedBlue requested a review from andrey-git as a code owner January 28, 2018 18:31

homeassistant added the cla-signed label Jan 28, 2018

dgomes requested changes Feb 1, 2018

View reviewed changes

PhracturedBlue force-pushed the camera2 branch from 7387138 to cc17067 Compare February 2, 2018 14:01

homeassistant added the cla-signed label Feb 2, 2018

balloob reviewed Feb 28, 2018

View reviewed changes

PhracturedBlue added 4 commits March 1, 2018 05:35

Add camera proxy

87c0ab2

Fix additional tox linting issues

5c07a5f

Trivial cleanup

e46159b

update to new async/await methods rather than decorators. Other minor…

1fc5328

… fixes from code review

PhracturedBlue force-pushed the camera2 branch from cc17067 to 1fc5328 Compare March 2, 2018 01:47

homeassistant added the cla-signed label Mar 2, 2018

balloob approved these changes Mar 9, 2018

View reviewed changes

balloob added this to the 0.65 milestone Mar 9, 2018

balloob merged commit c5228cb into home-assistant:dev Mar 9, 2018

PhracturedBlue mentioned this pull request Mar 9, 2018

Add Camera Proxy documentation home-assistant/home-assistant.io#4863

Merged

2 tasks

balloob pushed a commit that referenced this pull request Mar 9, 2018

Add camera proxy (#12006)

b2210f4

* Add camera proxy * Fix additional tox linting issues * Trivial cleanup * update to new async/await methods rather than decorators. Other minor fixes from code review

balloob mentioned this pull request Mar 9, 2018

0.65 #12995

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost added integration: proxy and removed platform: camera.proxy labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add camera proxy #12006

Add camera proxy #12006

PhracturedBlue commented Jan 28, 2018

pvizeli commented Jan 30, 2018

PhracturedBlue commented Jan 30, 2018

dgomes left a comment •

edited

dgomes Feb 1, 2018

dgomes Feb 1, 2018

dgomes Feb 1, 2018

dgomes Feb 1, 2018

dgomes Feb 1, 2018

PhracturedBlue Feb 2, 2018

dgomes Feb 2, 2018

dgomes Feb 1, 2018

dgomes Feb 1, 2018 •

edited

PhracturedBlue Feb 2, 2018

balloob Feb 28, 2018 •

edited

PhracturedBlue commented Feb 2, 2018

balloob Feb 28, 2018

balloob Feb 28, 2018

balloob Feb 28, 2018

PhracturedBlue Mar 2, 2018 •

edited

balloob Feb 28, 2018

balloob Feb 28, 2018

balloob Feb 28, 2018

PhracturedBlue Mar 2, 2018



		@asyncio.coroutine
		def async_setup_platform(hass, config, async_add_devices, discovery_info=None):

Add camera proxy #12006

Add camera proxy #12006

Conversation

PhracturedBlue commented Jan 28, 2018

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

pvizeli commented Jan 30, 2018

PhracturedBlue commented Jan 30, 2018

dgomes left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgomes Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob Feb 28, 2018 • edited

Choose a reason for hiding this comment

PhracturedBlue commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhracturedBlue Mar 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Example entry for `configuration.yaml` (if applicable):

dgomes left a comment •

edited

dgomes Feb 1, 2018 •

edited

balloob Feb 28, 2018 •

edited

PhracturedBlue Mar 2, 2018 •

edited