[{"id":14549,"web_url":"https://patchwork.libcamera.org/comment/14549/","msgid":"<20210115142452.wjfee4ic3hwaarwg@uno.localdomain>","date":"2021-01-15T14:24:52","subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Thu, Jan 14, 2021 at 07:40:34PM +0900, Paul Elder wrote:\n> In ChromeOS the camera make and model is saved in\n> /var/cache/camera/camera.prop. Load and save these values at\n> construction time, if available.\n>\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>  src/android/camera_device.cpp | 30 ++++++++++++++++++++++++++++++\n>  src/android/camera_device.h   |  5 +++++\n>  2 files changed, 35 insertions(+)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index d2a8e876..ed47c7cd 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -18,6 +18,7 @@\n>  #include <libcamera/formats.h>\n>  #include <libcamera/property_ids.h>\n>\n> +#include \"libcamera/internal/file.h\"\n>  #include \"libcamera/internal/formats.h\"\n>  #include \"libcamera/internal/log.h\"\n>  #include \"libcamera/internal/utils.h\"\n> @@ -344,6 +345,35 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer\n>  \t *  streamConfiguration.\n>  \t */\n>  \tmaxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */\n> +\n> +\tcameraMake_ = \"libcamera\";\n> +\tcameraModel_ = \"cameraModel\";\n\nYou know, library-wide support for configuration files should land\nsomewhen soon. I would rather hardcode the above values with a \\todo\nentry.\n\nOr does this make any test un-happy ?\n\n> +\n> +\tFile prop(\"/var/cache/camera/camera.prop\");\n> +\tif (!prop.open(File::ReadOnly))\n> +\t\treturn;\n> +\n> +\tsize_t fileSize = prop.size();\n> +\tif (fileSize <= 0)\n> +\t\treturn;\n> +\n> +\tstd::vector<uint8_t> buf(fileSize);\n> +\tif (prop.read(buf) < 0)\n> +\t\treturn;\n> +\n> +\tstd::string fileContents(buf.begin(), buf.end());\n> +\tfor (const auto &line : utils::split(fileContents, \"\\n\")) {\n> +\t\toff_t delimPos = line.find(\"=\");\n> +\t\tif (delimPos == std::string::npos)\n> +\t\t\tcontinue;\n> +\t\tstd::string key = line.substr(0, delimPos);\n> +\t\tstd::string val = line.substr(delimPos + 1, line.size());\n> +\n> +\t\tif (!key.compare(\"ro.product.model\"))\n> +\t\t\tcameraModel_ = val;\n> +\t\tif (!key.compare(\"ro.product.manufacturer\"))\n> +\t\t\tcameraMake_ = val;\n> +\t}\n>  }\n>\n>  CameraDevice::~CameraDevice()\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 0874c80f..a285d0a1 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -56,6 +56,8 @@ public:\n>  \t\treturn config_.get();\n>  \t}\n>\n> +\tconst std::string &cameraMake() const { return cameraMake_; }\n> +\tconst std::string &cameraModel() const { return cameraModel_; }\n>  \tint facing() const { return facing_; }\n>  \tint orientation() const { return orientation_; }\n>  \tunsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }\n> @@ -127,6 +129,9 @@ private:\n>  \tstd::map<int, libcamera::PixelFormat> formatsMap_;\n>  \tstd::vector<CameraStream> streams_;\n>\n> +\tstd::string cameraMake_;\n> +\tstd::string cameraModel_;\n> +\n>  \tint facing_;\n>  \tint orientation_;\n>\n> --\n> 2.27.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":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 05FBEC3383\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Jan 2021 14:24:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A5AB680EA;\n\tFri, 15 Jan 2021 15:24:35 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB8AC680D1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Jan 2021 15:24:34 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 54115240009;\n\tFri, 15 Jan 2021 14:24:34 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 15 Jan 2021 15:24:52 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20210115142452.wjfee4ic3hwaarwg@uno.localdomain>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210114104035.302968-6-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14557,"web_url":"https://patchwork.libcamera.org/comment/14557/","msgid":"<YAHlKknx6IYSKGBc@pendragon.ideasonboard.com>","date":"2021-01-15T18:55:38","subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jan 15, 2021 at 03:24:52PM +0100, Jacopo Mondi wrote:\n> On Thu, Jan 14, 2021 at 07:40:34PM +0900, Paul Elder wrote:\n> > In ChromeOS the camera make and model is saved in\n> > /var/cache/camera/camera.prop. Load and save these values at\n> > construction time, if available.\n> >\n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > ---\n> >  src/android/camera_device.cpp | 30 ++++++++++++++++++++++++++++++\n> >  src/android/camera_device.h   |  5 +++++\n> >  2 files changed, 35 insertions(+)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index d2a8e876..ed47c7cd 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -18,6 +18,7 @@\n> >  #include <libcamera/formats.h>\n> >  #include <libcamera/property_ids.h>\n> >\n> > +#include \"libcamera/internal/file.h\"\n> >  #include \"libcamera/internal/formats.h\"\n> >  #include \"libcamera/internal/log.h\"\n> >  #include \"libcamera/internal/utils.h\"\n> > @@ -344,6 +345,35 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer\n> >  \t *  streamConfiguration.\n> >  \t */\n> >  \tmaxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */\n> > +\n> > +\tcameraMake_ = \"libcamera\";\n> > +\tcameraModel_ = \"cameraModel\";\n> \n> You know, library-wide support for configuration files should land\n> somewhen soon. I would rather hardcode the above values with a \\todo\n> entry.\n> \n> Or does this make any test un-happy ?\n\nChrome OS expects the value to be taken from\n/var/cache/camera/camera.prop, so I think it makes sense to do so on\nthat platform.\n\nFor Android we need something else, possibly a custom configuration\nfile, or an Android-specific file or API. I'd record this in a todo.\n\n> > +\n> > +\tFile prop(\"/var/cache/camera/camera.prop\");\n> > +\tif (!prop.open(File::ReadOnly))\n> > +\t\treturn;\n> > +\n> > +\tsize_t fileSize = prop.size();\n> > +\tif (fileSize <= 0)\n> > +\t\treturn;\n> > +\n> > +\tstd::vector<uint8_t> buf(fileSize);\n> > +\tif (prop.read(buf) < 0)\n> > +\t\treturn;\n> > +\n> > +\tstd::string fileContents(buf.begin(), buf.end());\n> > +\tfor (const auto &line : utils::split(fileContents, \"\\n\")) {\n\nThis is a bit inefficient, as utils::split() makes a copy of\nfileContents, which itself makes a copy of buf. The configuration file\nisn't expected to be large so it may be OK, even if it's not great.\nAnother option would be to use std::ifstream() and std::getline(), which\nshould be fairly simple.\n\n> > +\t\toff_t delimPos = line.find(\"=\");\n> > +\t\tif (delimPos == std::string::npos)\n> > +\t\t\tcontinue;\n> > +\t\tstd::string key = line.substr(0, delimPos);\n> > +\t\tstd::string val = line.substr(delimPos + 1, line.size());\n\nYou can drop the second argument to substr().\n\n> > +\n> > +\t\tif (!key.compare(\"ro.product.model\"))\n> > +\t\t\tcameraModel_ = val;\n> > +\t\tif (!key.compare(\"ro.product.manufacturer\"))\n> > +\t\t\tcameraMake_ = val;\n> > +\t}\n\nShould we move all this to a separate private function to avoid\ncluttering the constructor ?\n\n> >  }\n> >\n> >  CameraDevice::~CameraDevice()\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 0874c80f..a285d0a1 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -56,6 +56,8 @@ public:\n> >  \t\treturn config_.get();\n> >  \t}\n> >\n> > +\tconst std::string &cameraMake() const { return cameraMake_; }\n> > +\tconst std::string &cameraModel() const { return cameraModel_; }\n> >  \tint facing() const { return facing_; }\n> >  \tint orientation() const { return orientation_; }\n> >  \tunsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }\n> > @@ -127,6 +129,9 @@ private:\n> >  \tstd::map<int, libcamera::PixelFormat> formatsMap_;\n> >  \tstd::vector<CameraStream> streams_;\n> >\n> > +\tstd::string cameraMake_;\n> > +\tstd::string cameraModel_;\n> > +\n> >  \tint facing_;\n> >  \tint orientation_;\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7DC9CC3383\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Jan 2021 18:55:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DE09680FA;\n\tFri, 15 Jan 2021 19:55:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19EE0680EF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Jan 2021 19:55:56 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D9EA58B;\n\tFri, 15 Jan 2021 19:55:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ioT2AXJ6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610736955;\n\tbh=b7TQ2fGRwhjxUxedBZN24vQuxlHMDE52RfBQ3SjakEE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ioT2AXJ64pKVeX2OkHY+LJzW+8fyvqKS32DZCwWXy15w9nXp31CqQ5LzSi7a8dfas\n\tIzWcZxkoURbeobMjFUJqMuzDIUJYku9ofQuMx0rSKTjoVAEkWI8fwuqvLSBwNwkrSc\n\tvZun0Q/FeQR/IjJpanYal+6zPojHnlycWgZDVGOE=","Date":"Fri, 15 Jan 2021 20:55:38 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<YAHlKknx6IYSKGBc@pendragon.ideasonboard.com>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-6-paul.elder@ideasonboard.com>\n\t<20210115142452.wjfee4ic3hwaarwg@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210115142452.wjfee4ic3hwaarwg@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14565,"web_url":"https://patchwork.libcamera.org/comment/14565/","msgid":"<20210118092123.2tvskpqj5zkdqgc3@uno.localdomain>","date":"2021-01-18T09:21:23","subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jan 15, 2021 at 08:55:38PM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n>\n> Thank you for the patch.\n>\n> On Fri, Jan 15, 2021 at 03:24:52PM +0100, Jacopo Mondi wrote:\n> > On Thu, Jan 14, 2021 at 07:40:34PM +0900, Paul Elder wrote:\n> > > In ChromeOS the camera make and model is saved in\n> > > /var/cache/camera/camera.prop. Load and save these values at\n> > > construction time, if available.\n> > >\n> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > ---\n> > >  src/android/camera_device.cpp | 30 ++++++++++++++++++++++++++++++\n> > >  src/android/camera_device.h   |  5 +++++\n> > >  2 files changed, 35 insertions(+)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index d2a8e876..ed47c7cd 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -18,6 +18,7 @@\n> > >  #include <libcamera/formats.h>\n> > >  #include <libcamera/property_ids.h>\n> > >\n> > > +#include \"libcamera/internal/file.h\"\n> > >  #include \"libcamera/internal/formats.h\"\n> > >  #include \"libcamera/internal/log.h\"\n> > >  #include \"libcamera/internal/utils.h\"\n> > > @@ -344,6 +345,35 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer\n> > >  \t *  streamConfiguration.\n> > >  \t */\n> > >  \tmaxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */\n> > > +\n> > > +\tcameraMake_ = \"libcamera\";\n> > > +\tcameraModel_ = \"cameraModel\";\n> >\n> > You know, library-wide support for configuration files should land\n> > somewhen soon. I would rather hardcode the above values with a \\todo\n> > entry.\n> >\n> > Or does this make any test un-happy ?\n>\n> Chrome OS expects the value to be taken from\n> /var/cache/camera/camera.prop, so I think it makes sense to do so on\n> that platform.\n\nI'm not debating this, but this patch accesses the file, while I think\nthe library-wide configuration file helper should be used.\n\n>\n> For Android we need something else, possibly a custom configuration\n> file, or an Android-specific file or API. I'd record this in a todo.\n>\n> > > +\n> > > +\tFile prop(\"/var/cache/camera/camera.prop\");\n> > > +\tif (!prop.open(File::ReadOnly))\n> > > +\t\treturn;\n> > > +\n> > > +\tsize_t fileSize = prop.size();\n> > > +\tif (fileSize <= 0)\n> > > +\t\treturn;\n> > > +\n> > > +\tstd::vector<uint8_t> buf(fileSize);\n> > > +\tif (prop.read(buf) < 0)\n> > > +\t\treturn;\n> > > +\n> > > +\tstd::string fileContents(buf.begin(), buf.end());\n> > > +\tfor (const auto &line : utils::split(fileContents, \"\\n\")) {\n>\n> This is a bit inefficient, as utils::split() makes a copy of\n> fileContents, which itself makes a copy of buf. The configuration file\n> isn't expected to be large so it may be OK, even if it's not great.\n> Another option would be to use std::ifstream() and std::getline(), which\n> should be fairly simple.\n>\n> > > +\t\toff_t delimPos = line.find(\"=\");\n> > > +\t\tif (delimPos == std::string::npos)\n> > > +\t\t\tcontinue;\n> > > +\t\tstd::string key = line.substr(0, delimPos);\n> > > +\t\tstd::string val = line.substr(delimPos + 1, line.size());\n>\n> You can drop the second argument to substr().\n>\n> > > +\n> > > +\t\tif (!key.compare(\"ro.product.model\"))\n> > > +\t\t\tcameraModel_ = val;\n> > > +\t\tif (!key.compare(\"ro.product.manufacturer\"))\n> > > +\t\t\tcameraMake_ = val;\n> > > +\t}\n>\n> Should we move all this to a separate private function to avoid\n> cluttering the constructor ?\n>\n> > >  }\n> > >\n> > >  CameraDevice::~CameraDevice()\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 0874c80f..a285d0a1 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -56,6 +56,8 @@ public:\n> > >  \t\treturn config_.get();\n> > >  \t}\n> > >\n> > > +\tconst std::string &cameraMake() const { return cameraMake_; }\n> > > +\tconst std::string &cameraModel() const { return cameraModel_; }\n> > >  \tint facing() const { return facing_; }\n> > >  \tint orientation() const { return orientation_; }\n> > >  \tunsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }\n> > > @@ -127,6 +129,9 @@ private:\n> > >  \tstd::map<int, libcamera::PixelFormat> formatsMap_;\n> > >  \tstd::vector<CameraStream> streams_;\n> > >\n> > > +\tstd::string cameraMake_;\n> > > +\tstd::string cameraModel_;\n> > > +\n> > >  \tint facing_;\n> > >  \tint orientation_;\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 09539BD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Jan 2021 09:21:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 922236810C;\n\tMon, 18 Jan 2021 10:21:05 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A75960314\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Jan 2021 10:21:04 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id C94B2240010;\n\tMon, 18 Jan 2021 09:21:03 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 18 Jan 2021 10:21:23 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210118092123.2tvskpqj5zkdqgc3@uno.localdomain>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-6-paul.elder@ideasonboard.com>\n\t<20210115142452.wjfee4ic3hwaarwg@uno.localdomain>\n\t<YAHlKknx6IYSKGBc@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<YAHlKknx6IYSKGBc@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14567,"web_url":"https://patchwork.libcamera.org/comment/14567/","msgid":"<YAVUyvW6RtXqqtGu@pendragon.ideasonboard.com>","date":"2021-01-18T09:28:42","subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jan 18, 2021 at 10:21:23AM +0100, Jacopo Mondi wrote:\n> On Fri, Jan 15, 2021 at 08:55:38PM +0200, Laurent Pinchart wrote:\n> > On Fri, Jan 15, 2021 at 03:24:52PM +0100, Jacopo Mondi wrote:\n> > > On Thu, Jan 14, 2021 at 07:40:34PM +0900, Paul Elder wrote:\n> > > > In ChromeOS the camera make and model is saved in\n> > > > /var/cache/camera/camera.prop. Load and save these values at\n> > > > construction time, if available.\n> > > >\n> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > > ---\n> > > >  src/android/camera_device.cpp | 30 ++++++++++++++++++++++++++++++\n> > > >  src/android/camera_device.h   |  5 +++++\n> > > >  2 files changed, 35 insertions(+)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index d2a8e876..ed47c7cd 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -18,6 +18,7 @@\n> > > >  #include <libcamera/formats.h>\n> > > >  #include <libcamera/property_ids.h>\n> > > >\n> > > > +#include \"libcamera/internal/file.h\"\n> > > >  #include \"libcamera/internal/formats.h\"\n> > > >  #include \"libcamera/internal/log.h\"\n> > > >  #include \"libcamera/internal/utils.h\"\n> > > > @@ -344,6 +345,35 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer\n> > > >  \t *  streamConfiguration.\n> > > >  \t */\n> > > >  \tmaxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */\n> > > > +\n> > > > +\tcameraMake_ = \"libcamera\";\n> > > > +\tcameraModel_ = \"cameraModel\";\n> > >\n> > > You know, library-wide support for configuration files should land\n> > > somewhen soon. I would rather hardcode the above values with a \\todo\n> > > entry.\n> > >\n> > > Or does this make any test un-happy ?\n> >\n> > Chrome OS expects the value to be taken from\n> > /var/cache/camera/camera.prop, so I think it makes sense to do so on\n> > that platform.\n> \n> I'm not debating this, but this patch accesses the file, while I think\n> the library-wide configuration file helper should be used.\n\nBut we'll use JSON for our configuration files, while this file is in a\nformat specific to Chrome OS :-S\n\n> > For Android we need something else, possibly a custom configuration\n> > file, or an Android-specific file or API. I'd record this in a todo.\n> >\n> > > > +\n> > > > +\tFile prop(\"/var/cache/camera/camera.prop\");\n> > > > +\tif (!prop.open(File::ReadOnly))\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tsize_t fileSize = prop.size();\n> > > > +\tif (fileSize <= 0)\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tstd::vector<uint8_t> buf(fileSize);\n> > > > +\tif (prop.read(buf) < 0)\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tstd::string fileContents(buf.begin(), buf.end());\n> > > > +\tfor (const auto &line : utils::split(fileContents, \"\\n\")) {\n> >\n> > This is a bit inefficient, as utils::split() makes a copy of\n> > fileContents, which itself makes a copy of buf. The configuration file\n> > isn't expected to be large so it may be OK, even if it's not great.\n> > Another option would be to use std::ifstream() and std::getline(), which\n> > should be fairly simple.\n> >\n> > > > +\t\toff_t delimPos = line.find(\"=\");\n> > > > +\t\tif (delimPos == std::string::npos)\n> > > > +\t\t\tcontinue;\n> > > > +\t\tstd::string key = line.substr(0, delimPos);\n> > > > +\t\tstd::string val = line.substr(delimPos + 1, line.size());\n> >\n> > You can drop the second argument to substr().\n> >\n> > > > +\n> > > > +\t\tif (!key.compare(\"ro.product.model\"))\n> > > > +\t\t\tcameraModel_ = val;\n> > > > +\t\tif (!key.compare(\"ro.product.manufacturer\"))\n> > > > +\t\t\tcameraMake_ = val;\n> > > > +\t}\n> >\n> > Should we move all this to a separate private function to avoid\n> > cluttering the constructor ?\n> >\n> > > >  }\n> > > >\n> > > >  CameraDevice::~CameraDevice()\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index 0874c80f..a285d0a1 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -56,6 +56,8 @@ public:\n> > > >  \t\treturn config_.get();\n> > > >  \t}\n> > > >\n> > > > +\tconst std::string &cameraMake() const { return cameraMake_; }\n> > > > +\tconst std::string &cameraModel() const { return cameraModel_; }\n> > > >  \tint facing() const { return facing_; }\n> > > >  \tint orientation() const { return orientation_; }\n> > > >  \tunsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }\n> > > > @@ -127,6 +129,9 @@ private:\n> > > >  \tstd::map<int, libcamera::PixelFormat> formatsMap_;\n> > > >  \tstd::vector<CameraStream> streams_;\n> > > >\n> > > > +\tstd::string cameraMake_;\n> > > > +\tstd::string cameraModel_;\n> > > > +\n> > > >  \tint facing_;\n> > > >  \tint orientation_;\n> > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4A5D8C0F1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Jan 2021 09:29:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CBA3668117;\n\tMon, 18 Jan 2021 10:29:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4870160314\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Jan 2021 10:28:59 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CBAFC2BB;\n\tMon, 18 Jan 2021 10:28:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"do6jXX5G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610962139;\n\tbh=/A7OwLeRFvswh8CvD6S0utk4sYDel8uSmlvzocVU7q0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=do6jXX5G1aFezcn8s3dqHGwb/t6GqkLZcOa53zPE98QSgtesnYGXYgoqbx++o9BYK\n\tz+rumXEJJ+dcJZR66j588rs7S6ad285OwAJKjnWyBYUfimEcU1UnYupnXdLXkBuymE\n\tZr5pLm7j3G51H0lSprhGyZKkwiNmwhRCVCsbhkEQ=","Date":"Mon, 18 Jan 2021 11:28:42 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YAVUyvW6RtXqqtGu@pendragon.ideasonboard.com>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-6-paul.elder@ideasonboard.com>\n\t<20210115142452.wjfee4ic3hwaarwg@uno.localdomain>\n\t<YAHlKknx6IYSKGBc@pendragon.ideasonboard.com>\n\t<20210118092123.2tvskpqj5zkdqgc3@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210118092123.2tvskpqj5zkdqgc3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14569,"web_url":"https://patchwork.libcamera.org/comment/14569/","msgid":"<20210118094323.y6y2thb4oytn4e6h@uno.localdomain>","date":"2021-01-18T09:43:23","subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jan 18, 2021 at 11:28:42AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jan 18, 2021 at 10:21:23AM +0100, Jacopo Mondi wrote:\n> > On Fri, Jan 15, 2021 at 08:55:38PM +0200, Laurent Pinchart wrote:\n> > > On Fri, Jan 15, 2021 at 03:24:52PM +0100, Jacopo Mondi wrote:\n> > > > On Thu, Jan 14, 2021 at 07:40:34PM +0900, Paul Elder wrote:\n> > > > > In ChromeOS the camera make and model is saved in\n> > > > > /var/cache/camera/camera.prop. Load and save these values at\n> > > > > construction time, if available.\n> > > > >\n> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > > > ---\n> > > > >  src/android/camera_device.cpp | 30 ++++++++++++++++++++++++++++++\n> > > > >  src/android/camera_device.h   |  5 +++++\n> > > > >  2 files changed, 35 insertions(+)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index d2a8e876..ed47c7cd 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -18,6 +18,7 @@\n> > > > >  #include <libcamera/formats.h>\n> > > > >  #include <libcamera/property_ids.h>\n> > > > >\n> > > > > +#include \"libcamera/internal/file.h\"\n> > > > >  #include \"libcamera/internal/formats.h\"\n> > > > >  #include \"libcamera/internal/log.h\"\n> > > > >  #include \"libcamera/internal/utils.h\"\n> > > > > @@ -344,6 +345,35 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer\n> > > > >  \t *  streamConfiguration.\n> > > > >  \t */\n> > > > >  \tmaxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */\n> > > > > +\n> > > > > +\tcameraMake_ = \"libcamera\";\n> > > > > +\tcameraModel_ = \"cameraModel\";\n> > > >\n> > > > You know, library-wide support for configuration files should land\n> > > > somewhen soon. I would rather hardcode the above values with a \\todo\n> > > > entry.\n> > > >\n> > > > Or does this make any test un-happy ?\n> > >\n> > > Chrome OS expects the value to be taken from\n> > > /var/cache/camera/camera.prop, so I think it makes sense to do so on\n> > > that platform.\n> >\n> > I'm not debating this, but this patch accesses the file, while I think\n> > the library-wide configuration file helper should be used.\n>\n> But we'll use JSON for our configuration files, while this file is in a\n> format specific to Chrome OS :-S\n\nDon't want to pester you with this discussion, but, what does it mean\n\"Chrome OS expects\" ? Is this a system-wide requirement ? Or is it a\nrequirement of the ChromeOS HAL implementations ?\n\nIn either cases, can't we let ChromeOS components that has this\nrequirement (outside of the HAL) maintain their dependency on said\nfile, while our HAL can encode information as it likes long as they're\nconsistent between the two files ?\n\n>\n> > > For Android we need something else, possibly a custom configuration\n> > > file, or an Android-specific file or API. I'd record this in a todo.\n> > >\n> > > > > +\n> > > > > +\tFile prop(\"/var/cache/camera/camera.prop\");\n> > > > > +\tif (!prop.open(File::ReadOnly))\n> > > > > +\t\treturn;\n> > > > > +\n> > > > > +\tsize_t fileSize = prop.size();\n> > > > > +\tif (fileSize <= 0)\n> > > > > +\t\treturn;\n> > > > > +\n> > > > > +\tstd::vector<uint8_t> buf(fileSize);\n> > > > > +\tif (prop.read(buf) < 0)\n> > > > > +\t\treturn;\n> > > > > +\n> > > > > +\tstd::string fileContents(buf.begin(), buf.end());\n> > > > > +\tfor (const auto &line : utils::split(fileContents, \"\\n\")) {\n> > >\n> > > This is a bit inefficient, as utils::split() makes a copy of\n> > > fileContents, which itself makes a copy of buf. The configuration file\n> > > isn't expected to be large so it may be OK, even if it's not great.\n> > > Another option would be to use std::ifstream() and std::getline(), which\n> > > should be fairly simple.\n> > >\n> > > > > +\t\toff_t delimPos = line.find(\"=\");\n> > > > > +\t\tif (delimPos == std::string::npos)\n> > > > > +\t\t\tcontinue;\n> > > > > +\t\tstd::string key = line.substr(0, delimPos);\n> > > > > +\t\tstd::string val = line.substr(delimPos + 1, line.size());\n> > >\n> > > You can drop the second argument to substr().\n> > >\n> > > > > +\n> > > > > +\t\tif (!key.compare(\"ro.product.model\"))\n> > > > > +\t\t\tcameraModel_ = val;\n> > > > > +\t\tif (!key.compare(\"ro.product.manufacturer\"))\n> > > > > +\t\t\tcameraMake_ = val;\n> > > > > +\t}\n> > >\n> > > Should we move all this to a separate private function to avoid\n> > > cluttering the constructor ?\n> > >\n> > > > >  }\n> > > > >\n> > > > >  CameraDevice::~CameraDevice()\n> > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > index 0874c80f..a285d0a1 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -56,6 +56,8 @@ public:\n> > > > >  \t\treturn config_.get();\n> > > > >  \t}\n> > > > >\n> > > > > +\tconst std::string &cameraMake() const { return cameraMake_; }\n> > > > > +\tconst std::string &cameraModel() const { return cameraModel_; }\n> > > > >  \tint facing() const { return facing_; }\n> > > > >  \tint orientation() const { return orientation_; }\n> > > > >  \tunsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }\n> > > > > @@ -127,6 +129,9 @@ private:\n> > > > >  \tstd::map<int, libcamera::PixelFormat> formatsMap_;\n> > > > >  \tstd::vector<CameraStream> streams_;\n> > > > >\n> > > > > +\tstd::string cameraMake_;\n> > > > > +\tstd::string cameraModel_;\n> > > > > +\n> > > > >  \tint facing_;\n> > > > >  \tint orientation_;\n> > > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 81E46C0F1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Jan 2021 09:43:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC86E68118;\n\tMon, 18 Jan 2021 10:43:06 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50D2260314\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Jan 2021 10:43:05 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 8FC0B1C0012;\n\tMon, 18 Jan 2021 09:43:04 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 18 Jan 2021 10:43:23 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210118094323.y6y2thb4oytn4e6h@uno.localdomain>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-6-paul.elder@ideasonboard.com>\n\t<20210115142452.wjfee4ic3hwaarwg@uno.localdomain>\n\t<YAHlKknx6IYSKGBc@pendragon.ideasonboard.com>\n\t<20210118092123.2tvskpqj5zkdqgc3@uno.localdomain>\n\t<YAVUyvW6RtXqqtGu@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<YAVUyvW6RtXqqtGu@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14570,"web_url":"https://patchwork.libcamera.org/comment/14570/","msgid":"<YAVaVVPy+cOXm9uo@pendragon.ideasonboard.com>","date":"2021-01-18T09:52:21","subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jan 18, 2021 at 10:43:23AM +0100, Jacopo Mondi wrote:\n> On Mon, Jan 18, 2021 at 11:28:42AM +0200, Laurent Pinchart wrote:\n> > On Mon, Jan 18, 2021 at 10:21:23AM +0100, Jacopo Mondi wrote:\n> > > On Fri, Jan 15, 2021 at 08:55:38PM +0200, Laurent Pinchart wrote:\n> > > > On Fri, Jan 15, 2021 at 03:24:52PM +0100, Jacopo Mondi wrote:\n> > > > > On Thu, Jan 14, 2021 at 07:40:34PM +0900, Paul Elder wrote:\n> > > > > > In ChromeOS the camera make and model is saved in\n> > > > > > /var/cache/camera/camera.prop. Load and save these values at\n> > > > > > construction time, if available.\n> > > > > >\n> > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > > > > ---\n> > > > > >  src/android/camera_device.cpp | 30 ++++++++++++++++++++++++++++++\n> > > > > >  src/android/camera_device.h   |  5 +++++\n> > > > > >  2 files changed, 35 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > index d2a8e876..ed47c7cd 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -18,6 +18,7 @@\n> > > > > >  #include <libcamera/formats.h>\n> > > > > >  #include <libcamera/property_ids.h>\n> > > > > >\n> > > > > > +#include \"libcamera/internal/file.h\"\n> > > > > >  #include \"libcamera/internal/formats.h\"\n> > > > > >  #include \"libcamera/internal/log.h\"\n> > > > > >  #include \"libcamera/internal/utils.h\"\n> > > > > > @@ -344,6 +345,35 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer\n> > > > > >  \t *  streamConfiguration.\n> > > > > >  \t */\n> > > > > >  \tmaxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */\n> > > > > > +\n> > > > > > +\tcameraMake_ = \"libcamera\";\n> > > > > > +\tcameraModel_ = \"cameraModel\";\n> > > > >\n> > > > > You know, library-wide support for configuration files should land\n> > > > > somewhen soon. I would rather hardcode the above values with a \\todo\n> > > > > entry.\n> > > > >\n> > > > > Or does this make any test un-happy ?\n> > > >\n> > > > Chrome OS expects the value to be taken from\n> > > > /var/cache/camera/camera.prop, so I think it makes sense to do so on\n> > > > that platform.\n> > >\n> > > I'm not debating this, but this patch accesses the file, while I think\n> > > the library-wide configuration file helper should be used.\n> >\n> > But we'll use JSON for our configuration files, while this file is in a\n> > format specific to Chrome OS :-S\n> \n> Don't want to pester you with this discussion, but, what does it mean\n> \"Chrome OS expects\" ? Is this a system-wide requirement ? Or is it a\n> requirement of the ChromeOS HAL implementations ?\n\n/var/cache/camera/camera.prop is a file created by Chrome OS, not by us.\n\n> In either cases, can't we let ChromeOS components that has this\n> requirement (outside of the HAL) maintain their dependency on said\n> file, while our HAL can encode information as it likes long as they're\n> consistent between the two files ?\n\nWon't storing the same information in two places cause more issues than\nit will solve ? If Chrome OS provides an official means, through this\nfile, to access the camera maker and model string, I think this can be\ntreated as platform integration and be supported with platform-specific\ncode. For Android we'll have to find a different method, which could be\na libcamera-specific configuration file if Android doesn't provide any\nAPI to get the same information, but I would be surprised if this was\nthe case.\n\n> > > > For Android we need something else, possibly a custom configuration\n> > > > file, or an Android-specific file or API. I'd record this in a todo.\n> > > >\n> > > > > > +\n> > > > > > +\tFile prop(\"/var/cache/camera/camera.prop\");\n> > > > > > +\tif (!prop.open(File::ReadOnly))\n> > > > > > +\t\treturn;\n> > > > > > +\n> > > > > > +\tsize_t fileSize = prop.size();\n> > > > > > +\tif (fileSize <= 0)\n> > > > > > +\t\treturn;\n> > > > > > +\n> > > > > > +\tstd::vector<uint8_t> buf(fileSize);\n> > > > > > +\tif (prop.read(buf) < 0)\n> > > > > > +\t\treturn;\n> > > > > > +\n> > > > > > +\tstd::string fileContents(buf.begin(), buf.end());\n> > > > > > +\tfor (const auto &line : utils::split(fileContents, \"\\n\")) {\n> > > >\n> > > > This is a bit inefficient, as utils::split() makes a copy of\n> > > > fileContents, which itself makes a copy of buf. The configuration file\n> > > > isn't expected to be large so it may be OK, even if it's not great.\n> > > > Another option would be to use std::ifstream() and std::getline(), which\n> > > > should be fairly simple.\n> > > >\n> > > > > > +\t\toff_t delimPos = line.find(\"=\");\n> > > > > > +\t\tif (delimPos == std::string::npos)\n> > > > > > +\t\t\tcontinue;\n> > > > > > +\t\tstd::string key = line.substr(0, delimPos);\n> > > > > > +\t\tstd::string val = line.substr(delimPos + 1, line.size());\n> > > >\n> > > > You can drop the second argument to substr().\n> > > >\n> > > > > > +\n> > > > > > +\t\tif (!key.compare(\"ro.product.model\"))\n> > > > > > +\t\t\tcameraModel_ = val;\n> > > > > > +\t\tif (!key.compare(\"ro.product.manufacturer\"))\n> > > > > > +\t\t\tcameraMake_ = val;\n> > > > > > +\t}\n> > > >\n> > > > Should we move all this to a separate private function to avoid\n> > > > cluttering the constructor ?\n> > > >\n> > > > > >  }\n> > > > > >\n> > > > > >  CameraDevice::~CameraDevice()\n> > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > > index 0874c80f..a285d0a1 100644\n> > > > > > --- a/src/android/camera_device.h\n> > > > > > +++ b/src/android/camera_device.h\n> > > > > > @@ -56,6 +56,8 @@ public:\n> > > > > >  \t\treturn config_.get();\n> > > > > >  \t}\n> > > > > >\n> > > > > > +\tconst std::string &cameraMake() const { return cameraMake_; }\n> > > > > > +\tconst std::string &cameraModel() const { return cameraModel_; }\n> > > > > >  \tint facing() const { return facing_; }\n> > > > > >  \tint orientation() const { return orientation_; }\n> > > > > >  \tunsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }\n> > > > > > @@ -127,6 +129,9 @@ private:\n> > > > > >  \tstd::map<int, libcamera::PixelFormat> formatsMap_;\n> > > > > >  \tstd::vector<CameraStream> streams_;\n> > > > > >\n> > > > > > +\tstd::string cameraMake_;\n> > > > > > +\tstd::string cameraModel_;\n> > > > > > +\n> > > > > >  \tint facing_;\n> > > > > >  \tint orientation_;\n> > > > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8EACFBD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Jan 2021 09:52:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B13A6811C;\n\tMon, 18 Jan 2021 10:52:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 892C360314\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Jan 2021 10:52:37 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3F5D2BB;\n\tMon, 18 Jan 2021 10:52:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XID1sioW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610963557;\n\tbh=8/+MnSjggNWFzofsaE+HUp+TAWunX1XDUpzp1SFa28k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XID1sioWqFYA7p7ezUal5K0ebWMnyibNMzeFZqYe3qLRspDiyZlxTBhi+zRoFDGJM\n\tEDdKjAyyDxB7xYViOs4nZ2t5YYwVSlP0f8gPf+eGtineVxLt5DNWdqOIIl2vDx9tmV\n\tna36XhtG50EsdHUKVhAg8wjxl4hWDa1daSO69aP0=","Date":"Mon, 18 Jan 2021 11:52:21 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YAVaVVPy+cOXm9uo@pendragon.ideasonboard.com>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-6-paul.elder@ideasonboard.com>\n\t<20210115142452.wjfee4ic3hwaarwg@uno.localdomain>\n\t<YAHlKknx6IYSKGBc@pendragon.ideasonboard.com>\n\t<20210118092123.2tvskpqj5zkdqgc3@uno.localdomain>\n\t<YAVUyvW6RtXqqtGu@pendragon.ideasonboard.com>\n\t<20210118094323.y6y2thb4oytn4e6h@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210118094323.y6y2thb4oytn4e6h@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 5/6] android: camera_device: Load make\n\tand model from platform settings","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]