[{"id":3456,"web_url":"https://patchwork.libcamera.org/comment/3456/","msgid":"<20200115200321.GE977577@oden.dyn.berto.se>","date":"2020-01-15T20:03:21","subject":"Re: [libcamera-devel] [RFC 3/7] libcamera: camera_sensor: Factorize\n\tout properties","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2019-12-18 15:49:57 +0100, Jacopo Mondi wrote:\n> Factorize CameraSensor properties initialization to a dedicated virtual\n\nFactorize don't seem to match the context :-)\n\nI would here and in the subject s/Factorize/Refactor/\n\n> function that dervied classes could subclass to register sensor-specific\n> properties.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 94 ++++++++++++++++-----------\n> src/libcamera/include/camera_sensor.h | 5 +-\n> 2 files changed, 60 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index d1c9c9bcd58f..9692895d9b7b 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -77,7 +77,7 @@ CameraSensor *CameraSensorFactory::create(const MediaEntity *entity)\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> @@ -119,42 +119,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> -\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 = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> -\tif (rotationControl != controls.end())\n> -\t\tpropertyValue = rotationControl->second.def().get();\n> -\tproperties_.set(properties::Rotation, propertyValue);\n> -\n\nI Wonder if we should not refactor this into three functions, one \nvirtual initProperties() as done already but go even further and add a \ntwo non-virtual functions initDefaultLocationProperty() and \ninitDefaultRotationProperty().\n\nThe refactored initProperties() would then call the two new functions \nand allow subclasses to reuse a standard way to parse common \ncontrols/properties.\n\n> \t/* Enumerate and cache media bus codes and sizes. */\n> \tconst ImageFormats formats = subdev_->formats(0);\n> \tif (formats.isEmpty()) {\n> @@ -185,6 +149,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> + *\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> + *\n> + * Derived classes are free to override this method to register sensor specific\n> + * properties as they like, by inspecting custom controls or by adding\n> + * properties with pre-defined values, and eventually call this base class\n> + * implementation to register standard ones.\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> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index 2c09b1bdb61b..43748a6c8535 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -38,6 +38,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> @@ -55,6 +56,8 @@ public:\n> \tconst ControlList &properties() const { return properties_; }\n> \n> protected:\n> +\tControlList properties_;\n> +\n> \tfriend class CameraSensorFactory;\n> \texplicit CameraSensor(const MediaEntity *entity);\n> \tstd::string logPrefix() const;\n> @@ -65,8 +68,6 @@ private:\n> \n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> -\n> -\tControlList properties_;\n> };\n> \n> } /* namespace libcamera */\n> -- \n> 2.24.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB6086045C\n\tfor ;\n\tWed, 15 Jan 2020 21:03:23 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id n12so13714953lfe.3\n\tfor ;\n\tWed, 15 Jan 2020 12:03:23 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tf11sm11167475lfa.9.2020.01.15.12.03.22\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 15 Jan 2020 12:03:22 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=yzg0/Ab+Gv3sxqT8pcatPx+V1RlGJrzijKeqhjNeYFA=;\n\tb=MmrDAQlTiYc5FDU9E1ufTZkgRDB0ZZxodBzdOEkO+wd6xzKjn/s7lawUQiGo0Vqod4\n\t4YBjkeWrtTO18QXrHaHGn3SrgWptW7ZbdRc24/3bje7N9uBLdHAGdOFkxZd1r/WGugEG\n\tbaLOb/gTXzQv98ntgKIcYgzZ+4O2npjkUmBCm3IJj7bsntbWXmweUrFNhX++uHqn3O6C\n\t6qWXQ1etspkZLaq88kFxWOk/o275PK/QKThmuAUmCVlKx4o1KPVz5DPfDR3p8Z2tIIij\n\tTnR3zO4sBG/GOMTC43BDnfgBcG5+zmh/burIq+5pjA7Vn3p6bzqMirMSmp+NQMXjUT55\n\tLDkA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=yzg0/Ab+Gv3sxqT8pcatPx+V1RlGJrzijKeqhjNeYFA=;\n\tb=kpqoHcFHvYmO6S0J4hpgMjTypWWA28uotJkgp7BJ+1okUIuriHGehnChhCyA5ET1lX\n\tpDtwXbGkTuuhBIwKnluDYe0+bdgU7uc2PxB5gsIB3+eUYLVf4P57r/qTpSjSYzGPvUiR\n\tLzIoBYiHwHKXjZo5fDzUweK3BwKAtOlrDuIaPXCjWx5ZlJo55ni1kjKtDpg7g9ejD2A6\n\tuCIYL8geLjZhG+AD8DJ5n8Z0SlxPaxLNZLNu9pE8AGrPx9PYjH7bb33YePOR0R2BaouB\n\tvoa9ulyZe+GXcY9TIW41A/zVkFUx51OdRT7UA1AANQgtHub4abBtGxhxAXwW2JRiAFoF\n\tsdVw==","X-Gm-Message-State":"APjAAAUsgnZtBGvveILiQ4TnKZ91XZO7yYBrAsostkQJKPhdqB3/GJKi\n\tbPBwgl31NAnMex7AZNpM68P3uxglE+g=","X-Google-Smtp-Source":"APXvYqzzby6vcewzma3ChshIna0ybmnfS20jBbAWW07IyGeLN44+dXZ41yolrx33ow7HA9jSLXibSw==","X-Received":"by 2002:ac2:4945:: with SMTP id o5mr339745lfi.93.1579118603063; \n\tWed, 15 Jan 2020 12:03:23 -0800 (PST)","Date":"Wed, 15 Jan 2020 21:03:21 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200115200321.GE977577@oden.dyn.berto.se>","References":"<20191218145001.22283-1-jacopo@jmondi.org>\n\t<20191218145001.22283-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191218145001.22283-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [RFC 3/7] libcamera: camera_sensor: Factorize\n\tout properties","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":"Wed, 15 Jan 2020 20:03:23 -0000"}},{"id":3545,"web_url":"https://patchwork.libcamera.org/comment/3545/","msgid":"<20200120155555.4nkuvkfemtludjha@uno.localdomain>","date":"2020-01-20T15:55:55","subject":"Re: [libcamera-devel] [RFC 3/7] libcamera: camera_sensor: Factorize\n\tout properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Jan 15, 2020 at 09:03:21PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your patch.\n>\n> On 2019-12-18 15:49:57 +0100, Jacopo Mondi wrote:\n> > Factorize CameraSensor properties initialization to a dedicated virtual\n>\n> Factorize don't seem to match the context :-)\n>\n> I would here and in the subject s/Factorize/Refactor/\n>\n> > function that dervied classes could subclass to register sensor-specific\n> > properties.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera_sensor.cpp | 94 ++++++++++++++++-----------\n> > src/libcamera/include/camera_sensor.h | 5 +-\n> > 2 files changed, 60 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index d1c9c9bcd58f..9692895d9b7b 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -77,7 +77,7 @@ CameraSensor *CameraSensorFactory::create(const MediaEntity *entity)\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> > @@ -119,42 +119,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> > -\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 = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > -\tif (rotationControl != controls.end())\n> > -\t\tpropertyValue = rotationControl->second.def().get();\n> > -\tproperties_.set(properties::Rotation, propertyValue);\n> > -\n>\n> I Wonder if we should not refactor this into three functions, one\n> virtual initProperties() as done already but go even further and add a\n> two non-virtual functions initDefaultLocationProperty() and\n> initDefaultRotationProperty().\n>\n> The refactored initProperties() would then call the two new functions\n> and allow subclasses to reuse a standard way to parse common\n> controls/properties.\n>\n\nWould yousplit this up to the point of having one helper per property ?\n\nI'm concerned about code reuse as well, and my idea was that\nsub-classes would call the base class CameraSensor::initProperties()\nas documented....\n\n> > \t/* Enumerate and cache media bus codes and sizes. */\n> > \tconst ImageFormats formats = subdev_->formats(0);\n> > \tif (formats.isEmpty()) {\n> > @@ -185,6 +149,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> > + *\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> > + *\n> > + * Derived classes are free to override this method to register sensor specific\n> > + * properties as they like, by inspecting custom controls or by adding\n> > + * properties with pre-defined values, and eventually call this base class\n> > + * implementation to register standard ones.\n\n... here to register the default ones. Would you really split it to one per\nproperty ? I would rather instrument the base class initProperty to\nregister all default ones, except the ones already overridden by the\nderived class.\n\nThanks\n j\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> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > index 2c09b1bdb61b..43748a6c8535 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -38,6 +38,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> > @@ -55,6 +56,8 @@ public:\n> > \tconst ControlList &properties() const { return properties_; }\n> >\n> > protected:\n> > +\tControlList properties_;\n> > +\n> > \tfriend class CameraSensorFactory;\n> > \texplicit CameraSensor(const MediaEntity *entity);\n> > \tstd::string logPrefix() const;\n> > @@ -65,8 +68,6 @@ private:\n> >\n> > \tstd::vector mbusCodes_;\n> > \tstd::vector sizes_;\n> > -\n> > -\tControlList properties_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > --\n> > 2.24.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 84BDB60804\n\tfor ;\n\tMon, 20 Jan 2020 16:53:24 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 0CA70C0013;\n\tMon, 20 Jan 2020 15:53:23 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Mon, 20 Jan 2020 16:55:55 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200120155555.4nkuvkfemtludjha@uno.localdomain>","References":"<20191218145001.22283-1-jacopo@jmondi.org>\n\t<20191218145001.22283-4-jacopo@jmondi.org>\n\t<20200115200321.GE977577@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"c2fonj5inle72hzj\"","Content-Disposition":"inline","In-Reply-To":"<20200115200321.GE977577@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [RFC 3/7] libcamera: camera_sensor: Factorize\n\tout properties","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":"Mon, 20 Jan 2020 15:53:24 -0000"}},{"id":3561,"web_url":"https://patchwork.libcamera.org/comment/3561/","msgid":"<20200122143517.GH1124294@oden.dyn.berto.se>","date":"2020-01-22T14:35:17","subject":"Re: [libcamera-devel] [RFC 3/7] libcamera: camera_sensor: Factorize\n\tout properties","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-01-20 16:55:55 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Jan 15, 2020 at 09:03:21PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your patch.\n> >\n> > On 2019-12-18 15:49:57 +0100, Jacopo Mondi wrote:\n> > > Factorize CameraSensor properties initialization to a dedicated virtual\n> >\n> > Factorize don't seem to match the context :-)\n> >\n> > I would here and in the subject s/Factorize/Refactor/\n> >\n> > > function that dervied classes could subclass to register sensor-specific\n> > > properties.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 94 ++++++++++++++++-----------\n> > > src/libcamera/include/camera_sensor.h | 5 +-\n> > > 2 files changed, 60 insertions(+), 39 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index d1c9c9bcd58f..9692895d9b7b 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -77,7 +77,7 @@ CameraSensor *CameraSensorFactory::create(const MediaEntity *entity)\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> > > @@ -119,42 +119,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> > > -\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 = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > > -\tif (rotationControl != controls.end())\n> > > -\t\tpropertyValue = rotationControl->second.def().get();\n> > > -\tproperties_.set(properties::Rotation, propertyValue);\n> > > -\n> >\n> > I Wonder if we should not refactor this into three functions, one\n> > virtual initProperties() as done already but go even further and add a\n> > two non-virtual functions initDefaultLocationProperty() and\n> > initDefaultRotationProperty().\n> >\n> > The refactored initProperties() would then call the two new functions\n> > and allow subclasses to reuse a standard way to parse common\n> > controls/properties.\n> >\n> \n> Would yousplit this up to the point of having one helper per property ?\n\nMaybe or perhaps group them so maybe location and rotation could be \nparsed by the same helper.\n\n> \n> I'm concerned about code reuse as well, and my idea was that\n> sub-classes would call the base class CameraSensor::initProperties()\n> as documented....\n> \n> > > \t/* Enumerate and cache media bus codes and sizes. */\n> > > \tconst ImageFormats formats = subdev_->formats(0);\n> > > \tif (formats.isEmpty()) {\n> > > @@ -185,6 +149,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> > > + *\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> > > + *\n> > > + * Derived classes are free to override this method to register sensor specific\n> > > + * properties as they like, by inspecting custom controls or by adding\n> > > + * properties with pre-defined values, and eventually call this base class\n> > > + * implementation to register standard ones.\n> \n> ... here to register the default ones. Would you really split it to one per\n> property ? I would rather instrument the base class initProperty to\n> register all default ones, except the ones already overridden by the\n> derived class.\n\nI'm not a big fan of the design where the subclass shall overload the \nbase and then call the base function it overloads. For me it's confusing \nto get a good overview of the logic and it can create ambiguity in \nimplementations. Different subclasses can call the base implementation \nfirst, last or somewhere in-between which can limit what later can be \nadded to the base. Sometimes the design is unavoidable but is this such \na case?\n\n> \n> Thanks\n> j\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> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > index 2c09b1bdb61b..43748a6c8535 100644\n> > > --- a/src/libcamera/include/camera_sensor.h\n> > > +++ b/src/libcamera/include/camera_sensor.h\n> > > @@ -38,6 +38,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> > > @@ -55,6 +56,8 @@ public:\n> > > \tconst ControlList &properties() const { return properties_; }\n> > >\n> > > protected:\n> > > +\tControlList properties_;\n> > > +\n> > > \tfriend class CameraSensorFactory;\n> > > \texplicit CameraSensor(const MediaEntity *entity);\n> > > \tstd::string logPrefix() const;\n> > > @@ -65,8 +68,6 @@ private:\n> > >\n> > > \tstd::vector mbusCodes_;\n> > > \tstd::vector sizes_;\n> > > -\n> > > -\tControlList properties_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > --\n> > > 2.24.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF1CB607F3\n\tfor ;\n\tWed, 22 Jan 2020 15:35:20 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id n25so5528376lfl.0\n\tfor ;\n\tWed, 22 Jan 2020 06:35:20 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tz205sm3908188lfa.52.2020.01.22.06.35.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 22 Jan 2020 06:35:18 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=z0iZKNrfiZQ31kVkUasPec+Cyyyy+5ucd4fM1Pvd8G8=;\n\tb=PjDfY9p9Fj3B2kCB8Zsq5mhBEzKzujC9SxgE0t+OfBCLhqx8kHBDp3LdaGYseZbaNt\n\tnmcZZEuClr0OsDVwC7w60EYNSoxDzuDdKqTePysfZzRcaQbzVjrFWe3KmwzMZmO5ILJd\n\tz3qVLMj/Ar5UUCKuzSd16yIcA9kn7+2Ztm3IOxG/mxqI2o50KhiUka8vIYdtM1cufpRi\n\tnfuOhye0imw6Nur1mnW1YKycqA1cF50H0T5TCFmfyUpcWjFqrC3InlzL5s7Xv0R+VjMi\n\tyLFKx+MaApx5VYYYjqa+n72TUaKLYoDH6xGSKkRNFKS3rIvsf6PShWpzbSKPIjSR6cHa\n\t/cig==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=z0iZKNrfiZQ31kVkUasPec+Cyyyy+5ucd4fM1Pvd8G8=;\n\tb=Fh7/yQrmztmnqDPx4wMw64lsVeSFAOFMuqQ7eeyMfM+WqYvlbb9yVY2Cw8OE0+gyVE\n\tgizhM7QFeNyvuLbkPVzbgq2U/bmPckTJh/WJx4hCANrrSQLT3qeLf0/DMbVOFqBepyOX\n\tNCcICc+X2NcxhSWkNQXnNmNsh6ma6cM182xpPt7R3/WBg7v4FvKV26Lif+zYqf3hR2d/\n\tHYEuF28MAOiBLn7NtzK0yVWvBzfH6f+MgZCCrJKuM1W4eHycv+Rn2upaS6J9jCAX7ebl\n\tUGRi2L8ZNtVYdJvVJZark8LogpuZ10DLkxg7SbEVf2B8uIBh68B4+tm5RqwZoF1wZltE\n\te+rQ==","X-Gm-Message-State":"APjAAAXUwUSKPXJfbsHI8cl0rO8qXn8pYsEjfbS4TJ/NkbxLoYjBDJEo\n\te23KFfwx16lf2+Q4OoIGKnrTIBXBAcU=","X-Google-Smtp-Source":"APXvYqxXIK91jOnUfZFh+awrlj+NtW0Wl3B/hcVN5WFyK+Ubj8vLvLYrHSWgPvZHV4386pMHreKtHg==","X-Received":"by 2002:a19:ca0e:: with SMTP id\n\ta14mr1972628lfg.186.1579703719621; \n\tWed, 22 Jan 2020 06:35:19 -0800 (PST)","Date":"Wed, 22 Jan 2020 15:35:17 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200122143517.GH1124294@oden.dyn.berto.se>","References":"<20191218145001.22283-1-jacopo@jmondi.org>\n\t<20191218145001.22283-4-jacopo@jmondi.org>\n\t<20200115200321.GE977577@oden.dyn.berto.se>\n\t<20200120155555.4nkuvkfemtludjha@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200120155555.4nkuvkfemtludjha@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC 3/7] libcamera: camera_sensor: Factorize\n\tout properties","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":"Wed, 22 Jan 2020 14:35:20 -0000"}}]