[{"id":353,"web_url":"https://patchwork.libcamera.org/comment/353/","msgid":"<20190116084455.6h7oxekekbtsledw@uno.localdomain>","date":"2019-01-16T08:44:55","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n a few more comments, please bear with me a little longer and thanks\nfor this series\n\nOn Tue, Jan 15, 2019 at 11:19:08PM +0000, Kieran Bingham wrote:\n> Provide a helper V4L2 device object capable of interacting with the\n> V4L2 Linux Kernel APIs.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/include/v4l2_device.h | 45 +++++++++\n> src/libcamera/meson.build | 2 +\n> src/libcamera/v4l2_device.cpp | 150 ++++++++++++++++++++++++++++\n> 3 files changed, 197 insertions(+)\n> create mode 100644 src/libcamera/include/v4l2_device.h\n> create mode 100644 src/libcamera/v4l2_device.cpp\n>\n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> new file mode 100644\n> index 000000000000..b0f5e9689654\n> --- /dev/null\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_device.h - V4L2 Device\n> + */\n> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> +#define __LIBCAMERA_V4L2_DEVICE_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Capability : public v4l2_capability\n> +{\n> +public:\n> +\tstd::string getDriver() const { return std::string((char *)driver); }\n> +\tstd::string getCard() const { return std::string((char *)card); }\n\nI still feel extending a kernel provided struct into an object is meh.\nHere you could have returned references to two members, intialized\nwith the values from struct v4l2_capability....\n\nI won't push anymore on this though.\n\n> +\n> +\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n\nYou pointed out in your self-review this was meant to be changed,\nright?\n\n> +\tbool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }\n> +};\n> +\n> +class V4L2Device\n> +{\n> +public:\n> +\tV4L2Device() = delete;\n\nShould this be private?\n\n> +\tV4L2Device(const std::string &devnode);\n> +\t~V4L2Device();\n> +\n> +\tint open();\n> +\tbool isOpen() const;\n> +\tvoid close();\n> +\n> +private:\n> +\tstd::string devnode_;\n> +\tint fd_;\n> +\tV4L2Capability caps_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index abf9a71d4172..f9f25c0ecf15 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -11,6 +11,7 @@ libcamera_sources = files([\n> 'pipeline_handler.cpp',\n> 'signal.cpp',\n> 'timer.cpp',\n> + 'v4l2_device.cpp',\n> ])\n>\n> libcamera_headers = files([\n> @@ -21,6 +22,7 @@ libcamera_headers = files([\n> 'include/media_object.h',\n> 'include/pipeline_handler.h',\n> 'include/utils.h',\n> + 'include/v4l2_device.h',\n> ])\n>\n> libcamera_internal_includes = include_directories('include')\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> new file mode 100644\n> index 000000000000..8c49d3ff56e2\n> --- /dev/null\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -0,0 +1,150 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_device.cpp - V4L2 Device\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"v4l2_device.h\"\n> +\n> +/**\n> + * \\file v4l2_device.h\n> + * \\brief V4L2 Device API\n> + */\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class V4L2Capability\n> + * \\brief struct v4l2_capability object wrapper and helpers\n> + *\n> + * The V4L2Capability class manages the information returned by the\n> + * VIDIOC_QUERYCAP ioctl.\n> + */\n> +\n> +/**\n> + * \\fn std::string V4L2Capability::getDriver()\n> + * \\brief Retrieve the driver module name\n> + * \\return The string containing the name of the driver module\n> + */\n> +\n> +/**\n> + * \\fn std::string V4L2Capability::getCard()\n> + * \\brief Retrieve the device card name\n> + * \\return The string containing the device name\n\ns/card/device ?\n\n> + */\n> +\n> +/**\n> + * \\fn bool V4L2Capability::isCapture()\n> + * \\brief Identify if the device is capable of capturing video\n> + * \\return boolean true if the device provides video frames\n> + */\n> +\n> +/**\n> + * \\fn bool V4L2Capability::hasStreaming()\n> + * \\brief Determine if the device can perform Streaming IO\n> + * \\return boolean true if the device provides Streaming IO IOCTLs\n> + */\n> +\n> +/**\n> + * \\class V4L2Device\n> + * \\brief V4L2Device object and API.\n> + *\n> + * The V4L2 Device API class models an instance of a V4L2 device node.\n> + * It is constructed with the path to a V4L2 video device node. The device node\n> + * is only opened upon a call to open() which must be checked for success.\n> + *\n> + * The device capabilities are validated and the device is rejected if it is\n> + * not a suitable capture device.\n> + *\n> + * No API call (except open(), isOpen() and close()) shall be called on an\n> + * unopened device instance.\n> + *\n> + * Upon destruction any device left open will be closed, and any resources\n> + * released.\n> + */\n> +\n> +/**\n> + * \\brief Construct a V4L2Device\n> + * \\param devnode The file-system path to the video device node\n> + */\n> +V4L2Device::V4L2Device(const std::string &devnode)\n> +\t: devnode_(devnode), fd_(-1)\n> +{\n> +}\n> +\n> +V4L2Device::~V4L2Device()\n> +{\n> +\tclose();\n> +}\n> +\n> +/**\n> + * \\brief Open a V4L2 device and query properties from the device.\n> + * \\return 0 on success, or a negative error code otherwise\n> + */\n> +int V4L2Device::open()\n> +{\n> +\tint ret;\n> +\n> +\tif (isOpen()) {\n> +\t\tLOG(Error) << \"Device already open\";\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n> +\tret = ::open(devnode_.c_str(), O_RDWR);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> +\t\t\t << \" : \" << strerror(ret);\n\nstrerror(-ret)\n\nEven if in this case you could have used errno directly. Sorry, my\ncomment might has mis-lead you\n\n> +\t\treturn ret;\n> +\t}\n> +\tfd_ = ret;\n> +\n> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n\nAs you pointed out, this might be the compiler performing down-casting\nto a plain 'struct v4l2_capability'\n\n> +\tif (ret < 0) {\n> +\t\tLOG(Error) << \"Failed to query device capabilities: \" << strerror(errno);\n> +\t\treturn -errno;\n> +\t}\n> +\n> +\tLOG(Debug) << \"Opened '\" << devnode_ << \"' \" << caps_.getDriver() << \": \" << caps_.getCard();\n> +\n> +\tif (!(caps_.isCapture())) {\n\nif (!caps_.isCapture())\n\n> +\t\tLOG(Error) << \"Device is not a capture device\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (!(caps_.hasStreaming())) {\n> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Check if device is successfully opened\n> + */\n> +bool V4L2Device::isOpen() const\n> +{\n> +\treturn (fd_ != -1);\n\nAh, I see what you've done here (return with no () )\n\n\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n\n> +}\n> +\n> +/**\n> + * \\brief Close the device, releasing any resources acquired by open()\n> + */\n> +void V4L2Device::close()\n> +{\n> +\tif (fd_ < 0)\n> +\t\treturn;\n> +\n> +\t::close(fd_);\n> +\tfd_ = -1;\n> +}\n> +\n> +} /* namespace libcamera */\n> --\n> 2.17.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 relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6C3860B2D\n\tfor ;\n\tWed, 16 Jan 2019 09:44:46 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 2ADA3E0017;\n\tWed, 16 Jan 2019 08:44:45 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 16 Jan 2019 09:44:55 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190116084455.6h7oxekekbtsledw@uno.localdomain>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"tpyxsu5i6t2ez3iv\"","Content-Disposition":"inline","In-Reply-To":"<20190115231909.19893-3-kieran.bingham@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 08:44:46 -0000"}},{"id":356,"web_url":"https://patchwork.libcamera.org/comment/356/","msgid":"<8bf2984f-cf86-3bc9-2fc3-cd65782da598@ideasonboard.com>","date":"2019-01-16T10:50:13","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 16/01/2019 08:44, Jacopo Mondi wrote:\n> Hi Kieran,\n> a few more comments, please bear with me a little longer and thanks\n> for this series\n> \n> On Tue, Jan 15, 2019 at 11:19:08PM +0000, Kieran Bingham wrote:\n>> Provide a helper V4L2 device object capable of interacting with the\n>> V4L2 Linux Kernel APIs.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/libcamera/include/v4l2_device.h | 45 +++++++++\n>> src/libcamera/meson.build | 2 +\n>> src/libcamera/v4l2_device.cpp | 150 ++++++++++++++++++++++++++++\n>> 3 files changed, 197 insertions(+)\n>> create mode 100644 src/libcamera/include/v4l2_device.h\n>> create mode 100644 src/libcamera/v4l2_device.cpp\n>>\n>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n>> new file mode 100644\n>> index 000000000000..b0f5e9689654\n>> --- /dev/null\n>> +++ b/src/libcamera/include/v4l2_device.h\n>> @@ -0,0 +1,45 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * v4l2_device.h - V4L2 Device\n>> + */\n>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n>> +#define __LIBCAMERA_V4L2_DEVICE_H__\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +class V4L2Capability : public v4l2_capability\n>> +{\n>> +public:\n>> +\tstd::string getDriver() const { return std::string((char *)driver); }\n>> +\tstd::string getCard() const { return std::string((char *)card); }\n> \n> I still feel extending a kernel provided struct into an object is meh.\n> Here you could have returned references to two members, intialized\n> with the values from struct v4l2_capability....\n\nI think this is a really good way to keep methods and code which\ninteracts with the struct v4l2_capability together, and treats the C\nstructure like an object (which it essentially is, it has defined data\nstructure and it's usage is defined by the kernel API).\n\nIt doesn't extend the size of storage - and only defines how to interact\nwith the data structures.\n\nThe getDriver() and getCard() wraps the casting required again into the\nclass definition so that it doesn't have to be done in arbitrary places\nin the code.\n\nI expect to add next:\n\n V4L2Device::driverName() { return caps_.getDriver(); }\n V4L2Device::deviceName() { return caps_.getCard(); }\n\t// Or ? getDeviceName()?\n V4L2Device::busName() { return caps_.getBus(); }\n\t// (Yes, I'll add this one in to the V4L2Capabiltiy)\n\nI anticipate that these strings will be useful to you in the pipeline\nclasses.\n\nThe UVC pipeline manager should certainly use them.\n\n\n> I won't push anymore on this though.\n\nI don't mind the push ... I knew this 'wrapping' would be potentially\ncontroversial it's partly me trying to experiment with good ways to\ninteract with the Kernel API.\n\nAt the moment, I must say I really like it though ...\n\t \n\nEspecially with the ease and cleanliness for exposing the capability\nstrings.\n\n>> +\n>> +\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n> \n> You pointed out in your self-review this was meant to be changed,\n> right?\n\nHalf-right.\n\nThen based on your other comments regarding MPLANE/Single Plane I\ndecided to drop MPLANE support.\n\nWe don't need it yet, the IPU3 doesn't support MPLANE, the RCAR-VIN\ndoesn't support MPLANE, UVC doesn't support MPLANE ...\n\n\n>> +\tbool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }\n>> +};\n>> +\n>> +class V4L2Device\n>> +{\n>> +public:\n>> +\tV4L2Device() = delete;\n> \n> Should this be private?\n\nI wondered that - but I figured - it doesn't matter if it's public or\nprivate. It's declaring that it's gone. And I thought it was better to\nkeep the constructors (or lack of) and destructors together.\n\nIf this is something we want to standardize (keeping deletions in\nprivate:) I can move it.\n\n\n> \n>> +\tV4L2Device(const std::string &devnode);\n>> +\t~V4L2Device();\n>> +\n>> +\tint open();\n>> +\tbool isOpen() const;\n>> +\tvoid close();\n>> +\n>> +private:\n>> +\tstd::string devnode_;\n>> +\tint fd_;\n>> +\tV4L2Capability caps_;\n>> +};\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index abf9a71d4172..f9f25c0ecf15 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -11,6 +11,7 @@ libcamera_sources = files([\n>> 'pipeline_handler.cpp',\n>> 'signal.cpp',\n>> 'timer.cpp',\n>> + 'v4l2_device.cpp',\n>> ])\n>>\n>> libcamera_headers = files([\n>> @@ -21,6 +22,7 @@ libcamera_headers = files([\n>> 'include/media_object.h',\n>> 'include/pipeline_handler.h',\n>> 'include/utils.h',\n>> + 'include/v4l2_device.h',\n>> ])\n>>\n>> libcamera_internal_includes = include_directories('include')\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> new file mode 100644\n>> index 000000000000..8c49d3ff56e2\n>> --- /dev/null\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -0,0 +1,150 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * v4l2_device.cpp - V4L2 Device\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"log.h\"\n>> +#include \"v4l2_device.h\"\n>> +\n>> +/**\n>> + * \\file v4l2_device.h\n>> + * \\brief V4L2 Device API\n>> + */\n>> +namespace libcamera {\n>> +\n>> +/**\n>> + * \\class V4L2Capability\n>> + * \\brief struct v4l2_capability object wrapper and helpers\n>> + *\n>> + * The V4L2Capability class manages the information returned by the\n>> + * VIDIOC_QUERYCAP ioctl.\n>> + */\n>> +\n>> +/**\n>> + * \\fn std::string V4L2Capability::getDriver()\n>> + * \\brief Retrieve the driver module name\n>> + * \\return The string containing the name of the driver module\n>> + */\n>> +\n>> +/**\n>> + * \\fn std::string V4L2Capability::getCard()\n>> + * \\brief Retrieve the device card name\n>> + * \\return The string containing the device name\n> \n> s/card/device ?\n\nI want to respect the existing naming of the V4L2 API. It's called a\ncard there.\n\nIt can be exposed through the V4L2Device API as a deviceName() as\nmentioned above.\n\n\n>> + */\n>> +\n>> +/**\n>> + * \\fn bool V4L2Capability::isCapture()\n>> + * \\brief Identify if the device is capable of capturing video\n>> + * \\return boolean true if the device provides video frames\n>> + */\n>> +\n>> +/**\n>> + * \\fn bool V4L2Capability::hasStreaming()\n>> + * \\brief Determine if the device can perform Streaming IO\n>> + * \\return boolean true if the device provides Streaming IO IOCTLs\n>> + */\n>> +\n>> +/**\n>> + * \\class V4L2Device\n>> + * \\brief V4L2Device object and API.\n>> + *\n>> + * The V4L2 Device API class models an instance of a V4L2 device node.\n>> + * It is constructed with the path to a V4L2 video device node. The device node\n>> + * is only opened upon a call to open() which must be checked for success.\n>> + *\n>> + * The device capabilities are validated and the device is rejected if it is\n>> + * not a suitable capture device.\n>> + *\n>> + * No API call (except open(), isOpen() and close()) shall be called on an\n>> + * unopened device instance.\n>> + *\n>> + * Upon destruction any device left open will be closed, and any resources\n>> + * released.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a V4L2Device\n>> + * \\param devnode The file-system path to the video device node\n>> + */\n>> +V4L2Device::V4L2Device(const std::string &devnode)\n>> +\t: devnode_(devnode), fd_(-1)\n>> +{\n>> +}\n>> +\n>> +V4L2Device::~V4L2Device()\n>> +{\n>> +\tclose();\n>> +}\n>> +\n>> +/**\n>> + * \\brief Open a V4L2 device and query properties from the device.\n>> + * \\return 0 on success, or a negative error code otherwise\n>> + */\n>> +int V4L2Device::open()\n>> +{\n>> +\tint ret;\n>> +\n>> +\tif (isOpen()) {\n>> +\t\tLOG(Error) << \"Device already open\";\n>> +\t\treturn -EBUSY;\n>> +\t}\n>> +\n>> +\tret = ::open(devnode_.c_str(), O_RDWR);\n>> +\tif (ret < 0) {\n>> +\t\tret = -errno;\n>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n>> +\t\t\t << \" : \" << strerror(ret);\n> \n> strerror(-ret)\n> \n> Even if in this case you could have used errno directly. Sorry, my\n> comment might has mis-lead you\n\nAhh oops - and good spot.\n\nBut yes - I'd much rather reference the errno directly for strerror().\n\n\n> \n>> +\t\treturn ret;\n>> +\t}\n>> +\tfd_ = ret;\n>> +\n>> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n> \n> As you pointed out, this might be the compiler performing down-casting\n> to a plain 'struct v4l2_capability'\n\nYes, which is the desired result.\n\n> \n>> +\tif (ret < 0) {\n>> +\t\tLOG(Error) << \"Failed to query device capabilities: \" << strerror(errno);\n>> +\t\treturn -errno;\n>> +\t}\n>> +\n>> +\tLOG(Debug) << \"Opened '\" << devnode_ << \"' \" << caps_.getDriver() << \": \" << caps_.getCard();\n>> +\n>> +\tif (!(caps_.isCapture())) {\n> \n> if (!caps_.isCapture())\n\nYes :)\n\n> \n>> +\t\tLOG(Error) << \"Device is not a capture device\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (!(caps_.hasStreaming())) {\n\nAnd here...\n\n>> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Check if device is successfully opened\n>> + */\n>> +bool V4L2Device::isOpen() const\n>> +{\n>> +\treturn (fd_ != -1);\n> \n> Ah, I see what you've done here (return with no () )\n> \n> \tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n\nAh I've been caught! (ok well it wasn't intentional hehe)\n\nI /almost/ want to add () to the isCapture() but I don't want to\nincrease that line length...\n\n/me cries a little and does:\n\n s/(fd_ != -1)/fd != -1/\n\n> \n>> +}\n>> +\n>> +/**\n>> + * \\brief Close the device, releasing any resources acquired by open()\n>> + */\n>> +void V4L2Device::close()\n>> +{\n>> +\tif (fd_ < 0)\n>> +\t\treturn;\n>> +\n>> +\t::close(fd_);\n>> +\tfd_ = -1;\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> --\n>> 2.17.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 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 71FE860C97\n\tfor ;\n\tWed, 16 Jan 2019 11:50:17 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BA2114F8;\n\tWed, 16 Jan 2019 11:50:16 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547635816;\n\tbh=J6oykX/CBtrmI/JZ5z73mNHJematAmL1bUoiq0DLZY8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=wFH5wMJ23QJUNMqlf5Imfk6/HzNTBFz0INigpweruk7mIroODVsAPNw0MUBk+Ruxf\n\tS4c3Y+ggikR9NvCgLcUULBdI3iSmEGjRMPgP9+Kegz7PoFw/Dlcai/8hKtePldCBzY\n\t4KRGpvkwhZGCj2aSvERqxplB7RJ2BYCSdGe6eEs0=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi ","Cc":"LibCamera Devel ","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<8bf2984f-cf86-3bc9-2fc3-cd65782da598@ideasonboard.com>","Date":"Wed, 16 Jan 2019 10:50:13 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20190116084455.6h7oxekekbtsledw@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 10:50:17 -0000"}},{"id":357,"web_url":"https://patchwork.libcamera.org/comment/357/","msgid":"<20190116111143.jconorwmbr2fzh6i@uno.localdomain>","date":"2019-01-16T11:11:43","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Wed, Jan 16, 2019 at 10:50:13AM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 16/01/2019 08:44, Jacopo Mondi wrote:\n> > Hi Kieran,\n> > a few more comments, please bear with me a little longer and thanks\n> > for this series\n> >\n> > On Tue, Jan 15, 2019 at 11:19:08PM +0000, Kieran Bingham wrote:\n> >> Provide a helper V4L2 device object capable of interacting with the\n> >> V4L2 Linux Kernel APIs.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/libcamera/include/v4l2_device.h | 45 +++++++++\n> >> src/libcamera/meson.build | 2 +\n> >> src/libcamera/v4l2_device.cpp | 150 ++++++++++++++++++++++++++++\n> >> 3 files changed, 197 insertions(+)\n> >> create mode 100644 src/libcamera/include/v4l2_device.h\n> >> create mode 100644 src/libcamera/v4l2_device.cpp\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >> new file mode 100644\n> >> index 000000000000..b0f5e9689654\n> >> --- /dev/null\n> >> +++ b/src/libcamera/include/v4l2_device.h\n> >> @@ -0,0 +1,45 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * v4l2_device.h - V4L2 Device\n> >> + */\n> >> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> >> +#define __LIBCAMERA_V4L2_DEVICE_H__\n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +class V4L2Capability : public v4l2_capability\n> >> +{\n> >> +public:\n> >> +\tstd::string getDriver() const { return std::string((char *)driver); }\n> >> +\tstd::string getCard() const { return std::string((char *)card); }\n> >\n> > I still feel extending a kernel provided struct into an object is meh.\n> > Here you could have returned references to two members, intialized\n> > with the values from struct v4l2_capability....\n>\n> I think this is a really good way to keep methods and code which\n> interacts with the struct v4l2_capability together, and treats the C\n> structure like an object (which it essentially is, it has defined data\n> structure and it's usage is defined by the kernel API).\n>\n> It doesn't extend the size of storage - and only defines how to interact\n> with the data structures.\n>\n> The getDriver() and getCard() wraps the casting required again into the\n> class definition so that it doesn't have to be done in arbitrary places\n> in the code.\n>\n> I expect to add next:\n>\n> V4L2Device::driverName() { return caps_.getDriver(); }\n> V4L2Device::deviceName() { return caps_.getCard(); }\n> \t// Or ? getDeviceName()?\n> V4L2Device::busName() { return caps_.getBus(); }\n> \t// (Yes, I'll add this one in to the V4L2Capabiltiy)\n>\n> I anticipate that these strings will be useful to you in the pipeline\n> classes.\n>\n> The UVC pipeline manager should certainly use them.\n>\n\nOne note I missed in both my previous comments: getter methods are named as\nthe member field they access, without the \"get\" prefix.\n\n>\n> > I won't push anymore on this though.\n>\n> I don't mind the push ... I knew this 'wrapping' would be potentially\n> controversial it's partly me trying to experiment with good ways to\n> interact with the Kernel API.\n>\n> At the moment, I must say I really like it though ...\n> \t \n>\n> Especially with the ease and cleanliness for exposing the capability\n> strings.\n>\n\nI'll let other express concerns, if any. Otherwise that's fine with\nme.\n\n> >> +\n> >> +\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n> >\n> > You pointed out in your self-review this was meant to be changed,\n> > right?\n>\n> Half-right.\n>\n> Then based on your other comments regarding MPLANE/Single Plane I\n> decided to drop MPLANE support.\n>\n> We don't need it yet, the IPU3 doesn't support MPLANE, the RCAR-VIN\n> doesn't support MPLANE, UVC doesn't support MPLANE ...\n>\n\nAre you sure?\n\n- entity 4: ipu3-cio2 0 (1 pad, 1 link)\n type Node subtype V4L flags 0\n device node name /dev/video0\n\n\n# v4l2-ctl -D -d /dev/video0\nDriver Info (not using libv4l2):\n\tDriver name : ipu3-cio2\n\tCard type : Intel IPU3 CIO2\n\tBus info : PCI:0000:00:14.3\n\tDriver version: 4.20.0\n\tCapabilities : 0x84201000\n\t\tVideo Capture Multiplanar <---------------\n\t\tStreaming\n\t\tExtended Pix Format\n\t\tDevice Capabilities\n\tDevice Caps : 0x04201000\n\t\tVideo Capture Multiplanar\n\t\tStreaming\n\t\tExtended Pix Format\n\n>\n> >> +\tbool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }\n> >> +};\n> >> +\n> >> +class V4L2Device\n> >> +{\n> >> +public:\n> >> +\tV4L2Device() = delete;\n> >\n> > Should this be private?\n>\n> I wondered that - but I figured - it doesn't matter if it's public or\n> private. It's declaring that it's gone. And I thought it was better to\n> keep the constructors (or lack of) and destructors together.\n>\n> If this is something we want to standardize (keeping deletions in\n> private:) I can move it.\n>\n\nNo big deal, right...\n\n>\n> >\n> >> +\tV4L2Device(const std::string &devnode);\n> >> +\t~V4L2Device();\n> >> +\n> >> +\tint open();\n> >> +\tbool isOpen() const;\n> >> +\tvoid close();\n> >> +\n> >> +private:\n> >> +\tstd::string devnode_;\n> >> +\tint fd_;\n> >> +\tV4L2Capability caps_;\n> >> +};\n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index abf9a71d4172..f9f25c0ecf15 100644\n> >> --- a/src/libcamera/meson.build\n> >> +++ b/src/libcamera/meson.build\n> >> @@ -11,6 +11,7 @@ libcamera_sources = files([\n> >> 'pipeline_handler.cpp',\n> >> 'signal.cpp',\n> >> 'timer.cpp',\n> >> + 'v4l2_device.cpp',\n> >> ])\n> >>\n> >> libcamera_headers = files([\n> >> @@ -21,6 +22,7 @@ libcamera_headers = files([\n> >> 'include/media_object.h',\n> >> 'include/pipeline_handler.h',\n> >> 'include/utils.h',\n> >> + 'include/v4l2_device.h',\n> >> ])\n> >>\n> >> libcamera_internal_includes = include_directories('include')\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> new file mode 100644\n> >> index 000000000000..8c49d3ff56e2\n> >> --- /dev/null\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -0,0 +1,150 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * v4l2_device.cpp - V4L2 Device\n> >> + */\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \"log.h\"\n> >> +#include \"v4l2_device.h\"\n> >> +\n> >> +/**\n> >> + * \\file v4l2_device.h\n> >> + * \\brief V4L2 Device API\n> >> + */\n> >> +namespace libcamera {\n> >> +\n> >> +/**\n> >> + * \\class V4L2Capability\n> >> + * \\brief struct v4l2_capability object wrapper and helpers\n> >> + *\n> >> + * The V4L2Capability class manages the information returned by the\n> >> + * VIDIOC_QUERYCAP ioctl.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn std::string V4L2Capability::getDriver()\n> >> + * \\brief Retrieve the driver module name\n> >> + * \\return The string containing the name of the driver module\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn std::string V4L2Capability::getCard()\n> >> + * \\brief Retrieve the device card name\n> >> + * \\return The string containing the device name\n> >\n> > s/card/device ?\n>\n> I want to respect the existing naming of the V4L2 API. It's called a\n> card there.\n>\n> It can be exposed through the V4L2Device API as a deviceName() as\n> mentioned above.\n>\n>\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn bool V4L2Capability::isCapture()\n> >> + * \\brief Identify if the device is capable of capturing video\n> >> + * \\return boolean true if the device provides video frames\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn bool V4L2Capability::hasStreaming()\n> >> + * \\brief Determine if the device can perform Streaming IO\n> >> + * \\return boolean true if the device provides Streaming IO IOCTLs\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\class V4L2Device\n> >> + * \\brief V4L2Device object and API.\n> >> + *\n> >> + * The V4L2 Device API class models an instance of a V4L2 device node.\n> >> + * It is constructed with the path to a V4L2 video device node. The device node\n> >> + * is only opened upon a call to open() which must be checked for success.\n> >> + *\n> >> + * The device capabilities are validated and the device is rejected if it is\n> >> + * not a suitable capture device.\n> >> + *\n> >> + * No API call (except open(), isOpen() and close()) shall be called on an\n> >> + * unopened device instance.\n> >> + *\n> >> + * Upon destruction any device left open will be closed, and any resources\n> >> + * released.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Construct a V4L2Device\n> >> + * \\param devnode The file-system path to the video device node\n> >> + */\n> >> +V4L2Device::V4L2Device(const std::string &devnode)\n> >> +\t: devnode_(devnode), fd_(-1)\n> >> +{\n> >> +}\n> >> +\n> >> +V4L2Device::~V4L2Device()\n> >> +{\n> >> +\tclose();\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Open a V4L2 device and query properties from the device.\n> >> + * \\return 0 on success, or a negative error code otherwise\n> >> + */\n> >> +int V4L2Device::open()\n> >> +{\n> >> +\tint ret;\n> >> +\n> >> +\tif (isOpen()) {\n> >> +\t\tLOG(Error) << \"Device already open\";\n> >> +\t\treturn -EBUSY;\n> >> +\t}\n> >> +\n> >> +\tret = ::open(devnode_.c_str(), O_RDWR);\n> >> +\tif (ret < 0) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> >> +\t\t\t << \" : \" << strerror(ret);\n> >\n> > strerror(-ret)\n> >\n> > Even if in this case you could have used errno directly. Sorry, my\n> > comment might has mis-lead you\n>\n> Ahh oops - and good spot.\n>\n> But yes - I'd much rather reference the errno directly for strerror().\n>\n>\n> >\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\tfd_ = ret;\n> >> +\n> >> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n> >\n> > As you pointed out, this might be the compiler performing down-casting\n> > to a plain 'struct v4l2_capability'\n>\n> Yes, which is the desired result.\n>\n> >\n> >> +\tif (ret < 0) {\n> >> +\t\tLOG(Error) << \"Failed to query device capabilities: \" << strerror(errno);\n> >> +\t\treturn -errno;\n> >> +\t}\n> >> +\n> >> +\tLOG(Debug) << \"Opened '\" << devnode_ << \"' \" << caps_.getDriver() << \": \" << caps_.getCard();\n> >> +\n> >> +\tif (!(caps_.isCapture())) {\n> >\n> > if (!caps_.isCapture())\n>\n> Yes :)\n>\n> >\n> >> +\t\tLOG(Error) << \"Device is not a capture device\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\tif (!(caps_.hasStreaming())) {\n>\n> And here...\n>\n> >> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Check if device is successfully opened\n> >> + */\n> >> +bool V4L2Device::isOpen() const\n> >> +{\n> >> +\treturn (fd_ != -1);\n> >\n> > Ah, I see what you've done here (return with no () )\n> >\n> > \tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n>\n> Ah I've been caught! (ok well it wasn't intentional hehe)\n>\n> I /almost/ want to add () to the isCapture() but I don't want to\n> increase that line length...\n>\n> /me cries a little and does:\n>\n> s/(fd_ != -1)/fd != -1/\n>\n\nAs you like the most!\n\nThanks\n j\n\n> >\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Close the device, releasing any resources acquired by open()\n> >> + */\n> >> +void V4L2Device::close()\n> >> +{\n> >> +\tif (fd_ < 0)\n> >> +\t\treturn;\n> >> +\n> >> +\t::close(fd_);\n> >> +\tfd_ = -1;\n> >> +}\n> >> +\n> >> +} /* namespace libcamera */\n> >> --\n> >> 2.17.1\n> >>\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88D2E60B2E\n\tfor ;\n\tWed, 16 Jan 2019 12:11:34 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id F349E40004;\n\tWed, 16 Jan 2019 11:11:33 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 16 Jan 2019 12:11:43 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190116111143.jconorwmbr2fzh6i@uno.localdomain>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>\n\t<8bf2984f-cf86-3bc9-2fc3-cd65782da598@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"wpd5b7prmdymiykp\"","Content-Disposition":"inline","In-Reply-To":"<8bf2984f-cf86-3bc9-2fc3-cd65782da598@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 11:11:34 -0000"}},{"id":358,"web_url":"https://patchwork.libcamera.org/comment/358/","msgid":"","date":"2019-01-16T12:07:23","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 16/01/2019 11:11, Jacopo Mondi wrote:\n\n\n>>\n>> The getDriver() and getCard() wraps the casting required again into the\n>> class definition so that it doesn't have to be done in arbitrary places\n>> in the code.\n>>\n>> I expect to add next:\n>>\n>> V4L2Device::driverName() { return caps_.getDriver(); }\n>> V4L2Device::deviceName() { return caps_.getCard(); }\n>> \t// Or ? getDeviceName()?\n>> V4L2Device::busName() { return caps_.getBus(); }\n>> \t// (Yes, I'll add this one in to the V4L2Capabiltiy)\n>>\n>> I anticipate that these strings will be useful to you in the pipeline\n>> classes.\n>>\n>> The UVC pipeline manager should certainly use them.\n>>\n> \n> One note I missed in both my previous comments: getter methods are named as\n> the member field they access, without the \"get\" prefix.\n\nI can't do that here.\nIf I name the method the same as the base struct member I get:\n\n../src/libcamera/include/v4l2_device.h: In member function\n‘std::__cxx11::string libcamera::V4L2Capability::driver() const’:\n../src/libcamera/include/v4l2_device.h:23:58: error: invalid use of\nmember function ‘std::__cxx11::string\nlibcamera::V4L2Capability::driver() const’ (did you forget the ‘()’ ?)\n std::string driver() const { return std::string((char *)driver); }\n\n\n>>> I won't push anymore on this though.\n>>\n>> I don't mind the push ... I knew this 'wrapping' would be potentially\n>> controversial it's partly me trying to experiment with good ways to\n>> interact with the Kernel API.\n>>\n>> At the moment, I must say I really like it though ...\n>> \t \n>>\n>> Especially with the ease and cleanliness for exposing the capability\n>> strings.\n>>\n> \n> I'll let other express concerns, if any. Otherwise that's fine with\n> me.\n\nOk thanks,\n\n\n>>>> +\n>>>> +\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n>>>\n>>> You pointed out in your self-review this was meant to be changed,\n>>> right?\n>>\n>> Half-right.\n>>\n>> Then based on your other comments regarding MPLANE/Single Plane I\n>> decided to drop MPLANE support.\n>>\n>> We don't need it yet, the IPU3 doesn't support MPLANE, the RCAR-VIN\n>> doesn't support MPLANE, UVC doesn't support MPLANE ...\n>>\n> \n> Are you sure?\n> \n> - entity 4: ipu3-cio2 0 (1 pad, 1 link)\n> type Node subtype V4L flags 0\n> device node name /dev/video0\n> \n> \n> # v4l2-ctl -D -d /dev/video0\n> Driver Info (not using libv4l2):\n> \tDriver name : ipu3-cio2\n> \tCard type : Intel IPU3 CIO2\n> \tBus info : PCI:0000:00:14.3\n> \tDriver version: 4.20.0\n> \tCapabilities : 0x84201000\n> \t\tVideo Capture Multiplanar <---------------\n\nHrm ... my grep foo failed me then ...\n\nOK - so MPLANE /is/ back on the requirements list :)\n\n> \t\tStreaming\n> \t\tExtended Pix Format\n> \t\tDevice Capabilities\n> \tDevice Caps : 0x04201000\n> \t\tVideo Capture Multiplanar\n> \t\tStreaming\n> \t\tExtended Pix Format\n> \n>>\n>>>> +\tbool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }\n>>>> +};\n>>>> +\n>>>> +class V4L2Device\n>>>> +{\n>>>> +public:\n>>>> +\tV4L2Device() = delete;\n>>>\n>>> Should this be private?\n>>\n>> I wondered that - but I figured - it doesn't matter if it's public or\n>> private. It's declaring that it's gone. And I thought it was better to\n>> keep the constructors (or lack of) and destructors together.\n>>\n>> If this is something we want to standardize (keeping deletions in\n>> private:) I can move it.\n>>\n> \n> No big deal, right...\n\nI also realise I should add a deletion for the copy constructor...\n\n\n\n\n\n>>>> +/**\n>>>> + * \\brief Open a V4L2 device and query properties from the device.\n>>>> + * \\return 0 on success, or a negative error code otherwise\n>>>> + */\n>>>> +int V4L2Device::open()\n>>>> +{\n>>>> +\tint ret;\n>>>> +\n>>>> +\tif (isOpen()) {\n>>>> +\t\tLOG(Error) << \"Device already open\";\n>>>> +\t\treturn -EBUSY;\n>>>> +\t}\n>>>> +\n>>>> +\tret = ::open(devnode_.c_str(), O_RDWR);\n>>>> +\tif (ret < 0) {\n>>>> +\t\tret = -errno;\n\nThis assignment feels a little redundant.\n\nI agree on only setting the fd_ = ret if success is good - but I'm\nfeeling like I can save a line (think of the bits) and just return -errno?\n\n\n>>>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n>>>> +\t\t\t << \" : \" << strerror(ret);\n>>>\n>>> strerror(-ret)\n>>>\n>>> Even if in this case you could have used errno directly. Sorry, my\n>>> comment might has mis-lead you\n>>\n>> Ahh oops - and good spot.\n>>\n>> But yes - I'd much rather reference the errno directly for strerror().\n>>\n>>\n>>>\n>>>> +\t\treturn ret;\n>>>> +\t}\n>>>> +\tfd_ = ret;\n>>>> +\n>>>> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n>>>\n\n\n\n>>>> +/**\n>>>> + * \\brief Check if device is successfully opened\n>>>> + */\n>>>> +bool V4L2Device::isOpen() const\n>>>> +{\n>>>> +\treturn (fd_ != -1);\n>>>\n>>> Ah, I see what you've done here (return with no () )\n>>>\n>>> \tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n>>\n>> Ah I've been caught! (ok well it wasn't intentional hehe)\n>>\n>> I /almost/ want to add () to the isCapture() but I don't want to\n>> increase that line length...\n>>\n>> /me cries a little and does:\n>>\n>> s/(fd_ != -1)/fd != -1/\n>>\n> \n> As you like the most!\n\nI've removed the brackets to simplify.\n\n\n> \n> Thanks\n> j\n\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 3CFE060B2E\n\tfor ;\n\tWed, 16 Jan 2019 13:07:28 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47ED64F8;\n\tWed, 16 Jan 2019 13:07:27 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547640447;\n\tbh=0d7P0WT108fPZdgbrgIFa0vrI73tHi2iYf9M/3Fu754=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=MkhXmQK9tfQ9VqyygR0XWihZ/XvwD9Ztct7RJyI8v+XPC33FLGPkxxKWujItaCHk8\n\tajMdkvV2FBpVsu7UdHax+ePWdEllYTWISgUb57sB+dhg7ChgJJrhxo47L2NvETUWCn\n\t823dMJTCIEPf3S2Wmu1DaPiN+DTRI/PiwfCBU9yM=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi ","Cc":"LibCamera Devel ","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>\n\t<8bf2984f-cf86-3bc9-2fc3-cd65782da598@ideasonboard.com>\n\t<20190116111143.jconorwmbr2fzh6i@uno.localdomain>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 16 Jan 2019 12:07:23 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20190116111143.jconorwmbr2fzh6i@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 12:07:28 -0000"}},{"id":360,"web_url":"https://patchwork.libcamera.org/comment/360/","msgid":"<20190116130322.qpzmjwenhiuav4cm@uno.localdomain>","date":"2019-01-16T13:03:22","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Wed, Jan 16, 2019 at 12:07:23PM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 16/01/2019 11:11, Jacopo Mondi wrote:\n>\n> \n> >>\n> >> The getDriver() and getCard() wraps the casting required again into the\n> >> class definition so that it doesn't have to be done in arbitrary places\n> >> in the code.\n> >>\n> >> I expect to add next:\n> >>\n> >> V4L2Device::driverName() { return caps_.getDriver(); }\n> >> V4L2Device::deviceName() { return caps_.getCard(); }\n> >> \t// Or ? getDeviceName()?\n> >> V4L2Device::busName() { return caps_.getBus(); }\n> >> \t// (Yes, I'll add this one in to the V4L2Capabiltiy)\n> >>\n> >> I anticipate that these strings will be useful to you in the pipeline\n> >> classes.\n> >>\n> >> The UVC pipeline manager should certainly use them.\n> >>\n> >\n> > One note I missed in both my previous comments: getter methods are named as\n> > the member field they access, without the \"get\" prefix.\n>\n> I can't do that here.\n> If I name the method the same as the base struct member I get:\n>\n> ../src/libcamera/include/v4l2_device.h: In member function\n> ‘std::__cxx11::string libcamera::V4L2Capability::driver() const’:\n> ../src/libcamera/include/v4l2_device.h:23:58: error: invalid use of\n> member function ‘std::__cxx11::string\n> libcamera::V4L2Capability::driver() const’ (did you forget the ‘()’ ?)\n> std::string driver() const { return std::string((char *)driver); }\n>\n\nAh! that's because of the v4l2_capability 'driver' field.\nThat would be called driver_ in a standalone V4L2Capabilities class...\n>\n> >>> I won't push anymore on this though.\n> >>\n> >> I don't mind the push ... I knew this 'wrapping' would be potentially\n> >> controversial it's partly me trying to experiment with good ways to\n> >> interact with the Kernel API.\n> >>\n> >> At the moment, I must say I really like it though ...\n> >> \t \n> >>\n> >> Especially with the ease and cleanliness for exposing the capability\n> >> strings.\n> >>\n> >\n> > I'll let other express concerns, if any. Otherwise that's fine with\n> > me.\n>\n> Ok thanks,\n>\n>\n> >>>> +\n> >>>> +\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n> >>>\n> >>> You pointed out in your self-review this was meant to be changed,\n> >>> right?\n> >>\n> >> Half-right.\n> >>\n> >> Then based on your other comments regarding MPLANE/Single Plane I\n> >> decided to drop MPLANE support.\n> >>\n> >> We don't need it yet, the IPU3 doesn't support MPLANE, the RCAR-VIN\n> >> doesn't support MPLANE, UVC doesn't support MPLANE ...\n> >>\n> >\n> > Are you sure?\n> >\n> > - entity 4: ipu3-cio2 0 (1 pad, 1 link)\n> > type Node subtype V4L flags 0\n> > device node name /dev/video0\n> >\n> >\n> > # v4l2-ctl -D -d /dev/video0\n> > Driver Info (not using libv4l2):\n> > \tDriver name : ipu3-cio2\n> > \tCard type : Intel IPU3 CIO2\n> > \tBus info : PCI:0000:00:14.3\n> > \tDriver version: 4.20.0\n> > \tCapabilities : 0x84201000\n> > \t\tVideo Capture Multiplanar <---------------\n>\n> Hrm ... my grep foo failed me then ...\n>\n> OK - so MPLANE /is/ back on the requirements list :)\n>\n\nYes please. MPLANE support should be there from the very beginning\nimho.\n\n> > \t\tStreaming\n> > \t\tExtended Pix Format\n> > \t\tDevice Capabilities\n> > \tDevice Caps : 0x04201000\n> > \t\tVideo Capture Multiplanar\n> > \t\tStreaming\n> > \t\tExtended Pix Format\n> >\n> >>\n> >>>> +\tbool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }\n> >>>> +};\n> >>>> +\n> >>>> +class V4L2Device\n> >>>> +{\n> >>>> +public:\n> >>>> +\tV4L2Device() = delete;\n> >>>\n> >>> Should this be private?\n> >>\n> >> I wondered that - but I figured - it doesn't matter if it's public or\n> >> private. It's declaring that it's gone. And I thought it was better to\n> >> keep the constructors (or lack of) and destructors together.\n> >>\n> >> If this is something we want to standardize (keeping deletions in\n> >> private:) I can move it.\n> >>\n> >\n> > No big deal, right...\n>\n> I also realise I should add a deletion for the copy constructor...\n>\n> \n>\n> \n\nGood point. Thanks.\n\n>\n> >>>> +/**\n> >>>> + * \\brief Open a V4L2 device and query properties from the device.\n> >>>> + * \\return 0 on success, or a negative error code otherwise\n> >>>> + */\n> >>>> +int V4L2Device::open()\n> >>>> +{\n> >>>> +\tint ret;\n> >>>> +\n> >>>> +\tif (isOpen()) {\n> >>>> +\t\tLOG(Error) << \"Device already open\";\n> >>>> +\t\treturn -EBUSY;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tret = ::open(devnode_.c_str(), O_RDWR);\n> >>>> +\tif (ret < 0) {\n> >>>> +\t\tret = -errno;\n>\n> This assignment feels a little redundant.\n>\n> I agree on only setting the fd_ = ret if success is good - but I'm\n> feeling like I can save a line (think of the bits) and just return -errno?\n>\n>\n> >>>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> >>>> +\t\t\t << \" : \" << strerror(ret);\n> >>>\n> >>> strerror(-ret)\n> >>>\n> >>> Even if in this case you could have used errno directly. Sorry, my\n> >>> comment might has mis-lead you\n> >>\n> >> Ahh oops - and good spot.\n> >>\n> >> But yes - I'd much rather reference the errno directly for strerror().\n> >>\n> >>\n> >>>\n> >>>> +\t\treturn ret;\n> >>>> +\t}\n> >>>> +\tfd_ = ret;\n> >>>> +\n> >>>> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n> >>>\n>\n> \n>\n> >>>> +/**\n> >>>> + * \\brief Check if device is successfully opened\n> >>>> + */\n> >>>> +bool V4L2Device::isOpen() const\n> >>>> +{\n> >>>> +\treturn (fd_ != -1);\n> >>>\n> >>> Ah, I see what you've done here (return with no () )\n> >>>\n> >>> \tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n> >>\n> >> Ah I've been caught! (ok well it wasn't intentional hehe)\n> >>\n> >> I /almost/ want to add () to the isCapture() but I don't want to\n> >> increase that line length...\n> >>\n> >> /me cries a little and does:\n> >>\n> >> s/(fd_ != -1)/fd != -1/\n> >>\n> >\n> > As you like the most!\n>\n> I've removed the brackets to simplify.\n\nThanks\n j\n\n>\n>\n> >\n> > Thanks\n> > j\n>\n> \n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA19D60B2E\n\tfor ;\n\tWed, 16 Jan 2019 14:03:13 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 37EC040003;\n\tWed, 16 Jan 2019 13:03:13 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 16 Jan 2019 14:03:22 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190116130322.qpzmjwenhiuav4cm@uno.localdomain>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>\n\t<8bf2984f-cf86-3bc9-2fc3-cd65782da598@ideasonboard.com>\n\t<20190116111143.jconorwmbr2fzh6i@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"yv523vvxoyvswegb\"","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 13:03:13 -0000"}},{"id":363,"web_url":"https://patchwork.libcamera.org/comment/363/","msgid":"<20190116145423.GB6484@bigcity.dyn.berto.se>","date":"2019-01-16T14:54:23","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran, Jacopo\n\n\n\nOn 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:\n\n[snip]\n\n> > +int V4L2Device::open()\n> > +{\n> > +\tint ret;\n> > +\n> > +\tif (isOpen()) {\n> > +\t\tLOG(Error) << \"Device already open\";\n> > +\t\treturn -EBUSY;\n> > +\t}\n> > +\n> > +\tret = ::open(devnode_.c_str(), O_RDWR);\n> > +\tif (ret < 0) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> > +\t\t\t << \" : \" << strerror(ret);\n> \n> strerror(-ret)\n> \n> Even if in this case you could have used errno directly. Sorry, my\n> comment might has mis-lead you\n\nNo you can't use errno in the call to strerror() as it might already \nhave been been changed by the previous strings passed to LOG(Error), \nunfortunately it needs to be cached.\n\n> \n> > +\t\treturn ret;\n> > +\t}","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9060460B2D\n\tfor ;\n\tWed, 16 Jan 2019 15:54:25 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id a8so5113608lfk.5\n\tfor ;\n\tWed, 16 Jan 2019 06:54:25 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tb69sm1244666lfl.28.2019.01.16.06.54.23\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 16 Jan 2019 06:54:23 -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=Nyr5OGFfUoj2hJ8sGlO7wY/hTLX0IeVtXbVgKcdQKvI=;\n\tb=d0WZWFXoMpFeCVmjkBiyHE7PnI0Ji3u58nOElt80XiqzVYYGpJEQmpZnoVCdowQD5R\n\tuE68e8V5Gb7494STQbecfs57FdbEpmBL7LejD94wBTNyMSLG32PWVEiwtL/afR93FYXR\n\txMi2MlDZFxNPAgQpVMVKPI+W7kmERi5+6urh+5/l8287QVxg6Dk9mmfoYd+vhGSe6PyS\n\tusVfaEwYz4gGD+l/OmtPuxtRT8XP99nyf0U9VkIvGRU0s/e6mH7EXrS7lfV1IJuVhOUA\n\t2+2eo//m5cFPdp0MhFDpk3UZgNrsZf9izsnUiNMZ/CG/hC44nsI/ZMGOkEEdKOANgB9j\n\tI9Fw==","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=Nyr5OGFfUoj2hJ8sGlO7wY/hTLX0IeVtXbVgKcdQKvI=;\n\tb=j2yKVUxQ4fYX8370pDVgIfEuyxqrsK8jLRw2xncuX3bpOHqElZE7cl2F0ljGXBgeqi\n\tN6r/+f2lIXk5qXbBV01r4ASMHCz0esgZh/HgWj0NP4/B2E9P/KCZi0ZLA7ftbVpXS7kz\n\tTJZcsBmlBXm6A71MbWmpKlYP7KGw0GWB2y2IwiTkjAeYxxEUY/6P+aa6f+bqCQQJEWfb\n\tjdE03L8eURGvRuXCu8Q2+jssntKKUUvTj6yon1wKAtOO9vJTLeRn1CXsw65ZMNq2yt5A\n\tFRgsRXKufqQNEEnoPQE6jSZxchVv/7KCRBGnb+1jB8NqplKo8FjTPH2cQaQ6vLa0Bje8\n\tpdnw==","X-Gm-Message-State":"AJcUukdfIK7O8wujLZwSnYNKJYd8LVzg4fH3I7kbxUHZxwBdslw0cSjx\n\thmQir01HMe8H9hHxtPbEm1IZOw==","X-Google-Smtp-Source":"ALg8bN7i/Y8faRlEPORmALBdkq/p1/NS/2xUnYcfssym7aQBXdbdGm3l8jKIxMLbbe653QOByIrVEQ==","X-Received":"by 2002:a19:5402:: with SMTP id i2mr7159802lfb.128.1547650464824;\n\tWed, 16 Jan 2019 06:54:24 -0800 (PST)","Date":"Wed, 16 Jan 2019 15:54:23 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"Kieran Bingham ,\n\tLibCamera Devel ","Message-ID":"<20190116145423.GB6484@bigcity.dyn.berto.se>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190116084455.6h7oxekekbtsledw@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 14:54:25 -0000"}},{"id":364,"web_url":"https://patchwork.libcamera.org/comment/364/","msgid":"<20190116145943.GC6484@bigcity.dyn.berto.se>","date":"2019-01-16T14:59:43","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nThanks for your work.\n\nAs Jacopo already provided a rich set of comments I try to comment on \ndifferent things.\n\nOn 2019-01-15 23:19:08 +0000, Kieran Bingham wrote:\n> Provide a helper V4L2 device object capable of interacting with the\n> V4L2 Linux Kernel APIs.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/include/v4l2_device.h | 45 +++++++++\n> src/libcamera/meson.build | 2 +\n> src/libcamera/v4l2_device.cpp | 150 ++++++++++++++++++++++++++++\n> 3 files changed, 197 insertions(+)\n> create mode 100644 src/libcamera/include/v4l2_device.h\n> create mode 100644 src/libcamera/v4l2_device.cpp\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> new file mode 100644\n> index 000000000000..b0f5e9689654\n> --- /dev/null\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_device.h - V4L2 Device\n> + */\n> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> +#define __LIBCAMERA_V4L2_DEVICE_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Capability : public v4l2_capability\n> +{\n> +public:\n> +\tstd::string getDriver() const { return std::string((char *)driver); }\n> +\tstd::string getCard() const { return std::string((char *)card); }\n\nI'm not loving this. I think this is a deal breaker in this case to \ninherit from struct v4l2_capability. I would make the struct a private \nmember of the class and make this drivre() and card().\n\n> +\n> +\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n> +\tbool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }\n\nNot saying it's wrong but it confuses me a bit. Why is one capability \ncheck prefixed with 'is' and the other 'has'?\n\n> +};\n> +\n> +class V4L2Device\n> +{\n> +public:\n> +\tV4L2Device() = delete;\n> +\tV4L2Device(const std::string &devnode);\n> +\t~V4L2Device();\n> +\n> +\tint open();\n> +\tbool isOpen() const;\n> +\tvoid close();\n> +\n> +private:\n> +\tstd::string devnode_;\n> +\tint fd_;\n> +\tV4L2Capability caps_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index abf9a71d4172..f9f25c0ecf15 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -11,6 +11,7 @@ libcamera_sources = files([\n> 'pipeline_handler.cpp',\n> 'signal.cpp',\n> 'timer.cpp',\n> + 'v4l2_device.cpp',\n> ])\n> \n> libcamera_headers = files([\n> @@ -21,6 +22,7 @@ libcamera_headers = files([\n> 'include/media_object.h',\n> 'include/pipeline_handler.h',\n> 'include/utils.h',\n> + 'include/v4l2_device.h',\n> ])\n> \n> libcamera_internal_includes = include_directories('include')\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> new file mode 100644\n> index 000000000000..8c49d3ff56e2\n> --- /dev/null\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -0,0 +1,150 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_device.cpp - V4L2 Device\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"v4l2_device.h\"\n> +\n> +/**\n> + * \\file v4l2_device.h\n> + * \\brief V4L2 Device API\n> + */\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class V4L2Capability\n> + * \\brief struct v4l2_capability object wrapper and helpers\n> + *\n> + * The V4L2Capability class manages the information returned by the\n> + * VIDIOC_QUERYCAP ioctl.\n> + */\n> +\n> +/**\n> + * \\fn std::string V4L2Capability::getDriver()\n> + * \\brief Retrieve the driver module name\n> + * \\return The string containing the name of the driver module\n> + */\n> +\n> +/**\n> + * \\fn std::string V4L2Capability::getCard()\n> + * \\brief Retrieve the device card name\n> + * \\return The string containing the device name\n> + */\n> +\n> +/**\n> + * \\fn bool V4L2Capability::isCapture()\n> + * \\brief Identify if the device is capable of capturing video\n> + * \\return boolean true if the device provides video frames\n> + */\n> +\n> +/**\n> + * \\fn bool V4L2Capability::hasStreaming()\n> + * \\brief Determine if the device can perform Streaming IO\n> + * \\return boolean true if the device provides Streaming IO IOCTLs\n> + */\n> +\n> +/**\n> + * \\class V4L2Device\n> + * \\brief V4L2Device object and API.\n> + *\n> + * The V4L2 Device API class models an instance of a V4L2 device node.\n> + * It is constructed with the path to a V4L2 video device node. The device node\n> + * is only opened upon a call to open() which must be checked for success.\n> + *\n> + * The device capabilities are validated and the device is rejected if it is\n> + * not a suitable capture device.\n> + *\n> + * No API call (except open(), isOpen() and close()) shall be called on an\n> + * unopened device instance.\n> + *\n> + * Upon destruction any device left open will be closed, and any resources\n> + * released.\n> + */\n> +\n> +/**\n> + * \\brief Construct a V4L2Device\n> + * \\param devnode The file-system path to the video device node\n> + */\n> +V4L2Device::V4L2Device(const std::string &devnode)\n> +\t: devnode_(devnode), fd_(-1)\n> +{\n> +}\n> +\n> +V4L2Device::~V4L2Device()\n> +{\n> +\tclose();\n> +}\n> +\n> +/**\n> + * \\brief Open a V4L2 device and query properties from the device.\n> + * \\return 0 on success, or a negative error code otherwise\n> + */\n> +int V4L2Device::open()\n> +{\n> +\tint ret;\n> +\n> +\tif (isOpen()) {\n> +\t\tLOG(Error) << \"Device already open\";\n> +\t\treturn -EBUSY;\n> +\t}\n> +\n> +\tret = ::open(devnode_.c_str(), O_RDWR);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> +\t\t\t << \" : \" << strerror(ret);\n> +\t\treturn ret;\n> +\t}\n> +\tfd_ = ret;\n> +\n> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n> +\tif (ret < 0) {\n> +\t\tLOG(Error) << \"Failed to query device capabilities: \" << strerror(errno);\n> +\t\treturn -errno;\n> +\t}\n> +\n> +\tLOG(Debug) << \"Opened '\" << devnode_ << \"' \" << caps_.getDriver() << \": \" << caps_.getCard();\n> +\n> +\tif (!(caps_.isCapture())) {\n> +\t\tLOG(Error) << \"Device is not a capture device\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (!(caps_.hasStreaming())) {\n> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Check if device is successfully opened\n> + */\n> +bool V4L2Device::isOpen() const\n> +{\n> +\treturn (fd_ != -1);\n> +}\n> +\n> +/**\n> + * \\brief Close the device, releasing any resources acquired by open()\n> + */\n> +void V4L2Device::close()\n> +{\n> +\tif (fd_ < 0)\n> +\t\treturn;\n> +\n> +\t::close(fd_);\n> +\tfd_ = -1;\n> +}\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.17.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-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81D6A60B2D\n\tfor ;\n\tWed, 16 Jan 2019 15:59:45 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id v1-v6so5730780ljd.0\n\tfor ;\n\tWed, 16 Jan 2019 06:59:45 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\t1-v6sm1043091lju.61.2019.01.16.06.59.43\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 16 Jan 2019 06:59:43 -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=AUry+OzrZQsc9IwT6hleeHWN0RBpcOBUyQrx1OoWaQA=;\n\tb=KfQZiGRhkbL9LMWQUBQp8No90pa+tNPqwQjl9grpXEN4Z463lw3/b3mopM7wOC6/dy\n\tAtsbXD1fJcyG5FDxRqcF+nsVtkgBzmuxRvGTjndffHqCvIMDfTilIKxfk0vrfd7zdKq0\n\tktq3uUdxHV5Aydqz+yNel6VfDzotHsg3wV/q0j36BnyB7RBH9KF7GyuDngJheCL7hUw0\n\tpYSbNDscxIkB81BKtSsrp/vuZkZxG/NdpE8a33WQMOIt73X1RhQcAjoR3kkmRIjGP8wG\n\twAGh3UCvG32DIjXNbMQ+GymdSxK+a6/3q5jUJpQ/mvbdQJfIh18ta4ImFQOc1vJQc25g\n\trVug==","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=AUry+OzrZQsc9IwT6hleeHWN0RBpcOBUyQrx1OoWaQA=;\n\tb=oEZZxy/lM5pGGOR0LtcNHcE55OGzvNWudclTDVR9+58Rg2xi8TNQNRZ1vtK49YTbav\n\tTig8L4gK2gWXNHp5aDelRuIdwJ/+mWxiQ0JcPXhUc3XmO0bIg5xEr7kefRTvCZSwAXjj\n\tDPl9irputvfYeKZg6k8KlRiN5oHQNbBXqYrbAkkveQOhLnwjgZGjlqaB0c5+TwEhx6WV\n\tZf+70ogT2UGs3rM9C0S7X97v8DkQJftg82hwNl7kyOll8RUb1Sh10S8LkNTSNX6BM1Bw\n\tIqQYDMXuJwuilF5RSOBdZ7qHKg4gVquv43UtB5MxwBVv76WCUJWMiuMolXAylmAgIG4g\n\tFmoQ==","X-Gm-Message-State":"AJcUukevrrb5UMW8jAU1fN431yW5/vWnnAnOf3ZFpfOsz1hmVZYxTqW9\n\t99KMLA2GxjTvCkrF3RmLg1kj9qlQAp8=","X-Google-Smtp-Source":"ALg8bN4u/E/9EGMN2f8vCTKBEJPZ5803HInCTTlhnVSaPAAQmid+lg6Eb0Yf87eOqjUpYXMahZ+nZQ==","X-Received":"by 2002:a2e:9f0b:: with SMTP id\n\tu11-v6mr6326906ljk.99.1547650784636; \n\tWed, 16 Jan 2019 06:59:44 -0800 (PST)","Date":"Wed, 16 Jan 2019 15:59:43 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190116145943.GC6484@bigcity.dyn.berto.se>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190115231909.19893-3-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 14:59:45 -0000"}},{"id":365,"web_url":"https://patchwork.libcamera.org/comment/365/","msgid":"<20190116150049.hstngawcoaxnrhvp@uno.localdomain>","date":"2019-01-16T15:00:49","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Jan 16, 2019 at 03:54:23PM +0100, Niklas Söderlund wrote:\n> Hi Kieran, Jacopo\n>\n>\n>\n> On 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:\n>\n> [snip]\n>\n> > > +int V4L2Device::open()\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\tif (isOpen()) {\n> > > +\t\tLOG(Error) << \"Device already open\";\n> > > +\t\treturn -EBUSY;\n> > > +\t}\n> > > +\n> > > +\tret = ::open(devnode_.c_str(), O_RDWR);\n> > > +\tif (ret < 0) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> > > +\t\t\t << \" : \" << strerror(ret);\n> >\n> > strerror(-ret)\n> >\n> > Even if in this case you could have used errno directly. Sorry, my\n> > comment might has mis-lead you\n>\n> No you can't use errno in the call to strerror() as it might already\n> have been been changed by the previous strings passed to LOG(Error),\n> unfortunately it needs to be cached.\n>\n\nRight, that's why we standardized on that patter... sorry, I've been\nsloppy commenting this...\n\nKieran, pay attention to this in the IOCTL error handling you have a\nfew lines below these ones...\n\nThanks\n j\n\n> >\n> > > +\t\treturn ret;\n> > > +\t}\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D9BFD60B2D\n\tfor ;\n\tWed, 16 Jan 2019 16:00:40 +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 relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 46CBB1BF219;\n\tWed, 16 Jan 2019 15:00:40 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 16 Jan 2019 16:00:49 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Kieran Bingham ,\n\tLibCamera Devel ","Message-ID":"<20190116150049.hstngawcoaxnrhvp@uno.localdomain>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>\n\t<20190116145423.GB6484@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"cfhhcbkfgbwl4utz\"","Content-Disposition":"inline","In-Reply-To":"<20190116145423.GB6484@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 15:00:41 -0000"}},{"id":373,"web_url":"https://patchwork.libcamera.org/comment/373/","msgid":"<4b274a4b-a6e9-19c8-2334-ee39e8e57d15@ideasonboard.com>","date":"2019-01-16T15:42:50","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 16/01/2019 15:00, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Jan 16, 2019 at 03:54:23PM +0100, Niklas Söderlund wrote:\n>> Hi Kieran, Jacopo\n>>\n>>\n>>\n>> On 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:\n>>\n>> [snip]\n>>\n>>>> +int V4L2Device::open()\n>>>> +{\n>>>> +\tint ret;\n>>>> +\n>>>> +\tif (isOpen()) {\n>>>> +\t\tLOG(Error) << \"Device already open\";\n>>>> +\t\treturn -EBUSY;\n>>>> +\t}\n>>>> +\n>>>> +\tret = ::open(devnode_.c_str(), O_RDWR);\n>>>> +\tif (ret < 0) {\n>>>> +\t\tret = -errno;\n>>>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n>>>> +\t\t\t << \" : \" << strerror(ret);\n>>>\n>>> strerror(-ret)\n>>>\n>>> Even if in this case you could have used errno directly. Sorry, my\n>>> comment might has mis-lead you\n>>\n>> No you can't use errno in the call to strerror() as it might already\n>> have been been changed by the previous strings passed to LOG(Error),\n>> unfortunately it needs to be cached.\n>>\n> \n> Right, that's why we standardized on that patter... sorry, I've been\n> sloppy commenting this...\n> \n> Kieran, pay attention to this in the IOCTL error handling you have a\n> few lines below these ones...\n> \n\nOk - updated. Can I change the location of the negative inversions?\n\nSetting ret = -errno, to then re-negate it in strerror() feels horrible\nto me.\n\nNegating it for the return code seems more acceptable as we know that\nreturn codes are checked for if(ret < 0)...\n\n\n\n{\n\t...\n\n\tret = ::open(devnode_.c_str(), O_RDWR);\n\tif (ret < 0) {\n\t\tret = errno;\n\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n\t\t\t << \" : \" << strerror(ret);\n\t\treturn -ret;\n\t}\n\tfd_ = ret;\n\n\tret = ioctl(fd_, VIDIOC_QUERYCAP, caps_.caps());\n\tif (ret < 0) {\n\t\tret = errno;\n\t\tLOG(Error) << \"Failed to query device capabilities: \" << strerror(ret);\n\t\treturn -ret;\n\t}\n}\n\n\n\n> Thanks\n> j\n> \n>>>\n>>>> +\t\treturn ret;\n>>>> +\t}\n>>\n>> --\n>> Regards,\n>> Niklas Söderlund","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 4C22060B2D\n\tfor ;\n\tWed, 16 Jan 2019 16:42:53 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3AA34F8;\n\tWed, 16 Jan 2019 16:42:52 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547653373;\n\tbh=MvyBDw5vlkzYhq5hcd3B3ukabfgV64r7EIxgH3DNIoQ=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=C8HwF0OJpx9ojmVcgcF7PjT1Kac2j+IZDOIJp4FUmt4jEKqHZqTu1uLdVm1JvR/MH\n\tM8acYdFcpBTqxCwgN1RPwdff3tL+8qMiT+liQ/G8b02X48g92q+Av5/tteN3CTOKIf\n\tT/EXNclWg6lF+geuVoqxlpIzqo4fDMuZUizGE/is=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi , =?utf-8?q?Niklas_S=C3=B6derlund?=\n\t","Cc":"LibCamera Devel ","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>\n\t<20190116145423.GB6484@bigcity.dyn.berto.se>\n\t<20190116150049.hstngawcoaxnrhvp@uno.localdomain>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<4b274a4b-a6e9-19c8-2334-ee39e8e57d15@ideasonboard.com>","Date":"Wed, 16 Jan 2019 15:42:50 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20190116150049.hstngawcoaxnrhvp@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 15:42:53 -0000"}},{"id":378,"web_url":"https://patchwork.libcamera.org/comment/378/","msgid":"<1e1badb9-89fc-3b00-241f-7f627e86da4e@ideasonboard.com>","date":"2019-01-16T18:00:55","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOk - here's an attempt at explaining my rationale and also hopefully\nsome updates which might make you like things more (I've dropped the 'get')\n\nOn 16/01/2019 14:59, Niklas Söderlund wrote:\n> Hi Kieran,\n> \n> Thanks for your work.\n> \n> As Jacopo already provided a rich set of comments I try to comment on \n> different things.\n> \n> On 2019-01-15 23:19:08 +0000, Kieran Bingham wrote:\n>> Provide a helper V4L2 device object capable of interacting with the\n>> V4L2 Linux Kernel APIs.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/libcamera/include/v4l2_device.h | 45 +++++++++\n>> src/libcamera/meson.build | 2 +\n>> src/libcamera/v4l2_device.cpp | 150 ++++++++++++++++++++++++++++\n>> 3 files changed, 197 insertions(+)\n>> create mode 100644 src/libcamera/include/v4l2_device.h\n>> create mode 100644 src/libcamera/v4l2_device.cpp\n>>\n>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n>> new file mode 100644\n>> index 000000000000..b0f5e9689654\n>> --- /dev/null\n>> +++ b/src/libcamera/include/v4l2_device.h\n>> @@ -0,0 +1,45 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * v4l2_device.h - V4L2 Device\n>> + */\n>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n>> +#define __LIBCAMERA_V4L2_DEVICE_H__\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +class V4L2Capability : public v4l2_capability\n>> +{\n>> +public:\n>> +\tstd::string getDriver() const { return std::string((char *)driver); }\n>> +\tstd::string getCard() const { return std::string((char *)card); }\n> \n> I'm not loving this. I think this is a deal breaker in this case to \n> inherit from struct v4l2_capability. I would make the struct a private \n> member of the class and make this drivre() and card().\n\nCan you explain /why/ you think it's a deal breaker please?\n\n\nIs your objection to the 'get' prefix (as in getDriver())?\nI now have a solution for that with appropriate casting:\n\n std::string driver() const {\n\treturn reinterpret_cast(v4l2_capability::driver);\n }\n\n\nOtherwise, I believe it does inherit from v4l2_capability.\n\nIf I could I would write write:\n\nstruct v4l2_capability : struct v4l2_capability {\npublic:\n\tstd::string driver() const;\n\tstd::string card() const;\n\tbool isCapture() const;\n};\n\n\nThe point is if I could add methods to the struct v4l2_capability, I'd\nbe doing that ... but I can't. And I certainly don't want to duplicate\nthe structure...\n\nThe V4L2Capability is simply providing an interface at the C++ level and\ndefining how to work with the data in the structure in an object\norientated way.\n\nThis means that any methods which need to interact with struct\nv4l2_capability are contained in a common location - and those methods\ncan only operate on the structure ...\n\nPutting a v4l2_capability as a private member in a class V4L2Capability\nproduces: [2] which I quite dislike. I've updated V4L2Capability to mark\nit final to express that this class should not be further extended or\noverridden.\n\n[1] https://paste.debian.net/1060853/ {inheritance}\n[2] https://paste.debian.net/1060852/ {composition}\n[3] https://paste.debian.net/1060839/ {flat in V4L2Device}\n\n\nMy biggest reason for continuing to push this - is because I feel this\npattern will be beneficial for the many complex structure types in V4L2.\nI'm thinking controls, pix_format, fmtdesc, requestbuffers...\n\n(Not everything, just ones where it makes sense to define helpers or\nbring the kernel API to a C++ interface)\n\nThey are all 'objects' which are managed by the kernel. It's just that\nthe data for them is in a c-struct because that's what the kernel has.\n\nIn my opinion - handling those types as objects in the C++ will keep the\ncode where it counts. On the object it deals with.\n\nIf we go with [3] (which I don't want), then the V4L2Device class is\ngoing to be polluted with all sorts of helpers (helpful as they may be)\nwhich operate on the various structures. It's also no good in the case\nwhere there are multiple objects (like v4l2_buffers).\n\n\nI don't think [2] is appropriate here. It doesn't protect us from\nanything, and we are not extending any types or fields in the struct\nv4l2_capability.\n\n\n(I'd also envisage re-using this V4L2Device class externally too, so\nhaving nice wrappings around things can provide further benefits)\n\n\n\n>> +\n>> +\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n>> +\tbool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }\n> \n> Not saying it's wrong but it confuses me a bit. Why is one capability \n> check prefixed with 'is' and the other 'has'?\n\nGrammar :)\n\nThe device 'is' a capture device.\n\t(_CAPTURE | _CAPTURE_MPLANE) vs (_OUTPUT / _M2M)\n\nThe device 'has' streaming IO ioctls\nThe device 'has' async io ioctls\n\n...\n\n(Now technically an M2M 'is' both a _CAPTURE and an _OUTPUT but that's\ndifferentiated by being an M2M...)\n\n> \n>> +};\n>> +\n>> +class V4L2Device\n>> +{\n>> +public:\n>> +\tV4L2Device() = delete;\n>> +\tV4L2Device(const std::string &devnode);\n>> +\t~V4L2Device();\n>> +\n>> +\tint open();\n>> +\tbool isOpen() const;\n>> +\tvoid close();\n>> +\n>> +private:\n>> +\tstd::string devnode_;\n>> +\tint fd_;\n>> +\tV4L2Capability caps_;\n>> +};\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index abf9a71d4172..f9f25c0ecf15 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -11,6 +11,7 @@ libcamera_sources = files([\n>> 'pipeline_handler.cpp',\n>> 'signal.cpp',\n>> 'timer.cpp',\n>> + 'v4l2_device.cpp',\n>> ])\n>> \n>> libcamera_headers = files([\n>> @@ -21,6 +22,7 @@ libcamera_headers = files([\n>> 'include/media_object.h',\n>> 'include/pipeline_handler.h',\n>> 'include/utils.h',\n>> + 'include/v4l2_device.h',\n>> ])\n>> \n>> libcamera_internal_includes = include_directories('include')\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> new file mode 100644\n>> index 000000000000..8c49d3ff56e2\n>> --- /dev/null\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -0,0 +1,150 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * v4l2_device.cpp - V4L2 Device\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"log.h\"\n>> +#include \"v4l2_device.h\"\n>> +\n>> +/**\n>> + * \\file v4l2_device.h\n>> + * \\brief V4L2 Device API\n>> + */\n>> +namespace libcamera {\n>> +\n>> +/**\n>> + * \\class V4L2Capability\n>> + * \\brief struct v4l2_capability object wrapper and helpers\n>> + *\n>> + * The V4L2Capability class manages the information returned by the\n>> + * VIDIOC_QUERYCAP ioctl.\n>> + */\n>> +\n>> +/**\n>> + * \\fn std::string V4L2Capability::getDriver()\n>> + * \\brief Retrieve the driver module name\n>> + * \\return The string containing the name of the driver module\n>> + */\n>> +\n>> +/**\n>> + * \\fn std::string V4L2Capability::getCard()\n>> + * \\brief Retrieve the device card name\n>> + * \\return The string containing the device name\n>> + */\n>> +\n>> +/**\n>> + * \\fn bool V4L2Capability::isCapture()\n>> + * \\brief Identify if the device is capable of capturing video\n>> + * \\return boolean true if the device provides video frames\n>> + */\n>> +\n>> +/**\n>> + * \\fn bool V4L2Capability::hasStreaming()\n>> + * \\brief Determine if the device can perform Streaming IO\n>> + * \\return boolean true if the device provides Streaming IO IOCTLs\n>> + */\n>> +\n>> +/**\n>> + * \\class V4L2Device\n>> + * \\brief V4L2Device object and API.\n>> + *\n>> + * The V4L2 Device API class models an instance of a V4L2 device node.\n>> + * It is constructed with the path to a V4L2 video device node. The device node\n>> + * is only opened upon a call to open() which must be checked for success.\n>> + *\n>> + * The device capabilities are validated and the device is rejected if it is\n>> + * not a suitable capture device.\n>> + *\n>> + * No API call (except open(), isOpen() and close()) shall be called on an\n>> + * unopened device instance.\n>> + *\n>> + * Upon destruction any device left open will be closed, and any resources\n>> + * released.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a V4L2Device\n>> + * \\param devnode The file-system path to the video device node\n>> + */\n>> +V4L2Device::V4L2Device(const std::string &devnode)\n>> +\t: devnode_(devnode), fd_(-1)\n>> +{\n>> +}\n>> +\n>> +V4L2Device::~V4L2Device()\n>> +{\n>> +\tclose();\n>> +}\n>> +\n>> +/**\n>> + * \\brief Open a V4L2 device and query properties from the device.\n>> + * \\return 0 on success, or a negative error code otherwise\n>> + */\n>> +int V4L2Device::open()\n>> +{\n>> +\tint ret;\n>> +\n>> +\tif (isOpen()) {\n>> +\t\tLOG(Error) << \"Device already open\";\n>> +\t\treturn -EBUSY;\n>> +\t}\n>> +\n>> +\tret = ::open(devnode_.c_str(), O_RDWR);\n>> +\tif (ret < 0) {\n>> +\t\tret = -errno;\n>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n>> +\t\t\t << \" : \" << strerror(ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\tfd_ = ret;\n>> +\n>> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);\n>> +\tif (ret < 0) {\n>> +\t\tLOG(Error) << \"Failed to query device capabilities: \" << strerror(errno);\n>> +\t\treturn -errno;\n>> +\t}\n>> +\n>> +\tLOG(Debug) << \"Opened '\" << devnode_ << \"' \" << caps_.getDriver() << \": \" << caps_.getCard();\n>> +\n>> +\tif (!(caps_.isCapture())) {\n>> +\t\tLOG(Error) << \"Device is not a capture device\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (!(caps_.hasStreaming())) {\n>> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Check if device is successfully opened\n>> + */\n>> +bool V4L2Device::isOpen() const\n>> +{\n>> +\treturn (fd_ != -1);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Close the device, releasing any resources acquired by open()\n>> + */\n>> +void V4L2Device::close()\n>> +{\n>> +\tif (fd_ < 0)\n>> +\t\treturn;\n>> +\n>> +\t::close(fd_);\n>> +\tfd_ = -1;\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> -- \n>> 2.17.1\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\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 A0BEC60C83\n\tfor ;\n\tWed, 16 Jan 2019 19:00:58 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0BC3553E;\n\tWed, 16 Jan 2019 19:00:57 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547661658;\n\tbh=DKzsrztJRl8IJffzT8en7ypEWzHWQpJSCW/yQsWk5Ds=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=WJ08L7nW448DpMP+3AeE6QHfXQ1A180zWoFiXElufr5PJ7ZftcE7OzZof8NBZ2jfc\n\tw1ZRiqpt5z+0btzgyUu+aGTUwioesspbVlOfB4xbZzZm908rrt+PVKVwd0DglA7kMZ\n\t0FeqzjzWK/XpESt2XCaN1c5fNhjoQcpSZvZR94Io=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Cc":"LibCamera Devel ","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116145943.GC6484@bigcity.dyn.berto.se>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<1e1badb9-89fc-3b00-241f-7f627e86da4e@ideasonboard.com>","Date":"Wed, 16 Jan 2019 18:00:55 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20190116145943.GC6484@bigcity.dyn.berto.se>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 16 Jan 2019 18:00:58 -0000"}},{"id":379,"web_url":"https://patchwork.libcamera.org/comment/379/","msgid":"<20190117153936.GA16967@pendragon.ideasonboard.com>","date":"2019-01-17T15:39:36","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jan 16, 2019 at 12:07:23PM +0000, Kieran Bingham wrote:\n> On 16/01/2019 11:11, Jacopo Mondi wrote:\n\n[snip]\n\n> >>>> +/**\n> >>>> + * \\brief Open a V4L2 device and query properties from the device.\n> >>>> + * \\return 0 on success, or a negative error code otherwise\n> >>>> + */\n> >>>> +int V4L2Device::open()\n> >>>> +{\n> >>>> +\tint ret;\n> >>>> +\n> >>>> +\tif (isOpen()) {\n> >>>> +\t\tLOG(Error) << \"Device already open\";\n> >>>> +\t\treturn -EBUSY;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tret = ::open(devnode_.c_str(), O_RDWR);\n> >>>> +\tif (ret < 0) {\n> >>>> +\t\tret = -errno;\n> \n> This assignment feels a little redundant.\n> \n> I agree on only setting the fd_ = ret if success is good - but I'm\n> feeling like I can save a line (think of the bits) and just return -errno?\n\nYou can't because LOG(Error) << ... may change errno. That's also why\nyou need to pass -ret to strerror and not errno, as it may change by the\ntime strerror is called.\n\n> >>>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> >>>> +\t\t\t << \" : \" << strerror(ret);\n> >>>\n> >>> strerror(-ret)\n> >>>\n> >>> Even if in this case you could have used errno directly. Sorry, my\n> >>> comment might has mis-lead you\n> >>\n> >> Ahh oops - and good spot.\n> >>\n> >> But yes - I'd much rather reference the errno directly for strerror().\n> >>\n> >>>> +\t\treturn ret;\n> >>>> +\t}\n\n[snip]","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 101BD60B1D\n\tfor ;\n\tThu, 17 Jan 2019 16:39:37 +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 364AB53E;\n\tThu, 17 Jan 2019 16:39:36 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547739576;\n\tbh=+py6KXesPY9ShNTOhbOExttWLtl0LGJqG1w83eWm/Nk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aIoFE1CIv5XTU4T+CorddkbBKeq5+0izRIHOzntaJ2C3GMqKa5we+ahBddSeKhWPA\n\tS0CAt7AEl+MN+P8gfPdx4qLV7XfCgItVLgu8FlHuXpXpE66aaaL2Zqsi6GtPA9nVLS\n\t+rFaImPP2jkCkKVE461aOT50eEiSoMcNZw7QlPcM=","Date":"Thu, 17 Jan 2019 17:39:36 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tLibCamera Devel ","Message-ID":"<20190117153936.GA16967@pendragon.ideasonboard.com>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>\n\t<8bf2984f-cf86-3bc9-2fc3-cd65782da598@ideasonboard.com>\n\t<20190116111143.jconorwmbr2fzh6i@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 17 Jan 2019 15:39:37 -0000"}},{"id":380,"web_url":"https://patchwork.libcamera.org/comment/380/","msgid":"<20190117154609.GB16967@pendragon.ideasonboard.com>","date":"2019-01-17T15:46:09","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jan 16, 2019 at 06:00:55PM +0000, Kieran Bingham wrote:\n> On 16/01/2019 14:59, Niklas Söderlund wrote:\n> > On 2019-01-15 23:19:08 +0000, Kieran Bingham wrote:\n> >> Provide a helper V4L2 device object capable of interacting with the\n> >> V4L2 Linux Kernel APIs.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/libcamera/include/v4l2_device.h | 45 +++++++++\n> >> src/libcamera/meson.build | 2 +\n> >> src/libcamera/v4l2_device.cpp | 150 ++++++++++++++++++++++++++++\n> >> 3 files changed, 197 insertions(+)\n> >> create mode 100644 src/libcamera/include/v4l2_device.h\n> >> create mode 100644 src/libcamera/v4l2_device.cpp\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >> new file mode 100644\n> >> index 000000000000..b0f5e9689654\n> >> --- /dev/null\n> >> +++ b/src/libcamera/include/v4l2_device.h\n> >> @@ -0,0 +1,45 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * v4l2_device.h - V4L2 Device\n> >> + */\n> >> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> >> +#define __LIBCAMERA_V4L2_DEVICE_H__\n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +class V4L2Capability : public v4l2_capability\n> >> +{\n> >> +public:\n> >> +\tstd::string getDriver() const { return std::string((char *)driver); }\n> >> +\tstd::string getCard() const { return std::string((char *)card); }\n> > \n> > I'm not loving this. I think this is a deal breaker in this case to \n> > inherit from struct v4l2_capability. I would make the struct a private \n> > member of the class and make this drivre() and card().\n> \n> Can you explain /why/ you think it's a deal breaker please?\n> \n> \n> Is your objection to the 'get' prefix (as in getDriver())?\n> I now have a solution for that with appropriate casting:\n> \n> std::string driver() const {\n> \treturn reinterpret_cast(v4l2_capability::driver);\n> }\n> \n> \n> Otherwise, I believe it does inherit from v4l2_capability.\n> \n> If I could I would write write:\n> \n> struct v4l2_capability : struct v4l2_capability {\n> public:\n> \tstd::string driver() const;\n> \tstd::string card() const;\n> \tbool isCapture() const;\n> };\n> \n> \n> The point is if I could add methods to the struct v4l2_capability, I'd\n> be doing that ... but I can't. And I certainly don't want to duplicate\n> the structure...\n> \n> The V4L2Capability is simply providing an interface at the C++ level and\n> defining how to work with the data in the structure in an object\n> orientated way.\n> \n> This means that any methods which need to interact with struct\n> v4l2_capability are contained in a common location - and those methods\n> can only operate on the structure ...\n> \n> Putting a v4l2_capability as a private member in a class V4L2Capability\n> produces: [2] which I quite dislike. I've updated V4L2Capability to mark\n> it final to express that this class should not be further extended or\n> overridden.\n> \n> [1] https://paste.debian.net/1060853/ {inheritance}\n> [2] https://paste.debian.net/1060852/ {composition}\n> [3] https://paste.debian.net/1060839/ {flat in V4L2Device}\n> \n> \n> My biggest reason for continuing to push this - is because I feel this\n> pattern will be beneficial for the many complex structure types in V4L2.\n> I'm thinking controls, pix_format, fmtdesc, requestbuffers...\n> \n> (Not everything, just ones where it makes sense to define helpers or\n> bring the kernel API to a C++ interface)\n> \n> They are all 'objects' which are managed by the kernel. It's just that\n> the data for them is in a c-struct because that's what the kernel has.\n> \n> In my opinion - handling those types as objects in the C++ will keep the\n> code where it counts. On the object it deals with.\n> \n> If we go with [3] (which I don't want), then the V4L2Device class is\n> going to be polluted with all sorts of helpers (helpful as they may be)\n> which operate on the various structures. It's also no good in the case\n> where there are multiple objects (like v4l2_buffers).\n> \n> \n> I don't think [2] is appropriate here. It doesn't protect us from\n> anything, and we are not extending any types or fields in the struct\n> v4l2_capability.\n> \n> \n> (I'd also envisage re-using this V4L2Device class externally too, so\n> having nice wrappings around things can provide further benefits)\n\nFOr what it's worth, I think wrapping v4l2_capabilities is useful in\nthis case, and I like the proposed implementation, especially with the\nsolution that creates getters with a get prefix.\n\n> >> +\n> >> +\tbool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }\n> >> +\tbool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }\n> > \n> > Not saying it's wrong but it confuses me a bit. Why is one capability \n> > check prefixed with 'is' and the other 'has'?\n> \n> Grammar :)\n> \n> The device 'is' a capture device.\n> \t(_CAPTURE | _CAPTURE_MPLANE) vs (_OUTPUT / _M2M)\n> \n> The device 'has' streaming IO ioctls\n> The device 'has' async io ioctls\n\nAnd the device 'has' capture capability ;-) Both prefixes are valid, I'm\nslightly tempted to standardize on has in this case, but I won't push\nfor it.\n\n> ...\n> \n> (Now technically an M2M 'is' both a _CAPTURE and an _OUTPUT but that's\n> differentiated by being an M2M...)\n> \n> > \n> >> +};\n\n[snip]","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 A193760B1D\n\tfor ;\n\tThu, 17 Jan 2019 16:46:09 +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 EC46B53E;\n\tThu, 17 Jan 2019 16:46:08 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547739969;\n\tbh=YC94yG8DZgsPa7IHeRPNJH5uAF1Zpg3WrLVHdQqT02Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Uxj+Ib2K9niis4HvecDYsrIBg16nPCVDZlYBn7OEnmSW8vbmItK1XuiuGxXmbL/iU\n\tCifAhhihTnfA+jBTLVHd3G7SzWxkEgC+FMPJGvhUbKE8QmS5ubmt4PZ/zZWLEJIofu\n\tDm9KahrIub1CmM8smychGAHmbVNd7IEWhB5PJyY8=","Date":"Thu, 17 Jan 2019 17:46:09 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tLibCamera Devel ","Message-ID":"<20190117154609.GB16967@pendragon.ideasonboard.com>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116145943.GC6484@bigcity.dyn.berto.se>\n\t<1e1badb9-89fc-3b00-241f-7f627e86da4e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1e1badb9-89fc-3b00-241f-7f627e86da4e@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 17 Jan 2019 15:46:09 -0000"}},{"id":381,"web_url":"https://patchwork.libcamera.org/comment/381/","msgid":"<20190117154836.GC16967@pendragon.ideasonboard.com>","date":"2019-01-17T15:48:36","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jan 16, 2019 at 03:42:50PM +0000, Kieran Bingham wrote:\n> On 16/01/2019 15:00, Jacopo Mondi wrote:\n> > On Wed, Jan 16, 2019 at 03:54:23PM +0100, Niklas Söderlund wrote:\n> >> On 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:\n> >>\n> >> [snip]\n> >>\n> >>>> +int V4L2Device::open()\n> >>>> +{\n> >>>> +\tint ret;\n> >>>> +\n> >>>> +\tif (isOpen()) {\n> >>>> +\t\tLOG(Error) << \"Device already open\";\n> >>>> +\t\treturn -EBUSY;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tret = ::open(devnode_.c_str(), O_RDWR);\n> >>>> +\tif (ret < 0) {\n> >>>> +\t\tret = -errno;\n> >>>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> >>>> +\t\t\t << \" : \" << strerror(ret);\n> >>>\n> >>> strerror(-ret)\n> >>>\n> >>> Even if in this case you could have used errno directly. Sorry, my\n> >>> comment might has mis-lead you\n> >>\n> >> No you can't use errno in the call to strerror() as it might already\n> >> have been been changed by the previous strings passed to LOG(Error),\n> >> unfortunately it needs to be cached.\n> >>\n> > \n> > Right, that's why we standardized on that patter... sorry, I've been\n> > sloppy commenting this...\n> > \n> > Kieran, pay attention to this in the IOCTL error handling you have a\n> > few lines below these ones...\n> > \n> \n> Ok - updated. Can I change the location of the negative inversions?\n> \n> Setting ret = -errno, to then re-negate it in strerror() feels horrible\n> to me.\n> \n> Negating it for the return code seems more acceptable as we know that\n> return codes are checked for if(ret < 0)...\n> \n> {\n> \t...\n> \n> \tret = ::open(devnode_.c_str(), O_RDWR);\n> \tif (ret < 0) {\n> \t\tret = errno;\n> \t\tLOG(Error) << \"Failed to open V4L2 device \" << devnode_\n> \t\t\t << \" : \" << strerror(ret);\n> \t\treturn -ret;\n> \t}\n> \tfd_ = ret;\n> \n> \tret = ioctl(fd_, VIDIOC_QUERYCAP, caps_.caps());\n> \tif (ret < 0) {\n> \t\tret = errno;\n> \t\tLOG(Error) << \"Failed to query device capabilities: \" << strerror(ret);\n> \t\treturn -ret;\n> \t}\n> }\n\nI think both options have their pros and cons. I'm fine with this\nproposal, but in that case, please change it library-wide.","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 8FABC60B1D\n\tfor ;\n\tThu, 17 Jan 2019 16:48:36 +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 C97B153E;\n\tThu, 17 Jan 2019 16:48:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547740116;\n\tbh=iteiLq2PoaeCcFlyee0s9K0uVL82/ov7pJqFKtlhAA0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QIpubWZDpKZl8aJjeqxqfmvRq3ssLvc0cjzq8eaY8oH4IS/q1ye1isSy1mqWrNjLf\n\tYGt9Cz4MdGxA+lB/rrnhyQMbELusGpz3Xd6jxE2ANoWuILa55r9TER9hrHWsDl+7sF\n\tWfCzCm7slmExVodtvBY4YTsZwj/nt2ofgcqcD/BE=","Date":"Thu, 17 Jan 2019 17:48:36 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , Niklas =?utf-8?q?S=C3=B6derlund?=\n\t, LibCamera Devel\n\t","Message-ID":"<20190117154836.GC16967@pendragon.ideasonboard.com>","References":"<20190115231909.19893-1-kieran.bingham@ideasonboard.com>\n\t<20190115231909.19893-3-kieran.bingham@ideasonboard.com>\n\t<20190116084455.6h7oxekekbtsledw@uno.localdomain>\n\t<20190116145423.GB6484@bigcity.dyn.berto.se>\n\t<20190116150049.hstngawcoaxnrhvp@uno.localdomain>\n\t<4b274a4b-a6e9-19c8-2334-ee39e8e57d15@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4b274a4b-a6e9-19c8-2334-ee39e8e57d15@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device\n\tobject","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, 17 Jan 2019 15:48:36 -0000"}}]