[{"id":632,"web_url":"https://patchwork.libcamera.org/comment/632/","msgid":"<20190127003956.GX4127@bigcity.dyn.berto.se>","date":"2019-01-27T00:39:56","subject":"Re: [libcamera-devel] [RFC 2/3] libcamera: v4l2_device: Add methods\n\tto get/set format","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-01-26 16:13:17 +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 | 13 +++\n> src/libcamera/v4l2_device.cpp | 131 ++++++++++++++++++++++++++++\n> 2 files changed, 144 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index ee0c4eb..01dbf45 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -14,6 +14,7 @@\n> \n> #include \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -81,6 +82,9 @@ public:\n> \tint requestBuffers(unsigned int qty = 8);\n> \tint requestBuffers(unsigned int qty, std::vector &buffers);\n> \n> +\tint format(StreamConfiguration *config);\n> +\tint setFormat(StreamConfiguration *config);\n> +\n\nI'm not sure these should take StreamConfiguration as an argument. Maybe \nthe PipelineHandler should translate the StreamConfigurations it \nreceives into a structure which is V4L2 specific?\n\n> \tint queueBuffer(FrameBuffer *frame);\n> \tFrameBuffer *dequeueBuffer();\n> \n> @@ -93,6 +97,7 @@ private:\n> \tstd::string deviceNode_;\n> \tint fd_;\n> \tV4L2Capability caps_;\n> +\tunsigned int buftype_;\n> \n> \tenum v4l2_memory memoryType_;\n> \tenum v4l2_buf_type bufferType_;\n> @@ -101,6 +106,14 @@ private:\n> \tvoid bufferAvailable(EventNotifier *notifier);\n> \n> \tEventNotifier *fdEvent_;\n> +\n> +\tunsigned int getPlanesFromFormat(unsigned int pixfmt);\n> +\tunsigned int getBppFromFormat(unsigned int pixfmt);\n\nShould these methods be part of the V4L2Device or some other v4l2 util \ncollection? I wonder if this even could be useful for applications as we \ncurrently expose the pixel format as a V4L2 FOURCC there. I'm not sure \nwhat the end design will look like so this is just an open question.\n\n\n> +\tint getFormatSingleplane(StreamConfiguration *config);\n> +\tint setFormatSingleplane(StreamConfiguration *config);\n> +\n> +\tint getFormatMultiplane(StreamConfiguration *config);\n> +\tint setFormatMultiplane(StreamConfiguration *config);\n\nHere I like the deviation from our usual style of not using get*().\n\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 4d39c85..d16691d 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -203,6 +203,15 @@ int V4L2Device::open()\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tif (caps_.isCapture())\n> +\t\tbuftype_ = 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\tbuftype_ = 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> @@ -314,6 +323,26 @@ int V4L2Device::requestBuffers(unsigned int qty, std::vector &buf\n> \treturn 0;\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(StreamConfiguration *config)\n> +{\n> +\treturn caps_.isMultiplanar() ? getFormatMultiplane(config) :\n> +\t\t\t\t getFormatSingleplane(config);\n> +}\n> +\n> +/**\n> + * \\brief Program an image format on the V4L2 device\n> + * \\return 0 for success, a negative error code otherwise\n> + */\n> +int V4L2Device::setFormat(StreamConfiguration *config)\n> +{\n> +\treturn caps_.isMultiplanar() ? setFormatMultiplane(config) :\n> +\t\t\t\t setFormatSingleplane(config);\n> +}\n\nLooking at the implementation of the get*() and set*() bellow I'm \nwondering if it's possible to handle the difference between the two \ninside just one {get,set}Format() function. Or do you expect these \nfunctions to grow over time?\n\nIf you really want to keep them separate maybe it's possible to split \nout the creation of the v4l2_format strucutre and keep the actual ioctl \nand config->set*() common somehow? Or maybe I'm just obsessing about two \nthings that looks similar but really are not.\n\n> +\n> int V4L2Device::queueBuffer(FrameBuffer *frame)\n> {\n> \tstruct v4l2_buffer buf = {};\n> @@ -422,6 +451,108 @@ int V4L2Device::streamOff()\n> \t}\n> \n> \tdelete fdEvent_;\n> +}\n> +\n> +/* TODO: this is a simple stub at the moment. */\n> +unsigned int V4L2Device::getPlanesFromFormat(unsigned int pixfmt)\n> +{\n> +\treturn 1;\n> +}\n> +\n> +/* TODO: this is a simple stub at the moment. */\n> +unsigned int V4L2Device::getBppFromFormat(unsigned int pixfmt)\n> +{\n> +\treturn 16;\n> +}\n> +\n> +int V4L2Device::getFormatSingleplane(StreamConfiguration *config)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> +\tint ret;\n> +\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> +\tconfig->setWidth(pix->width);\n> +\tconfig->setHeight(pix->height);\n> +\tconfig->setPixelFormat(pix->pixelformat);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2Device::setFormatSingleplane(StreamConfiguration *config)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = buftype_;\n> +\tpix->width = config->width();\n> +\tpix->height = config->height();\n> +\n> +\tpix->width = config->width();\n> +\tpix->height = config->height();\n> +\tpix->pixelformat = config->pixelformat();\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(StreamConfiguration *config)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> +\tint ret;\n> +\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> +\tconfig->setWidth(pix->width);\n> +\tconfig->setHeight(pix->height);\n> +\tconfig->setPixelFormat(pix->pixelformat);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2Device::setFormatMultiplane(StreamConfiguration *config)\n> +{\n> +\tstruct v4l2_format v4l2Fmt;\n> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> +\tint ret;\n> +\n> +\tv4l2Fmt.type = buftype_;\n> +\tpix->width = config->width();\n> +\tpix->height = config->height();\n> +\tpix->pixelformat = config->pixelformat();\n> +\n> +\tunsigned int numPlanes = getPlanesFromFormat(pix->pixelformat);\n> +\tunsigned int bpp = getBppFromFormat(pix->pixelformat);\n> +\tfor (unsigned int i = 0; i < numPlanes; ++i) {\n> +\t\tpix->plane_fmt[i].bytesperline = bpp * pix->width;\n> +\t\tpix->plane_fmt[i].sizeimage = bpp * pix->width * pix->height;\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> 2.20.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7DCF760B1B\n\tfor ;\n\tSun, 27 Jan 2019 01:39:58 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id y11so9434936lfj.4\n\tfor ;\n\tSat, 26 Jan 2019 16:39:58 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tp10-v6sm2282398ljg.19.2019.01.26.16.39.56\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSat, 26 Jan 2019 16:39:57 -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=mgkRv4IW7yVQ15F+8zHgFst2ucQ/3SZwx8MBAPwdkuI=;\n\tb=PS7Mta1TY5KkcGZodfqg7EQRqukMzqiLAimSigjIWHLNePv74pzjDxk5SIddjsOG7L\n\t2tsbxUwEgQ2jcVfRJESC5sviR9qxSTB4SeLOvKp5AVAlzMAsfvl3oB9y+pLJ8W/T/YwM\n\tlEVHrlqqH3C+nPerowpIHFkdiBKaNhRgNsTRH9l5CBTbmZPQZGw1ooz8AF+a/JdDORiJ\n\tR+NEVmLgvaMizWuGk/3SKMi2DnucIN5XYMm19aKj6sg2uWqsLQCE2we3rhXmhZ0rMlgF\n\tM9fV63EaZ97CHIAayGiA2NJq0Js7B0F/9dDKWE3g8R0t9wKG5otjJK01uAYH6nQcL5BC\n\tfCow==","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=mgkRv4IW7yVQ15F+8zHgFst2ucQ/3SZwx8MBAPwdkuI=;\n\tb=Mzv/Df6HZ9PrARvAzehByC2f/aC7eOruPArycBJZxUzlHNFxBL9cypLFzaHQds0l/W\n\tCLun92XFH4vvzMcjOSVRaSqc3K7bgpagDeopds2I7CC1KOISM81Xi9kNrfWxQ3ZZCd4g\n\tX8pU1GB/b5bni8buDVPnePjzyEfLklJ7gJULqi6vArqD/oFZgtu4bDJGPLq3fpPk+nC+\n\t8GepFCsLjKcfV2f/+MfwqDapPEMt4Vwc9GcYerLSMWYq2MwRoV6db9Fe021DTmWybYLv\n\tLd598E5zVWP+sw8332hPl1vpqOTlXUKgtQTfxAnBIAaOLdoScosQV2Ol1NmDq57lK+XF\n\tSzng==","X-Gm-Message-State":"AJcUukdvzmBJOn0Nq3+epLFaodWNLMFTVkUlhF3kj5VMgH8trtraWELx\n\tJttFmHrMrACCTEfgAKkuFxv/Ig==","X-Google-Smtp-Source":"ALg8bN7aq0HdEiZOHI9GeK9404S7RFfrtkTjCMgFM5q1J0qrbquUpkCC2zwQg8M2x143gzN6LXbV/A==","X-Received":"by 2002:a19:c502:: with SMTP id w2mr3451316lfe.168.1548549597655;\n\tSat, 26 Jan 2019 16:39:57 -0800 (PST)","Date":"Sun, 27 Jan 2019 01:39:56 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190127003956.GX4127@bigcity.dyn.berto.se>","References":"<20190126151318.28087-1-jacopo@jmondi.org>\n\t<20190126151318.28087-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190126151318.28087-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 2/3] libcamera: v4l2_device: Add methods\n\tto 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":"Sun, 27 Jan 2019 00:39:58 -0000"}},{"id":636,"web_url":"https://patchwork.libcamera.org/comment/636/","msgid":"<20190127103857.xgatemvq477lsqqt@uno.localdomain>","date":"2019-01-27T10:38:57","subject":"Re: [libcamera-devel] [RFC 2/3] libcamera: v4l2_device: Add methods\n\tto get/set format","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sun, Jan 27, 2019 at 01:39:56AM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-01-26 16:13:17 +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 | 13 +++\n> > src/libcamera/v4l2_device.cpp | 131 ++++++++++++++++++++++++++++\n> > 2 files changed, 144 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index ee0c4eb..01dbf45 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -14,6 +14,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> >\n> > namespace libcamera {\n> >\n> > @@ -81,6 +82,9 @@ public:\n> > \tint requestBuffers(unsigned int qty = 8);\n> > \tint requestBuffers(unsigned int qty, std::vector &buffers);\n> >\n> > +\tint format(StreamConfiguration *config);\n> > +\tint setFormat(StreamConfiguration *config);\n> > +\n>\n> I'm not sure these should take StreamConfiguration as an argument. Maybe\n> the PipelineHandler should translate the StreamConfigurations it\n> receives into a structure which is V4L2 specific?\n>\n\nI agree with you and Laurent and we should have an intermediate v4l2\nformat class, that gets not exposed to applications.\n\nIt will probably be shared with v4l2-subdev, as it can be equally used\nto configure a 'struct v4l2_pix_format', a 'struct\nv4l2_pix_format_mplane' or a 'struct v4l2_mbus_framefmt', and possibly\nto instruct the v4l2 device on crop/composing too.\n\nIt will probably merit a dedicated v4l2-format.h where to collect\nthose, and which can be included by other modules that requires\ninteractinc with v4l2 devices and subdevices format configuration (the\nv4l2 devs themselves and reasonably pipeline handlers only right now).\n\n> > \tint queueBuffer(FrameBuffer *frame);\n> > \tFrameBuffer *dequeueBuffer();\n> >\n> > @@ -93,6 +97,7 @@ private:\n> > \tstd::string deviceNode_;\n> > \tint fd_;\n> > \tV4L2Capability caps_;\n> > +\tunsigned int buftype_;\n> >\n> > \tenum v4l2_memory memoryType_;\n> > \tenum v4l2_buf_type bufferType_;\n> > @@ -101,6 +106,14 @@ private:\n> > \tvoid bufferAvailable(EventNotifier *notifier);\n> >\n> > \tEventNotifier *fdEvent_;\n> > +\n> > +\tunsigned int getPlanesFromFormat(unsigned int pixfmt);\n> > +\tunsigned int getBppFromFormat(unsigned int pixfmt);\n>\n> Should these methods be part of the V4L2Device or some other v4l2 util\n> collection? I wonder if this even could be useful for applications as we\n> currently expose the pixel format as a V4L2 FOURCC there. I'm not sure\n> what the end design will look like so this is just an open question.\n>\n\nActually, the number of planes and bpp of a v4l2-defined format should\ncome from the kernel iteself, am I wrong?\n\nShould we provide an applications facing v4l2-formats, where V4L2\nFOURCCs are associated with their bpp and number of planes? Is every\npiece in userspace re-implementing this? :/\n\n>\n> > +\tint getFormatSingleplane(StreamConfiguration *config);\n> > +\tint setFormatSingleplane(StreamConfiguration *config);\n> > +\n> > +\tint getFormatMultiplane(StreamConfiguration *config);\n> > +\tint setFormatMultiplane(StreamConfiguration *config);\n>\n> Here I like the deviation from our usual style of not using get*().\n\nI considered this appropriate as this is a private API anyhow.\n\n>\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 4d39c85..d16691d 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -203,6 +203,15 @@ int V4L2Device::open()\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > +\tif (caps_.isCapture())\n> > +\t\tbuftype_ = 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\tbuftype_ = 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> > @@ -314,6 +323,26 @@ int V4L2Device::requestBuffers(unsigned int qty, std::vector &buf\n> > \treturn 0;\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(StreamConfiguration *config)\n> > +{\n> > +\treturn caps_.isMultiplanar() ? getFormatMultiplane(config) :\n> > +\t\t\t\t getFormatSingleplane(config);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Program an image format on the V4L2 device\n> > + * \\return 0 for success, a negative error code otherwise\n> > + */\n> > +int V4L2Device::setFormat(StreamConfiguration *config)\n> > +{\n> > +\treturn caps_.isMultiplanar() ? setFormatMultiplane(config) :\n> > +\t\t\t\t setFormatSingleplane(config);\n> > +}\n>\n> Looking at the implementation of the get*() and set*() bellow I'm\n> wondering if it's possible to handle the difference between the two\n> inside just one {get,set}Format() function. Or do you expect these\n> functions to grow over time?\n>\n\nAs the s/mplane use case use two different members of struct\nv4l2_format.fmt union, the code gets un-necessarly complicated when I\ntried that.\n\n> If you really want to keep them separate maybe it's possible to split\n> out the creation of the v4l2_format strucutre and keep the actual ioctl\n> and config->set*() common somehow? Or maybe I'm just obsessing about two\n> things that looks similar but really are not.\n>\n\nYeah that could be given a shot to.\n\nThanks\n j\n\n> > +\n> > int V4L2Device::queueBuffer(FrameBuffer *frame)\n> > {\n> > \tstruct v4l2_buffer buf = {};\n> > @@ -422,6 +451,108 @@ int V4L2Device::streamOff()\n> > \t}\n> >\n> > \tdelete fdEvent_;\n> > +}\n> > +\n> > +/* TODO: this is a simple stub at the moment. */\n> > +unsigned int V4L2Device::getPlanesFromFormat(unsigned int pixfmt)\n> > +{\n> > +\treturn 1;\n> > +}\n> > +\n> > +/* TODO: this is a simple stub at the moment. */\n> > +unsigned int V4L2Device::getBppFromFormat(unsigned int pixfmt)\n> > +{\n> > +\treturn 16;\n> > +}\n> > +\n> > +int V4L2Device::getFormatSingleplane(StreamConfiguration *config)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> > +\tint ret;\n> > +\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> > +\tconfig->setWidth(pix->width);\n> > +\tconfig->setHeight(pix->height);\n> > +\tconfig->setPixelFormat(pix->pixelformat);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2Device::setFormatSingleplane(StreamConfiguration *config)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = buftype_;\n> > +\tpix->width = config->width();\n> > +\tpix->height = config->height();\n> > +\n> > +\tpix->width = config->width();\n> > +\tpix->height = config->height();\n> > +\tpix->pixelformat = config->pixelformat();\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(StreamConfiguration *config)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> > +\tint ret;\n> > +\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> > +\tconfig->setWidth(pix->width);\n> > +\tconfig->setHeight(pix->height);\n> > +\tconfig->setPixelFormat(pix->pixelformat);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2Device::setFormatMultiplane(StreamConfiguration *config)\n> > +{\n> > +\tstruct v4l2_format v4l2Fmt;\n> > +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> > +\tint ret;\n> > +\n> > +\tv4l2Fmt.type = buftype_;\n> > +\tpix->width = config->width();\n> > +\tpix->height = config->height();\n> > +\tpix->pixelformat = config->pixelformat();\n> > +\n> > +\tunsigned int numPlanes = getPlanesFromFormat(pix->pixelformat);\n> > +\tunsigned int bpp = getBppFromFormat(pix->pixelformat);\n> > +\tfor (unsigned int i = 0; i < numPlanes; ++i) {\n> > +\t\tpix->plane_fmt[i].bytesperline = bpp * pix->width;\n> > +\t\tpix->plane_fmt[i].sizeimage = bpp * pix->width * pix->height;\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> > 2.20.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> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 295AA60C6A\n\tfor ;\n\tSun, 27 Jan 2019 11:38:44 +0100 (CET)","from uno.localdomain\n\t(host20-49-dynamic.18-79-r.retail.telecomitalia.it [79.18.49.20])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 3A092FF805;\n\tSun, 27 Jan 2019 10:38:42 +0000 (UTC)"],"X-Originating-IP":"79.18.49.20","Date":"Sun, 27 Jan 2019 11:38:57 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190127103857.xgatemvq477lsqqt@uno.localdomain>","References":"<20190126151318.28087-1-jacopo@jmondi.org>\n\t<20190126151318.28087-3-jacopo@jmondi.org>\n\t<20190127003956.GX4127@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"x43swdkcahjzr2ss\"","Content-Disposition":"inline","In-Reply-To":"<20190127003956.GX4127@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [RFC 2/3] libcamera: v4l2_device: Add methods\n\tto 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":"Sun, 27 Jan 2019 10:38:44 -0000"}},{"id":643,"web_url":"https://patchwork.libcamera.org/comment/643/","msgid":"<20190127203618.GB4323@pendragon.ideasonboard.com>","date":"2019-01-27T20:36:18","subject":"Re: [libcamera-devel] [RFC 2/3] libcamera: v4l2_device: Add methods\n\tto 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 Sun, Jan 27, 2019 at 11:38:57AM +0100, Jacopo Mondi wrote:\n> On Sun, Jan 27, 2019 at 01:39:56AM +0100, Niklas Söderlund wrote:\n> > On 2019-01-26 16:13:17 +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 | 13 +++\n> >> src/libcamera/v4l2_device.cpp | 131 ++++++++++++++++++++++++++++\n> >> 2 files changed, 144 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >> index ee0c4eb..01dbf45 100644\n> >> --- a/src/libcamera/include/v4l2_device.h\n> >> +++ b/src/libcamera/include/v4l2_device.h\n> >> @@ -14,6 +14,7 @@\n> >>\n> >> #include \n> >> #include \n> >> +#include \n> >>\n> >> namespace libcamera {\n> >>\n> >> @@ -81,6 +82,9 @@ public:\n> >> \tint requestBuffers(unsigned int qty = 8);\n> >> \tint requestBuffers(unsigned int qty, std::vector &buffers);\n> >>\n> >> +\tint format(StreamConfiguration *config);\n> >> +\tint setFormat(StreamConfiguration *config);\n> >> +\n> >\n> > I'm not sure these should take StreamConfiguration as an argument. Maybe\n> > the PipelineHandler should translate the StreamConfigurations it\n> > receives into a structure which is V4L2 specific?\n> \n> I agree with you and Laurent and we should have an intermediate v4l2\n> format class, that gets not exposed to applications.\n> \n> It will probably be shared with v4l2-subdev, as it can be equally used\n> to configure a 'struct v4l2_pix_format', a 'struct\n> v4l2_pix_format_mplane' or a 'struct v4l2_mbus_framefmt', and possibly\n> to instruct the v4l2 device on crop/composing too.\n\nI think we'll end up needing different structures for V4L2 devices and\nsubdevs, as the respective kernel APIs are quite different. I'll have a\nlook at the other patches you posted and comment there.\n\n> It will probably merit a dedicated v4l2-format.h where to collect\n> those, and which can be included by other modules that requires\n> interactinc with v4l2 devices and subdevices format configuration (the\n> v4l2 devs themselves and reasonably pipeline handlers only right now).\n> \n> >> \tint queueBuffer(FrameBuffer *frame);\n> >> \tFrameBuffer *dequeueBuffer();\n> >>\n> >> @@ -93,6 +97,7 @@ private:\n> >> \tstd::string deviceNode_;\n> >> \tint fd_;\n> >> \tV4L2Capability caps_;\n> >> +\tunsigned int buftype_;\n> >>\n> >> \tenum v4l2_memory memoryType_;\n> >> \tenum v4l2_buf_type bufferType_;\n> >> @@ -101,6 +106,14 @@ private:\n> >> \tvoid bufferAvailable(EventNotifier *notifier);\n> >>\n> >> \tEventNotifier *fdEvent_;\n> >> +\n> >> +\tunsigned int getPlanesFromFormat(unsigned int pixfmt);\n> >> +\tunsigned int getBppFromFormat(unsigned int pixfmt);\n> >\n> > Should these methods be part of the V4L2Device or some other v4l2 util\n> > collection? I wonder if this even could be useful for applications as we\n> > currently expose the pixel format as a V4L2 FOURCC there. I'm not sure\n> > what the end design will look like so this is just an open question.\n> \n> Actually, the number of planes and bpp of a v4l2-defined format should\n> come from the kernel iteself, am I wrong?\n\nIt could make sense, but the kernel doesn't provide that information.\nThe kernel will round the bytesperline value properly, but if you need\nthe direct bpp value, you need to get it from somewhere in userspace. We\nwill likely need more information than the bpp or number of planes, such\nas whether the format is YUV or RGB (or something else ?), compressed or\nnot, ... Maybe we should create a database of format information\nstructures.\n\n> Should we provide an applications facing v4l2-formats, where V4L2\n> FOURCCs are associated with their bpp and number of planes? Is every\n> piece in userspace re-implementing this? :/\n\nEvery piece in userspace is implementing that :-/ I think it would make\nsense to provide that service to applications, but not name it\nv4l2-formats, it should be libcamera formats, even if we use the V4L2\n4CCs.\n\n> >> +\tint getFormatSingleplane(StreamConfiguration *config);\n> >> +\tint setFormatSingleplane(StreamConfiguration *config);\n> >> +\n> >> +\tint getFormatMultiplane(StreamConfiguration *config);\n> >> +\tint setFormatMultiplane(StreamConfiguration *config);\n> >\n> > Here I like the deviation from our usual style of not using get*().\n> \n> I considered this appropriate as this is a private API anyhow.\n\nGiven that the V4L2 API defines G_FMT and S_FMT, I think this deviation\nmakes sense too. Those methods are not just a class accessors, they wrap\nthe V4L2 API.\n\n> >> };\n> >>\n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> index 4d39c85..d16691d 100644\n> >> --- a/src/libcamera/v4l2_device.cpp\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -203,6 +203,15 @@ int V4L2Device::open()\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >>\n> >> +\tif (caps_.isCapture())\n> >> +\t\tbuftype_ = 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\tbuftype_ = 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> >> @@ -314,6 +323,26 @@ int V4L2Device::requestBuffers(unsigned int qty, std::vector &buf\n> >> \treturn 0;\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(StreamConfiguration *config)\n> >> +{\n> >> +\treturn caps_.isMultiplanar() ? getFormatMultiplane(config) :\n> >> +\t\t\t\t getFormatSingleplane(config);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Program an image format on the V4L2 device\n> >> + * \\return 0 for success, a negative error code otherwise\n> >> + */\n> >> +int V4L2Device::setFormat(StreamConfiguration *config)\n> >> +{\n> >> +\treturn caps_.isMultiplanar() ? setFormatMultiplane(config) :\n> >> +\t\t\t\t setFormatSingleplane(config);\n> >> +}\n> >\n> > Looking at the implementation of the get*() and set*() bellow I'm\n> > wondering if it's possible to handle the difference between the two\n> > inside just one {get,set}Format() function. Or do you expect these\n> > functions to grow over time?\n> \n> As the s/mplane use case use two different members of struct\n> v4l2_format.fmt union, the code gets un-necessarly complicated when I\n> tried that.\n> \n> > If you really want to keep them separate maybe it's possible to split\n> > out the creation of the v4l2_format strucutre and keep the actual ioctl\n> > and config->set*() common somehow? Or maybe I'm just obsessing about two\n> > things that looks similar but really are not.\n> \n> Yeah that could be given a shot to.\n> \n> >> +\n> >> int V4L2Device::queueBuffer(FrameBuffer *frame)\n> >> {\n> >> \tstruct v4l2_buffer buf = {};\n> >> @@ -422,6 +451,108 @@ int V4L2Device::streamOff()\n> >> \t}\n> >>\n> >> \tdelete fdEvent_;\n> >> +}\n> >> +\n> >> +/* TODO: this is a simple stub at the moment. */\n> >> +unsigned int V4L2Device::getPlanesFromFormat(unsigned int pixfmt)\n> >> +{\n> >> +\treturn 1;\n> >> +}\n> >> +\n> >> +/* TODO: this is a simple stub at the moment. */\n> >> +unsigned int V4L2Device::getBppFromFormat(unsigned int pixfmt)\n> >> +{\n> >> +\treturn 16;\n> >> +}\n> >> +\n> >> +int V4L2Device::getFormatSingleplane(StreamConfiguration *config)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> >> +\tint ret;\n> >> +\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> >> +\tconfig->setWidth(pix->width);\n> >> +\tconfig->setHeight(pix->height);\n> >> +\tconfig->setPixelFormat(pix->pixelformat);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2Device::setFormatSingleplane(StreamConfiguration *config)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format *pix = &v4l2Fmt.fmt.pix;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = buftype_;\n> >> +\tpix->width = config->width();\n> >> +\tpix->height = config->height();\n> >> +\n> >> +\tpix->width = config->width();\n> >> +\tpix->height = config->height();\n> >> +\tpix->pixelformat = config->pixelformat();\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(StreamConfiguration *config)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> >> +\tint ret;\n> >> +\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> >> +\tconfig->setWidth(pix->width);\n> >> +\tconfig->setHeight(pix->height);\n> >> +\tconfig->setPixelFormat(pix->pixelformat);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int V4L2Device::setFormatMultiplane(StreamConfiguration *config)\n> >> +{\n> >> +\tstruct v4l2_format v4l2Fmt;\n> >> +\tstruct v4l2_pix_format_mplane *pix = &v4l2Fmt.fmt.pix_mp;\n> >> +\tint ret;\n> >> +\n> >> +\tv4l2Fmt.type = buftype_;\n> >> +\tpix->width = config->width();\n> >> +\tpix->height = config->height();\n> >> +\tpix->pixelformat = config->pixelformat();\n> >> +\n> >> +\tunsigned int numPlanes = getPlanesFromFormat(pix->pixelformat);\n> >> +\tunsigned int bpp = getBppFromFormat(pix->pixelformat);\n> >> +\tfor (unsigned int i = 0; i < numPlanes; ++i) {\n> >> +\t\tpix->plane_fmt[i].bytesperline = bpp * pix->width;\n> >> +\t\tpix->plane_fmt[i].sizeimage = bpp * pix->width * pix->height;\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> >> }","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 E4AAF60DB4\n\tfor ;\n\tSun, 27 Jan 2019 21:36:28 +0100 (CET)","from pendragon.ideasonboard.com (85-76-73-155-nat.elisa-mobile.fi\n\t[85.76.73.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 08D7385;\n\tSun, 27 Jan 2019 21:36:22 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548621388;\n\tbh=n59MucBfz0d2I1XH7yHM9riT/8azpa12bTrT8x0dBj0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Cth+ki32CQUv92D3iX8VT8Ck6Jbpdtsmu9aPAv1AyG8UBH1MFR/4ltL1B+SjnRWPZ\n\tMZJzEaZuzr02j83nTsspQP54exi0M7/+lxolHGO/q85gEMDOlwlPwqTcyDKN/y+bBA\n\t/afMuGzJMgwgZvlcfS/EcqMZbWhzaKzokiZOY534=","Date":"Sun, 27 Jan 2019 22:36:18 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190127203618.GB4323@pendragon.ideasonboard.com>","References":"<20190126151318.28087-1-jacopo@jmondi.org>\n\t<20190126151318.28087-3-jacopo@jmondi.org>\n\t<20190127003956.GX4127@bigcity.dyn.berto.se>\n\t<20190127103857.xgatemvq477lsqqt@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190127103857.xgatemvq477lsqqt@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 2/3] libcamera: v4l2_device: Add methods\n\tto 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":"Sun, 27 Jan 2019 20:36:29 -0000"}}]