[{"id":18489,"web_url":"https://patchwork.libcamera.org/comment/18489/","msgid":"<20210802143323.txw5zlfc52pmdla4@uno.localdomain>","date":"2021-08-02T14:33:23","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: base:\n\tcamera_sensor: Expose sensor's formats map","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello Umang,\n\nOn Fri, Jul 30, 2021 at 01:58:26PM +0530, Umang Jain wrote:\n> Add a getter function formats() to retrieve the V4L2Subdevice::format\n> map of all the formats that the sensor supports. This will probably be\n> used by pipeline handlers to match against a custom list of formats\n> internally while making a selection on sensor's format to be used for\n> image capture.\n>\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index db12b07e..d8826e8a 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -37,6 +37,7 @@ public:\n> \tconst std::string &model() const { return model_; }\n> \tconst std::string &id() const { return id_; }\n> \tconst MediaEntity *entity() const { return entity_; }\n> +\tconst V4L2Subdevice::Formats &formats() const { return formats_; }\n\nWe currently have\n\n \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n \tconst std::vector &sizes() const { return sizes_; }\n\nsizes() is used by the CIO2 class only to enumerate the supported raw\nstream resolution.\n\nHonestly exposing the list of all resolution through sizes() without\ncontext (the mbus code those sizes can be produced with) is not nice.\nIt only works so far as we know on IPU3 that all the produced raw formats\nare good.\n\nI would transform sizes() in sizes(unsigned int) and if one needs to\nknow all the sizes it will be required to\n\n std::vector allSizes;\n for (mbusCode : sensor->mbusCodes())\n for (size : sensor->sizes(mbusCode))\n allSizes.push_back(size);\n\nbut this will have duplicates, so probably the caller should use an\nstd::set<>.\n\nAnyway, my point is that having\n\n\tconst V4L2Subdevice::Formats &formats() const { return formats_; }\n\nRenders\n\n \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n \tconst std::vector &sizes() const { return sizes_; }\n\nA bit useless and exposing the v4l2 subdev formats map is possibly\nundesirable, as ph will have to use the V4L2Subdevice::Format which is\nV4L2 specific.\n\nI would then rather\n\n \tconst std::vector &sizes(unsigned int mbusCode) const;\n\nWhat do you think ?\n\n\n\n> \tSize resolution() const;\n> --\n> 2.31.0\n>","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 59D35C3235\n\tfor ;\n\tMon, 2 Aug 2021 14:32:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 723DA687CC;\n\tMon, 2 Aug 2021 16:32:41 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 137E76026F\n\tfor ;\n\tMon, 2 Aug 2021 16:32:40 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 945F424000D;\n\tMon, 2 Aug 2021 14:32:39 +0000 (UTC)"],"Date":"Mon, 2 Aug 2021 16:33:23 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210802143323.txw5zlfc52pmdla4@uno.localdomain>","References":"<20210730082832.152626-1-umang.jain@ideasonboard.com>\n\t<20210730082832.152626-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210730082832.152626-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: base:\n\tcamera_sensor: Expose sensor's formats map","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18499,"web_url":"https://patchwork.libcamera.org/comment/18499/","msgid":"<6d02930e-3000-fcce-b895-6357d7a2eb67@ideasonboard.com>","date":"2021-08-03T05:18:51","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: base:\n\tcamera_sensor: Expose sensor's formats map","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 8/2/21 8:03 PM, Jacopo Mondi wrote:\n> Hello Umang,\n>\n> On Fri, Jul 30, 2021 at 01:58:26PM +0530, Umang Jain wrote:\n>> Add a getter function formats() to retrieve the V4L2Subdevice::format\n>> map of all the formats that the sensor supports. This will probably be\n>> used by pipeline handlers to match against a custom list of formats\n>> internally while making a selection on sensor's format to be used for\n>> image capture.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/internal/camera_sensor.h | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n>> index db12b07e..d8826e8a 100644\n>> --- a/include/libcamera/internal/camera_sensor.h\n>> +++ b/include/libcamera/internal/camera_sensor.h\n>> @@ -37,6 +37,7 @@ public:\n>> \tconst std::string &model() const { return model_; }\n>> \tconst std::string &id() const { return id_; }\n>> \tconst MediaEntity *entity() const { return entity_; }\n>> +\tconst V4L2Subdevice::Formats &formats() const { return formats_; }\n> We currently have\n>\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> \tconst std::vector &sizes() const { return sizes_; }\n>\n> sizes() is used by the CIO2 class only to enumerate the supported raw\n> stream resolution.\n>\n> Honestly exposing the list of all resolution through sizes() without\n> context (the mbus code those sizes can be produced with) is not nice.\n> It only works so far as we know on IPU3 that all the produced raw formats\n> are good.\n>\n> I would transform sizes() in sizes(unsigned int) and if one needs to\n> know all the sizes it will be required to\n>\n> std::vector allSizes;\n> for (mbusCode : sensor->mbusCodes())\n> for (size : sensor->sizes(mbusCode))\n> allSizes.push_back(size);\n>\n> but this will have duplicates, so probably the caller should use an\n> std::set<>.\n\nI agree on the approach, but the good thing about having sizes_ queried \nfrom  CameraSensor, was that, it was assured to be non-duplicated and \nsorted everytime.\n\nLike you said, now we have to take care of these nuances for -all-sizes- \nuse-cause, use std::set<> to maintain unique-ness, probably transform \nthem into a vector as well before returning. All in all, cubersome to \nhave to deal with it everything some ph needs to know about all the sizes.\n\nCurrently we have only one call to sizes(), in CIO2Device::sizes(), so \nmaybe we can go ahead with this approach, but I think it's /not/ looking \nthe best.\n\n\n>\n> Anyway, my point is that having\n>\n> \tconst V4L2Subdevice::Formats &formats() const { return formats_; }\n>\n> Renders\n>\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> \tconst std::vector &sizes() const { return sizes_; }\n>\n> A bit useless and exposing the v4l2 subdev formats map is possibly\n> undesirable, as ph will have to use the V4L2Subdevice::Format which is\n> V4L2 specific.\nMakes sense.\n>\n> I would then rather\n>\n> \tconst std::vector &sizes(unsigned int mbusCode) const;\n>\n> What do you think ?\n\nSince the vector will be created and populated inside the function \nitself, I don't think we can return via a reference std::vector <>&  here.\n\n\n>\n>\n>\n>> \tSize resolution() const;\n>> --\n>> 2.31.0\n>>","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 87F08C3235\n\tfor ;\n\tTue, 3 Aug 2021 05:19:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3D79687CF;\n\tTue, 3 Aug 2021 07:18:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB13760269\n\tfor ;\n\tTue, 3 Aug 2021 07:18:57 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.12])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 229C14A3;\n\tTue, 3 Aug 2021 07:18:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jfKhULoe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627967937;\n\tbh=6Ua7rEQnyIaSPCKXwvot1U1dHH6PaFuZwFC8byrrVbM=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=jfKhULoev+IsQA9yLsSIUOfq/zGMUTkmBED/AZOKi2R///JV7OgmvyO8YxVdLpucP\n\tTifF4Nsx/7anNzvC8MYBLD7eK6ZcqAzVMePzBI65K1KygpdapIbA+tjBMByihwdzKg\n\tdVe+WqsHlkQgnRsjIceAL0/urr73i4Rq5qSlOZKk=","To":"Jacopo Mondi ","References":"<20210730082832.152626-1-umang.jain@ideasonboard.com>\n\t<20210730082832.152626-2-umang.jain@ideasonboard.com>\n\t<20210802143323.txw5zlfc52pmdla4@uno.localdomain>","From":"Umang Jain ","Message-ID":"<6d02930e-3000-fcce-b895-6357d7a2eb67@ideasonboard.com>","Date":"Tue, 3 Aug 2021 10:48:51 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210802143323.txw5zlfc52pmdla4@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: base:\n\tcamera_sensor: Expose sensor's formats map","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18505,"web_url":"https://patchwork.libcamera.org/comment/18505/","msgid":"<20210803074923.dksg7syi5xpaqsbu@uno.localdomain>","date":"2021-08-03T07:49:23","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: base:\n\tcamera_sensor: Expose sensor's formats map","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Tue, Aug 03, 2021 at 10:48:51AM +0530, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 8/2/21 8:03 PM, Jacopo Mondi wrote:\n> > Hello Umang,\n> >\n> > On Fri, Jul 30, 2021 at 01:58:26PM +0530, Umang Jain wrote:\n> > > Add a getter function formats() to retrieve the V4L2Subdevice::format\n> > > map of all the formats that the sensor supports. This will probably be\n> > > used by pipeline handlers to match against a custom list of formats\n> > > internally while making a selection on sensor's format to be used for\n> > > image capture.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 1 +\n> > > 1 file changed, 1 insertion(+)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index db12b07e..d8826e8a 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -37,6 +37,7 @@ public:\n> > > \tconst std::string &model() const { return model_; }\n> > > \tconst std::string &id() const { return id_; }\n> > > \tconst MediaEntity *entity() const { return entity_; }\n> > > +\tconst V4L2Subdevice::Formats &formats() const { return formats_; }\n> > We currently have\n> >\n> > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > \tconst std::vector &sizes() const { return sizes_; }\n> >\n> > sizes() is used by the CIO2 class only to enumerate the supported raw\n> > stream resolution.\n> >\n> > Honestly exposing the list of all resolution through sizes() without\n> > context (the mbus code those sizes can be produced with) is not nice.\n> > It only works so far as we know on IPU3 that all the produced raw formats\n> > are good.\n> >\n> > I would transform sizes() in sizes(unsigned int) and if one needs to\n> > know all the sizes it will be required to\n> >\n> > std::vector allSizes;\n> > for (mbusCode : sensor->mbusCodes())\n> > for (size : sensor->sizes(mbusCode))\n> > allSizes.push_back(size);\n> >\n> > but this will have duplicates, so probably the caller should use an\n> > std::set<>.\n>\n> I agree on the approach, but the good thing about having sizes_ queried\n> from  CameraSensor, was that, it was assured to be non-duplicated and sorted\n> everytime.\n>\n> Like you said, now we have to take care of these nuances for -all-sizes-\n> use-cause, use std::set<> to maintain unique-ness, probably transform them\n> into a vector as well before returning. All in all, cubersome to have to\n> deal with it everything some ph needs to know about all the sizes.\n>\n> Currently we have only one call to sizes(), in CIO2Device::sizes(), so maybe\n> we can go ahead with this approach, but I think it's /not/ looking the best.\n>\n>\n> >\n> > Anyway, my point is that having\n> >\n> > \tconst V4L2Subdevice::Formats &formats() const { return formats_; }\n> >\n> > Renders\n> >\n> > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > \tconst std::vector &sizes() const { return sizes_; }\n> >\n> > A bit useless and exposing the v4l2 subdev formats map is possibly\n> > undesirable, as ph will have to use the V4L2Subdevice::Format which is\n> > V4L2 specific.\n> Makes sense.\n> >\n> > I would then rather\n> >\n> > \tconst std::vector &sizes(unsigned int mbusCode) const;\n> >\n> > What do you think ?\n>\n> Since the vector will be created and populated inside the function itself, I\n> don't think we can return via a reference std::vector <>&  here.\n\nIndeed, that was a copy&paste leftover :)\n\n>\n>\n> >\n> >\n> >\n> > > \tSize resolution() const;\n> > > --\n> > > 2.31.0\n> > >","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 DB828C3232\n\tfor ;\n\tTue, 3 Aug 2021 07:48:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49305687CF;\n\tTue, 3 Aug 2021 09:48:38 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79DE46026D\n\tfor ;\n\tTue, 3 Aug 2021 09:48:37 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id CC178C0012;\n\tTue, 3 Aug 2021 07:48:36 +0000 (UTC)"],"Date":"Tue, 3 Aug 2021 09:49:23 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210803074923.dksg7syi5xpaqsbu@uno.localdomain>","References":"<20210730082832.152626-1-umang.jain@ideasonboard.com>\n\t<20210730082832.152626-2-umang.jain@ideasonboard.com>\n\t<20210802143323.txw5zlfc52pmdla4@uno.localdomain>\n\t<6d02930e-3000-fcce-b895-6357d7a2eb67@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<6d02930e-3000-fcce-b895-6357d7a2eb67@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: base:\n\tcamera_sensor: Expose sensor's formats map","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]