[{"id":657,"web_url":"https://patchwork.libcamera.org/comment/657/","msgid":"<1e0a0257-96c5-b274-d508-8f2ceed9688e@ideasonboard.com>","date":"2019-01-28T15:42:27","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nI only have variable/layout concerns here - which is certainly not a\nblocker at the moment.\n\nAlso - if the outcome of the discussion below changes this patches\nexpectations - then we'll need to adapt other patches too - so that\nwould be a global patch fix at that point.\n\nThus I don't think that discussion blocks this patch.\n\n\nOn 28/01/2019 15:11, Jacopo Mondi wrote:\n> Add methods to set and get the image format programmed on a V4L2Device\n> for both the single and multi planar use case.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Kieran Bingham \n\n\n> ---\n> src/libcamera/include/v4l2_device.h | 10 +++\n> src/libcamera/v4l2_device.cpp | 128 ++++++++++++++++++++++++++++\n> 2 files changed, 138 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index c70959e..fe54220 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -86,10 +86,20 @@ public:\n> \tconst char *deviceName() const { return caps_.card(); }\n> \tconst char *busName() const { return caps_.bus_info(); }\n> \n> +\tint format(V4L2DeviceFormat *fmt);\n> +\tint setFormat(V4L2DeviceFormat *fmt);\n> +\n> private:\n> \tstd::string deviceNode_;\n> \tint fd_;\n> \tV4L2Capability caps_;\n> +\tenum v4l2_buf_type bufferType_;\n> +\n> +\tint getFormatSingleplane(V4L2DeviceFormat *fmt);\n> +\tint setFormatSingleplane(V4L2DeviceFormat *fmt);\n> +\n> +\tint getFormatMultiplane(V4L2DeviceFormat *fmt);\n> +\tint setFormatMultiplane(V4L2DeviceFormat *fmt);\n\n\nCan I confirm on our class layouts?\n\nHave we defined a layout anywhere?\n\nHere this creates:\n\n\nclass ClassName\n{\npublic:\n\tClassName();\n\t~ClassName();\n\tvoid publicFunctions();\n\n\tint publicData;\n\tbool publicBool;\n\nprivate:\n\tint privateData;\n\tbool privateBool;\n\n\tint privateFunctions();\n\tint morePrivateFunctions();\n}\n\nWhich is:\n\tPublicFunctions\n\tPublicData\n\tPrivateData\n\tPrivateFunctions\n\n\nSo the 'data' both public and private is sandwiched in the middle.\nI don't mind this - but in my mind I thought we were doing:\n\n\nclass ClassName\n{\npublic:\n\tClassName();\n\t~ClassName();\n\tvoid publicFunctions();\n\n\tint publicData;\n\tbool publicBool;\n\nprivate:\n\tint privateFunctions();\n\tint morePrivateFunctions();\n\n\tint privateData;\n\tbool privateBool;\n}\n\nWhich is:\n\tPublicFunctions\n\tPublicData\n\tPrivateFunctions\n\tPrivateData\n\nso that the public, and private (or protected) sections follow the same\nsequence?\n\nAny comments from the team here?\n\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index d6143f2..5c415d0 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -227,6 +227,15 @@ int V4L2Device::open()\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tif (caps_.isCapture())\n> +\t\tbufferType_ = caps_.isMultiplanar()\n> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> +\t\t\t : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> +\telse\n> +\t\tbufferType_ = caps_.isMultiplanar()\n> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> +\t\t\t : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> +\n> \treturn 0;\n> }\n> \n> @@ -269,4 +278,123 @@ void V4L2Device::close()\n> * \\return The string containing the device location\n> */\n> \n> +/**\n> + * \\brief Retrieve the image format set on the V4L2 device\n> + * \\return 0 for success, a negative error code otherwise\n> + */\n> +int V4L2Device::format(V4L2DeviceFormat *fmt)\n> +{> +\treturn caps_.isMultiplanar() ? getFormatMultiplane(fmt) :\n\nI think the precedence is that the : would be below the ? here ?\n\nBut I'm doubting myself - so hopefully Laurent will chime in with his\npreferences here.\n\n\nIf so perhaps we should set the following change in .clang-format:\n\n-BreakBeforeTernaryOperators: false\n+BreakBeforeTernaryOperators: true\n\nwhich would produce the following:\n\n\n+ return caps_.isMultiplanar() ? getFormatMultiplane(fmt)\n+ : getFormatSingleplane(fmt);\n\n\n\nIt doesn't however align the ? with the = in the bufferType_ above:\n\n bufferType_ = caps_.isMultiplanar()\n ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n\nSo again - I think that's a non-blocker currently.\n\n\n> +\t\t\t\t getFormatSingleplane(fmt);\n> +}\n> +\n> +/**\n> + * \\brief Configure an image format on the V4L2 device\n> + * \\return 0 for success, a negative error code otherwise\n> + */\n> +int V4L2Device::setFormat(V4L2DeviceFormat *fmt)\n> +{\n> +\treturn caps_.isMultiplanar() ? setFormatMultiplane(fmt) :\n> +\t\t\t\t setFormatSingleplane(fmt);\n> +}\n> +\n> +int V4L2Device::getFormatSingleplane(V4L2DeviceFormat *fmt)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = bufferType_;\n> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfmt->width = pix->width;\n> +\tfmt->height = pix->height;\n> +\tfmt->fourcc = pix->pixelformat;\n> +\tfmt->planes = 1;\n> +\tfmt->planesFmt[0].bpl = pix->bytesperline;\n> +\tfmt->planesFmt[0].size = pix->sizeimage;\n> +\n\nI think we might need to cache the V4L2DeviceFormat in the V4L2Device\nsomewhere...\n\nBut perhaps that's not required by this patch - if something needs to\naccess this (like the BufferPool creation) it can handle caching the\nformat object.\n\n\n\n> +\treturn 0;\n> +}\n> +\n> +int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *fmt)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = bufferType_;\n> +\tpix->width = fmt->width;\n> +\tpix->height = fmt->height;\n> +\tpix->pixelformat = fmt->fourcc;\n> +\n> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2Device::getFormatMultiplane(V4L2DeviceFormat *fmt)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = bufferType_;\n> +\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Fmt);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfmt->width = pix->width;\n> +\tfmt->height = pix->height;\n> +\tfmt->fourcc = pix->pixelformat;\n> +\tfmt->planes = pix->num_planes;\n> +\n> +\tfor (unsigned int i = 0; i < fmt->planes; ++i) {\n> +\t\tfmt->planesFmt[i].bpl = pix->plane_fmt[i].bytesperline;\n> +\t\tfmt->planesFmt[i].size = pix->plane_fmt[i].sizeimage;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *fmt)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = bufferType_;\n> +\tpix->width = fmt->width;\n> +\tpix->height = fmt->height;\n> +\tpix->pixelformat = fmt->fourcc;\n> +\tpix->num_planes = fmt->planes;\n> +\n> +\tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n> +\t\tpix->plane_fmt[i].bytesperline = fmt->planesFmt[i].bpl;\n> +\t\tpix->plane_fmt[i].sizeimage = fmt->planesFmt[i].size;\n> +\t}\n> +\n> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> } /* namespace libcamera */\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 0150C60DB6\n\tfor ;\n\tMon, 28 Jan 2019 16:42:30 +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 76E1185;\n\tMon, 28 Jan 2019 16:42:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548690149;\n\tbh=t4HkXWG6J93KiFewHJeBzZgcmOv2gQ8gmp3I8MK2akk=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Tk6V0IlB/d7Rh8DkX8ngByoVbRvqdzXoUgQOkbxkGbusnfFGSVVe6joN2iZDpfQaR\n\tQWhQorL74xWqQqujAe78vR4EEemqtrf3kShTQ2SGArbqBTP825uGwtysFvvrYS5qMZ\n\tpRgo+afPqvg3UdYYj96oP4uMjA7DQjXBaFUM5qcc=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20190128151137.31075-1-jacopo@jmondi.org>\n\t<20190128151137.31075-3-jacopo@jmondi.org>","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":"<1e0a0257-96c5-b274-d508-8f2ceed9688e@ideasonboard.com>","Date":"Mon, 28 Jan 2019 15:42:27 +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":"<20190128151137.31075-3-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","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":"Mon, 28 Jan 2019 15:42:30 -0000"}},{"id":659,"web_url":"https://patchwork.libcamera.org/comment/659/","msgid":"<20190128160732.ziirvt4umxpssrwi@uno.localdomain>","date":"2019-01-28T16:07:32","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Mon, Jan 28, 2019 at 03:42:27PM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> I only have variable/layout concerns here - which is certainly not a\n> blocker at the moment.\n>\n> Also - if the outcome of the discussion below changes this patches\n> expectations - then we'll need to adapt other patches too - so that\n> would be a global patch fix at that point.\n>\n> Thus I don't think that discussion blocks this patch.\n>\n>\n> On 28/01/2019 15:11, Jacopo Mondi wrote:\n> > Add methods to set and get the image format programmed on a V4L2Device\n> > for both the single and multi planar use case.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > ---\n> > src/libcamera/include/v4l2_device.h | 10 +++\n> > src/libcamera/v4l2_device.cpp | 128 ++++++++++++++++++++++++++++\n> > 2 files changed, 138 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index c70959e..fe54220 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -86,10 +86,20 @@ public:\n> > \tconst char *deviceName() const { return caps_.card(); }\n> > \tconst char *busName() const { return caps_.bus_info(); }\n> >\n> > +\tint format(V4L2DeviceFormat *fmt);\n> > +\tint setFormat(V4L2DeviceFormat *fmt);\n> > +\n> > private:\n> > \tstd::string deviceNode_;\n> > \tint fd_;\n> > \tV4L2Capability caps_;\n> > +\tenum v4l2_buf_type bufferType_;\n> > +\n> > +\tint getFormatSingleplane(V4L2DeviceFormat *fmt);\n> > +\tint setFormatSingleplane(V4L2DeviceFormat *fmt);\n> > +\n> > +\tint getFormatMultiplane(V4L2DeviceFormat *fmt);\n> > +\tint setFormatMultiplane(V4L2DeviceFormat *fmt);\n>\n>\n> Can I confirm on our class layouts?\n>\n> Have we defined a layout anywhere?\n>\n> Here this creates:\n>\n>\n> class ClassName\n> {\n> public:\n> \tClassName();\n> \t~ClassName();\n> \tvoid publicFunctions();\n>\n> \tint publicData;\n> \tbool publicBool;\n>\n> private:\n> \tint privateData;\n> \tbool privateBool;\n>\n> \tint privateFunctions();\n> \tint morePrivateFunctions();\n> }\n>\n> Which is:\n> \tPublicFunctions\n> \tPublicData\n> \tPrivateData\n> \tPrivateFunctions\n>\n>\n> So the 'data' both public and private is sandwiched in the middle.\n> I don't mind this - but in my mind I thought we were doing:\n>\n>\n> class ClassName\n> {\n> public:\n> \tClassName();\n> \t~ClassName();\n> \tvoid publicFunctions();\n>\n> \tint publicData;\n> \tbool publicBool;\n>\n> private:\n> \tint privateFunctions();\n> \tint morePrivateFunctions();\n>\n> \tint privateData;\n> \tbool privateBool;\n> }\n>\n> Which is:\n> \tPublicFunctions\n> \tPublicData\n> \tPrivateFunctions\n> \tPrivateData\n>\n> so that the public, and private (or protected) sections follow the same\n> sequence?\n>\n> Any comments from the team here?\n\nQuoting the Google's C++ style guide (which I refer to because we\nactually started from there when we defined our coding guidelines):\nhttps://google.github.io/styleguide/cppguide.html#Declaration_Order\n\n... generally prefer the following order: types (including typedef, using,\nand nested structs and classes), constants, factory functions, constructors,\nassignment operators, destructor, all other methods, data members.\n\nSo your\n \tPublicFunctions\n \tPublicData\n \tPrivateFunctions\n \tPrivateData\n\nIs accurate.\n\nI fear we would need a library-wide cleanup, but I can start by making\nthis right at least.\n\n>\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index d6143f2..5c415d0 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -227,6 +227,15 @@ int V4L2Device::open()\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > +\tif (caps_.isCapture())\n> > +\t\tbufferType_ = caps_.isMultiplanar()\n> > +\t\t\t ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> > +\t\t\t : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> > +\telse\n> > +\t\tbufferType_ = caps_.isMultiplanar()\n> > +\t\t\t ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> > +\t\t\t : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -269,4 +278,123 @@ void V4L2Device::close()\n> > * \\return The string containing the device location\n> > */\n> >\n> > +/**\n> > + * \\brief Retrieve the image format set on the V4L2 device\n> > + * \\return 0 for success, a negative error code otherwise\n> > + */\n> > +int V4L2Device::format(V4L2DeviceFormat *fmt)\n> > +{> +\treturn caps_.isMultiplanar() ? getFormatMultiplane(fmt) :\n>\n> I think the precedence is that the : would be below the ? here ?\n>\n> But I'm doubting myself - so hopefully Laurent will chime in with his\n> preferences here.\n>\n>\n> If so perhaps we should set the following change in .clang-format:\n>\n> -BreakBeforeTernaryOperators: false\n> +BreakBeforeTernaryOperators: true\n>\n> which would produce the following:\n>\n>\n> + return caps_.isMultiplanar() ? getFormatMultiplane(fmt)\n> + : getFormatSingleplane(fmt);\n>\n>\n>\n> It doesn't however align the ? with the = in the bufferType_ above:\n>\n> bufferType_ = caps_.isMultiplanar()\n> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>\n> So again - I think that's a non-blocker currently.\n\nWhat makes the style checker and most of the team happy, makes me\nhappy, so I'm open to all this solutions.\n\n>\n>\n> > +\t\t\t\t getFormatSingleplane(fmt);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Configure an image format on the V4L2 device\n> > + * \\return 0 for success, a negative error code otherwise\n> > + */\n> > +int V4L2Device::setFormat(V4L2DeviceFormat *fmt)\n> > +{\n> > +\treturn caps_.isMultiplanar() ? setFormatMultiplane(fmt) :\n> > +\t\t\t\t setFormatSingleplane(fmt);\n> > +}\n> > +\n> > +int V4L2Device::getFormatSingleplane(V4L2DeviceFormat *fmt)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = bufferType_;\n> > +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfmt->width = pix->width;\n> > +\tfmt->height = pix->height;\n> > +\tfmt->fourcc = pix->pixelformat;\n> > +\tfmt->planes = 1;\n> > +\tfmt->planesFmt[0].bpl = pix->bytesperline;\n> > +\tfmt->planesFmt[0].size = pix->sizeimage;\n> > +\n>\n> I think we might need to cache the V4L2DeviceFormat in the V4L2Device\n> somewhere...\n>\n\nYes, indeed. I would have had a V4L2DataFormat member initialized at\ndevice 'open()' time that could be cached, and re-freshed at every\ns_fmt..\n\n> But perhaps that's not required by this patch - if something needs to\n> access this (like the BufferPool creation) it can handle caching the\n> format object.\n>\n\nI left it out from this two patches to ease integration, but I agree\nthis is a possible optimization, and if any class needs that, it shall\nbe done here.\n\nThanks\n j\n>\n>\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *fmt)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = bufferType_;\n> > +\tpix->width = fmt->width;\n> > +\tpix->height = fmt->height;\n> > +\tpix->pixelformat = fmt->fourcc;\n> > +\n> > +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2Device::getFormatMultiplane(V4L2DeviceFormat *fmt)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = bufferType_;\n> > +\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Fmt);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfmt->width = pix->width;\n> > +\tfmt->height = pix->height;\n> > +\tfmt->fourcc = pix->pixelformat;\n> > +\tfmt->planes = pix->num_planes;\n> > +\n> > +\tfor (unsigned int i = 0; i < fmt->planes; ++i) {\n> > +\t\tfmt->planesFmt[i].bpl = pix->plane_fmt[i].bytesperline;\n> > +\t\tfmt->planesFmt[i].size = pix->plane_fmt[i].sizeimage;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *fmt)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = bufferType_;\n> > +\tpix->width = fmt->width;\n> > +\tpix->height = fmt->height;\n> > +\tpix->pixelformat = fmt->fourcc;\n> > +\tpix->num_planes = fmt->planes;\n> > +\n> > +\tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > +\t\tpix->plane_fmt[i].bytesperline = fmt->planesFmt[i].bpl;\n> > +\t\tpix->plane_fmt[i].sizeimage = fmt->planesFmt[i].size;\n> > +\t}\n> > +\n> > +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > } /* namespace libcamera */\n> >\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7538060DB7\n\tfor ;\n\tMon, 28 Jan 2019 17:07:17 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id 0085D24000E;\n\tMon, 28 Jan 2019 16:07:16 +0000 (UTC)"],"Date":"Mon, 28 Jan 2019 17:07:32 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190128160732.ziirvt4umxpssrwi@uno.localdomain>","References":"<20190128151137.31075-1-jacopo@jmondi.org>\n\t<20190128151137.31075-3-jacopo@jmondi.org>\n\t<1e0a0257-96c5-b274-d508-8f2ceed9688e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"fewmpo4lz6strtag\"","Content-Disposition":"inline","In-Reply-To":"<1e0a0257-96c5-b274-d508-8f2ceed9688e@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","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":"Mon, 28 Jan 2019 16:07:17 -0000"}},{"id":688,"web_url":"https://patchwork.libcamera.org/comment/688/","msgid":"<7ddbd6a4-333d-9df3-a984-2a46290bd115@ideasonboard.com>","date":"2019-01-29T14:01:03","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 28/01/2019 16:07, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Mon, Jan 28, 2019 at 03:42:27PM +0000, Kieran Bingham wrote:\n>> Hi Jacopo,\n>>\n>> I only have variable/layout concerns here - which is certainly not a\n>> blocker at the moment.\n>>\n>> Also - if the outcome of the discussion below changes this patches\n>> expectations - then we'll need to adapt other patches too - so that\n>> would be a global patch fix at that point.\n>>\n>> Thus I don't think that discussion blocks this patch.\n>>\n>>\n>> On 28/01/2019 15:11, Jacopo Mondi wrote:\n>>> Add methods to set and get the image format programmed on a V4L2Device\n>>> for both the single and multi planar use case.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>\n>>> ---\n>>> src/libcamera/include/v4l2_device.h | 10 +++\n>>> src/libcamera/v4l2_device.cpp | 128 ++++++++++++++++++++++++++++\n>>> 2 files changed, 138 insertions(+)\n>>>\n>>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n>>> index c70959e..fe54220 100644\n>>> --- a/src/libcamera/include/v4l2_device.h\n>>> +++ b/src/libcamera/include/v4l2_device.h\n>>> @@ -86,10 +86,20 @@ public:\n>>> \tconst char *deviceName() const { return caps_.card(); }\n>>> \tconst char *busName() const { return caps_.bus_info(); }\n>>>\n>>> +\tint format(V4L2DeviceFormat *fmt);\n>>> +\tint setFormat(V4L2DeviceFormat *fmt);\n>>> +\n>>> private:\n>>> \tstd::string deviceNode_;\n>>> \tint fd_;\n>>> \tV4L2Capability caps_;\n>>> +\tenum v4l2_buf_type bufferType_;\n>>> +\n>>> +\tint getFormatSingleplane(V4L2DeviceFormat *fmt);\n>>> +\tint setFormatSingleplane(V4L2DeviceFormat *fmt);\n>>> +\n>>> +\tint getFormatMultiplane(V4L2DeviceFormat *fmt);\n>>> +\tint setFormatMultiplane(V4L2DeviceFormat *fmt);\n>>\n>>\n>> Can I confirm on our class layouts?\n>>\n>> Have we defined a layout anywhere?\n>>\n>> Here this creates:\n>>\n>>\n>> class ClassName\n>> {\n>> public:\n>> \tClassName();\n>> \t~ClassName();\n>> \tvoid publicFunctions();\n>>\n>> \tint publicData;\n>> \tbool publicBool;\n>>\n>> private:\n>> \tint privateData;\n>> \tbool privateBool;\n>>\n>> \tint privateFunctions();\n>> \tint morePrivateFunctions();\n>> }\n>>\n>> Which is:\n>> \tPublicFunctions\n>> \tPublicData\n>> \tPrivateData\n>> \tPrivateFunctions\n>>\n>>\n>> So the 'data' both public and private is sandwiched in the middle.\n>> I don't mind this - but in my mind I thought we were doing:\n>>\n>>\n>> class ClassName\n>> {\n>> public:\n>> \tClassName();\n>> \t~ClassName();\n>> \tvoid publicFunctions();\n>>\n>> \tint publicData;\n>> \tbool publicBool;\n>>\n>> private:\n>> \tint privateFunctions();\n>> \tint morePrivateFunctions();\n>>\n>> \tint privateData;\n>> \tbool privateBool;\n>> }\n>>\n>> Which is:\n>> \tPublicFunctions\n>> \tPublicData\n>> \tPrivateFunctions\n>> \tPrivateData\n>>\n>> so that the public, and private (or protected) sections follow the same\n>> sequence?\n>>\n>> Any comments from the team here?\n> \n> Quoting the Google's C++ style guide (which I refer to because we\n> actually started from there when we defined our coding guidelines):\n> https://google.github.io/styleguide/cppguide.html#Declaration_Order\n> \n> ... generally prefer the following order: types (including typedef, using,\n> and nested structs and classes), constants, factory functions, constructors,\n> assignment operators, destructor, all other methods, data members.\n> \n> So your\n> \tPublicFunctions\n> \tPublicData\n> \tPrivateFunctions\n> \tPrivateData\n> \n> Is accurate.\n> \n> I fear we would need a library-wide cleanup, but I can start by making\n> this right at least.\n> \n\nAha - great - something was right in my head :)\n\nI'd be happy with a push to master with this ordering corrected.\n\n--\nThanks\n\nKieran\n\n\n>>\n>>> };\n>>>\n>>> } /* namespace libcamera */\n>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>> index d6143f2..5c415d0 100644\n>>> --- a/src/libcamera/v4l2_device.cpp\n>>> +++ b/src/libcamera/v4l2_device.cpp\n>>> @@ -227,6 +227,15 @@ int V4L2Device::open()\n>>> \t\treturn -EINVAL;\n>>> \t}\n>>>\n>>> +\tif (caps_.isCapture())\n>>> +\t\tbufferType_ = caps_.isMultiplanar()\n>>> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n>>> +\t\t\t : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n>>> +\telse\n>>> +\t\tbufferType_ = caps_.isMultiplanar()\n>>> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>>> +\t\t\t : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>>> +\n>>> \treturn 0;\n>>> }\n>>>\n>>> @@ -269,4 +278,123 @@ void V4L2Device::close()\n>>> * \\return The string containing the device location\n>>> */\n>>>\n>>> +/**\n>>> + * \\brief Retrieve the image format set on the V4L2 device\n>>> + * \\return 0 for success, a negative error code otherwise\n>>> + */\n>>> +int V4L2Device::format(V4L2DeviceFormat *fmt)\n>>> +{> +\treturn caps_.isMultiplanar() ? getFormatMultiplane(fmt) :\n>>\n>> I think the precedence is that the : would be below the ? here ?\n>>\n>> But I'm doubting myself - so hopefully Laurent will chime in with his\n>> preferences here.\n>>\n>>\n>> If so perhaps we should set the following change in .clang-format:\n>>\n>> -BreakBeforeTernaryOperators: false\n>> +BreakBeforeTernaryOperators: true\n>>\n>> which would produce the following:\n>>\n>>\n>> + return caps_.isMultiplanar() ? getFormatMultiplane(fmt)\n>> + : getFormatSingleplane(fmt);\n>>\n>>\n>>\n>> It doesn't however align the ? with the = in the bufferType_ above:\n>>\n>> bufferType_ = caps_.isMultiplanar()\n>> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>> : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n>> : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n>>\n>> So again - I think that's a non-blocker currently.\n> \n> What makes the style checker and most of the team happy, makes me\n> happy, so I'm open to all this solutions.\n\nI think the : on the new line makes sense, as it ensures it can't be\nconfused to be a ; at the end of the line.\n\nThe indentation - well I think that's authors choice at this stage.\nEither vertically aligned, or whatever makes clang-format happy.\n\n\n\n> \n>>\n>>\n>>> +\t\t\t\t getFormatSingleplane(fmt);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Configure an image format on the V4L2 device\n>>> + * \\return 0 for success, a negative error code otherwise\n>>> + */\n>>> +int V4L2Device::setFormat(V4L2DeviceFormat *fmt)\n>>> +{\n>>> +\treturn caps_.isMultiplanar() ? setFormatMultiplane(fmt) :\n>>> +\t\t\t\t setFormatSingleplane(fmt);\n>>> +}\n>>> +\n>>> +int V4L2Device::getFormatSingleplane(V4L2DeviceFormat *fmt)\n>>> +{\n>>> +\tstruct v4l2_format v4l2Fmt;\n>>> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n>>> +\tint ret;\n>>> +\n>>> +\tv4l2Fmt.type = bufferType_;\n>>> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n>>> +\tif (ret) {\n>>> +\t\tret = -errno;\n>>> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n>>> +\t\treturn ret;\n>>> +\t}\n>>> +\n>>> +\tfmt->width = pix->width;\n>>> +\tfmt->height = pix->height;\n>>> +\tfmt->fourcc = pix->pixelformat;\n>>> +\tfmt->planes = 1;\n>>> +\tfmt->planesFmt[0].bpl = pix->bytesperline;\n>>> +\tfmt->planesFmt[0].size = pix->sizeimage;\n>>> +\n>>\n>> I think we might need to cache the V4L2DeviceFormat in the V4L2Device\n>> somewhere...\n>>\n> \n> Yes, indeed. I would have had a V4L2DataFormat member initialized at\n> device 'open()' time that could be cached, and re-freshed at every\n> s_fmt..\n> \n>> But perhaps that's not required by this patch - if something needs to\n>> access this (like the BufferPool creation) it can handle caching the\n>> format object.\n>>\n> \n> I left it out from this two patches to ease integration, but I agree\n> this is a possible optimization, and if any class needs that, it shall\n> be done here.\n\nI agree - not needed by this patch. Just thinking out loud as ever.\n\n--\nKieran\n\n\n> \n> Thanks\n> j\n>>\n>>\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *fmt)\n>>> +{\n>>> +\tstruct v4l2_format v4l2Fmt;\n>>> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n>>> +\tint ret;\n>>> +\n>>> +\tv4l2Fmt.type = bufferType_;\n>>> +\tpix->width = fmt->width;\n>>> +\tpix->height = fmt->height;\n>>> +\tpix->pixelformat = fmt->fourcc;\n>>> +\n>>> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n>>> +\tif (ret) {\n>>> +\t\tret = -errno;\n>>> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n>>> +\t\treturn ret;\n>>> +\t}\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +int V4L2Device::getFormatMultiplane(V4L2DeviceFormat *fmt)\n>>> +{\n>>> +\tstruct v4l2_format v4l2Fmt;\n>>> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n>>> +\tint ret;\n>>> +\n>>> +\tv4l2Fmt.type = bufferType_;\n>>> +\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Fmt);\n>>> +\tif (ret) {\n>>> +\t\tret = -errno;\n>>> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n>>> +\t\treturn ret;\n>>> +\t}\n>>> +\n>>> +\tfmt->width = pix->width;\n>>> +\tfmt->height = pix->height;\n>>> +\tfmt->fourcc = pix->pixelformat;\n>>> +\tfmt->planes = pix->num_planes;\n>>> +\n>>> +\tfor (unsigned int i = 0; i < fmt->planes; ++i) {\n>>> +\t\tfmt->planesFmt[i].bpl = pix->plane_fmt[i].bytesperline;\n>>> +\t\tfmt->planesFmt[i].size = pix->plane_fmt[i].sizeimage;\n>>> +\t}\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *fmt)\n>>> +{\n>>> +\tstruct v4l2_format v4l2Fmt;\n>>> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n>>> +\tint ret;\n>>> +\n>>> +\tv4l2Fmt.type = bufferType_;\n>>> +\tpix->width = fmt->width;\n>>> +\tpix->height = fmt->height;\n>>> +\tpix->pixelformat = fmt->fourcc;\n>>> +\tpix->num_planes = fmt->planes;\n>>> +\n>>> +\tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n>>> +\t\tpix->plane_fmt[i].bytesperline = fmt->planesFmt[i].bpl;\n>>> +\t\tpix->plane_fmt[i].sizeimage = fmt->planesFmt[i].size;\n>>> +\t}\n>>> +\n>>> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n>>> +\tif (ret) {\n>>> +\t\tret = -errno;\n>>> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n>>> +\t\treturn ret;\n>>> +\t}\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> } /* namespace libcamera */\n>>>\n>>\n>> --\n>> Regards\n>> --\n>> Kieran","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 B384B60DB6\n\tfor ;\n\tTue, 29 Jan 2019 15:01:06 +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 1935A41;\n\tTue, 29 Jan 2019 15:01:06 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548770466;\n\tbh=80RJPELENdMiPBd7v3aoY3qcnaafcy+eTpOh6j0Z2mU=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=cIr1isPERGaUfZzKCvBW03YQ4Sw8Da2G2Pu3mfIEo0MgtJ8+6vF1Y7k705+JSUxTf\n\tR/NYXFXMtALW1msTMzTn6hfN1TjZowvoak2J+C5Y8aervk/PPPIy5JmNt3zMcWgcw5\n\tLLx1e57w+qFVFOGg24V2kh7QWI29MQK+8qMTJcEQ=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20190128151137.31075-1-jacopo@jmondi.org>\n\t<20190128151137.31075-3-jacopo@jmondi.org>\n\t<1e0a0257-96c5-b274-d508-8f2ceed9688e@ideasonboard.com>\n\t<20190128160732.ziirvt4umxpssrwi@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":"<7ddbd6a4-333d-9df3-a984-2a46290bd115@ideasonboard.com>","Date":"Tue, 29 Jan 2019 14:01:03 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.4.0","MIME-Version":"1.0","In-Reply-To":"<20190128160732.ziirvt4umxpssrwi@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","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":"Tue, 29 Jan 2019 14:01:06 -0000"}},{"id":689,"web_url":"https://patchwork.libcamera.org/comment/689/","msgid":"<20190129145442.4gszunqreue5hiwk@uno.localdomain>","date":"2019-01-29T14:54:42","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Tue, Jan 29, 2019 at 02:01:03PM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 28/01/2019 16:07, Jacopo Mondi wrote:\n> > Hi Kieran,\n> >\n> > On Mon, Jan 28, 2019 at 03:42:27PM +0000, Kieran Bingham wrote:\n> >> Hi Jacopo,\n> >>\n> >> I only have variable/layout concerns here - which is certainly not a\n> >> blocker at the moment.\n> >>\n> >> Also - if the outcome of the discussion below changes this patches\n> >> expectations - then we'll need to adapt other patches too - so that\n> >> would be a global patch fix at that point.\n> >>\n> >> Thus I don't think that discussion blocks this patch.\n> >>\n> >>\n> >> On 28/01/2019 15:11, Jacopo Mondi wrote:\n> >>> Add methods to set and get the image format programmed on a V4L2Device\n> >>> for both the single and multi planar use case.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi \n> >>\n> >> Reviewed-by: Kieran Bingham \n> >>\n> >>\n> >>> ---\n> >>> src/libcamera/include/v4l2_device.h | 10 +++\n> >>> src/libcamera/v4l2_device.cpp | 128 ++++++++++++++++++++++++++++\n> >>> 2 files changed, 138 insertions(+)\n> >>>\n> >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >>> index c70959e..fe54220 100644\n> >>> --- a/src/libcamera/include/v4l2_device.h\n> >>> +++ b/src/libcamera/include/v4l2_device.h\n> >>> @@ -86,10 +86,20 @@ public:\n> >>> \tconst char *deviceName() const { return caps_.card(); }\n> >>> \tconst char *busName() const { return caps_.bus_info(); }\n> >>>\n> >>> +\tint format(V4L2DeviceFormat *fmt);\n> >>> +\tint setFormat(V4L2DeviceFormat *fmt);\n> >>> +\n> >>> private:\n> >>> \tstd::string deviceNode_;\n> >>> \tint fd_;\n> >>> \tV4L2Capability caps_;\n> >>> +\tenum v4l2_buf_type bufferType_;\n> >>> +\n> >>> +\tint getFormatSingleplane(V4L2DeviceFormat *fmt);\n> >>> +\tint setFormatSingleplane(V4L2DeviceFormat *fmt);\n> >>> +\n> >>> +\tint getFormatMultiplane(V4L2DeviceFormat *fmt);\n> >>> +\tint setFormatMultiplane(V4L2DeviceFormat *fmt);\n> >>\n> >>\n> >> Can I confirm on our class layouts?\n> >>\n> >> Have we defined a layout anywhere?\n> >>\n> >> Here this creates:\n> >>\n> >>\n> >> class ClassName\n> >> {\n> >> public:\n> >> \tClassName();\n> >> \t~ClassName();\n> >> \tvoid publicFunctions();\n> >>\n> >> \tint publicData;\n> >> \tbool publicBool;\n> >>\n> >> private:\n> >> \tint privateData;\n> >> \tbool privateBool;\n> >>\n> >> \tint privateFunctions();\n> >> \tint morePrivateFunctions();\n> >> }\n> >>\n> >> Which is:\n> >> \tPublicFunctions\n> >> \tPublicData\n> >> \tPrivateData\n> >> \tPrivateFunctions\n> >>\n> >>\n> >> So the 'data' both public and private is sandwiched in the middle.\n> >> I don't mind this - but in my mind I thought we were doing:\n> >>\n> >>\n> >> class ClassName\n> >> {\n> >> public:\n> >> \tClassName();\n> >> \t~ClassName();\n> >> \tvoid publicFunctions();\n> >>\n> >> \tint publicData;\n> >> \tbool publicBool;\n> >>\n> >> private:\n> >> \tint privateFunctions();\n> >> \tint morePrivateFunctions();\n> >>\n> >> \tint privateData;\n> >> \tbool privateBool;\n> >> }\n> >>\n> >> Which is:\n> >> \tPublicFunctions\n> >> \tPublicData\n> >> \tPrivateFunctions\n> >> \tPrivateData\n> >>\n> >> so that the public, and private (or protected) sections follow the same\n> >> sequence?\n> >>\n> >> Any comments from the team here?\n> >\n> > Quoting the Google's C++ style guide (which I refer to because we\n> > actually started from there when we defined our coding guidelines):\n> > https://google.github.io/styleguide/cppguide.html#Declaration_Order\n> >\n> > ... generally prefer the following order: types (including typedef, using,\n> > and nested structs and classes), constants, factory functions, constructors,\n> > assignment operators, destructor, all other methods, data members.\n> >\n> > So your\n> > \tPublicFunctions\n> > \tPublicData\n> > \tPrivateFunctions\n> > \tPrivateData\n> >\n> > Is accurate.\n> >\n> > I fear we would need a library-wide cleanup, but I can start by making\n> > this right at least.\n> >\n>\n> Aha - great - something was right in my head :)\n>\n> I'd be happy with a push to master with this ordering corrected.\n>\n\nThanks, fixed and pushed!\n\nThanks\n j","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 6C04360DB8\n\tfor ;\n\tTue, 29 Jan 2019 15:54:27 +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 E3930E000C;\n\tTue, 29 Jan 2019 14:54:26 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 29 Jan 2019 15:54:42 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190129145442.4gszunqreue5hiwk@uno.localdomain>","References":"<20190128151137.31075-1-jacopo@jmondi.org>\n\t<20190128151137.31075-3-jacopo@jmondi.org>\n\t<1e0a0257-96c5-b274-d508-8f2ceed9688e@ideasonboard.com>\n\t<20190128160732.ziirvt4umxpssrwi@uno.localdomain>\n\t<7ddbd6a4-333d-9df3-a984-2a46290bd115@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"plfomji4rsivwuws\"","Content-Disposition":"inline","In-Reply-To":"<7ddbd6a4-333d-9df3-a984-2a46290bd115@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","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":"Tue, 29 Jan 2019 14:54:27 -0000"}},{"id":697,"web_url":"https://patchwork.libcamera.org/comment/697/","msgid":"<20190130110038.GG4336@pendragon.ideasonboard.com>","date":"2019-01-30T11:00:38","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Jan 28, 2019 at 05:07:32PM +0100, Jacopo Mondi wrote:\n> On Mon, Jan 28, 2019 at 03:42:27PM +0000, Kieran Bingham wrote:\n> > Hi Jacopo,\n> >\n> > I only have variable/layout concerns here - which is certainly not a\n> > blocker at the moment.\n> >\n> > Also - if the outcome of the discussion below changes this patches\n> > expectations - then we'll need to adapt other patches too - so that\n> > would be a global patch fix at that point.\n> >\n> > Thus I don't think that discussion blocks this patch.\n> >\n> > On 28/01/2019 15:11, Jacopo Mondi wrote:\n> >> Add methods to set and get the image format programmed on a V4L2Device\n> >> for both the single and multi planar use case.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >> ---\n> >> src/libcamera/include/v4l2_device.h | 10 +++\n> >> src/libcamera/v4l2_device.cpp | 128 ++++++++++++++++++++++++++++\n> >> 2 files changed, 138 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >> index c70959e..fe54220 100644\n> >> --- a/src/libcamera/include/v4l2_device.h\n> >> +++ b/src/libcamera/include/v4l2_device.h\n> >> @@ -86,10 +86,20 @@ public:\n> >> \tconst char *deviceName() const { return caps_.card(); }\n> >> \tconst char *busName() const { return caps_.bus_info(); }\n> >>\n> >> +\tint format(V4L2DeviceFormat *fmt);\n> >> +\tint setFormat(V4L2DeviceFormat *fmt);\n> >> +\n> >> private:\n> >> \tstd::string deviceNode_;\n> >> \tint fd_;\n> >> \tV4L2Capability caps_;\n> >> +\tenum v4l2_buf_type bufferType_;\n> >> +\n> >> +\tint getFormatSingleplane(V4L2DeviceFormat *fmt);\n> >> +\tint setFormatSingleplane(V4L2DeviceFormat *fmt);\n> >> +\n> >> +\tint getFormatMultiplane(V4L2DeviceFormat *fmt);\n> >> +\tint setFormatMultiplane(V4L2DeviceFormat *fmt);\n> >\n> > Can I confirm on our class layouts?\n> >\n> > Have we defined a layout anywhere?\n> >\n> > Here this creates:\n> >\n> >\n> > class ClassName\n> > {\n> > public:\n> > \tClassName();\n> > \t~ClassName();\n> > \tvoid publicFunctions();\n> >\n> > \tint publicData;\n> > \tbool publicBool;\n> >\n> > private:\n> > \tint privateData;\n> > \tbool privateBool;\n> >\n> > \tint privateFunctions();\n> > \tint morePrivateFunctions();\n> > }\n> >\n> > Which is:\n> > \tPublicFunctions\n> > \tPublicData\n> > \tPrivateData\n> > \tPrivateFunctions\n> >\n> >\n> > So the 'data' both public and private is sandwiched in the middle.\n> > I don't mind this - but in my mind I thought we were doing:\n> >\n> >\n> > class ClassName\n> > {\n> > public:\n> > \tClassName();\n> > \t~ClassName();\n> > \tvoid publicFunctions();\n> >\n> > \tint publicData;\n> > \tbool publicBool;\n> >\n> > private:\n> > \tint privateFunctions();\n> > \tint morePrivateFunctions();\n> >\n> > \tint privateData;\n> > \tbool privateBool;\n> > }\n> >\n> > Which is:\n> > \tPublicFunctions\n> > \tPublicData\n> > \tPrivateFunctions\n> > \tPrivateData\n> >\n> > so that the public, and private (or protected) sections follow the same\n> > sequence?\n> >\n> > Any comments from the team here?\n> \n> Quoting the Google's C++ style guide (which I refer to because we\n> actually started from there when we defined our coding guidelines):\n> https://google.github.io/styleguide/cppguide.html#Declaration_Order\n> \n> ... generally prefer the following order: types (including typedef, using,\n> and nested structs and classes), constants, factory functions, constructors,\n> assignment operators, destructor, all other methods, data members.\n> \n> So your\n> \tPublicFunctions\n> \tPublicData\n> \tPrivateFunctions\n> \tPrivateData\n> \n> Is accurate.\n\nThat looks good to me.\n\n> I fear we would need a library-wide cleanup, but I can start by making\n> this right at least.\n\nThe cleanup would be quite simple, so let's do it :-)\n\n> >> };\n> >>\n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> index d6143f2..5c415d0 100644\n> >> --- a/src/libcamera/v4l2_device.cpp\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -227,6 +227,15 @@ int V4L2Device::open()\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >>\n> >> +\tif (caps_.isCapture())\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> >> +\t\t\t : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> >> +\telse\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> >> +\t\t\t : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> >> +\n> >> \treturn 0;\n> >> }\n> >>\n> >> @@ -269,4 +278,123 @@ void V4L2Device::close()\n> >> * \\return The string containing the device location\n> >> */\n> >>\n> >> +/**\n> >> + * \\brief Retrieve the image format set on the V4L2 device\n> >> + * \\return 0 for success, a negative error code otherwise\n> >> + */\n> >> +int V4L2Device::format(V4L2DeviceFormat *fmt)\n> >> +{> +\treturn caps_.isMultiplanar() ? getFormatMultiplane(fmt) :\n> >\n> > I think the precedence is that the : would be below the ? here ?\n> >\n> > But I'm doubting myself - so hopefully Laurent will chime in with his\n> > preferences here.\n> >\n> >\n> > If so perhaps we should set the following change in .clang-format:\n> >\n> > -BreakBeforeTernaryOperators: false\n> > +BreakBeforeTernaryOperators: true\n> >\n> > which would produce the following:\n> >\n> >\n> > + return caps_.isMultiplanar() ? getFormatMultiplane(fmt)\n> > + : getFormatSingleplane(fmt);\n\nI also prefer this. In general I find code more readable when the\noperator is a the beginning of the line, not the end. Jacopo, could you\nplease fix this ?\n\n> >\n> > It doesn't however align the ? with the = in the bufferType_ above:\n> >\n> > bufferType_ = caps_.isMultiplanar()\n> > ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> > : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> > ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> > : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n\nIt should definitely be aligned with the =.\n\n> > So again - I think that's a non-blocker currently.\n> \n> What makes the style checker and most of the team happy, makes me\n> happy, so I'm open to all this solutions.\n> \n> >> +\t\t\t\t getFormatSingleplane(fmt);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Configure an image format on the V4L2 device\n> >> + * \\return 0 for success, a negative error code otherwise\n> >> + */\n> >> +int V4L2Device::setFormat(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\treturn caps_.isMultiplanar() ? setFormatMultiplane(fmt) :\n> >> +\t\t\t\t setFormatSingleplane(fmt);\n> >> +}\n> >> +\n> >> +int V4L2Device::getFormatSingleplane(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = bufferType_;\n> >> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tfmt->width = pix->width;\n> >> +\tfmt->height = pix->height;\n> >> +\tfmt->fourcc = pix->pixelformat;\n> >> +\tfmt->planes = 1;\n> >> +\tfmt->planesFmt[0].bpl = pix->bytesperline;\n> >> +\tfmt->planesFmt[0].size = pix->sizeimage;\n> >> +\n> >\n> > I think we might need to cache the V4L2DeviceFormat in the V4L2Device\n> > somewhere...\n> \n> Yes, indeed. I would have had a V4L2DataFormat member initialized at\n> device 'open()' time that could be cached, and re-freshed at every\n> s_fmt..\n> \n> > But perhaps that's not required by this patch - if something needs to\n> > access this (like the BufferPool creation) it can handle caching the\n> > format object.\n> \n> I left it out from this two patches to ease integration, but I agree\n> this is a possible optimization, and if any class needs that, it shall\n> be done here.\n\nCould the device allowed to change format without userspace intervention ?\n\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = bufferType_;\n> >> +\tpix->width = fmt->width;\n> >> +\tpix->height = fmt->height;\n> >> +\tpix->pixelformat = fmt->fourcc;\n> >> +\n> >> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2Device::getFormatMultiplane(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = bufferType_;\n> >> +\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Fmt);\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tfmt->width = pix->width;\n> >> +\tfmt->height = pix->height;\n> >> +\tfmt->fourcc = pix->pixelformat;\n> >> +\tfmt->planes = pix->num_planes;\n> >> +\n> >> +\tfor (unsigned int i = 0; i < fmt->planes; ++i) {\n> >> +\t\tfmt->planesFmt[i].bpl = pix->plane_fmt[i].bytesperline;\n> >> +\t\tfmt->planesFmt[i].size = pix->plane_fmt[i].sizeimage;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = bufferType_;\n> >> +\tpix->width = fmt->width;\n> >> +\tpix->height = fmt->height;\n> >> +\tpix->pixelformat = fmt->fourcc;\n> >> +\tpix->num_planes = fmt->planes;\n> >> +\n> >> +\tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n> >> +\t\tpix->plane_fmt[i].bytesperline = fmt->planesFmt[i].bpl;\n> >> +\t\tpix->plane_fmt[i].sizeimage = fmt->planesFmt[i].size;\n> >> +\t}\n> >> +\n> >> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> } /* namespace libcamera */","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 7A3B360B2D\n\tfor ;\n\tWed, 30 Jan 2019 12:00:47 +0100 (CET)","from pendragon.ideasonboard.com (85-76-73-0-nat.elisa-mobile.fi\n\t[85.76.73.0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EE98F41;\n\tWed, 30 Jan 2019 12:00:42 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548846046;\n\tbh=llrB6vlrQdlIN78U+13u0CDAs66u0lCmyFIQghRKVUQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sIyJMqbrKZbVAIA6eN/yPFIIljcXN0dvcrbrXlUJ30RPmhqbdao+JjUvQhQ8T1OAH\n\tB5OhWnRyB7qQSX0+HqdcyjoRmy3qdByQUI3wtv9jfupSrOOqa/6LH8KJtKV4Yi3wb0\n\tLXE32HxXKHCLNia7mc9ce4D89EPhBXXZSGhRraY0=","Date":"Wed, 30 Jan 2019 13:00:38 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190130110038.GG4336@pendragon.ideasonboard.com>","References":"<20190128151137.31075-1-jacopo@jmondi.org>\n\t<20190128151137.31075-3-jacopo@jmondi.org>\n\t<1e0a0257-96c5-b274-d508-8f2ceed9688e@ideasonboard.com>\n\t<20190128160732.ziirvt4umxpssrwi@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190128160732.ziirvt4umxpssrwi@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","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, 30 Jan 2019 11:00:47 -0000"}},{"id":698,"web_url":"https://patchwork.libcamera.org/comment/698/","msgid":"<20190130111000.GH4336@pendragon.ideasonboard.com>","date":"2019-01-30T11:10:00","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Jan 28, 2019 at 04:11:37PM +0100, Jacopo Mondi wrote:\n> Add methods to set and get the image format programmed on a V4L2Device\n> for both the single and multi planar use case.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_device.h | 10 +++\n> src/libcamera/v4l2_device.cpp | 128 ++++++++++++++++++++++++++++\n> 2 files changed, 138 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index c70959e..fe54220 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -86,10 +86,20 @@ public:\n> \tconst char *deviceName() const { return caps_.card(); }\n> \tconst char *busName() const { return caps_.bus_info(); }\n> \n> +\tint format(V4L2DeviceFormat *fmt);\n\nAs an exception to the general rule, I'd rename format() to getFormat()\nto follow the V4L2 naming. I won't push hard for this, but I think this\nwas the general preference of the team as well.\n\n> +\tint setFormat(V4L2DeviceFormat *fmt);\n\nHow about spelling format in full (here and below) for the function\nparameter ?\n\n> +\n> private:\n> \tstd::string deviceNode_;\n> \tint fd_;\n> \tV4L2Capability caps_;\n> +\tenum v4l2_buf_type bufferType_;\n> +\n> +\tint getFormatSingleplane(V4L2DeviceFormat *fmt);\n> +\tint setFormatSingleplane(V4L2DeviceFormat *fmt);\n> +\n> +\tint getFormatMultiplane(V4L2DeviceFormat *fmt);\n> +\tint setFormatMultiplane(V4L2DeviceFormat *fmt);\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index d6143f2..5c415d0 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -227,6 +227,15 @@ int V4L2Device::open()\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tif (caps_.isCapture())\n> +\t\tbufferType_ = caps_.isMultiplanar()\n> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> +\t\t\t : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> +\telse\n> +\t\tbufferType_ = caps_.isMultiplanar()\n> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> +\t\t\t : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> +\n> \treturn 0;\n> }\n> \n> @@ -269,4 +278,123 @@ void V4L2Device::close()\n> * \\return The string containing the device location\n> */\n> \n> +/**\n> + * \\brief Retrieve the image format set on the V4L2 device\n> + * \\return 0 for success, a negative error code otherwise\n\ns/for/on/\n\n> + */\n> +int V4L2Device::format(V4L2DeviceFormat *fmt)\n> +{\n> +\treturn caps_.isMultiplanar() ? getFormatMultiplane(fmt) :\n> +\t\t\t\t getFormatSingleplane(fmt);\n> +}\n> +\n> +/**\n> + * \\brief Configure an image format on the V4L2 device\n> + * \\return 0 for success, a negative error code otherwise\n\ns/for/on/\n\n> + */\n> +int V4L2Device::setFormat(V4L2DeviceFormat *fmt)\n> +{\n> +\treturn caps_.isMultiplanar() ? setFormatMultiplane(fmt) :\n> +\t\t\t\t setFormatSingleplane(fmt);\n> +}\n> +\n> +int V4L2Device::getFormatSingleplane(V4L2DeviceFormat *fmt)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n\nv4l2Format ? Or maybe just fmt if you rename the function parameter to\nformat ?\n\nYou need to initialize v4l2Fmt to 0, that's a requirement of the V4L2\nAPI. = {}; will suffice.\n\n> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = bufferType_;\n> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n\nShouldn't this be VIDIOC_G_FMT ? This outlines a missing test case :-)\n\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfmt->width = pix->width;\n> +\tfmt->height = pix->height;\n> +\tfmt->fourcc = pix->pixelformat;\n> +\tfmt->planes = 1;\n> +\tfmt->planesFmt[0].bpl = pix->bytesperline;\n> +\tfmt->planesFmt[0].size = pix->sizeimage;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *fmt)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = bufferType_;\n> +\tpix->width = fmt->width;\n> +\tpix->height = fmt->height;\n> +\tpix->pixelformat = fmt->fourcc;\n\nHow about bytesperline and sizeimage ?\n\n> +\n> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n\nShouldn't fmt be updated ?\n\n> +\treturn 0;\n> +}\n> +\n> +int V4L2Device::getFormatMultiplane(V4L2DeviceFormat *fmt)\n\nThe above comments hold for the mplane functions too.\n\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = bufferType_;\n> +\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Fmt);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfmt->width = pix->width;\n> +\tfmt->height = pix->height;\n> +\tfmt->fourcc = pix->pixelformat;\n> +\tfmt->planes = pix->num_planes;\n> +\n> +\tfor (unsigned int i = 0; i < fmt->planes; ++i) {\n> +\t\tfmt->planesFmt[i].bpl = pix->plane_fmt[i].bytesperline;\n> +\t\tfmt->planesFmt[i].size = pix->plane_fmt[i].sizeimage;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *fmt)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = bufferType_;\n> +\tpix->width = fmt->width;\n> +\tpix->height = fmt->height;\n> +\tpix->pixelformat = fmt->fourcc;\n> +\tpix->num_planes = fmt->planes;\n> +\n> +\tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n> +\t\tpix->plane_fmt[i].bytesperline = fmt->planesFmt[i].bpl;\n> +\t\tpix->plane_fmt[i].sizeimage = fmt->planesFmt[i].size;\n> +\t}\n> +\n> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> } /* namespace libcamera */","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 411D860B2D\n\tfor ;\n\tWed, 30 Jan 2019 12:10:04 +0100 (CET)","from pendragon.ideasonboard.com (85-76-73-0-nat.elisa-mobile.fi\n\t[85.76.73.0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A840241;\n\tWed, 30 Jan 2019 12:10:02 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548846603;\n\tbh=sxRV2AE0wjPLMb3qfiptfCi228LRaw2uCeuIyJT5ZpE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EJbgFzX2pUfYtARWC7kol/1hbDVSQnE0b8PKjs+ec/EfXrDAy02EKrC+uK/72FetO\n\to6ncmK+sGaneDiVPz0w5TcCWTG7ERye19Hj5Cp2qbUbmpNrsE7ZA23MGnZ78AtkNlg\n\tFxL0/PosnUaCgaZc8SJNiF7gztTOa+AcsFtTc97Q=","Date":"Wed, 30 Jan 2019 13:10:00 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190130111000.GH4336@pendragon.ideasonboard.com>","References":"<20190128151137.31075-1-jacopo@jmondi.org>\n\t<20190128151137.31075-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190128151137.31075-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","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, 30 Jan 2019 11:10:04 -0000"}},{"id":703,"web_url":"https://patchwork.libcamera.org/comment/703/","msgid":"<20190130143713.bc5mbqmyupvwsgnn@uno.localdomain>","date":"2019-01-30T14:37:13","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Jan 30, 2019 at 01:10:00PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Jan 28, 2019 at 04:11:37PM +0100, Jacopo Mondi wrote:\n> > Add methods to set and get the image format programmed on a V4L2Device\n> > for both the single and multi planar use case.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/v4l2_device.h | 10 +++\n> > src/libcamera/v4l2_device.cpp | 128 ++++++++++++++++++++++++++++\n> > 2 files changed, 138 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index c70959e..fe54220 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -86,10 +86,20 @@ public:\n> > \tconst char *deviceName() const { return caps_.card(); }\n> > \tconst char *busName() const { return caps_.bus_info(); }\n> >\n> > +\tint format(V4L2DeviceFormat *fmt);\n>\n> As an exception to the general rule, I'd rename format() to getFormat()\n> to follow the V4L2 naming. I won't push hard for this, but I think this\n> was the general preference of the team as well.\n>\n\niirc we discussed that and went for this solution. I can re-change it\nin case..\n\n> > +\tint setFormat(V4L2DeviceFormat *fmt);\n>\n> How about spelling format in full (here and below) for the function\n> parameter ?\n>\n\nOk\n\n> > +\n> > private:\n> > \tstd::string deviceNode_;\n> > \tint fd_;\n> > \tV4L2Capability caps_;\n> > +\tenum v4l2_buf_type bufferType_;\n> > +\n> > +\tint getFormatSingleplane(V4L2DeviceFormat *fmt);\n> > +\tint setFormatSingleplane(V4L2DeviceFormat *fmt);\n> > +\n> > +\tint getFormatMultiplane(V4L2DeviceFormat *fmt);\n> > +\tint setFormatMultiplane(V4L2DeviceFormat *fmt);\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index d6143f2..5c415d0 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -227,6 +227,15 @@ int V4L2Device::open()\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > +\tif (caps_.isCapture())\n> > +\t\tbufferType_ = caps_.isMultiplanar()\n> > +\t\t\t ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> > +\t\t\t : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> > +\telse\n> > +\t\tbufferType_ = caps_.isMultiplanar()\n> > +\t\t\t ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> > +\t\t\t : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -269,4 +278,123 @@ void V4L2Device::close()\n> > * \\return The string containing the device location\n> > */\n> >\n> > +/**\n> > + * \\brief Retrieve the image format set on the V4L2 device\n> > + * \\return 0 for success, a negative error code otherwise\n>\n> s/for/on/\n>\n> > + */\n> > +int V4L2Device::format(V4L2DeviceFormat *fmt)\n> > +{\n> > +\treturn caps_.isMultiplanar() ? getFormatMultiplane(fmt) :\n> > +\t\t\t\t getFormatSingleplane(fmt);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Configure an image format on the V4L2 device\n> > + * \\return 0 for success, a negative error code otherwise\n>\n> s/for/on/\n>\n> > + */\n> > +int V4L2Device::setFormat(V4L2DeviceFormat *fmt)\n> > +{\n> > +\treturn caps_.isMultiplanar() ? setFormatMultiplane(fmt) :\n> > +\t\t\t\t setFormatSingleplane(fmt);\n> > +}\n> > +\n> > +int V4L2Device::getFormatSingleplane(V4L2DeviceFormat *fmt)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n>\n> v4l2Format ? Or maybe just fmt if you rename the function parameter to\n> format ?\n>\n\nOk\n\n> You need to initialize v4l2Fmt to 0, that's a requirement of the V4L2\n> API. = {}; will suffice.\n>\n\ncorrect.\n\n> > +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = bufferType_;\n> > +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n>\n> Shouldn't this be VIDIOC_G_FMT ? This outlines a missing test case :-)\n>\n\nMy bad, I'm sorry.\nFor a test case, how should we make sure we test all possible use\ncases (planar/multiplanar, capture/output). The device that runs the\ntest might not have all of these... Could VIMC or vivid help us here?\n\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfmt->width = pix->width;\n> > +\tfmt->height = pix->height;\n> > +\tfmt->fourcc = pix->pixelformat;\n> > +\tfmt->planes = 1;\n> > +\tfmt->planesFmt[0].bpl = pix->bytesperline;\n> > +\tfmt->planesFmt[0].size = pix->sizeimage;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *fmt)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = bufferType_;\n> > +\tpix->width = fmt->width;\n> > +\tpix->height = fmt->height;\n> > +\tpix->pixelformat = fmt->fourcc;\n>\n> How about bytesperline and sizeimage ?\n>\n\nThe description here\nhttps://www.kernel.org/doc/html/v4.13/media/uapi/v4l/pixfmt-002.html#c.v4l2_pix_format\nquite confused me.\n\nParticularly: \"Size in bytes of the buffer to hold a complete image,\nset by the driver.\"\n\n> > +\n> > +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n>\n> Shouldn't fmt be updated ?\n>\n\nGood question, what semantic would we want to assign to this\nfunctions? A setFormat returns an updated format? I here thought to\nclearly separate this: to know what is actually set, you have to call\ng_fmt again. Honestly, now that I re-think this, I am not convinced\nanymore this is the right way, as the V4L2 S_FMT actually returns an\nupdated format, so it might be worth updating it here before return.\n\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2Device::getFormatMultiplane(V4L2DeviceFormat *fmt)\n>\n> The above comments hold for the mplane functions too.\n>\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = bufferType_;\n> > +\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Fmt);\n\nAt least this one is right...\n\nThanks\n j\n\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfmt->width = pix->width;\n> > +\tfmt->height = pix->height;\n> > +\tfmt->fourcc = pix->pixelformat;\n> > +\tfmt->planes = pix->num_planes;\n> > +\n> > +\tfor (unsigned int i = 0; i < fmt->planes; ++i) {\n> > +\t\tfmt->planesFmt[i].bpl = pix->plane_fmt[i].bytesperline;\n> > +\t\tfmt->planesFmt[i].size = pix->plane_fmt[i].sizeimage;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *fmt)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = bufferType_;\n> > +\tpix->width = fmt->width;\n> > +\tpix->height = fmt->height;\n> > +\tpix->pixelformat = fmt->fourcc;\n> > +\tpix->num_planes = fmt->planes;\n> > +\n> > +\tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > +\t\tpix->plane_fmt[i].bytesperline = fmt->planesFmt[i].bpl;\n> > +\t\tpix->plane_fmt[i].sizeimage = fmt->planesFmt[i].size;\n> > +\t}\n> > +\n> > +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E94A260C78\n\tfor ;\n\tWed, 30 Jan 2019 15:36:58 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 57B7F1C0002;\n\tWed, 30 Jan 2019 14:36:58 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 30 Jan 2019 15:37:13 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190130143713.bc5mbqmyupvwsgnn@uno.localdomain>","References":"<20190128151137.31075-1-jacopo@jmondi.org>\n\t<20190128151137.31075-3-jacopo@jmondi.org>\n\t<20190130111000.GH4336@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"dhczxoev7kthbjsd\"","Content-Disposition":"inline","In-Reply-To":"<20190130111000.GH4336@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","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, 30 Jan 2019 14:36:59 -0000"}},{"id":706,"web_url":"https://patchwork.libcamera.org/comment/706/","msgid":"<20190130223100.GA5358@pendragon.ideasonboard.com>","date":"2019-01-30T22:31:00","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jan 30, 2019 at 03:37:13PM +0100, Jacopo Mondi wrote:\n> On Wed, Jan 30, 2019 at 01:10:00PM +0200, Laurent Pinchart wrote:\n> > On Mon, Jan 28, 2019 at 04:11:37PM +0100, Jacopo Mondi wrote:\n> >> Add methods to set and get the image format programmed on a V4L2Device\n> >> for both the single and multi planar use case.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/include/v4l2_device.h | 10 +++\n> >> src/libcamera/v4l2_device.cpp | 128 ++++++++++++++++++++++++++++\n> >> 2 files changed, 138 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >> index c70959e..fe54220 100644\n> >> --- a/src/libcamera/include/v4l2_device.h\n> >> +++ b/src/libcamera/include/v4l2_device.h\n> >> @@ -86,10 +86,20 @@ public:\n> >> \tconst char *deviceName() const { return caps_.card(); }\n> >> \tconst char *busName() const { return caps_.bus_info(); }\n> >>\n> >> +\tint format(V4L2DeviceFormat *fmt);\n> >\n> > As an exception to the general rule, I'd rename format() to getFormat()\n> > to follow the V4L2 naming. I won't push hard for this, but I think this\n> > was the general preference of the team as well.\n> \n> iirc we discussed that and went for this solution. I can re-change it\n> in case..\n\nI think I replied to a previous e-mail explaining that I thought an\nexception would make sense here, and I recall (possibly incorrectly)\nthat you were in favour of getFormat(). As I said, I won't push, but if\nwe all agree getFormat() makes sense, they I think we should go for it.\n\n> >> +\tint setFormat(V4L2DeviceFormat *fmt);\n> >\n> > How about spelling format in full (here and below) for the function\n> > parameter ?\n> \n> Ok\n> \n> >> +\n> >> private:\n> >> \tstd::string deviceNode_;\n> >> \tint fd_;\n> >> \tV4L2Capability caps_;\n> >> +\tenum v4l2_buf_type bufferType_;\n> >> +\n> >> +\tint getFormatSingleplane(V4L2DeviceFormat *fmt);\n> >> +\tint setFormatSingleplane(V4L2DeviceFormat *fmt);\n> >> +\n> >> +\tint getFormatMultiplane(V4L2DeviceFormat *fmt);\n> >> +\tint setFormatMultiplane(V4L2DeviceFormat *fmt);\n> >> };\n> >>\n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> index d6143f2..5c415d0 100644\n> >> --- a/src/libcamera/v4l2_device.cpp\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -227,6 +227,15 @@ int V4L2Device::open()\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >>\n> >> +\tif (caps_.isCapture())\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE\n> >> +\t\t\t : V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> >> +\telse\n> >> +\t\tbufferType_ = caps_.isMultiplanar()\n> >> +\t\t\t ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE\n> >> +\t\t\t : V4L2_BUF_TYPE_VIDEO_OUTPUT;\n> >> +\n> >> \treturn 0;\n> >> }\n> >>\n> >> @@ -269,4 +278,123 @@ void V4L2Device::close()\n> >> * \\return The string containing the device location\n> >> */\n> >>\n> >> +/**\n> >> + * \\brief Retrieve the image format set on the V4L2 device\n> >> + * \\return 0 for success, a negative error code otherwise\n> >\n> > s/for/on/\n> >\n> >> + */\n> >> +int V4L2Device::format(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\treturn caps_.isMultiplanar() ? getFormatMultiplane(fmt) :\n> >> +\t\t\t\t getFormatSingleplane(fmt);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Configure an image format on the V4L2 device\n> >> + * \\return 0 for success, a negative error code otherwise\n> >\n> > s/for/on/\n> >\n> >> + */\n> >> +int V4L2Device::setFormat(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\treturn caps_.isMultiplanar() ? setFormatMultiplane(fmt) :\n> >> +\t\t\t\t setFormatSingleplane(fmt);\n> >> +}\n> >> +\n> >> +int V4L2Device::getFormatSingleplane(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >\n> > v4l2Format ? Or maybe just fmt if you rename the function parameter to\n> > format ?\n> \n> Ok\n> \n> > You need to initialize v4l2Fmt to 0, that's a requirement of the V4L2\n> > API. = {}; will suffice.\n> >\n> \n> correct.\n> \n> >> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = bufferType_;\n> >> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> >\n> > Shouldn't this be VIDIOC_G_FMT ? This outlines a missing test case :-)\n> >\n> \n> My bad, I'm sorry.\n> For a test case, how should we make sure we test all possible use\n> cases (planar/multiplanar, capture/output). The device that runs the\n> test might not have all of these... Could VIMC or vivid help us here?\n\nI think they could help. I haven't checked which of the\nsingle/multi-planar APIs those drivers use, and whether they could help\ntesting both, but we could at least test one of the two.\n\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tfmt->width = pix->width;\n> >> +\tfmt->height = pix->height;\n> >> +\tfmt->fourcc = pix->pixelformat;\n> >> +\tfmt->planes = 1;\n> >> +\tfmt->planesFmt[0].bpl = pix->bytesperline;\n> >> +\tfmt->planesFmt[0].size = pix->sizeimage;\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2Device::setFormatSingleplane(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = bufferType_;\n> >> +\tpix->width = fmt->width;\n> >> +\tpix->height = fmt->height;\n> >> +\tpix->pixelformat = fmt->fourcc;\n> >\n> > How about bytesperline and sizeimage ?\n> \n> The description here\n> https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/pixfmt-002.html#c.v4l2_pix_format\n> quite confused me.\n> \n> Particularly: \"Size in bytes of the buffer to hold a complete image,\n> set by the driver.\"\n\nYou're right about sizeimage. For bytesperline, though, you should pass\nit to the ioctl.\n\n> >> +\n> >> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >\n> > Shouldn't fmt be updated ?\n> \n> Good question, what semantic would we want to assign to this\n> functions? A setFormat returns an updated format? I here thought to\n> clearly separate this: to know what is actually set, you have to call\n> g_fmt again. Honestly, now that I re-think this, I am not convinced\n> anymore this is the right way, as the V4L2 S_FMT actually returns an\n> updated format, so it might be worth updating it here before return.\n\nI think mimicking the VIDIOC_S_FMT semantics would make sense, as it\nwould save a VIDIOC_G_FMT call for users of this class.\n\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2Device::getFormatMultiplane(V4L2DeviceFormat *fmt)\n> >\n> > The above comments hold for the mplane functions too.\n> >\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = bufferType_;\n> >> +\tret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Fmt);\n> \n> At least this one is right...\n\nSometimes copy & paste works ;-)\n\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Unable to get format: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tfmt->width = pix->width;\n> >> +\tfmt->height = pix->height;\n> >> +\tfmt->fourcc = pix->pixelformat;\n> >> +\tfmt->planes = pix->num_planes;\n> >> +\n> >> +\tfor (unsigned int i = 0; i < fmt->planes; ++i) {\n> >> +\t\tfmt->planesFmt[i].bpl = pix->plane_fmt[i].bytesperline;\n> >> +\t\tfmt->planesFmt[i].size = pix->plane_fmt[i].sizeimage;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *fmt)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = bufferType_;\n> >> +\tpix->width = fmt->width;\n> >> +\tpix->height = fmt->height;\n> >> +\tpix->pixelformat = fmt->fourcc;\n> >> +\tpix->num_planes = fmt->planes;\n> >> +\n> >> +\tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n> >> +\t\tpix->plane_fmt[i].bytesperline = fmt->planesFmt[i].bpl;\n> >> +\t\tpix->plane_fmt[i].sizeimage = fmt->planesFmt[i].size;\n> >> +\t}\n> >> +\n> >> +\tret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Fmt);\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Unable to set format: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> } /* namespace libcamera */","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 94C0860C78\n\tfor ;\n\tWed, 30 Jan 2019 23:31:05 +0100 (CET)","from pendragon.ideasonboard.com (85-76-34-136-nat.elisa-mobile.fi\n\t[85.76.34.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E17FB41;\n\tWed, 30 Jan 2019 23:31:03 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548887465;\n\tbh=XbVNvPDKtJNUkvB3WAX8iZXMjhEm75pMsBMPBzbbSc8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CmY5MEpNzrrDlSAIaUSocYQ4/UkDLjhRgE69TQKRbnylUJM3wq3lEPp3k6BtOFSPw\n\tN/oTa2ixywrINFfVFFzgZaz7FSqB78JFSt8uVfQcFTvuGuhmRNSEyS7iiEMKMjgFUP\n\tw3S1ce98A7bg1ltHMS6tAuxpjvY4poNAHJCnpNoo=","Date":"Thu, 31 Jan 2019 00:31:00 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190130223100.GA5358@pendragon.ideasonboard.com>","References":"<20190128151137.31075-1-jacopo@jmondi.org>\n\t<20190128151137.31075-3-jacopo@jmondi.org>\n\t<20190130111000.GH4336@pendragon.ideasonboard.com>\n\t<20190130143713.bc5mbqmyupvwsgnn@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190130143713.bc5mbqmyupvwsgnn@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add\n\tmethods to get/set format","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, 30 Jan 2019 22:31:05 -0000"}}]