[{"id":5242,"web_url":"https://patchwork.libcamera.org/comment/5242/","msgid":"<20200617140839.ncsdud6rk5rg2gan@uno.localdomain>","date":"2020-06-17T14:08:39","subject":"Re: [libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_ENUM_FRAMESIZES","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Jun 16, 2020 at 10:12:35PM +0900, Paul Elder wrote:\n> Implement VIDIOC_ENUM_FRAMESIZES in the V4L2 compatibility layer.\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/v4l2/v4l2_camera_proxy.cpp | 24 ++++++++++++++++++++++++\n> src/v4l2/v4l2_camera_proxy.h | 1 +\n> 2 files changed, 25 insertions(+)\n>\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 6c0dacc..a5fa478 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -251,6 +251,27 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)\n> \treturn 0;\n> }\n>\n> +int V4L2CameraProxy::vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_framesizes fd = \" << fd;\n> +\n> +\tif (arg == nullptr)\n> +\t\treturn -EFAULT;\n> +\n> +\tPixelFormat argFormat = v4l2ToDrm(arg->pixel_format);\n just format ?\n\n> +\tconst std::vector &frameSizes = streamConfig_.formats().sizes(argFormat);\n\njust sizes ?\n\nAnyway, very few pipeline handlers report StreamFormats at the moment,\nI would record this with a \\todo entry.\n\nAnother point is that the streamConfig_ you here use to enumerate formats is\ngenerated using the Viewfinder role only. In this case, I expect\npipeline handlers not to report any entry for, for example, raw\nformats, while the Camera actually supports it.\n\nThere's no easy way around this I'm afraid, or at least I don't see an\neasy one. I had to do a similar thing for Android, as you can see in\nsrc/android/camera_device.cpp:243\n\nThere I decided to create a list of image resolutions, and test\nthem against the application provided pixel format, by using\nCameraConfiguration::validate(). This probably won't scale here, as\nthe list of resolution would be quite arbitrary, while in Android I\nhave a specification to follow which dictates which resolution are\nmandatory to be supported.\n\nGenerating multiple CameraConfiguration (one for each supported role)\nand test all of them until we don't find on which reports the desired\nimage format is another option, but would require you to introduce an\nAPI to do so in your V4L2Camera class. In both cases, a bit of rework\nis required. What do you think ?\n\n> +\n> +\tif (arg->index >= frameSizes.size())\n> +\t\treturn -EINVAL;\n\nThat's fine, as if the format is not supported StreamFormats::sizes()\nwill return an empty vector, but the v4l2 specs do not say what is the\nexpected behaviour in that case. Does v4l2-compliance try this call\nwith an invalid format ?\n\n> +\n> +\targ->type = V4L2_FRMSIZE_TYPE_DISCRETE;\n> +\targ->discrete.width = frameSizes[arg->index].width;\n> +\targ->discrete.height = frameSizes[arg->index].height;\n> +\tmemset(arg->reserved, 0, sizeof(arg->reserved));\n> +\n> +\treturn 0;\n> +}\n> +\n> int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg)\n> {\n> \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt fd = \" << fd;\n> @@ -676,6 +697,9 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)\n> \tcase VIDIOC_QUERYCAP:\n> \t\tret = vidioc_querycap(static_cast(arg));\n> \t\tbreak;\n> +\tcase VIDIOC_ENUM_FRAMESIZES:\n> +\t\tret = vidioc_enum_framesizes(fd, static_cast(arg));\n> +\t\tbreak;\n> \tcase VIDIOC_ENUM_FMT:\n> \t\tret = vidioc_enum_fmt(fd, static_cast(arg));\n> \t\tbreak;\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index 2e90bfd..2ff9571 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -45,6 +45,7 @@ private:\n> \tint freeBuffers();\n>\n> \tint vidioc_querycap(struct v4l2_capability *arg);\n> +\tint vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg);\n> \tint vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg);\n> \tint vidioc_g_fmt(int fd, struct v4l2_format *arg);\n> \tint vidioc_s_fmt(int fd, struct v4l2_format *arg);\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":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4E002603BF\n\tfor ;\n\tWed, 17 Jun 2020 16:05:15 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id A018110000D;\n\tWed, 17 Jun 2020 14:05:14 +0000 (UTC)"],"Date":"Wed, 17 Jun 2020 16:08:39 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200617140839.ncsdud6rk5rg2gan@uno.localdomain>","References":"<20200616131244.70308-1-paul.elder@ideasonboard.com>\n\t<20200616131244.70308-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200616131244.70308-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_ENUM_FRAMESIZES","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","X-List-Received-Date":"Wed, 17 Jun 2020 14:05:15 -0000"}},{"id":5255,"web_url":"https://patchwork.libcamera.org/comment/5255/","msgid":"<20200617154333.GJ5838@pendragon.ideasonboard.com>","date":"2020-06-17T15:43:33","subject":"Re: [libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_ENUM_FRAMESIZES","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, Jun 17, 2020 at 04:08:39PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 16, 2020 at 10:12:35PM +0900, Paul Elder wrote:\n> > Implement VIDIOC_ENUM_FRAMESIZES in the V4L2 compatibility layer.\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/v4l2/v4l2_camera_proxy.cpp | 24 ++++++++++++++++++++++++\n> > src/v4l2/v4l2_camera_proxy.h | 1 +\n> > 2 files changed, 25 insertions(+)\n> >\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index 6c0dacc..a5fa478 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -251,6 +251,27 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)\n> > \treturn 0;\n> > }\n> >\n> > +int V4L2CameraProxy::vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_framesizes fd = \" << fd;\n> > +\n> > +\tif (arg == nullptr)\n> > +\t\treturn -EFAULT;\n> > +\n> > +\tPixelFormat argFormat = v4l2ToDrm(arg->pixel_format);\n> just format ?\n> \n> > +\tconst std::vector &frameSizes = streamConfig_.formats().sizes(argFormat);\n> \n> just sizes ?\n> \n> Anyway, very few pipeline handlers report StreamFormats at the moment,\n> I would record this with a \\todo entry.\n> \n> Another point is that the streamConfig_ you here use to enumerate formats is\n> generated using the Viewfinder role only. In this case, I expect\n> pipeline handlers not to report any entry for, for example, raw\n> formats, while the Camera actually supports it.\n\nIs that a problem though ? The V4L2 compat layer is meant to support\nexisting V4L2 applications that are not yet ported to libcamera (or that\ncan't easily be ported, for instance because they're not open-source),\non a best effort basis. I would not expect such applications to really\nhave a use for raw formats. I think the viewfinder role makes sense as a\nbest-effort implementation.\n\n> There's no easy way around this I'm afraid, or at least I don't see an\n> easy one. I had to do a similar thing for Android, as you can see in\n> src/android/camera_device.cpp:243\n> \n> There I decided to create a list of image resolutions, and test\n> them against the application provided pixel format, by using\n> CameraConfiguration::validate(). This probably won't scale here, as\n> the list of resolution would be quite arbitrary, while in Android I\n> have a specification to follow which dictates which resolution are\n> mandatory to be supported.\n> \n> Generating multiple CameraConfiguration (one for each supported role)\n> and test all of them until we don't find on which reports the desired\n> image format is another option, but would require you to introduce an\n> API to do so in your V4L2Camera class. In both cases, a bit of rework\n> is required. What do you think ?\n> \n> > +\n> > +\tif (arg->index >= frameSizes.size())\n> > +\t\treturn -EINVAL;\n> \n> That's fine, as if the format is not supported StreamFormats::sizes()\n> will return an empty vector, but the v4l2 specs do not say what is the\n> expected behaviour in that case. Does v4l2-compliance try this call\n> with an invalid format ?\n\n\tret = testEnumFrameSizes(node, 0x20202020);\n\tif (ret != ENOTTY)\n\t\treturn fail(\"Accepted framesize for invalid format\\n\");\n\nand testEnumFrameSizes has a special case:\n\n\t\tif (f == 0 && ret == EINVAL)\n\t\t\treturn ENOTTY;\n\n(f being the loop index). So I think the code is fine.\n\nReviewed-by: Laurent Pinchart \n\n> > +\n> > +\targ->type = V4L2_FRMSIZE_TYPE_DISCRETE;\n> > +\targ->discrete.width = frameSizes[arg->index].width;\n> > +\targ->discrete.height = frameSizes[arg->index].height;\n> > +\tmemset(arg->reserved, 0, sizeof(arg->reserved));\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg)\n> > {\n> > \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt fd = \" << fd;\n> > @@ -676,6 +697,9 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)\n> > \tcase VIDIOC_QUERYCAP:\n> > \t\tret = vidioc_querycap(static_cast(arg));\n> > \t\tbreak;\n> > +\tcase VIDIOC_ENUM_FRAMESIZES:\n> > +\t\tret = vidioc_enum_framesizes(fd, static_cast(arg));\n> > +\t\tbreak;\n> > \tcase VIDIOC_ENUM_FMT:\n> > \t\tret = vidioc_enum_fmt(fd, static_cast(arg));\n> > \t\tbreak;\n> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > index 2e90bfd..2ff9571 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.h\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -45,6 +45,7 @@ private:\n> > \tint freeBuffers();\n> >\n> > \tint vidioc_querycap(struct v4l2_capability *arg);\n> > +\tint vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg);\n> > \tint vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg);\n> > \tint vidioc_g_fmt(int fd, struct v4l2_format *arg);\n> > \tint vidioc_s_fmt(int fd, struct v4l2_format *arg);","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 954F5603BF\n\tfor ;\n\tWed, 17 Jun 2020 17:43:56 +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 05BC12BD;\n\tWed, 17 Jun 2020 17:43:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UBcwDn3b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592408636;\n\tbh=9b9S6iVFgsL8BJIVadjcllBhH2+wqipUyHp7THY/4b4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UBcwDn3b412/raShhSlOXDJwXTG/ySmO0aS4w3eq49LZenAALIsEvK+hhXG3YMyva\n\tSSxkjtF/2U0uJ8/S1xW/VbapvDc9dpOAOWG/qncxUZggpB5MybD8tE7s5bgIgtkIjM\n\tQoiEa0UoTcSID+XhrM05qWc0woAPIE2TQoLL7oQc=","Date":"Wed, 17 Jun 2020 18:43:33 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200617154333.GJ5838@pendragon.ideasonboard.com>","References":"<20200616131244.70308-1-paul.elder@ideasonboard.com>\n\t<20200616131244.70308-7-paul.elder@ideasonboard.com>\n\t<20200617140839.ncsdud6rk5rg2gan@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200617140839.ncsdud6rk5rg2gan@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_ENUM_FRAMESIZES","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","X-List-Received-Date":"Wed, 17 Jun 2020 15:43:56 -0000"}},{"id":5263,"web_url":"https://patchwork.libcamera.org/comment/5263/","msgid":"<20200618052425.GA2095@jade.amanokami.net>","date":"2020-06-18T05:24:25","subject":"Re: [libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_ENUM_FRAMESIZES","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for the review.\n\nOn Wed, Jun 17, 2020 at 04:08:39PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Tue, Jun 16, 2020 at 10:12:35PM +0900, Paul Elder wrote:\n> > Implement VIDIOC_ENUM_FRAMESIZES in the V4L2 compatibility layer.\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/v4l2/v4l2_camera_proxy.cpp | 24 ++++++++++++++++++++++++\n> > src/v4l2/v4l2_camera_proxy.h | 1 +\n> > 2 files changed, 25 insertions(+)\n> >\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index 6c0dacc..a5fa478 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -251,6 +251,27 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)\n> > \treturn 0;\n> > }\n> >\n> > +int V4L2CameraProxy::vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_framesizes fd = \" << fd;\n> > +\n> > +\tif (arg == nullptr)\n> > +\t\treturn -EFAULT;\n> > +\n> > +\tPixelFormat argFormat = v4l2ToDrm(arg->pixel_format);\n> just format ?\n> \n> > +\tconst std::vector &frameSizes = streamConfig_.formats().sizes(argFormat);\n> \n> just sizes ?\n\nI don't understand; what's wrong with just format and just sizes? The\nonly two input parameters to enum_framesizes are index and pixel_format.\n\n> Anyway, very few pipeline handlers report StreamFormats at the moment,\n> I would record this with a \\todo entry.\n\nOh, okay.\n\n> Another point is that the streamConfig_ you here use to enumerate formats is\n> generated using the Viewfinder role only. In this case, I expect\n> pipeline handlers not to report any entry for, for example, raw\n> formats, while the Camera actually supports it.\n> There's no easy way around this I'm afraid, or at least I don't see an\n> easy one. I had to do a similar thing for Android, as you can see in\n> src/android/camera_device.cpp:243\n> \n> There I decided to create a list of image resolutions, and test\n> them against the application provided pixel format, by using\n> CameraConfiguration::validate(). This probably won't scale here, as\n> the list of resolution would be quite arbitrary, while in Android I\n\nYeah :/ I'd rather not.\n\n> have a specification to follow which dictates which resolution are\n> mandatory to be supported.\n> \n> Generating multiple CameraConfiguration (one for each supported role)\n> and test all of them until we don't find on which reports the desired\n> image format is another option, but would require you to introduce an\n> API to do so in your V4L2Camera class. In both cases, a bit of rework\n> is required. What do you think ?\n\nI can't keep what I already have here? tbh I just added this as a\nconvenience for applications where the user can explicitly choose the\nformat, like qv4l2, and not for applications that choose it\nautomatically, like firefox and skype. So in the absence of this ioctl,\napplications would just go with the usual g/s_fmt negotiation.\n\n> > +\n> > +\tif (arg->index >= frameSizes.size())\n> > +\t\treturn -EINVAL;\n> \n> That's fine, as if the format is not supported StreamFormats::sizes()\n> will return an empty vector, but the v4l2 specs do not say what is the\n\nYeah, that was my intention.\n\n> expected behaviour in that case. Does v4l2-compliance try this call\n> with an invalid format ?\n\n\nThanks,\n\nPaul\n\n> > +\n> > +\targ->type = V4L2_FRMSIZE_TYPE_DISCRETE;\n> > +\targ->discrete.width = frameSizes[arg->index].width;\n> > +\targ->discrete.height = frameSizes[arg->index].height;\n> > +\tmemset(arg->reserved, 0, sizeof(arg->reserved));\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg)\n> > {\n> > \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt fd = \" << fd;\n> > @@ -676,6 +697,9 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)\n> > \tcase VIDIOC_QUERYCAP:\n> > \t\tret = vidioc_querycap(static_cast(arg));\n> > \t\tbreak;\n> > +\tcase VIDIOC_ENUM_FRAMESIZES:\n> > +\t\tret = vidioc_enum_framesizes(fd, static_cast(arg));\n> > +\t\tbreak;\n> > \tcase VIDIOC_ENUM_FMT:\n> > \t\tret = vidioc_enum_fmt(fd, static_cast(arg));\n> > \t\tbreak;\n> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > index 2e90bfd..2ff9571 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.h\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -45,6 +45,7 @@ private:\n> > \tint freeBuffers();\n> >\n> > \tint vidioc_querycap(struct v4l2_capability *arg);\n> > +\tint vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg);\n> > \tint vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg);\n> > \tint vidioc_g_fmt(int fd, struct v4l2_format *arg);\n> > \tint vidioc_s_fmt(int fd, struct v4l2_format *arg);\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":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADA32603BD\n\tfor ;\n\tThu, 18 Jun 2020 07:24:35 +0200 (CEST)","from jade.amanokami.net (unknown\n\t[IPv6:2400:4051:61:600:a17c:96b8:ddb0:63b7])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB2CCF9;\n\tThu, 18 Jun 2020 07:24:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dp1PhzqV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592457875;\n\tbh=xt8yP6Ja6/LyMAcP+QuyDI+a81hro1D1PAv8cjS4v+c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dp1PhzqV85B2R+H1Zt4q1RoVZ7MYSAxtSLVsS6+9qzyFHfB6cK34O24g1lTCZk5z5\n\t7UBkFvl17B+4pFcjgwHeHFnq2HHRK27XH/KUTq9Frl+D5z+zeoz6c3Nn1nZ1JUqHZu\n\t3dWZLOygiZ83xOAs/aE5/Cfq+kk7s501c/lDPnx4=","Date":"Thu, 18 Jun 2020 14:24:25 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200618052425.GA2095@jade.amanokami.net>","References":"<20200616131244.70308-1-paul.elder@ideasonboard.com>\n\t<20200616131244.70308-7-paul.elder@ideasonboard.com>\n\t<20200617140839.ncsdud6rk5rg2gan@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20200617140839.ncsdud6rk5rg2gan@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_ENUM_FRAMESIZES","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","X-List-Received-Date":"Thu, 18 Jun 2020 05:24:35 -0000"}},{"id":5265,"web_url":"https://patchwork.libcamera.org/comment/5265/","msgid":"<20200618075832.ajlrjnthg7yeocex@uno.localdomain>","date":"2020-06-18T07:58:32","subject":"Re: [libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_ENUM_FRAMESIZES","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Thu, Jun 18, 2020 at 02:24:25PM +0900, Paul Elder wrote:\n> Hi Jacopo,\n>\n> Thanks for the review.\n>\n> On Wed, Jun 17, 2020 at 04:08:39PM +0200, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Tue, Jun 16, 2020 at 10:12:35PM +0900, Paul Elder wrote:\n> > > Implement VIDIOC_ENUM_FRAMESIZES in the V4L2 compatibility layer.\n> > >\n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > src/v4l2/v4l2_camera_proxy.cpp | 24 ++++++++++++++++++++++++\n> > > src/v4l2/v4l2_camera_proxy.h | 1 +\n> > > 2 files changed, 25 insertions(+)\n> > >\n> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > > index 6c0dacc..a5fa478 100644\n> > > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > > @@ -251,6 +251,27 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)\n> > > \treturn 0;\n> > > }\n> > >\n> > > +int V4L2CameraProxy::vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg)\n> > > +{\n> > > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_framesizes fd = \" << fd;\n> > > +\n> > > +\tif (arg == nullptr)\n> > > +\t\treturn -EFAULT;\n> > > +\n> > > +\tPixelFormat argFormat = v4l2ToDrm(arg->pixel_format);\n> > just format ?\n> >\n> > > +\tconst std::vector &frameSizes = streamConfig_.formats().sizes(argFormat);\n> >\n> > just sizes ?\n>\n> I don't understand; what's wrong with just format and just sizes? The\n> only two input parameters to enum_framesizes are index and pixel_format.\n>\n\nahah, I was suggesting to use \"format\" and \"sizes\" in place of\nargFormat and frameSizes :)\n\n> > Anyway, very few pipeline handlers report StreamFormats at the moment,\n> > I would record this with a \\todo entry.\n>\n> Oh, okay.\n>\n> > Another point is that the streamConfig_ you here use to enumerate formats is\n> > generated using the Viewfinder role only. In this case, I expect\n> > pipeline handlers not to report any entry for, for example, raw\n> > formats, while the Camera actually supports it.\n> > There's no easy way around this I'm afraid, or at least I don't see an\n> > easy one. I had to do a similar thing for Android, as you can see in\n> > src/android/camera_device.cpp:243\n> >\n> > There I decided to create a list of image resolutions, and test\n> > them against the application provided pixel format, by using\n> > CameraConfiguration::validate(). This probably won't scale here, as\n> > the list of resolution would be quite arbitrary, while in Android I\n>\n> Yeah :/ I'd rather not.\n>\n> > have a specification to follow which dictates which resolution are\n> > mandatory to be supported.\n> >\n> > Generating multiple CameraConfiguration (one for each supported role)\n> > and test all of them until we don't find on which reports the desired\n> > image format is another option, but would require you to introduce an\n> > API to do so in your V4L2Camera class. In both cases, a bit of rework\n> > is required. What do you think ?\n>\n> I can't keep what I already have here? tbh I just added this as a\n> convenience for applications where the user can explicitly choose the\n> format, like qv4l2, and not for applications that choose it\n> automatically, like firefox and skype. So in the absence of this ioctl,\n> applications would just go with the usual g/s_fmt negotiation.\n>\n\nAs Laurent pointed out, this is a best effort support for existing\napplications, and they likely are not interested in raw formats or\nadvanced capture modes, so I think it's fine!\n\n> > > +\n> > > +\tif (arg->index >= frameSizes.size())\n> > > +\t\treturn -EINVAL;\n> >\n> > That's fine, as if the format is not supported StreamFormats::sizes()\n> > will return an empty vector, but the v4l2 specs do not say what is the\n>\n> Yeah, that was my intention.\n>\n> > expected behaviour in that case. Does v4l2-compliance try this call\n> > with an invalid format ?\n\nI guess the last question was the relevant one, but Laurent has\nreported that an invalid format is tested with:\n ret = testEnumFrameSizes(node, 0x20202020);\n\nSo I think we're good with reporting EINVAL here\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n>\n> Thanks,\n>\n> Paul\n>\n> > > +\n> > > +\targ->type = V4L2_FRMSIZE_TYPE_DISCRETE;\n> > > +\targ->discrete.width = frameSizes[arg->index].width;\n> > > +\targ->discrete.height = frameSizes[arg->index].height;\n> > > +\tmemset(arg->reserved, 0, sizeof(arg->reserved));\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg)\n> > > {\n> > > \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt fd = \" << fd;\n> > > @@ -676,6 +697,9 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)\n> > > \tcase VIDIOC_QUERYCAP:\n> > > \t\tret = vidioc_querycap(static_cast(arg));\n> > > \t\tbreak;\n> > > +\tcase VIDIOC_ENUM_FRAMESIZES:\n> > > +\t\tret = vidioc_enum_framesizes(fd, static_cast(arg));\n> > > +\t\tbreak;\n> > > \tcase VIDIOC_ENUM_FMT:\n> > > \t\tret = vidioc_enum_fmt(fd, static_cast(arg));\n> > > \t\tbreak;\n> > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > > index 2e90bfd..2ff9571 100644\n> > > --- a/src/v4l2/v4l2_camera_proxy.h\n> > > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > > @@ -45,6 +45,7 @@ private:\n> > > \tint freeBuffers();\n> > >\n> > > \tint vidioc_querycap(struct v4l2_capability *arg);\n> > > +\tint vidioc_enum_framesizes(int fd, struct v4l2_frmsizeenum *arg);\n> > > \tint vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg);\n> > > \tint vidioc_g_fmt(int fd, struct v4l2_format *arg);\n> > > \tint vidioc_s_fmt(int fd, struct v4l2_format *arg);\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":"","Received":["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 D169161167\n\tfor ;\n\tThu, 18 Jun 2020 09:55:07 +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 4C84760008;\n\tThu, 18 Jun 2020 07:55:06 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 18 Jun 2020 09:58:32 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200618075832.ajlrjnthg7yeocex@uno.localdomain>","References":"<20200616131244.70308-1-paul.elder@ideasonboard.com>\n\t<20200616131244.70308-7-paul.elder@ideasonboard.com>\n\t<20200617140839.ncsdud6rk5rg2gan@uno.localdomain>\n\t<20200618052425.GA2095@jade.amanokami.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200618052425.GA2095@jade.amanokami.net>","Subject":"Re: [libcamera-devel] [PATCH 06/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_ENUM_FRAMESIZES","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","X-List-Received-Date":"Thu, 18 Jun 2020 07:55:08 -0000"}}]