[{"id":562,"web_url":"https://patchwork.libcamera.org/comment/562/","msgid":"<20190124185449.GD4127@bigcity.dyn.berto.se>","date":"2019-01-24T18:54:49","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add\n\tCameraData","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nNice work!\n\nOn 2019-01-24 12:30:19 +0100, Jacopo Mondi wrote:\n> Add class definition and methods to associate a Camera with specific data\n> in the pipeline_handler base class.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/include/pipeline_handler.h | 24 +++++++-\n> src/libcamera/pipeline_handler.cpp | 73 ++++++++++++++++++++++++\n> 2 files changed, 96 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index b03217d..41699a5 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -11,17 +11,39 @@\n> #include \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> class CameraManager;\n> class DeviceEnumerator;\n> \n> +class CameraData\n> +{\n> +public:\n> +\tvirtual ~CameraData() {}\n> +\n> +protected:\n> +\tCameraData() {}\n> +\n> +private:\n> +\tCameraData(const CameraData &) = delete;\n> +\tvoid operator=(const CameraData &) = delete;\n> +};\n> +\n> class PipelineHandler\n> {\n> public:\n> -\tvirtual ~PipelineHandler() { };\n> +\tvirtual ~PipelineHandler();\n> \n> \tvirtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0;\n> +\n> +protected:\n> +\tCameraData *cameraData(const Camera *camera);\n> +\tvoid setCameraData(const Camera *camera, std::unique_ptr data);\n> +\n> +private:\n> +\tstd::map> cameraData_;\n> };\n> \n> class PipelineHandlerFactory\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index c24feea..fb49fde 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -8,6 +8,8 @@\n> #include \"log.h\"\n> #include \"pipeline_handler.h\"\n> \n> +#include \n> +\n> /**\n> * \\file pipeline_handler.h\n> * \\brief Create pipelines and cameras from a set of media devices\n> @@ -26,6 +28,20 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Pipeline)\n> \n> +/**\n> + * \\class CameraData\n> + * \\brief Base class for platform-specific data associated with a camera\n> + *\n> + * The CameraData base abstract class represents platform specific-data\n> + * a pipeline handler might want to associate with a Camera to access them\n> + * at a later time.\n> + *\n> + * Pipeline handlers are expected to extend this base class with platform\n> + * specific implementation, associate instances of the derived classes\n> + * using the setCameraData() method, and access them at a later time\n> + * with cameraData().\n> + */\n> +\n> /**\n> * \\class PipelineHandler\n> * \\brief Create and manage cameras based on a set of media devices\n> @@ -66,6 +82,63 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * created, or false otherwise\n> */\n> \n> +/**\n> + * \\brief Delete the pipeline handler\n> + *\n> + * Release the cameraData_ map, causing all data there referenced to be\n> + * deleted, as they are stored as unique_ptr\n> + */\n> +PipelineHandler::~PipelineHandler()\n> +{\n> +\tcameraData_.clear();\n> +};\n> +\n> +/**\n> + * \\brief Retrieve the pipeline-specific data associated with a Camera\n> + * \\param camera The camera data is associate with\n> + *\n> + * \\return A pointer to the pipeline-specific data set with setCameraData().\n> + * The returned pointer lifetime is associated with the one of the pipeline\n> + * handler, and caller of this function shall never release it manually.\n> + */\n> +CameraData *PipelineHandler::cameraData(const Camera *camera)\n> +{\n> +\tif (!cameraData_.count(camera)) {\n> +\t\tLOG(Pipeline, Error)\n> +\t\t\t<< \"Cannot get data associated with camera \"\n> +\t\t\t<< camera->name();\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\treturn cameraData_[camera].get();\n> +}\n> +\n> +/**\n> + * \\brief Set pipeline-specific data in the camera\n> + * \\param camera The camera to associate data to\n> + * \\param data The pipeline-specific data\n> + *\n> + * This method allows pipeline handlers to associate pipeline-specific\n> + * information with \\a camera. The \\a data lifetime gets associated with\n> + * the pipeline handler one, and gets released at deletion time.\n> + *\n> + * If pipeline-specific data has already been associated with the camera by a\n> + * previous call to this method, is it replaced by \\a data and the previous data\n> + * are deleted, rendering all references to them invalid.\n> + *\n> + * The data can be retrieved by pipeline handlers using the cameraData() method.\n> + */\n> +void PipelineHandler::setCameraData(const Camera *camera,\n> +\t\t\t\t std::unique_ptr data)\n> +{\n> +\tif (cameraData_.count(camera))\n> +\t\tLOG(Pipeline, Debug)\n> +\t\t\t<< \"Replacing data associated with \"\n> +\t\t\t<< camera->name();\n> +\n> +\tcameraData_[camera] = std::move(data);\n> +}\n> +\n> /**\n> * \\class PipelineHandlerFactory\n> * \\brief Registration of PipelineHandler classes and creation of instances\n> -- \n> 2.20.1\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-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 50A4360C82\n\tfor ;\n\tThu, 24 Jan 2019 19:54:51 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id e26so5123070lfc.2\n\tfor ;\n\tThu, 24 Jan 2019 10:54:51 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ta18-v6sm1118793ljk.86.2019.01.24.10.54.49\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 24 Jan 2019 10:54:49 -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\t:user-agent; bh=2tLROZEKDSnUW1xp12wF8WQEbt3plIauuUw9lDrVWxg=;\n\tb=IDdJzO2rws0kWOYv2APxKKSrBVHt/K/DLXzMXYRjqyBWmfIodTlKk2cvEXLiX4/BSG\n\tHfzcbiM9ryR1DeIk4x8AdHPqy384aynTtJpF1pwP8KG56AjVWBJ9M1VTa91TJ6kUF9Zl\n\tchSzdGK5K2hY3jz/adRO3NeWm+6zriwE6/MhjWnlShZO4yd0VaqvANwK5JTxQ6d1jpWR\n\tBhQ21UOlBHUzok36KRvFY8X3QCyFAeqCVC1pOwrsnLWNkAMi+4jr8+KA2iIoTZwxf3Fd\n\tMAq0/QVgEFBG5I/uV9sjpHzIzLt4XyejDxDUMw6mgysv8WzYb834sT6TGxwW/+XGR665\n\t7alw==","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:user-agent;\n\tbh=2tLROZEKDSnUW1xp12wF8WQEbt3plIauuUw9lDrVWxg=;\n\tb=gPRvR/9nrz/16XImTis1mxlSe/WEZum76zvX5bCUIdjcaNa9dJGKbC+BWByf0FYsaI\n\tFivUzGZ4LMmYB0rPcMOmFVmbGNbugTJDZiS8lAYvxx7+6VSECkTzr9jsGim8uU+/yfI0\n\tWElzy74SsAm2KMb/Ur6wfnCe/OKO/J5QMhC6+/AHQc//VktHKSLXK1vqiKUraF1SUv2L\n\tfu/b3f5hKOuLfcTzwFHH6/rzGAnR4qJ4hQATHq5zDQNV0Ly87/9iypZbM83ANjw7NyUV\n\tCrOM9xFzPsac2hVV4igR0q5PWoe0QB7IBseKOYt1+juxGCfrIM7Ftd2++pMI6+H1kCBu\n\tklDg==","X-Gm-Message-State":"AJcUukfUcAEp8cBuEayyxYAhs7THENgUe2SWDVyeCQRtCWmkXLDXHAQH\n\tS+DYKZ99TX5lV1wJSKv//ZAKuVEhiuc=","X-Google-Smtp-Source":"ALg8bN4pQR5MXDGR/iUQadK8Tx9ibzEx3MVYz8lHWjGyDbuY+4dGyx6IGmU2tn8rK7aqnieDxKV6Mw==","X-Received":"by 2002:a19:6806:: with SMTP id d6mr6145649lfc.48.1548356090503; \n\tThu, 24 Jan 2019 10:54:50 -0800 (PST)","Date":"Thu, 24 Jan 2019 19:54:49 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124185449.GD4127@bigcity.dyn.berto.se>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-2-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":"<20190124113020.7203-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add\n\tCameraData","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 24 Jan 2019 18:54:51 -0000"}},{"id":597,"web_url":"https://patchwork.libcamera.org/comment/597/","msgid":"<20190125152425.GE4320@pendragon.ideasonboard.com>","date":"2019-01-25T15:24:25","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add\n\tCameraData","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, Jan 24, 2019 at 12:30:19PM +0100, Jacopo Mondi wrote:\n> Add class definition and methods to associate a Camera with specific data\n> in the pipeline_handler base class.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/pipeline_handler.h | 24 +++++++-\n> src/libcamera/pipeline_handler.cpp | 73 ++++++++++++++++++++++++\n> 2 files changed, 96 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index b03217d..41699a5 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -11,17 +11,39 @@\n> #include \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> class CameraManager;\n> class DeviceEnumerator;\n> \n> +class CameraData\n> +{\n> +public:\n> +\tvirtual ~CameraData() {}\n> +\n> +protected:\n> +\tCameraData() {}\n> +\n> +private:\n> +\tCameraData(const CameraData &) = delete;\n> +\tvoid operator=(const CameraData &) = delete;\n\nI think you meant\n\n\tCameraData &operator=(const CameraData &) = delete;\n\n> +};\n> +\n> class PipelineHandler\n> {\n> public:\n> -\tvirtual ~PipelineHandler() { };\n> +\tvirtual ~PipelineHandler();\n> \n> \tvirtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0;\n> +\n> +protected:\n> +\tCameraData *cameraData(const Camera *camera);\n> +\tvoid setCameraData(const Camera *camera, std::unique_ptr data);\n> +\n> +private:\n> +\tstd::map> cameraData_;\n\nI don't know why indexing on a const pointer seems a bit weird, but I\ndon't see why it wouldn't work.\n\n> };\n> \n> class PipelineHandlerFactory\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index c24feea..fb49fde 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -8,6 +8,8 @@\n> #include \"log.h\"\n> #include \"pipeline_handler.h\"\n> \n> +#include \n> +\n\nPlease move this above the private headers.\n\n> /**\n> * \\file pipeline_handler.h\n> * \\brief Create pipelines and cameras from a set of media devices\n> @@ -26,6 +28,20 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Pipeline)\n> \n> +/**\n> + * \\class CameraData\n> + * \\brief Base class for platform-specific data associated with a camera\n> + *\n> + * The CameraData base abstract class represents platform specific-data\n> + * a pipeline handler might want to associate with a Camera to access them\n> + * at a later time.\n\nHow about starting by explaining the usage instead of what the class\nrepresent ?\n\nThe CameraData abstract class is part of a mechanism that allows\npipeline handlers to associate pipeline-specific data to a camera for\ntheir own usage.\n\n> + *\n> + * Pipeline handlers are expected to extend this base class with platform\n> + * specific implementation, associate instances of the derived classes\n> + * using the setCameraData() method, and access them at a later time\n> + * with cameraData().\n> + */\n> +\n> /**\n> * \\class PipelineHandler\n> * \\brief Create and manage cameras based on a set of media devices\n> @@ -66,6 +82,63 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * created, or false otherwise\n> */\n> \n> +/**\n> + * \\brief Delete the pipeline handler\n\ns/Delete/Destroy/\n\nThe destructor doesn't delete, it's called by the delete operation.\n\n> + *\n> + * Release the cameraData_ map, causing all data there referenced to be\n> + * deleted, as they are stored as unique_ptr\n\ns/$/./\n\nI think you can drop that comment, it doesn't seem very necessary.\n\n> + */\n> +PipelineHandler::~PipelineHandler()\n> +{\n> +\tcameraData_.clear();\n\nThis isn't needed, the cameraData_ vector is deleted, so there's no need\nto clear it first.\n\n> +};\n> +\n> +/**\n> + * \\brief Retrieve the pipeline-specific data associated with a Camera\n> + * \\param camera The camera data is associate with\n\ns/associate/associated/\n\nor\n\n\"The camera whose data to retrieve\"\n\n> + *\n> + * \\return A pointer to the pipeline-specific data set with setCameraData().\n> + * The returned pointer lifetime is associated with the one of the pipeline\n> + * handler, and caller of this function shall never release it manually.\n\n\"The returned pointer is a borrowed reference and is guaranteed to remain\nvalid until the pipeline handler is destroyed. It shall not be deleted\nmanually by the caller.\"\n\nIt would be really nice if we could make the class friend of\nstd::unique_ptr and make the destructor private :-S\n\n> + */\n> +CameraData *PipelineHandler::cameraData(const Camera *camera)\n> +{\n> +\tif (!cameraData_.count(camera)) {\n> +\t\tLOG(Pipeline, Error)\n> +\t\t\t<< \"Cannot get data associated with camera \"\n> +\t\t\t<< camera->name();\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\treturn cameraData_[camera].get();\n> +}\n> +\n> +/**\n> + * \\brief Set pipeline-specific data in the camera\n\nMaybe s/in the/for the/ ?\n\n> + * \\param camera The camera to associate data to\n\ns/to$/with/\n\n> + * \\param data The pipeline-specific data\n> + *\n> + * This method allows pipeline handlers to associate pipeline-specific\n> + * information with \\a camera. The \\a data lifetime gets associated with\n> + * the pipeline handler one, and gets released at deletion time.\n\nI would write the second sentence as just\n\n\"Ownership of \\a data is transferred to the PipelineHandler.\"\n\n> + *\n> + * If pipeline-specific data has already been associated with the camera by a\n> + * previous call to this method, is it replaced by \\a data and the previous data\n> + * are deleted, rendering all references to them invalid.\n\nI wonder whether we should disallow this and return an error instead, as\nI don't think it's a valid use case. It would avoid potential invalid\nreferences problems caused by\n\n\tsetCameraData(camera, std::move(data_ptr));\n\tdata_ = cameraData(camera);\n\t...\n\n\tsetCameraData(camera, std::move(new_data_ptr));\n\t...\n\tdata_->foo = bar;\t/* CRASH */\n\nI would also violate the cameraData() documentation's promise that the\npointer stays valid as long as the pipeline handler exists.\n\n> + *\n> + * The data can be retrieved by pipeline handlers using the cameraData() method.\n> + */\n> +void PipelineHandler::setCameraData(const Camera *camera,\n> +\t\t\t\t std::unique_ptr data)\n> +{\n> +\tif (cameraData_.count(camera))\n\nI'd use find() instead of count().\n\n> +\t\tLOG(Pipeline, Debug)\n\nMaybe error instead of debug ?\n\n> +\t\t\t<< \"Replacing data associated with \"\n> +\t\t\t<< camera->name();\n> +\n> +\tcameraData_[camera] = std::move(data);\n> +}\n> +\n> /**\n> * \\class PipelineHandlerFactory\n> * \\brief Registration of PipelineHandler classes and creation of instances","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9026160B1B\n\tfor ;\n\tFri, 25 Jan 2019 16:24:31 +0100 (CET)","from pendragon.ideasonboard.com (unknown [109.132.30.169])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F2F5C325;\n\tFri, 25 Jan 2019 16:24:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548429867;\n\tbh=vUl6lHA7xQ6IlZmGK8RuW/pAj7NZU+FoG97s6x7KXbY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QzC9BUXEYU/OZ+L0ixH9KJbNeia+Wes2dap+loZF9Q8JsvqEW5vRw48UUFOe4Zt+8\n\tSUjWzqGZIj1mZ6PpzfdmVaeB6KmGIuoxt4VAOkqsZ2zw8XatqnuPYDNJEMiTKubcpd\n\ta+jVeeCtj7n86CHeJJNym/1mGfpKw+Q+YATgEh9M=","Date":"Fri, 25 Jan 2019 17:24:25 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125152425.GE4320@pendragon.ideasonboard.com>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190124113020.7203-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add\n\tCameraData","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 25 Jan 2019 15:24:31 -0000"}},{"id":606,"web_url":"https://patchwork.libcamera.org/comment/606/","msgid":"<20190125163434.4tf3spuvikf6ybb7@uno.localdomain>","date":"2019-01-25T16:34:34","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add\n\tCameraData","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 25, 2019 at 05:24:25PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Jan 24, 2019 at 12:30:19PM +0100, Jacopo Mondi wrote:\n> > Add class definition and methods to associate a Camera with specific data\n> > in the pipeline_handler base class.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/pipeline_handler.h | 24 +++++++-\n> > src/libcamera/pipeline_handler.cpp | 73 ++++++++++++++++++++++++\n> > 2 files changed, 96 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index b03217d..41699a5 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -11,17 +11,39 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > namespace libcamera {\n> >\n> > class CameraManager;\n> > class DeviceEnumerator;\n> >\n> > +class CameraData\n> > +{\n> > +public:\n> > +\tvirtual ~CameraData() {}\n> > +\n> > +protected:\n> > +\tCameraData() {}\n> > +\n> > +private:\n> > +\tCameraData(const CameraData &) = delete;\n> > +\tvoid operator=(const CameraData &) = delete;\n>\n> I think you meant\n>\n> \tCameraData &operator=(const CameraData &) = delete;\n>\n\nThanks, fixed\n\n> > +};\n> > +\n> > class PipelineHandler\n> > {\n> > public:\n> > -\tvirtual ~PipelineHandler() { };\n> > +\tvirtual ~PipelineHandler();\n> >\n> > \tvirtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0;\n> > +\n> > +protected:\n> > +\tCameraData *cameraData(const Camera *camera);\n> > +\tvoid setCameraData(const Camera *camera, std::unique_ptr data);\n> > +\n> > +private:\n> > +\tstd::map> cameraData_;\n>\n> I don't know why indexing on a const pointer seems a bit weird, but I\n> don't see why it wouldn't work.\n>\n\nwhy does this bother you?\n\n> > };\n> >\n> > class PipelineHandlerFactory\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index c24feea..fb49fde 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -8,6 +8,8 @@\n> > #include \"log.h\"\n> > #include \"pipeline_handler.h\"\n> >\n> > +#include \n> > +\n>\n> Please move this above the private headers.\n>\n\nIt should be dropped, it was there already.\n\n> > /**\n> > * \\file pipeline_handler.h\n> > * \\brief Create pipelines and cameras from a set of media devices\n> > @@ -26,6 +28,20 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(Pipeline)\n> >\n> > +/**\n> > + * \\class CameraData\n> > + * \\brief Base class for platform-specific data associated with a camera\n> > + *\n> > + * The CameraData base abstract class represents platform specific-data\n> > + * a pipeline handler might want to associate with a Camera to access them\n> > + * at a later time.\n>\n> How about starting by explaining the usage instead of what the class\n> represent ?\n>\n> The CameraData abstract class is part of a mechanism that allows\n> pipeline handlers to associate pipeline-specific data to a camera for\n> their own usage.\n>\n\nI don't see any more precise indication on how the class should be\nused more than in the original comment, but ok.\n\n> > + *\n> > + * Pipeline handlers are expected to extend this base class with platform\n> > + * specific implementation, associate instances of the derived classes\n> > + * using the setCameraData() method, and access them at a later time\n> > + * with cameraData().\n> > + */\n> > +\n> > /**\n> > * \\class PipelineHandler\n> > * \\brief Create and manage cameras based on a set of media devices\n> > @@ -66,6 +82,63 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * created, or false otherwise\n> > */\n> >\n> > +/**\n> > + * \\brief Delete the pipeline handler\n>\n> s/Delete/Destroy/\n>\n> The destructor doesn't delete, it's called by the delete operation.\n>\n> > + *\n> > + * Release the cameraData_ map, causing all data there referenced to be\n> > + * deleted, as they are stored as unique_ptr\n>\n> s/$/./\n>\n> I think you can drop that comment, it doesn't seem very necessary.\n>\n> > + */\n> > +PipelineHandler::~PipelineHandler()\n> > +{\n> > +\tcameraData_.clear();\n>\n> This isn't needed, the cameraData_ vector is deleted, so there's no need\n> to clear it first.\n>\n\nI've dropped it completely.\n\n\n> > +};\n> > +\n> > +/**\n> > + * \\brief Retrieve the pipeline-specific data associated with a Camera\n> > + * \\param camera The camera data is associate with\n>\n> s/associate/associated/\n>\n> or\n>\n> \"The camera whose data to retrieve\"\n>\n> > + *\n> > + * \\return A pointer to the pipeline-specific data set with setCameraData().\n> > + * The returned pointer lifetime is associated with the one of the pipeline\n> > + * handler, and caller of this function shall never release it manually.\n>\n> \"The returned pointer is a borrowed reference and is guaranteed to remain\n> valid until the pipeline handler is destroyed. It shall not be deleted\n> manually by the caller.\"\n>\n> It would be really nice if we could make the class friend of\n> std::unique_ptr and make the destructor private :-S\n>\n> > + */\n> > +CameraData *PipelineHandler::cameraData(const Camera *camera)\n> > +{\n> > +\tif (!cameraData_.count(camera)) {\n> > +\t\tLOG(Pipeline, Error)\n> > +\t\t\t<< \"Cannot get data associated with camera \"\n> > +\t\t\t<< camera->name();\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\treturn cameraData_[camera].get();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Set pipeline-specific data in the camera\n>\n> Maybe s/in the/for the/ ?\n>\n> > + * \\param camera The camera to associate data to\n>\n> s/to$/with/\n>\n> > + * \\param data The pipeline-specific data\n> > + *\n> > + * This method allows pipeline handlers to associate pipeline-specific\n> > + * information with \\a camera. The \\a data lifetime gets associated with\n> > + * the pipeline handler one, and gets released at deletion time.\n>\n> I would write the second sentence as just\n>\n> \"Ownership of \\a data is transferred to the PipelineHandler.\"\n>\n\nTaken in.\n\n\n> > + *\n> > + * If pipeline-specific data has already been associated with the camera by a\n> > + * previous call to this method, is it replaced by \\a data and the previous data\n> > + * are deleted, rendering all references to them invalid.\n>\n> I wonder whether we should disallow this and return an error instead, as\n> I don't think it's a valid use case. It would avoid potential invalid\n> references problems caused by\n>\n> \tsetCameraData(camera, std::move(data_ptr));\n> \tdata_ = cameraData(camera);\n> \t...\n>\n> \tsetCameraData(camera, std::move(new_data_ptr));\n> \t...\n> \tdata_->foo = bar;\t/* CRASH */\n>\n> I would also violate the cameraData() documentation's promise that the\n> pointer stays valid as long as the pipeline handler exists.\n\nI see. I would like to add a return value to the function, to make\nsure we return success or failure to the caller.\n\n>\n> > + *\n> > + * The data can be retrieved by pipeline handlers using the cameraData() method.\n> > + */\n> > +void PipelineHandler::setCameraData(const Camera *camera,\n> > +\t\t\t\t std::unique_ptr data)\n> > +{\n> > +\tif (cameraData_.count(camera))\n>\n> I'd use find() instead of count().\n>\n> > +\t\tLOG(Pipeline, Debug)\n>\n> Maybe error instead of debug ?\n\nI'll change this, thanks.\n\n\n>\n> > +\t\t\t<< \"Replacing data associated with \"\n> > +\t\t\t<< camera->name();\n> > +\n> > +\tcameraData_[camera] = std::move(data);\n> > +}\n> > +\n> > /**\n> > * \\class PipelineHandlerFactory\n> > * \\brief Registration of PipelineHandler classes and creation of instances\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C959660C65\n\tfor ;\n\tFri, 25 Jan 2019 17:34:21 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 2CCD710000D;\n\tFri, 25 Jan 2019 16:34:20 +0000 (UTC)"],"Date":"Fri, 25 Jan 2019 17:34:34 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125163434.4tf3spuvikf6ybb7@uno.localdomain>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-2-jacopo@jmondi.org>\n\t<20190125152425.GE4320@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"6oasx3zg3uhkinma\"","Content-Disposition":"inline","In-Reply-To":"<20190125152425.GE4320@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add\n\tCameraData","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 25 Jan 2019 16:34:21 -0000"}},{"id":608,"web_url":"https://patchwork.libcamera.org/comment/608/","msgid":"<20190125170155.GA4515@pendragon.ideasonboard.com>","date":"2019-01-25T17:01:55","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add\n\tCameraData","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Jan 25, 2019 at 05:34:34PM +0100, Jacopo Mondi wrote:\n> On Fri, Jan 25, 2019 at 05:24:25PM +0200, Laurent Pinchart wrote:\n> > On Thu, Jan 24, 2019 at 12:30:19PM +0100, Jacopo Mondi wrote:\n> >> Add class definition and methods to associate a Camera with specific data\n> >> in the pipeline_handler base class.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/include/pipeline_handler.h | 24 +++++++-\n> >> src/libcamera/pipeline_handler.cpp | 73 ++++++++++++++++++++++++\n> >> 2 files changed, 96 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> >> index b03217d..41699a5 100644\n> >> --- a/src/libcamera/include/pipeline_handler.h\n> >> +++ b/src/libcamera/include/pipeline_handler.h\n> >> @@ -11,17 +11,39 @@\n> >> #include \n> >> #include \n> >>\n> >> +#include \n> >> +\n> >> namespace libcamera {\n> >>\n> >> class CameraManager;\n> >> class DeviceEnumerator;\n> >>\n> >> +class CameraData\n> >> +{\n> >> +public:\n> >> +\tvirtual ~CameraData() {}\n> >> +\n> >> +protected:\n> >> +\tCameraData() {}\n> >> +\n> >> +private:\n> >> +\tCameraData(const CameraData &) = delete;\n> >> +\tvoid operator=(const CameraData &) = delete;\n> >\n> > I think you meant\n> >\n> > \tCameraData &operator=(const CameraData &) = delete;\n> \n> Thanks, fixed\n> \n> >> +};\n> >> +\n> >> class PipelineHandler\n> >> {\n> >> public:\n> >> -\tvirtual ~PipelineHandler() { };\n> >> +\tvirtual ~PipelineHandler();\n> >>\n> >> \tvirtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0;\n> >> +\n> >> +protected:\n> >> +\tCameraData *cameraData(const Camera *camera);\n> >> +\tvoid setCameraData(const Camera *camera, std::unique_ptr data);\n> >> +\n> >> +private:\n> >> +\tstd::map> cameraData_;\n> >\n> > I don't know why indexing on a const pointer seems a bit weird, but I\n> > don't see why it wouldn't work.\n> \n> why does this bother you?\n\nI'm not sure, it's the const there that seems weird to me, but I have no\nidea why.\n\n> >> };\n> >>\n> >> class PipelineHandlerFactory\n> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >> index c24feea..fb49fde 100644\n> >> --- a/src/libcamera/pipeline_handler.cpp\n> >> +++ b/src/libcamera/pipeline_handler.cpp\n> >> @@ -8,6 +8,8 @@\n> >> #include \"log.h\"\n> >> #include \"pipeline_handler.h\"\n> >>\n> >> +#include \n> >> +\n> >\n> > Please move this above the private headers.\n> \n> It should be dropped, it was there already.\n> \n> >> /**\n> >> * \\file pipeline_handler.h\n> >> * \\brief Create pipelines and cameras from a set of media devices\n> >> @@ -26,6 +28,20 @@ namespace libcamera {\n> >>\n> >> LOG_DEFINE_CATEGORY(Pipeline)\n> >>\n> >> +/**\n> >> + * \\class CameraData\n> >> + * \\brief Base class for platform-specific data associated with a camera\n> >> + *\n> >> + * The CameraData base abstract class represents platform specific-data\n> >> + * a pipeline handler might want to associate with a Camera to access them\n> >> + * at a later time.\n> >\n> > How about starting by explaining the usage instead of what the class\n> > represent ?\n> >\n> > The CameraData abstract class is part of a mechanism that allows\n> > pipeline handlers to associate pipeline-specific data to a camera for\n> > their own usage.\n> \n> I don't see any more precise indication on how the class should be\n> used more than in the original comment, but ok.\n\nMaybe it's not, maybe my version isn't better, I don't write perfect\ndocumentation :-)\n\n> >> + *\n> >> + * Pipeline handlers are expected to extend this base class with platform\n> >> + * specific implementation, associate instances of the derived classes\n> >> + * using the setCameraData() method, and access them at a later time\n> >> + * with cameraData().\n> >> + */\n> >> +\n> >> /**\n> >> * \\class PipelineHandler\n> >> * \\brief Create and manage cameras based on a set of media devices\n> >> @@ -66,6 +82,63 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> >> * created, or false otherwise\n> >> */\n> >>\n> >> +/**\n> >> + * \\brief Delete the pipeline handler\n> >\n> > s/Delete/Destroy/\n> >\n> > The destructor doesn't delete, it's called by the delete operation.\n> >\n> >> + *\n> >> + * Release the cameraData_ map, causing all data there referenced to be\n> >> + * deleted, as they are stored as unique_ptr\n> >\n> > s/$/./\n> >\n> > I think you can drop that comment, it doesn't seem very necessary.\n> >\n> >> + */\n> >> +PipelineHandler::~PipelineHandler()\n> >> +{\n> >> +\tcameraData_.clear();\n> >\n> > This isn't needed, the cameraData_ vector is deleted, so there's no need\n> > to clear it first.\n> >\n> \n> I've dropped it completely.\n> \n> >> +};\n> >> +\n> >> +/**\n> >> + * \\brief Retrieve the pipeline-specific data associated with a Camera\n> >> + * \\param camera The camera data is associate with\n> >\n> > s/associate/associated/\n> >\n> > or\n> >\n> > \"The camera whose data to retrieve\"\n> >\n> >> + *\n> >> + * \\return A pointer to the pipeline-specific data set with setCameraData().\n> >> + * The returned pointer lifetime is associated with the one of the pipeline\n> >> + * handler, and caller of this function shall never release it manually.\n> >\n> > \"The returned pointer is a borrowed reference and is guaranteed to remain\n> > valid until the pipeline handler is destroyed. It shall not be deleted\n> > manually by the caller.\"\n> >\n> > It would be really nice if we could make the class friend of\n> > std::unique_ptr and make the destructor private :-S\n> >\n> >> + */\n> >> +CameraData *PipelineHandler::cameraData(const Camera *camera)\n> >> +{\n> >> +\tif (!cameraData_.count(camera)) {\n> >> +\t\tLOG(Pipeline, Error)\n> >> +\t\t\t<< \"Cannot get data associated with camera \"\n> >> +\t\t\t<< camera->name();\n> >> +\t\treturn nullptr;\n> >> +\t}\n> >> +\n> >> +\treturn cameraData_[camera].get();\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Set pipeline-specific data in the camera\n> >\n> > Maybe s/in the/for the/ ?\n> >\n> >> + * \\param camera The camera to associate data to\n> >\n> > s/to$/with/\n> >\n> >> + * \\param data The pipeline-specific data\n> >> + *\n> >> + * This method allows pipeline handlers to associate pipeline-specific\n> >> + * information with \\a camera. The \\a data lifetime gets associated with\n> >> + * the pipeline handler one, and gets released at deletion time.\n> >\n> > I would write the second sentence as just\n> >\n> > \"Ownership of \\a data is transferred to the PipelineHandler.\"\n> \n> Taken in.\n> \n> >> + *\n> >> + * If pipeline-specific data has already been associated with the camera by a\n> >> + * previous call to this method, is it replaced by \\a data and the previous data\n> >> + * are deleted, rendering all references to them invalid.\n> >\n> > I wonder whether we should disallow this and return an error instead, as\n> > I don't think it's a valid use case. It would avoid potential invalid\n> > references problems caused by\n> >\n> > \tsetCameraData(camera, std::move(data_ptr));\n> > \tdata_ = cameraData(camera);\n> > \t...\n> >\n> > \tsetCameraData(camera, std::move(new_data_ptr));\n> > \t...\n> > \tdata_->foo = bar;\t/* CRASH */\n> >\n> > I would also violate the cameraData() documentation's promise that the\n> > pointer stays valid as long as the pipeline handler exists.\n> \n> I see. I would like to add a return value to the function, to make\n> sure we return success or failure to the caller.\n\nI'm fine with that, but I don't expect callers to check the return\nvalue. If callers are careful enough to add error checks (and handle\nerrors correctly), then I think they'll call the function correctly :-)\n\n> >> + *\n> >> + * The data can be retrieved by pipeline handlers using the cameraData() method.\n> >> + */\n> >> +void PipelineHandler::setCameraData(const Camera *camera,\n> >> +\t\t\t\t std::unique_ptr data)\n> >> +{\n> >> +\tif (cameraData_.count(camera))\n> >\n> > I'd use find() instead of count().\n> >\n> >> +\t\tLOG(Pipeline, Debug)\n> >\n> > Maybe error instead of debug ?\n> \n> I'll change this, thanks.\n> \n> >> +\t\t\t<< \"Replacing data associated with \"\n> >> +\t\t\t<< camera->name();\n> >> +\n> >> +\tcameraData_[camera] = std::move(data);\n> >> +}\n> >> +\n> >> /**\n> >> * \\class PipelineHandlerFactory\n> >> * \\brief Registration of PipelineHandler classes and creation of instances","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 1CC6E60C65\n\tfor ;\n\tFri, 25 Jan 2019 18:01:57 +0100 (CET)","from pendragon.ideasonboard.com\n\t(250-166-145-178.mobileinternet.proximus.be [178.145.166.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8FE4B325;\n\tFri, 25 Jan 2019 18:01:56 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548435716;\n\tbh=/q0ANOndVV/IjYLVo+29hgI+BSPsRBY+WcEW0rYPSsg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Mm5vA6ztzV5J+LnZ7nZw2th7RRblCIusSj7l8zFiOskCUvjIVKOlw/YMt9y8M1rv6\n\tbHQSWWSKCxsFrQkl86z8Mi6DuJurStpOIRYitJfkz1Gpa32a2XjmuW8fDFSwnqa59v\n\t8j9+vQHkXGCjSrMiMe08v8M0MOhjGPSytXWWv5xk=","Date":"Fri, 25 Jan 2019 19:01:55 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125170155.GA4515@pendragon.ideasonboard.com>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-2-jacopo@jmondi.org>\n\t<20190125152425.GE4320@pendragon.ideasonboard.com>\n\t<20190125163434.4tf3spuvikf6ybb7@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190125163434.4tf3spuvikf6ybb7@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Add\n\tCameraData","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 25 Jan 2019 17:01:57 -0000"}}]