[{"id":11071,"web_url":"https://patchwork.libcamera.org/comment/11071/","msgid":"<20200702084254.n5gel6wcqztgasmd@uno.localdomain>","date":"2020-07-02T08:42:54","subject":"Re: [libcamera-devel] [RFC 0/2] libcamera: formats: Remove\n\tImageFormats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Jul 01, 2020 at 11:16:48PM +0200, Niklas Söderlund wrote:\n> Hello,\n>\n> This is in no way an attempt to conflict with the pending patches to\n> ImageFormats on the ML from Jacopo. Following his patches there was a\n\nOh it should, this is much better :)\n\n> discussion if we could instead of making ImageFormats a template make\n> use of the new utils::map_keys() helper. I liked the idea and gave it a\n> try and the result was not so bad so I thought I send it out as an RFC.\n>\n> As the new helper is not yet merged this series depends on [1].\n>\n> 1. [PATCH v2 0/2] libcamera: utils: Add map_keys() function\n>\n\nI'm still a little debated here.\n\nOne direction is to use plain std::map with type helpers. Then we'll\nhave V4L2VideoDevice::Formats, V4L2VideoSubdevice::Formats and\n(hopefully) Stream::Formats as I really hope we could find a way to\nget rid of StreamFormats, as most likely ImageFormats, they're halfway\nbetween a custom class and a map, and I agree this is bad.\n\nExposing an std::map and it's API will frown-upon all attempt to expand\nthe *::Formats API whenever we deem necessary. This might be bad, but\nnot that nasty.\n\nI'm toying with another ideas here. If I look at the current usage of\nV4L2*Device::Formats it seems to me that the pattern is pretty\nstraightforward: we ask for keys, ask for sizes by key, and sometimes\nas for the whole map to transform keys in something else\n(V4L2PixelFormat to PixelFormat, mostly). Can all of this go through\nthe video (sub)device and hide the map handling behind a few accessors\nmethods ?\n\nSo we'll have\n std::vector formats = device->pixelFormats()\n\nin place of\n V4L2VideoDevice::Formats formats = device->formats();\n std::vector pixFormats = formats.map_keys()\n\nThis would not solve Stream, but I think it could be treated\ndifferently, being application API, we'll probably need something more\nthan a map there.\n\n\n> Niklas Söderlund (2):\n> libcamera: v4l2_subdevice: Replace ImageFormats with a map\n> libcamera: formats: Remove ImageFormats\n>\n> include/libcamera/internal/camera_sensor.h | 6 +-\n> include/libcamera/internal/formats.h | 14 ----\n> include/libcamera/internal/v4l2_subdevice.h | 4 +-\n> src/libcamera/camera_sensor.cpp | 12 +--\n> src/libcamera/formats.cpp | 88 ---------------------\n> src/libcamera/v4l2_subdevice.cpp | 16 ++--\n> test/v4l2_subdevice/list_formats.cpp | 16 ++--\n> 7 files changed, 27 insertions(+), 129 deletions(-)\n>\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CD9C0BE905\n\tfor ;\n\tThu, 2 Jul 2020 08:39:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6559460C56;\n\tThu, 2 Jul 2020 10:39:27 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C230E603AE\n\tfor ;\n\tThu, 2 Jul 2020 10:39:25 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 3D80160005;\n\tThu, 2 Jul 2020 08:39:24 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 2 Jul 2020 10:42:54 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200702084254.n5gel6wcqztgasmd@uno.localdomain>","References":"<20200701211650.1002567-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701211650.1002567-1-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [RFC 0/2] libcamera: formats: Remove\n\tImageFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11075,"web_url":"https://patchwork.libcamera.org/comment/11075/","msgid":"<20200702104540.GD12562@pendragon.ideasonboard.com>","date":"2020-07-02T10:45:40","subject":"Re: [libcamera-devel] [RFC 0/2] libcamera: formats: Remove\n\tImageFormats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Jul 02, 2020 at 10:42:54AM +0200, Jacopo Mondi wrote:\n> On Wed, Jul 01, 2020 at 11:16:48PM +0200, Niklas Söderlund wrote:\n> > Hello,\n> >\n> > This is in no way an attempt to conflict with the pending patches to\n> > ImageFormats on the ML from Jacopo. Following his patches there was a\n> \n> Oh it should, this is much better :)\n> \n> > discussion if we could instead of making ImageFormats a template make\n> > use of the new utils::map_keys() helper. I liked the idea and gave it a\n> > try and the result was not so bad so I thought I send it out as an RFC.\n> >\n> > As the new helper is not yet merged this series depends on [1].\n> >\n> > 1. [PATCH v2 0/2] libcamera: utils: Add map_keys() function\n> \n> I'm still a little debated here.\n> \n> One direction is to use plain std::map with type helpers. Then we'll\n> have V4L2VideoDevice::Formats, V4L2VideoSubdevice::Formats and\n> (hopefully) Stream::Formats as I really hope we could find a way to\n> get rid of StreamFormats, as most likely ImageFormats, they're halfway\n> between a custom class and a map, and I agree this is bad.\n> \n> Exposing an std::map and it's API will frown-upon all attempt to expand\n> the *::Formats API whenever we deem necessary. This might be bad, but\n> not that nasty.\n> \n> I'm toying with another ideas here. If I look at the current usage of\n> V4L2*Device::Formats it seems to me that the pattern is pretty\n> straightforward: we ask for keys, ask for sizes by key, and sometimes\n> as for the whole map to transform keys in something else\n> (V4L2PixelFormat to PixelFormat, mostly). Can all of this go through\n> the video (sub)device and hide the map handling behind a few accessors\n> methods ?\n> \n> So we'll have\n> std::vector formats = device->pixelFormats()\n\nYou'll have to duplicate that in two classes, and it would also\nduplicate device->formats() that gives the same information (and more).\nIf helpers are needed to access information contained in the formats for\ndifferent use cases, I think it's best to have those helpers provided by\nthe container and not the class that initially provides the information.\n\n> in place of\n> V4L2VideoDevice::Formats formats = device->formats();\n> std::vector pixFormats = formats.map_keys()\n> \n> This would not solve Stream, but I think it could be treated\n> differently, being application API, we'll probably need something more\n> than a map there.\n> \n> > Niklas Söderlund (2):\n> > libcamera: v4l2_subdevice: Replace ImageFormats with a map\n> > libcamera: formats: Remove ImageFormats\n> >\n> > include/libcamera/internal/camera_sensor.h | 6 +-\n> > include/libcamera/internal/formats.h | 14 ----\n> > include/libcamera/internal/v4l2_subdevice.h | 4 +-\n> > src/libcamera/camera_sensor.cpp | 12 +--\n> > src/libcamera/formats.cpp | 88 ---------------------\n> > src/libcamera/v4l2_subdevice.cpp | 16 ++--\n> > test/v4l2_subdevice/list_formats.cpp | 16 ++--\n> > 7 files changed, 27 insertions(+), 129 deletions(-)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CFC30BFFE2\n\tfor ;\n\tThu, 2 Jul 2020 10:45:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 519DE60C56;\n\tThu, 2 Jul 2020 12:45:46 +0200 (CEST)","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 9F02D603B3\n\tfor ;\n\tThu, 2 Jul 2020 12:45:45 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A12F29CB;\n\tThu, 2 Jul 2020 12:45:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UpLCwB3f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593686745;\n\tbh=8ykZj9lm+Xn+3p875S97/hWZ9gX9JszoUUA/QJ5NwXo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UpLCwB3fVonnWWBhIDxf68pPlCaYMaiy7L2dOF3O8SBP3yzFVzdcCQoXnfwt4kCQB\n\te2ml3AB7D9HwYi4mIHRyupbGx/sDoFlq3UHkhYqoZvHxNE0n4aHkEGyL82ueVdq2ig\n\tMPNjVCVa9WCKZ45fqwOUKPAi81txYJbPSb/2qZUg=","Date":"Thu, 2 Jul 2020 13:45:40 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200702104540.GD12562@pendragon.ideasonboard.com>","References":"<20200701211650.1002567-1-niklas.soderlund@ragnatech.se>\n\t<20200702084254.n5gel6wcqztgasmd@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200702084254.n5gel6wcqztgasmd@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC 0/2] libcamera: formats: Remove\n\tImageFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11080,"web_url":"https://patchwork.libcamera.org/comment/11080/","msgid":"<20200702142954.xew5cphsfoygsqna@uno.localdomain>","date":"2020-07-02T14:29:54","subject":"Re: [libcamera-devel] [RFC 0/2] libcamera: formats: Remove\n\tImageFormats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi again,\n\nOn Wed, Jul 01, 2020 at 11:16:48PM +0200, Niklas Söderlund wrote:\n> Hello,\n>\n> This is in no way an attempt to conflict with the pending patches to\n> ImageFormats on the ML from Jacopo. Following his patches there was a\n> discussion if we could instead of making ImageFormats a template make\n> use of the new utils::map_keys() helper. I liked the idea and gave it a\n> try and the result was not so bad so I thought I send it out as an RFC.\n>\n> As the new helper is not yet merged this series depends on [1].\n>\n\nHappy to see this series getting traction, but to get it merged it\nshould address the custom maps used by V4L2VideoDevice users,\notherwise the main goal of unifying the handling maps of mbus codes\nand v4l2 fourccs is lost.\n\n> 1. [PATCH v2 0/2] libcamera: utils: Add map_keys() function\n>\n> Niklas Söderlund (2):\n> libcamera: v4l2_subdevice: Replace ImageFormats with a map\n> libcamera: formats: Remove ImageFormats\n>\n> include/libcamera/internal/camera_sensor.h | 6 +-\n> include/libcamera/internal/formats.h | 14 ----\n> include/libcamera/internal/v4l2_subdevice.h | 4 +-\n> src/libcamera/camera_sensor.cpp | 12 +--\n> src/libcamera/formats.cpp | 88 ---------------------\n> src/libcamera/v4l2_subdevice.cpp | 16 ++--\n> test/v4l2_subdevice/list_formats.cpp | 16 ++--\n> 7 files changed, 27 insertions(+), 129 deletions(-)\n>\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 86207BFFE2\n\tfor ;\n\tThu, 2 Jul 2020 14:26:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A70460C56;\n\tThu, 2 Jul 2020 16:26:27 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93D3E603B3\n\tfor ;\n\tThu, 2 Jul 2020 16:26:25 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 1639160004;\n\tThu, 2 Jul 2020 14:26:24 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 2 Jul 2020 16:29:54 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200702142954.xew5cphsfoygsqna@uno.localdomain>","References":"<20200701211650.1002567-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701211650.1002567-1-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [RFC 0/2] libcamera: formats: Remove\n\tImageFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]