[{"id":517,"web_url":"https://patchwork.libcamera.org/comment/517/","msgid":"<20190123102915.GG4485@pendragon.ideasonboard.com>","date":"2019-01-23T10:29:15","subject":"Re: [libcamera-devel] [RFC 1/2] libcamera: camera: Add CameraData","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 Tue, Jan 22, 2019 at 07:12:24PM +0100, Jacopo Mondi wrote:\n> Add abstract base class CameraData for pipeline handlers to store\n> pipeline specific data in Camera class instances.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/camera.h | 13 ++++++++++\n> src/libcamera/camera.cpp | 50 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 63 insertions(+)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 2ea1a68..50041f1 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -12,6 +12,15 @@\n> \n> namespace libcamera {\n> \n> +class CameraData\n> +{\n> +public:\n> +\tvirtual ~CameraData() { }\n\nMissing blank line.\n\n> +protected:\n> +\tCameraData() { }\n> +\tCameraData(CameraData &) = delete;\n\nNote that the copy constructor is CameraData(const CameraData &). I\nthink you also want to delete the operator=(const CameraData &).\n\n> +};\n> +\n> class Camera final\n> {\n> public:\n> @@ -22,11 +31,15 @@ public:\n> \n> \tconst std::string &name() const;\n> \n> +\tCameraData *cameraData() const { return data_.get(); }\n> +\tvoid setCameraData(CameraData *data);\n\nYou should define this as\n\n\tvoid setCameraData(std::unique_ptr data);\n\nto show that the function takes ownership of the object.\n\nBikeshedding, this is pipeline-specific data tied to a camera instance,\nshould the function be called setCameraData or setPipelineData ? I think\nthe \"camera\" part of the name is redundant as the method is in the\nCamera class, so I'd go for setPipelineData(), or even setData().\n\nThe next bikeshedding question is whether the class should be called\nCameraData or PipelineData, and I'm not sure about which option is best.\nMaybe CameraPipelineData to make sure everybody is happy ? :-)\n\nWe should make those two functions available to pipeline handlers only.\nOne option is to make them private and add the PipelineHandler class as\na friend. Classes deriving from PipelineHandler won't be able to access\nthem directly though, so you will need a method in PipelineHandler to do\nso:\n\n\tstatic CameraData *cameraData(Camera *camera)\n\t{\n\t\treturn camera->cameraData();\n\t}\n\n\tstatic void setCameraData(Camera *camera, CameraData *data)\n\t{\n\t\tcamera->setCameraData(data);\n\t}\n\nFeel free to propose a different option.\n\n> +\n> private:\n> \texplicit Camera(const std::string &name);\n> \t~Camera();\n> \n> \tstd::string name_;\n> +\tstd::unique_ptr data_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index acf912b..1e2c858 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -33,6 +33,26 @@\n> \n> namespace libcamera {\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 Camera::setCameraData() method, and access them at a later time\n> + * with Camera::cameraData().\n> + *\n> + * Once associated with a camera, lifetime of derived classes instances will\n> + * be tied to the one of the Camera instance itself.\n> + *\n> + * \\sa Camera::setCameraData()\n> + * \\sa Camera::cameraData()\n> + */\n> +\n> /**\n> * \\class Camera\n> * \\brief Camera device\n> @@ -83,6 +103,36 @@ const std::string &Camera::name() const\n> \treturn name_;\n> }\n> \n> +/**\n> + * \\fn CameraData *Camera::cameraData()\n> + * \\brief Retrieve the pipeline specific data\n\ns/pipeline specific/pipeline-specific/\n\n> + *\n> + * Borrow a reference to the platform specific data, associated to a camera\n> + * by pipeline handlers using the setCameraData() method.\n\nHow about\n\n * \\return A pointer to the pipeline-specific data set by\n * setCameraData(). The returned pointer is guaranteed to be valid until the\n * reference to the Camera object is released.\n\n> + */\n> +\n> +/**\n> + * \\brief Set pipeline specific data in the camera\n\ns/pipeline specific/pipeline-specific/\n\n> + *\n> + * Pipeline handlers might need to associate platform-specific informations to\n\ns/platform-specific/pipeline-specific/\ns/informations to/information with/\n\n> + * camera instances, this method allows them to do so while also transferring\n\ns/, this/. This/\n\n> + * ownership of the \\a cameraData to the Camera instance.\n\n\"allows them to do so\" sounds a bit heavy. How about\n\n * This method allows pipeline handlers to associate pipeline-specific\n * information with the camera. Ownership of the \\a data is transferred to the\n * Camera, and the data will be deleted when the camera is destroyed.\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> + *\n> + * Pipeline specific data are stored as unique_ptr<> to guarantee its\n> + * destruction at Camera deletion time.\n> + *\n> + * Pipeline specific data can be accessed again by pipeline handlers by\n> + * borrowing a (mutable) reference using the cameraData() method.\n> + *\n> + * \\sa Camera::cameraData()\n> + * \\sa CameraData\n\nDo we need these given that both the CameraData class and the\ncameraData() function are referenced from the text, and should thus\ngenerate html links already ?\n\n> + */\n> +void Camera::setCameraData(CameraData *data)\n> +{\n> +\tdata_ = std::unique_ptr(data);\n\n\tdata_ = std::move(data);\n\nafter changing the function prototype to pass an std::unique_ptr<>.\n\n> +\n\nUnneeded blank line.\n\n> +}\n> +\n> Camera::Camera(const std::string &name)\n> \t: name_(name)\n> {","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 AC67660B1B\n\tfor ;\n\tWed, 23 Jan 2019 11:29:16 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1CA1623D;\n\tWed, 23 Jan 2019 11:29:16 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548239356;\n\tbh=YBF0araMsUeeHOOKt6VhJ9aBPshuoEfBhnlDJSEgEe0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wDyGkMrgMDq/NG8RltGbT1C4RNl65DCCvMhcwWCjUJjBmiCMda91OzYTKV2WiOQNV\n\taJ+svtR3GAFA8BeJJqU3pxknd18KanFlc8ii11dpdF0lejYmGdQl8mm+ot6MK2kP8c\n\tRNLCOe1eld3mv9bho6qsf1p0qlRMQzH4HJCZ6hbY=","Date":"Wed, 23 Jan 2019 12:29:15 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190123102915.GG4485@pendragon.ideasonboard.com>","References":"<20190122181225.12922-1-jacopo@jmondi.org>\n\t<20190122181225.12922-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190122181225.12922-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 1/2] libcamera: camera: Add CameraData","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":"Wed, 23 Jan 2019 10:29:16 -0000"}},{"id":528,"web_url":"https://patchwork.libcamera.org/comment/528/","msgid":"<20190123135505.zex7jgzka4ysww4p@uno.localdomain>","date":"2019-01-23T13:55:05","subject":"Re: [libcamera-devel] [RFC 1/2] libcamera: camera: Add CameraData","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Jan 23, 2019 at 12:29:15PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Jan 22, 2019 at 07:12:24PM +0100, Jacopo Mondi wrote:\n> > Add abstract base class CameraData for pipeline handlers to store\n> > pipeline specific data in Camera class instances.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/camera.h | 13 ++++++++++\n> > src/libcamera/camera.cpp | 50 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 63 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 2ea1a68..50041f1 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -12,6 +12,15 @@\n> >\n> > namespace libcamera {\n> >\n> > +class CameraData\n> > +{\n> > +public:\n> > +\tvirtual ~CameraData() { }\n>\n> Missing blank line.\n>\n> > +protected:\n> > +\tCameraData() { }\n> > +\tCameraData(CameraData &) = delete;\n>\n> Note that the copy constructor is CameraData(const CameraData &). I\n> think you also want to delete the operator=(const CameraData &).\n\nYes, thanks\n\n>\n> > +};\n> > +\n> > class Camera final\n> > {\n> > public:\n> > @@ -22,11 +31,15 @@ public:\n> >\n> > \tconst std::string &name() const;\n> >\n> > +\tCameraData *cameraData() const { return data_.get(); }\n> > +\tvoid setCameraData(CameraData *data);\n>\n> You should define this as\n>\n> \tvoid setCameraData(std::unique_ptr data);\n>\n> to show that the function takes ownership of the object.\n\nwe're then going through 2 moves... Is it worth it? I agree that\nforcing a move in the caller clarifies the reference is not valid\nafterwards, but it also forces the caller to a very specific pattern:\n\n unique_ptr u = make_unique()\n camera->setData(move(u))\n\nwhile the caller might have the data to set not stored as unique_ptr.\nIs this good in your opinion?\n\n>\n> Bikeshedding, this is pipeline-specific data tied to a camera instance,\n> should the function be called setCameraData or setPipelineData ? I think\n> the \"camera\" part of the name is redundant as the method is in the\n> Camera class, so I'd go for setPipelineData(), or even setData().\n>\n\nsetData() is good\n\n> The next bikeshedding question is whether the class should be called\n> CameraData or PipelineData, and I'm not sure about which option is best.\n> Maybe CameraPipelineData to make sure everybody is happy ? :-)\n>\n\nI sincerely do not find disturbing having a member field of a Camera\nclass of type CameraData, seems quite natural to me.\n\nTrue, those are pipeline specific data, as platform data are embedded\nin a struct device... let's go with PipelineData\n\n> We should make those two functions available to pipeline handlers only.\n> One option is to make them private and add the PipelineHandler class as\n> a friend. Classes deriving from PipelineHandler won't be able to access\n> them directly though, so you will need a method in PipelineHandler to do\n> so:\n>\n> \tstatic CameraData *cameraData(Camera *camera)\n> \t{\n> \t\treturn camera->cameraData();\n> \t}\n>\n> \tstatic void setCameraData(Camera *camera, CameraData *data)\n> \t{\n> \t\tcamera->setCameraData(data);\n> \t}\n>\n> Feel free to propose a different option.\n>\n\nI think this is good and I will take this in\n\n> > +\n> > private:\n> > \texplicit Camera(const std::string &name);\n> > \t~Camera();\n> >\n> > \tstd::string name_;\n> > +\tstd::unique_ptr data_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index acf912b..1e2c858 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -33,6 +33,26 @@\n> >\n> > namespace libcamera {\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 Camera::setCameraData() method, and access them at a later time\n> > + * with Camera::cameraData().\n> > + *\n> > + * Once associated with a camera, lifetime of derived classes instances will\n> > + * be tied to the one of the Camera instance itself.\n> > + *\n> > + * \\sa Camera::setCameraData()\n> > + * \\sa Camera::cameraData()\n> > + */\n> > +\n> > /**\n> > * \\class Camera\n> > * \\brief Camera device\n> > @@ -83,6 +103,36 @@ const std::string &Camera::name() const\n> > \treturn name_;\n> > }\n> >\n> > +/**\n> > + * \\fn CameraData *Camera::cameraData()\n> > + * \\brief Retrieve the pipeline specific data\n>\n> s/pipeline specific/pipeline-specific/\n>\n> > + *\n> > + * Borrow a reference to the platform specific data, associated to a camera\n> > + * by pipeline handlers using the setCameraData() method.\n>\n> How about\n>\n> * \\return A pointer to the pipeline-specific data set by\n> * setCameraData(). The returned pointer is guaranteed to be valid until the\n> * reference to the Camera object is released.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\brief Set pipeline specific data in the camera\n>\n> s/pipeline specific/pipeline-specific/\n>\n> > + *\n> > + * Pipeline handlers might need to associate platform-specific informations to\n>\n> s/platform-specific/pipeline-specific/\n> s/informations to/information with/\n>\n> > + * camera instances, this method allows them to do so while also transferring\n>\n> s/, this/. This/\n>\n> > + * ownership of the \\a cameraData to the Camera instance.\n>\n> \"allows them to do so\" sounds a bit heavy. How about\n>\n> * This method allows pipeline handlers to associate pipeline-specific\n> * information with the camera. Ownership of the \\a data is transferred to the\n> * Camera, and the data will be deleted when the camera is destroyed.\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> > + *\n> > + * Pipeline specific data are stored as unique_ptr<> to guarantee its\n> > + * destruction at Camera deletion time.\n> > + *\n> > + * Pipeline specific data can be accessed again by pipeline handlers by\n> > + * borrowing a (mutable) reference using the cameraData() method.\n> > + *\n> > + * \\sa Camera::cameraData()\n> > + * \\sa CameraData\n>\n> Do we need these given that both the CameraData class and the\n> cameraData() function are referenced from the text, and should thus\n> generate html links already ?\n>\n> > + */\n> > +void Camera::setCameraData(CameraData *data)\n> > +{\n> > +\tdata_ = std::unique_ptr(data);\n>\n> \tdata_ = std::move(data);\n>\n> after changing the function prototype to pass an std::unique_ptr<>.\n>\n> > +\n>\n> Unneeded blank line.\n\nThanks\n j\n\n>\n> > +}\n> > +\n> > Camera::Camera(const std::string &name)\n> > \t: name_(name)\n> > {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2380A60C7D\n\tfor ;\n\tWed, 23 Jan 2019 14:54:54 +0100 (CET)","from uno.localdomain (unknown [37.176.170.194])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 0C339FF803;\n\tWed, 23 Jan 2019 13:54:52 +0000 (UTC)"],"X-Originating-IP":"37.176.170.194","Date":"Wed, 23 Jan 2019 14:55:05 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190123135505.zex7jgzka4ysww4p@uno.localdomain>","References":"<20190122181225.12922-1-jacopo@jmondi.org>\n\t<20190122181225.12922-2-jacopo@jmondi.org>\n\t<20190123102915.GG4485@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"3iadzo4wze5hminx\"","Content-Disposition":"inline","In-Reply-To":"<20190123102915.GG4485@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [RFC 1/2] libcamera: camera: Add CameraData","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":"Wed, 23 Jan 2019 13:54:54 -0000"}},{"id":547,"web_url":"https://patchwork.libcamera.org/comment/547/","msgid":"<20190123165922.GK31885@pendragon.ideasonboard.com>","date":"2019-01-23T16:59:22","subject":"Re: [libcamera-devel] [RFC 1/2] libcamera: camera: Add CameraData","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jan 23, 2019 at 02:55:05PM +0100, Jacopo Mondi wrote:\n> On Wed, Jan 23, 2019 at 12:29:15PM +0200, Laurent Pinchart wrote:\n> > On Tue, Jan 22, 2019 at 07:12:24PM +0100, Jacopo Mondi wrote:\n> >> Add abstract base class CameraData for pipeline handlers to store\n> >> pipeline specific data in Camera class instances.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> include/libcamera/camera.h | 13 ++++++++++\n> >> src/libcamera/camera.cpp | 50 ++++++++++++++++++++++++++++++++++++++\n> >> 2 files changed, 63 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> >> index 2ea1a68..50041f1 100644\n> >> --- a/include/libcamera/camera.h\n> >> +++ b/include/libcamera/camera.h\n> >> @@ -12,6 +12,15 @@\n> >>\n> >> namespace libcamera {\n> >>\n> >> +class CameraData\n> >> +{\n> >> +public:\n> >> +\tvirtual ~CameraData() { }\n> >\n> > Missing blank line.\n> >\n> >> +protected:\n> >> +\tCameraData() { }\n> >> +\tCameraData(CameraData &) = delete;\n> >\n> > Note that the copy constructor is CameraData(const CameraData &). I\n> > think you also want to delete the operator=(const CameraData &).\n> \n> Yes, thanks\n> \n> >> +};\n> >> +\n> >> class Camera final\n> >> {\n> >> public:\n> >> @@ -22,11 +31,15 @@ public:\n> >>\n> >> \tconst std::string &name() const;\n> >>\n> >> +\tCameraData *cameraData() const { return data_.get(); }\n> >> +\tvoid setCameraData(CameraData *data);\n> >\n> > You should define this as\n> >\n> > \tvoid setCameraData(std::unique_ptr data);\n> >\n> > to show that the function takes ownership of the object.\n> \n> we're then going through 2 moves... Is it worth it? I agree that\n> forcing a move in the caller clarifies the reference is not valid\n> afterwards, but it also forces the caller to a very specific pattern:\n> \n> unique_ptr u = make_unique()\n> camera->setData(move(u))\n> \n> while the caller might have the data to set not stored as unique_ptr.\n> Is this good in your opinion?\n\nI think that clarifying object ownership is worth it. The setData()\nfunction takes ownership of the object, and while it could be useful for\nthe caller to access it after calling setData(), it can only do so if\nthe setData() function allows the caller to borrow the object. We have\nno way in the API to express this at the level of the setData()\nfunction, so calling data() afterwards is the best way I found to\nexpress the ownership contract.\n\n> > Bikeshedding, this is pipeline-specific data tied to a camera instance,\n> > should the function be called setCameraData or setPipelineData ? I think\n> > the \"camera\" part of the name is redundant as the method is in the\n> > Camera class, so I'd go for setPipelineData(), or even setData().\n> \n> setData() is good\n> \n> > The next bikeshedding question is whether the class should be called\n> > CameraData or PipelineData, and I'm not sure about which option is best.\n> > Maybe CameraPipelineData to make sure everybody is happy ? :-)\n> >\n> \n> I sincerely do not find disturbing having a member field of a Camera\n> class of type CameraData, seems quite natural to me.\n\nThe CameraData type isn't disturbing as such, it was the cameraData()\nfunction name that bothered me.\n\n> True, those are pipeline specific data, as platform data are embedded\n> in a struct device... let's go with PipelineData\n> \n> > We should make those two functions available to pipeline handlers only.\n> > One option is to make them private and add the PipelineHandler class as\n> > a friend. Classes deriving from PipelineHandler won't be able to access\n> > them directly though, so you will need a method in PipelineHandler to do\n> > so:\n> >\n> > \tstatic CameraData *cameraData(Camera *camera)\n> > \t{\n> > \t\treturn camera->cameraData();\n> > \t}\n> >\n> > \tstatic void setCameraData(Camera *camera, CameraData *data)\n> > \t{\n> > \t\tcamera->setCameraData(data);\n> > \t}\n> >\n> > Feel free to propose a different option.\n> \n> I think this is good and I will take this in\n> \n> >> +\n> >> private:\n> >> \texplicit Camera(const std::string &name);\n> >> \t~Camera();\n> >>\n> >> \tstd::string name_;\n> >> +\tstd::unique_ptr data_;\n> >> };\n> >>\n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >> index acf912b..1e2c858 100644\n> >> --- a/src/libcamera/camera.cpp\n> >> +++ b/src/libcamera/camera.cpp\n> >> @@ -33,6 +33,26 @@\n> >>\n> >> namespace libcamera {\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 Camera::setCameraData() method, and access them at a later time\n> >> + * with Camera::cameraData().\n> >> + *\n> >> + * Once associated with a camera, lifetime of derived classes instances will\n> >> + * be tied to the one of the Camera instance itself.\n> >> + *\n> >> + * \\sa Camera::setCameraData()\n> >> + * \\sa Camera::cameraData()\n> >> + */\n> >> +\n> >> /**\n> >> * \\class Camera\n> >> * \\brief Camera device\n> >> @@ -83,6 +103,36 @@ const std::string &Camera::name() const\n> >> \treturn name_;\n> >> }\n> >>\n> >> +/**\n> >> + * \\fn CameraData *Camera::cameraData()\n> >> + * \\brief Retrieve the pipeline specific data\n> >\n> > s/pipeline specific/pipeline-specific/\n> >\n> >> + *\n> >> + * Borrow a reference to the platform specific data, associated to a camera\n> >> + * by pipeline handlers using the setCameraData() method.\n> >\n> > How about\n> >\n> > * \\return A pointer to the pipeline-specific data set by\n> > * setCameraData(). The returned pointer is guaranteed to be valid until the\n> > * reference to the Camera object is released.\n> >\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Set pipeline specific data in the camera\n> >\n> > s/pipeline specific/pipeline-specific/\n> >\n> >> + *\n> >> + * Pipeline handlers might need to associate platform-specific informations to\n> >\n> > s/platform-specific/pipeline-specific/\n> > s/informations to/information with/\n> >\n> >> + * camera instances, this method allows them to do so while also transferring\n> >\n> > s/, this/. This/\n> >\n> >> + * ownership of the \\a cameraData to the Camera instance.\n> >\n> > \"allows them to do so\" sounds a bit heavy. How about\n> >\n> > * This method allows pipeline handlers to associate pipeline-specific\n> > * information with the camera. Ownership of the \\a data is transferred to the\n> > * Camera, and the data will be deleted when the camera is destroyed.\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> >> + *\n> >> + * Pipeline specific data are stored as unique_ptr<> to guarantee its\n> >> + * destruction at Camera deletion time.\n> >> + *\n> >> + * Pipeline specific data can be accessed again by pipeline handlers by\n> >> + * borrowing a (mutable) reference using the cameraData() method.\n> >> + *\n> >> + * \\sa Camera::cameraData()\n> >> + * \\sa CameraData\n> >\n> > Do we need these given that both the CameraData class and the\n> > cameraData() function are referenced from the text, and should thus\n> > generate html links already ?\n> >\n> >> + */\n> >> +void Camera::setCameraData(CameraData *data)\n> >> +{\n> >> +\tdata_ = std::unique_ptr(data);\n> >\n> > \tdata_ = std::move(data);\n> >\n> > after changing the function prototype to pass an std::unique_ptr<>.\n> >\n> >> +\n> >\n> > Unneeded blank line.\n> >\n> >> +}\n> >> +\n> >> Camera::Camera(const std::string &name)\n> >> \t: name_(name)\n> >> {","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 4017760C7D\n\tfor ;\n\tWed, 23 Jan 2019 17:59:24 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B210723D;\n\tWed, 23 Jan 2019 17:59:23 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548262763;\n\tbh=9RFFE78vEzwELwknakI3A9IONGBk/zhhxoGgnv9J4Fc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Bf6cEfx9dArsdPkly7xKGsjnHIMpfyGb7dqV3QZkHWxEpAZiUjKIq4XzKDbmlWBHM\n\tXpwnx6W1GbBg9oEQ3Izr+20Wu8dOqBGMW00eS19ve3yxU0w3MQH+ahKldZ6YCxogj8\n\tHi4vnVXw/QhdWyZGx4Jq6to5WlZB0b7k79mIfpEI=","Date":"Wed, 23 Jan 2019 18:59:22 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190123165922.GK31885@pendragon.ideasonboard.com>","References":"<20190122181225.12922-1-jacopo@jmondi.org>\n\t<20190122181225.12922-2-jacopo@jmondi.org>\n\t<20190123102915.GG4485@pendragon.ideasonboard.com>\n\t<20190123135505.zex7jgzka4ysww4p@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190123135505.zex7jgzka4ysww4p@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 1/2] libcamera: camera: Add CameraData","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":"Wed, 23 Jan 2019 16:59:24 -0000"}}]