[{"id":3639,"web_url":"https://patchwork.libcamera.org/comment/3639/","msgid":"<20200207233355.GG4726@pendragon.ideasonboard.com>","date":"2020-02-07T23:33:55","subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tBreak out properties initialization","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Feb 06, 2020 at 07:52:45PM +0100, Jacopo Mondi wrote:\n> Refactor the CameraSensor properties initialization to a dedicated virtual\n> function that derived classes could subclass to register sensor-specific\n\ns/subclass/override/\n\n> properties values.\n\nI would add, because it took me a bit of time to realize, \"The private\nproperties_ member field is made protected to support this.\".\n\n> \n> While at it, move documentation of the properties() method to match the\n> declaration order in the class definition.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 114 +++++++++++++++-----------\n> src/libcamera/include/camera_sensor.h | 5 +-\n> 2 files changed, 71 insertions(+), 48 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 06d10295a80e..08b2d2acdbfc 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -64,7 +64,7 @@ LOG_DEFINE_CATEGORY(CameraSensor);\n> * Once constructed the instance must be initialized with init().\n> */\n> CameraSensor::CameraSensor(const MediaEntity *entity)\n> -\t: entity_(entity), properties_(properties::properties)\n> +\t: properties_(properties::properties), entity_(entity)\n> {\n> \tsubdev_ = new V4L2Subdevice(entity);\n> }\n> @@ -106,45 +106,6 @@ int CameraSensor::init()\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\t/* Retrieve and store the camera sensor properties. */\n> -\tconst ControlInfoMap &controls = subdev_->controls();\n> -\tint32_t propertyValue;\n> -\n> -\t/* Camera Location: default is front location. */\n> -\tconst auto &locationControl = controls.find(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> -\tif (locationControl != controls.end()) {\n> -\t\tint32_t v4l2Location =\n> -\t\t\tlocationControl->second.def().get();\n> -\n> -\t\tswitch (v4l2Location) {\n> -\t\tdefault:\n> -\t\t\tLOG(CameraSensor, Warning)\n> -\t\t\t\t<< \"Unsupported camera location \"\n> -\t\t\t\t<< v4l2Location << \", setting to Front\";\n> -\t\t\t/* Fall-through */\n> -\t\tcase V4L2_LOCATION_FRONT:\n> -\t\t\tpropertyValue = properties::CameraLocationFront;\n> -\t\t\tbreak;\n> -\t\tcase V4L2_LOCATION_BACK:\n> -\t\t\tpropertyValue = properties::CameraLocationBack;\n> -\t\t\tbreak;\n> -\t\tcase V4L2_LOCATION_EXTERNAL:\n> -\t\t\tpropertyValue = properties::CameraLocationExternal;\n> -\t\t\tbreak;\n> -\t\t}\n> -\t} else {\n> -\t\tpropertyValue = properties::CameraLocationFront;\n> -\t}\n> -\tproperties_.set(properties::Location, propertyValue);\n> -\n> -\t/* Camera Rotation: default is 0 degrees. */\n> -\tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> -\tif (rotationControl != controls.end())\n> -\t\tpropertyValue = rotationControl->second.def().get();\n> -\telse\n> -\t\tpropertyValue = 0;\n> -\tproperties_.set(properties::Rotation, propertyValue);\n> -\n> \t/* Enumerate and cache media bus codes and sizes. */\n> \tconst ImageFormats formats = subdev_->formats(0);\n> \tif (formats.isEmpty()) {\n> @@ -175,6 +136,62 @@ int CameraSensor::init()\n> \tstd::sort(mbusCodes_.begin(), mbusCodes_.end());\n> \tstd::sort(sizes_.begin(), sizes_.end());\n> \n> +\treturn initProperties(subdev_->controls());\n> +}\n> +\n> +/**\n> + * \\brief Initialize the camera sensor properties\n> + * \\param[in] controlMap The map of control information provided by the sensor\n\nDerived classes could call controls() to get the controls, there's no\nneed to pass them to this method. We could even make the subdev_ member\nfield protected if needed.\n\n> + *\n> + * This method initializes the camera sensor properties, by inspecting the\n> + * control information reported by the sensor media entity in \\a controlMap.\n> + * For each supported standard V4L2 control reported by the sensor, a libcamera\n> + * property is created and registered in the list of properties supported by the\n> + * sensor.\n\nThat's not fully accurate, there's not necessarily a 1:1 mapping between\nV4L2 controls and libcamera properties.\n\n> + *\n> + * Derived classes are free to override this method to register sensor specific\n\ns/are free to/may/\ns/sensor specific/sensor-specific/\n\n> + * properties as they like, by inspecting custom controls or by adding\n\ns/ as they like//\n\n> + * properties with pre-defined values, and eventually call this base class\n> + * implementation to register standard ones.\n\n * Derived classes may override this method to register sensor-specific\n * properties if needed. If they do so, they shall call the base\n * initProperties() method to register standard properties.\n\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + */\n> +int CameraSensor::initProperties(const ControlInfoMap &controlMap)\n> +{\n> +\tint32_t propertyValue;\n> +\n> +\t/* Camera Location: default is front location. */\n> +\tconst auto &locationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> +\tif (locationControl != controlMap.end()) {\n> +\t\tint32_t v4l2Location =\n> +\t\t\tlocationControl->second.def().get();\n> +\t\tswitch (v4l2Location) {\n> +\t\tcase V4L2_LOCATION_EXTERNAL:\n> +\t\t\tpropertyValue = properties::CameraLocationExternal;\n> +\t\t\tbreak;\n> +\t\tcase V4L2_LOCATION_FRONT:\n> +\t\t\tpropertyValue = properties::CameraLocationFront;\n> +\t\t\tbreak;\n> +\t\tcase V4L2_LOCATION_BACK:\n> +\t\t\tpropertyValue = properties::CameraLocationBack;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tLOG(CameraSensor, Error)\n> +\t\t\t\t<< \"Unsupported camera location: \" << v4l2Location;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t} else {\n> +\t\tpropertyValue = properties::CameraLocationFront;\n> +\t}\n> +\tproperties_.set(properties::Location, propertyValue);\n> +\n> +\t/* Camera Rotation: default is 0 degrees. */\n> +\tpropertyValue = 0;\n> +\tconst auto &rotationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> +\tif (rotationControl != controlMap.end())\n> +\t\tpropertyValue = rotationControl->second.def().get();\n> +\tproperties_.set(properties::Rotation, propertyValue);\n> +\n> \treturn 0;\n> }\n> \n> @@ -340,12 +357,6 @@ int CameraSensor::getControls(ControlList *ctrls)\n> \treturn subdev_->getControls(ctrls);\n> }\n> \n> -/**\n> - * \\fn CameraSensor::properties()\n> - * \\brief Retrieve the camera sensor properties\n> - * \\return The list of camera sensor properties\n> - */\n> -\n> /**\n> * \\brief Write controls to the sensor\n> * \\param[in] ctrls The list of controls to write\n> @@ -376,6 +387,17 @@ int CameraSensor::setControls(ControlList *ctrls)\n> \treturn subdev_->setControls(ctrls);\n> }\n> \n> +/**\n> + * \\fn CameraSensor::properties()\n> + * \\brief Retrieve the camera sensor properties\n> + * \\return The list of camera sensor properties\n> + */\n> +\n> +/**\n> + * \\var CameraSensor::properties_\n> + * \\brief The list of camera sensor properties\n> + */\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index 2f4a0cc8ad3f..87d3516aaae7 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -34,6 +34,7 @@ public:\n> \tCameraSensor &operator=(const CameraSensor &) = delete;\n> \n> \tint init();\n> +\tvirtual int initProperties(const ControlInfoMap &controlMap);\n> \n> \tconst MediaEntity *entity() const { return entity_; }\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> @@ -51,6 +52,8 @@ public:\n> \tconst ControlList &properties() const { return properties_; }\n> \n> protected:\n> +\tControlList properties_;\n> +\n\nMember data should go after member functions.\n\n> \tfriend class CameraSensorFactory;\n> \texplicit CameraSensor(const MediaEntity *entity);\n> \tstd::string logPrefix() const;\n> @@ -61,8 +64,6 @@ private:\n> \n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> -\n> -\tControlList properties_;\n> };\n> \n> class CameraSensorFactory","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C85E60445\n\tfor ;\n\tSat, 8 Feb 2020 00:34:12 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C74289F3;\n\tSat, 8 Feb 2020 00:34:10 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581118451;\n\tbh=5MP7wotgupHVoFrst8C8zjxGmnmspQCAfPnlkKA0yH4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pqzLQZg5uV6hibhbaoFW5FaBhdbWPU3/xk8ucGyBqs07McXdwCxSy43x+XUxPi+cc\n\tPLFjCQjKNXus0oo61JBXFjUJA9be9FZspGrpXHL7yVEFQsnadgAppXe3mYjU9zxFoK\n\t1bqGJiUDGJjXWBUp5qRwJayQv2/HnuBZDWzBYAfs=","Date":"Sat, 8 Feb 2020 01:33:55 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200207233355.GG4726@pendragon.ideasonboard.com>","References":"<20200206185247.202233-1-jacopo@jmondi.org>\n\t<20200206185247.202233-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200206185247.202233-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tBreak out properties initialization","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 07 Feb 2020 23:34:12 -0000"}}]