[{"id":5264,"web_url":"https://patchwork.libcamera.org/comment/5264/","msgid":"","date":"2020-06-18T07:31:07","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nGentle ping to see if anyone can review this when you get a chance.\n\nThanks,\n\nNaush\n\nOn Wed, 10 Jun 2020 at 15:26, Naushir Patuck wrote:\n>\n> In generateConfiguration(), add the device node specific formats to the\n> StreamConfiguration for each StreamRole requested.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> 1 file changed, 36 insertions(+), 16 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index e16a9c7f..03a1e641 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> RPiCameraData *data = cameraData(camera);\n> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> V4L2DeviceFormat sensorFormat;\n> + unsigned int bufferCount;\n> + PixelFormat pixelFormat;\n> V4L2PixFmtMap fmts;\n> + Size size;\n>\n> if (roles.empty())\n> return config;\n>\n> for (const StreamRole role : roles) {\n> - StreamConfiguration cfg{};\n> -\n> switch (role) {\n> case StreamRole::StillCaptureRaw:\n> - cfg.size = data->sensor_->resolution();\n> + size = data->sensor_->resolution();\n> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> - sensorFormat = findBestMode(fmts, cfg.size);\n> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> - ASSERT(cfg.pixelFormat.isValid());\n> - cfg.bufferCount = 1;\n> + sensorFormat = findBestMode(fmts, size);\n> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> + ASSERT(pixelFormat.isValid());\n> + bufferCount = 1;\n> break;\n>\n> case StreamRole::StillCapture:\n> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> /* Return the largest sensor resolution. */\n> - cfg.size = data->sensor_->resolution();\n> - cfg.bufferCount = 1;\n> + size = data->sensor_->resolution();\n> + bufferCount = 1;\n> break;\n>\n> case StreamRole::VideoRecording:\n> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> - cfg.size = { 1920, 1080 };\n> - cfg.bufferCount = 4;\n> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> + size = { 1920, 1080 };\n> + bufferCount = 4;\n> break;\n>\n> case StreamRole::Viewfinder:\n> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> - cfg.size = { 800, 600 };\n> - cfg.bufferCount = 4;\n> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> + size = { 800, 600 };\n> + bufferCount = 4;\n> break;\n>\n> default:\n> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> break;\n> }\n>\n> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> + std::map> deviceFormats;\n> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> + [&](const decltype(fmts)::value_type &format) {\n> + return decltype(deviceFormats)::value_type{\n> + format.first.toPixelFormat(),\n> + format.second\n> + };\n> + });\n> +\n> + /* Add the stream format based on the device node used for the use case. */\n> + StreamFormats formats(deviceFormats);\n> + StreamConfiguration cfg(formats);\n> + cfg.size = size;\n> + cfg.pixelFormat = pixelFormat;\n> + cfg.bufferCount = bufferCount;\n> config->addConfiguration(cfg);\n> }\n>\n> --\n> 2.25.1\n>","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 4F56961167\n\tfor ;\n\tThu, 18 Jun 2020 09:31:25 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id g2so2891508lfb.0\n\tfor ;\n\tThu, 18 Jun 2020 00:31:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"W3dWwM8Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=3p49yveGuNNwd5aBxveV56P0LxvWlYaKuhvgAuiuZvI=;\n\tb=W3dWwM8ZMj21GzRmUYqoXgKJAz+g3xtb6RDwOO9h9WT6ALk5s/J7PmCGITxRTk6i5U\n\tFYjoGnDWCd4xzmNpfVsxGDPklNsZXCXxjbTEDxXNhQB6p9Ui+YPdn0likiVT9W19Ljsy\n\tzJoxjKs1HuGM9KGjn54VHO9VmiKqBBt3mUe2+nLn5ocq7vdHebNWMuLhYALf7J/sr3bA\n\t2RzzMxTpR4cZ1uexEyYpqWm8Cjwjhz03yvj4entWIEPXWVX1MzlxlqBO/a2+tRuhGzmZ\n\tpzuAhTnNe7yCYevo+Ok8YYNWbP8FRWIycguy+9Q9q3CCr7SNVNg/Lv3V0XMA49MP1j3N\n\tl+Jw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=3p49yveGuNNwd5aBxveV56P0LxvWlYaKuhvgAuiuZvI=;\n\tb=QIzTOqE7t4Vlmn3As8xvUEQqzPzBn0MU4RwsB9Hp6ITt+cIL5HRB9BkMnDlWYgC+zP\n\tZgs28e8LOET0+k9aO4B8VFuc17SlIpoPRGWqkKDH+ODqNYGYNoOZT2KTY6NrlSFQXUQk\n\tb4Gt1y7AqbXAt/C8jzjfASqzL8ZZeWQutCVuCpBi7ttSB8FKkByoq0utYnBxAFAT1AFv\n\t+nEXC7UL+QV6ZLW1lupt4dEPzZ8IkkugTlJvddpRppYc3C0h8/QgBtDcqPfmYwDDbT5B\n\t2AxdpGdK+wKyrXipS8xZzORH4EbEsibGs3FO4StWtLcsX/bWuZQj12yHvJczvrxygpFb\n\twC0w==","X-Gm-Message-State":"AOAM530rwmd5rj2ypk+RamIReGDmLE0PjbNn06eoY4dwuFItDWtsbkSI\n\t5T041nOjK0Ux3YJkXdohDxWho1TSM1I1iB7t08Db3Jp/","X-Google-Smtp-Source":"ABdhPJxSg0VYf6koNLkcjTwnLj9dQBPDMt7Aj9Y2ugMwfm54M4+32g+mTJ10hIU26FRhsjKiruurLSkZXI1635gcgeY=","X-Received":"by 2002:a05:6512:3107:: with SMTP id\n\tn7mr1639716lfb.63.1592465483940; \n\tThu, 18 Jun 2020 00:31:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20200610142640.576529-1-naush@raspberrypi.com>","In-Reply-To":"<20200610142640.576529-1-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Thu, 18 Jun 2020 08:31:07 +0100","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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:31:25 -0000"}},{"id":5267,"web_url":"https://patchwork.libcamera.org/comment/5267/","msgid":"<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>","date":"2020-06-18T08:41:44","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n sorry for the delay, this fell through the cracks\n\nOn Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> In generateConfiguration(), add the device node specific formats to the\n> StreamConfiguration for each StreamRole requested.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> 1 file changed, 36 insertions(+), 16 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index e16a9c7f..03a1e641 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \tRPiCameraData *data = cameraData(camera);\n> \tCameraConfiguration *config = new RPiCameraConfiguration(data);\n> \tV4L2DeviceFormat sensorFormat;\n> +\tunsigned int bufferCount;\n> +\tPixelFormat pixelFormat;\n> \tV4L2PixFmtMap fmts;\n> +\tSize size;\n>\n> \tif (roles.empty())\n> \t\treturn config;\n>\n> \tfor (const StreamRole role : roles) {\n> -\t\tStreamConfiguration cfg{};\n> -\n> \t\tswitch (role) {\n> \t\tcase StreamRole::StillCaptureRaw:\n> -\t\t\tcfg.size = data->sensor_->resolution();\n> +\t\t\tsize = data->sensor_->resolution();\n> \t\t\tfmts = data->unicam_[Unicam::Image].dev()->formats();\n> -\t\t\tsensorFormat = findBestMode(fmts, cfg.size);\n> -\t\t\tcfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> -\t\t\tASSERT(cfg.pixelFormat.isValid());\n> -\t\t\tcfg.bufferCount = 1;\n> +\t\t\tsensorFormat = findBestMode(fmts, size);\n> +\t\t\tpixelFormat = sensorFormat.fourcc.toPixelFormat();\n> +\t\t\tASSERT(pixelFormat.isValid());\n> +\t\t\tbufferCount = 1;\n> \t\t\tbreak;\n>\n> \t\tcase StreamRole::StillCapture:\n> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> +\t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> +\t\t\tpixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> \t\t\t/* Return the largest sensor resolution. */\n> -\t\t\tcfg.size = data->sensor_->resolution();\n> -\t\t\tcfg.bufferCount = 1;\n> +\t\t\tsize = data->sensor_->resolution();\n> +\t\t\tbufferCount = 1;\n> \t\t\tbreak;\n>\n> \t\tcase StreamRole::VideoRecording:\n> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> -\t\t\tcfg.size = { 1920, 1080 };\n> -\t\t\tcfg.bufferCount = 4;\n> +\t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> +\t\t\tpixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> +\t\t\tsize = { 1920, 1080 };\n> +\t\t\tbufferCount = 4;\n> \t\t\tbreak;\n>\n> \t\tcase StreamRole::Viewfinder:\n> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> -\t\t\tcfg.size = { 800, 600 };\n> -\t\t\tcfg.bufferCount = 4;\n> +\t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> +\t\t\tpixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> +\t\t\tsize = { 800, 600 };\n> +\t\t\tbufferCount = 4;\n> \t\t\tbreak;\n>\n> \t\tdefault:\n> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\t\tbreak;\n> \t\t}\n>\n> +\t\t/* Translate the V4L2PixelFormat to PixelFormat. */\n> +\t\tstd::map> deviceFormats;\n> +\t\tstd::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> +\t\t\t [&](const decltype(fmts)::value_type &format) {\n> +\t\t\t\t\treturn decltype(deviceFormats)::value_type{\n> +\t\t\t\t\t\tformat.first.toPixelFormat(),\n> +\t\t\t\t\t\tformat.second\n> +\t\t\t\t\t};\n> +\t\t\t });\n\nReally took me a while to parse this, as I was not expecting to see\nstd::inserter() but just deviceFormats.begin() there, but then I\nlearned about inserter, and indeed if I remove std::inserter() I get\nan error due to the fact the copy operator of std::pair is deleted.\n\n> +\n> +\t\t/* Add the stream format based on the device node used for the use case. */\n> +\t\tStreamFormats formats(deviceFormats);\n> +\t\tStreamConfiguration cfg(formats);\n> +\t\tcfg.size = size;\n> +\t\tcfg.pixelFormat = pixelFormat;\n> +\t\tcfg.bufferCount = bufferCount;\n\nPatch looks good to me\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t\tconfig->addConfiguration(cfg);\n> \t}\n>\n> --\n> 2.25.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 relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A46061167\n\tfor ;\n\tThu, 18 Jun 2020 10:38:20 +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 83483100006;\n\tThu, 18 Jun 2020 08:38:19 +0000 (UTC)"],"Date":"Thu, 18 Jun 2020 10:41:44 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>","References":"<20200610142640.576529-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200610142640.576529-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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 08:38:20 -0000"}},{"id":5268,"web_url":"https://patchwork.libcamera.org/comment/5268/","msgid":"","date":"2020-06-18T09:03:38","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush, Jacopo,\n\nOn 18/06/2020 09:41, Jacopo Mondi wrote:\n> Hi Naush,\n> sorry for the delay, this fell through the cracks\n> \n> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n>> In generateConfiguration(), add the device node specific formats to the\n>> StreamConfiguration for each StreamRole requested.\n>>\n>> Signed-off-by: Naushir Patuck \n>> ---\n>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n>> 1 file changed, 36 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> index e16a9c7f..03a1e641 100644\n>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>> \tRPiCameraData *data = cameraData(camera);\n>> \tCameraConfiguration *config = new RPiCameraConfiguration(data);\n>> \tV4L2DeviceFormat sensorFormat;\n>> +\tunsigned int bufferCount;\n>> +\tPixelFormat pixelFormat;\n>> \tV4L2PixFmtMap fmts;\n>> +\tSize size;\n>>\n>> \tif (roles.empty())\n>> \t\treturn config;\n>>\n>> \tfor (const StreamRole role : roles) {\n>> -\t\tStreamConfiguration cfg{};\n>> -\n>> \t\tswitch (role) {\n>> \t\tcase StreamRole::StillCaptureRaw:\n>> -\t\t\tcfg.size = data->sensor_->resolution();\n>> +\t\t\tsize = data->sensor_->resolution();\n>> \t\t\tfmts = data->unicam_[Unicam::Image].dev()->formats();\n>> -\t\t\tsensorFormat = findBestMode(fmts, cfg.size);\n>> -\t\t\tcfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n>> -\t\t\tASSERT(cfg.pixelFormat.isValid());\n>> -\t\t\tcfg.bufferCount = 1;\n>> +\t\t\tsensorFormat = findBestMode(fmts, size);\n>> +\t\t\tpixelFormat = sensorFormat.fourcc.toPixelFormat();\n>> +\t\t\tASSERT(pixelFormat.isValid());\n>> +\t\t\tbufferCount = 1;\n>> \t\t\tbreak;\n>>\n>> \t\tcase StreamRole::StillCapture:\n>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n>> +\t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n>> +\t\t\tpixelFormat = PixelFormat(DRM_FORMAT_NV12);\n>> \t\t\t/* Return the largest sensor resolution. */\n>> -\t\t\tcfg.size = data->sensor_->resolution();\n>> -\t\t\tcfg.bufferCount = 1;\n>> +\t\t\tsize = data->sensor_->resolution();\n>> +\t\t\tbufferCount = 1;\n>> \t\t\tbreak;\n>>\n>> \t\tcase StreamRole::VideoRecording:\n>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n>> -\t\t\tcfg.size = { 1920, 1080 };\n>> -\t\t\tcfg.bufferCount = 4;\n>> +\t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n>> +\t\t\tpixelFormat = PixelFormat(DRM_FORMAT_NV12);\n>> +\t\t\tsize = { 1920, 1080 };\n>> +\t\t\tbufferCount = 4;\n>> \t\t\tbreak;\n>>\n>> \t\tcase StreamRole::Viewfinder:\n>> -\t\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n>> -\t\t\tcfg.size = { 800, 600 };\n>> -\t\t\tcfg.bufferCount = 4;\n>> +\t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n>> +\t\t\tpixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n>> +\t\t\tsize = { 800, 600 };\n>> +\t\t\tbufferCount = 4;\n>> \t\t\tbreak;\n>>\n>> \t\tdefault:\n>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>> \t\t\tbreak;\n>> \t\t}\n>>\n>> +\t\t/* Translate the V4L2PixelFormat to PixelFormat. */\n>> +\t\tstd::map> deviceFormats;\n>> +\t\tstd::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n>> +\t\t\t [&](const decltype(fmts)::value_type &format) {\n>> +\t\t\t\t\treturn decltype(deviceFormats)::value_type{\n>> +\t\t\t\t\t\tformat.first.toPixelFormat(),\n>> +\t\t\t\t\t\tformat.second\n>> +\t\t\t\t\t};\n>> +\t\t\t });\n> \n> Really took me a while to parse this, as I was not expecting to see\n> std::inserter() but just deviceFormats.begin() there, but then I\n> learned about inserter, and indeed if I remove std::inserter() I get\n> an error due to the fact the copy operator of std::pair is deleted.\n\nI think that's the same as the conversion routine in the UVC pipeline\nhandler (which I think came from Laurent).\n\nNot for this patch (I think this can go in as is) but we should really\nmake a helper for that conversion as it's likely to be used in multiple\nplaces, and it is quite hard to parse on it's own ;-(.\n\nHowever, I think that overlaps with changes that you (Jacopo) were\nworking on with ImageFormats anyway.\n\nEitherway, this patch will help support more formats and enumeration on\nthe RPi pipeline handler:\n\nReviewed-by: Kieran Bingham \n\n\n> \n>> +\n>> +\t\t/* Add the stream format based on the device node used for the use case. */\n>> +\t\tStreamFormats formats(deviceFormats);\n>> +\t\tStreamConfiguration cfg(formats);\n>> +\t\tcfg.size = size;\n>> +\t\tcfg.pixelFormat = pixelFormat;\n>> +\t\tcfg.bufferCount = bufferCount;\n> \n> Patch looks good to me\n> \n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n>> \t\tconfig->addConfiguration(cfg);\n>> \t}\n>>\n>> --\n>> 2.25.1\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 56A6361167\n\tfor ;\n\tThu, 18 Jun 2020 11:03:41 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AF905F9;\n\tThu, 18 Jun 2020 11:03:40 +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=\"FhLhIqyP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592471020;\n\tbh=/4z/om7J1Knv/vnIqk0tBM0KYiOaIG54N2RMqg+f1mA=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=FhLhIqyPr/a7edV+eiw+u65a8hQZNDTnMwhful2mLgAW5UIeG5gV2mZbKvlxjwz5s\n\tbNV3TE23pmkCyc1M4do2HS4Eq/bGq66sZMpNLkLq0YcvZDSrceYW/4vzsGdjg8Fydl\n\tyZqWJGI5yQAYFExb6l674bq1t0GnY7n4sPiZfuPE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi , Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 18 Jun 2020 10:03:38 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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 09:03:41 -0000"}},{"id":5270,"web_url":"https://patchwork.libcamera.org/comment/5270/","msgid":"","date":"2020-06-18T09:18:38","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\nOn Thu, 18 Jun 2020 at 10:03, Kieran Bingham\n wrote:\n>\n> Hi Naush, Jacopo,\n>\n> On 18/06/2020 09:41, Jacopo Mondi wrote:\n> > Hi Naush,\n> > sorry for the delay, this fell through the cracks\n> >\n> > On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> >> In generateConfiguration(), add the device node specific formats to the\n> >> StreamConfiguration for each StreamRole requested.\n> >>\n> >> Signed-off-by: Naushir Patuck \n> >> ---\n> >> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> >> 1 file changed, 36 insertions(+), 16 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> index e16a9c7f..03a1e641 100644\n> >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >> RPiCameraData *data = cameraData(camera);\n> >> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> >> V4L2DeviceFormat sensorFormat;\n> >> + unsigned int bufferCount;\n> >> + PixelFormat pixelFormat;\n> >> V4L2PixFmtMap fmts;\n> >> + Size size;\n> >>\n> >> if (roles.empty())\n> >> return config;\n> >>\n> >> for (const StreamRole role : roles) {\n> >> - StreamConfiguration cfg{};\n> >> -\n> >> switch (role) {\n> >> case StreamRole::StillCaptureRaw:\n> >> - cfg.size = data->sensor_->resolution();\n> >> + size = data->sensor_->resolution();\n> >> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> >> - sensorFormat = findBestMode(fmts, cfg.size);\n> >> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >> - ASSERT(cfg.pixelFormat.isValid());\n> >> - cfg.bufferCount = 1;\n> >> + sensorFormat = findBestMode(fmts, size);\n> >> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >> + ASSERT(pixelFormat.isValid());\n> >> + bufferCount = 1;\n> >> break;\n> >>\n> >> case StreamRole::StillCapture:\n> >> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >> /* Return the largest sensor resolution. */\n> >> - cfg.size = data->sensor_->resolution();\n> >> - cfg.bufferCount = 1;\n> >> + size = data->sensor_->resolution();\n> >> + bufferCount = 1;\n> >> break;\n> >>\n> >> case StreamRole::VideoRecording:\n> >> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >> - cfg.size = { 1920, 1080 };\n> >> - cfg.bufferCount = 4;\n> >> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >> + size = { 1920, 1080 };\n> >> + bufferCount = 4;\n> >> break;\n> >>\n> >> case StreamRole::Viewfinder:\n> >> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >> - cfg.size = { 800, 600 };\n> >> - cfg.bufferCount = 4;\n> >> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >> + size = { 800, 600 };\n> >> + bufferCount = 4;\n> >> break;\n> >>\n> >> default:\n> >> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >> break;\n> >> }\n> >>\n> >> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> >> + std::map> deviceFormats;\n> >> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> >> + [&](const decltype(fmts)::value_type &format) {\n> >> + return decltype(deviceFormats)::value_type{\n> >> + format.first.toPixelFormat(),\n> >> + format.second\n> >> + };\n> >> + });\n> >\n> > Really took me a while to parse this, as I was not expecting to see\n> > std::inserter() but just deviceFormats.begin() there, but then I\n> > learned about inserter, and indeed if I remove std::inserter() I get\n> > an error due to the fact the copy operator of std::pair is deleted.\n>\n> I think that's the same as the conversion routine in the UVC pipeline\n> handler (which I think came from Laurent).\n\nYes, I picked this from the uvc pipeline handler. Took me some goes\nto understand it as well :)\n\n>\n> Not for this patch (I think this can go in as is) but we should really\n> make a helper for that conversion as it's likely to be used in multiple\n> places, and it is quite hard to parse on it's own ;-(.\n\nThat would make sense. All pipeline handlers will require this\nconversion, I think!\n\n>\n> However, I think that overlaps with changes that you (Jacopo) were\n> working on with ImageFormats anyway.\n>\n> Eitherway, this patch will help support more formats and enumeration on\n> the RPi pipeline handler:\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> >\n> >> +\n> >> + /* Add the stream format based on the device node used for the use case. */\n> >> + StreamFormats formats(deviceFormats);\n> >> + StreamConfiguration cfg(formats);\n> >> + cfg.size = size;\n> >> + cfg.pixelFormat = pixelFormat;\n> >> + cfg.bufferCount = bufferCount;\n> >\n> > Patch looks good to me\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> >> config->addConfiguration(cfg);\n> >> }\n> >>\n> >> --\n> >> 2.25.1\n> >>\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n\nRegards,\nNaush","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F0C0161167\n\tfor ;\n\tThu, 18 Jun 2020 11:18:55 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id 9so6347094ljv.5\n\tfor ;\n\tThu, 18 Jun 2020 02:18:55 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"a1QUJmH3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=OzWePUq/gYMqQ48nSChduE8s9Oew5GLLykCe3lzMGgE=;\n\tb=a1QUJmH3X1VlHsvTr9Rk4AsqikVZhNkwHlBke+vG6o5eRNcVjIuqKwDIEzNhMAjaQQ\n\tkwSwLL21kqqQkgPZF50wXHHnIvwYaAb9PDa791qHijvt4iLJWRT7CyWiHeBivpgqsCnF\n\t26HFBAh1ucQPpt4NRc/XYjsm5MffDvwICJQyqgMEYqUshCMbmTTqlt6aW3joVgM4JUh7\n\tNTrUwroL2WmTkB4ThiV9LWLKlhtcerRBkWteRgKV1xLU94SjTaoKMe14LLPpwAMiH3cq\n\tZFlsRZ5WsciCpprz9bya2JiV0Ux3fuRPrVKwAH+UTmuSGig4GdVrQ82hLb8AVayQVKsB\n\tJGGQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=OzWePUq/gYMqQ48nSChduE8s9Oew5GLLykCe3lzMGgE=;\n\tb=JpLuvA4zu3ce52rnvb8GRdk0fASwRt7VAIAnGZsCw1FJZvXy+/RyPAfRMW1qq9ewkD\n\t5waWPnCygmZeKMHCEfilCsYi591QpfJQVJvFpu967kAU9FmGU6EAo0OAceXgBpFleWn9\n\t3KMy3a7l3+QtQBK2JVtGIfo259tRbVAKizRlVPwHZugpS5TcoebbhUXcfO04zkPLlgNj\n\tabnbYGuPP1Mfu8831JXChbOYIPnPd/xD5mYRlr5bG4k8cb0FcM5tuAl04oeUZAQnmI2z\n\tT8DpU+34+ZB2ngh97cCZa18DlxPIRjiAFN9Hvq+jX8dnrfK6EZKagtHteGV+onEI5zw9\n\tiqrw==","X-Gm-Message-State":"AOAM530a+QwpqxQYP0dmLIFYo5bxE+s3OqXP5qd5uBoYkN1ISFX2c2kV\n\t0fO3BNyN+lK72/8Kb9KkPR6FADRS0DypXsTZKb5McIrV","X-Google-Smtp-Source":"ABdhPJz+4BdzLtXMI4WXIFgu1TBxEr7Y1v0byzgpngn48Q1kAi+fADtFAF8i2WRk+K7XWFq+IuakDvHGeQ/t2JxPeLM=","X-Received":"by 2002:a2e:3a04:: with SMTP id h4mr1861388lja.103.1592471935055;\n\tThu, 18 Jun 2020 02:18:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 18 Jun 2020 10:18:38 +0100","Message-ID":"","To":"Kieran Bingham ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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 09:18:56 -0000"}},{"id":5319,"web_url":"https://patchwork.libcamera.org/comment/5319/","msgid":"<20200622045840.GN25355@pendragon.ideasonboard.com>","date":"2020-06-22T04:58:40","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush and everybody,\n\nThank you for the patch.\n\nOn Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> > On 18/06/2020 09:41, Jacopo Mondi wrote:\n> >> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> >>> In generateConfiguration(), add the device node specific formats to the\n> >>> StreamConfiguration for each StreamRole requested.\n> >>>\n> >>> Signed-off-by: Naushir Patuck \n> >>> ---\n> >>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> >>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> index e16a9c7f..03a1e641 100644\n> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>> RPiCameraData *data = cameraData(camera);\n> >>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> >>> V4L2DeviceFormat sensorFormat;\n> >>> + unsigned int bufferCount;\n> >>> + PixelFormat pixelFormat;\n> >>> V4L2PixFmtMap fmts;\n> >>> + Size size;\n> >>>\n> >>> if (roles.empty())\n> >>> return config;\n> >>>\n> >>> for (const StreamRole role : roles) {\n> >>> - StreamConfiguration cfg{};\n> >>> -\n> >>> switch (role) {\n> >>> case StreamRole::StillCaptureRaw:\n> >>> - cfg.size = data->sensor_->resolution();\n> >>> + size = data->sensor_->resolution();\n> >>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> >>> - sensorFormat = findBestMode(fmts, cfg.size);\n> >>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >>> - ASSERT(cfg.pixelFormat.isValid());\n> >>> - cfg.bufferCount = 1;\n> >>> + sensorFormat = findBestMode(fmts, size);\n> >>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >>> + ASSERT(pixelFormat.isValid());\n> >>> + bufferCount = 1;\n> >>> break;\n> >>>\n> >>> case StreamRole::StillCapture:\n> >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n\nThis conflicts with recent rework of format handling, this line should\nnow be\n\n+\t\t\tpixelFormat = formats::NV12;\n\nSame for the other formats below.\n\n> >>> /* Return the largest sensor resolution. */\n> >>> - cfg.size = data->sensor_->resolution();\n> >>> - cfg.bufferCount = 1;\n> >>> + size = data->sensor_->resolution();\n> >>> + bufferCount = 1;\n> >>> break;\n> >>>\n> >>> case StreamRole::VideoRecording:\n> >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> - cfg.size = { 1920, 1080 };\n> >>> - cfg.bufferCount = 4;\n> >>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> + size = { 1920, 1080 };\n> >>> + bufferCount = 4;\n> >>> break;\n> >>>\n> >>> case StreamRole::Viewfinder:\n> >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>> - cfg.size = { 800, 600 };\n> >>> - cfg.bufferCount = 4;\n> >>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>> + size = { 800, 600 };\n> >>> + bufferCount = 4;\n> >>> break;\n> >>>\n> >>> default:\n> >>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>> break;\n> >>> }\n> >>>\n> >>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> >>> + std::map> deviceFormats;\n> >>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> >>> + [&](const decltype(fmts)::value_type &format) {\n> >>> + return decltype(deviceFormats)::value_type{\n> >>> + format.first.toPixelFormat(),\n> >>> + format.second\n> >>> + };\n> >>> + });\n\nFor the unicam device this looks fine to me, as the driver will\ngive us formats and sizes that match the capabilities of the sensor. For\nthe ISP, the list of supported pixel formats is queried from the\nfirmware, filtered against the list of pixel formats supported by the\ndriver, and all should be fine. However, for the frame sizes, the ISP\ndriver reports hardcoded minimum and maximum values of 64 and 16384\nrespectively. Shouldn't we instead, in the pipeline handler, take the\ncapabilities of both the sensor and the ISP into account to create a\nrange of sizes that would be closer to what can actually be achieved ?\n\nFor UVC the situation is different, as the kernel directly reports the\nlist of formats and sizes supported by the device.\n\n> >> Really took me a while to parse this, as I was not expecting to see\n> >> std::inserter() but just deviceFormats.begin() there, but then I\n> >> learned about inserter, and indeed if I remove std::inserter() I get\n> >> an error due to the fact the copy operator of std::pair is deleted.\n> >\n> > I think that's the same as the conversion routine in the UVC pipeline\n> > handler (which I think came from Laurent).\n> \n> Yes, I picked this from the uvc pipeline handler. Took me some goes\n> to understand it as well :)\n> \n> > Not for this patch (I think this can go in as is) but we should really\n> > make a helper for that conversion as it's likely to be used in multiple\n> > places, and it is quite hard to parse on it's own ;-(.\n> \n> That would make sense. All pipeline handlers will require this\n> conversion, I think!\n> \n> > However, I think that overlaps with changes that you (Jacopo) were\n> > working on with ImageFormats anyway.\n\nYes, we know this API needs to be reworked, so I wouldn't spend time on\ncreating a helper right now, but would rather land the ImageFormats\nseries first, and then discuss how we should rework stream configuration\nhandling. The fact that this code should consider both the sensor and\nthe ISP, as explained above, also makes it more difficult to create a\nsingle helper function.\n\n> > Eitherway, this patch will help support more formats and enumeration on\n> > the RPi pipeline handler:\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >>> +\n> >>> + /* Add the stream format based on the device node used for the use case. */\n> >>> + StreamFormats formats(deviceFormats);\n> >>> + StreamConfiguration cfg(formats);\n> >>> + cfg.size = size;\n> >>> + cfg.pixelFormat = pixelFormat;\n> >>> + cfg.bufferCount = bufferCount;\n> >>\n> >> Patch looks good to me\n> >>\n> >> Reviewed-by: Jacopo Mondi \n> >>\n> >>> config->addConfiguration(cfg);\n> >>> }\n> >>>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5958B603BC\n\tfor ;\n\tMon, 22 Jun 2020 06:59:05 +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 C77DC30D;\n\tMon, 22 Jun 2020 06:59:04 +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=\"n09bqYun\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592801945;\n\tbh=M0d5SXUixlvFC4usz0MjtqTRHv+s6ozHHVzKNCx5P1c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n09bqYunBCljlKnweBjnDRdpDHG3F9GkOBHm9M3litfrWc1F+fVDSEjq9aMMqsfdU\n\tRG4ZLcd2f/s5k55ttCqw7AKYKbk1isYAlciyOVIDTjHojZu5hvW9MDt9hLmSSChPSm\n\tG4YCkb4+nLIxSH60p7qK1JBte5uEm3pMbLzp/ev0=","Date":"Mon, 22 Jun 2020 07:58:40 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200622045840.GN25355@pendragon.ideasonboard.com>","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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":"Mon, 22 Jun 2020 04:59:05 -0000"}},{"id":5327,"web_url":"https://patchwork.libcamera.org/comment/5327/","msgid":"","date":"2020-06-22T10:25:47","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Mon, 22 Jun 2020 at 05:59, Laurent Pinchart\n wrote:\n>\n> Hi Naush and everybody,\n>\n> Thank you for the patch.\n>\n> On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> > On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> > > On 18/06/2020 09:41, Jacopo Mondi wrote:\n> > >> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> > >>> In generateConfiguration(), add the device node specific formats to the\n> > >>> StreamConfiguration for each StreamRole requested.\n> > >>>\n> > >>> Signed-off-by: Naushir Patuck \n> > >>> ---\n> > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> > >>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> > >>>\n> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> index e16a9c7f..03a1e641 100644\n> > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > >>> RPiCameraData *data = cameraData(camera);\n> > >>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > >>> V4L2DeviceFormat sensorFormat;\n> > >>> + unsigned int bufferCount;\n> > >>> + PixelFormat pixelFormat;\n> > >>> V4L2PixFmtMap fmts;\n> > >>> + Size size;\n> > >>>\n> > >>> if (roles.empty())\n> > >>> return config;\n> > >>>\n> > >>> for (const StreamRole role : roles) {\n> > >>> - StreamConfiguration cfg{};\n> > >>> -\n> > >>> switch (role) {\n> > >>> case StreamRole::StillCaptureRaw:\n> > >>> - cfg.size = data->sensor_->resolution();\n> > >>> + size = data->sensor_->resolution();\n> > >>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > >>> - sensorFormat = findBestMode(fmts, cfg.size);\n> > >>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > >>> - ASSERT(cfg.pixelFormat.isValid());\n> > >>> - cfg.bufferCount = 1;\n> > >>> + sensorFormat = findBestMode(fmts, size);\n> > >>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > >>> + ASSERT(pixelFormat.isValid());\n> > >>> + bufferCount = 1;\n> > >>> break;\n> > >>>\n> > >>> case StreamRole::StillCapture:\n> > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n>\n> This conflicts with recent rework of format handling, this line should\n> now be\n>\n> + pixelFormat = formats::NV12;\n>\n> Same for the other formats below.\n\nWill update in the next patch.\n\n>\n> > >>> /* Return the largest sensor resolution. */\n> > >>> - cfg.size = data->sensor_->resolution();\n> > >>> - cfg.bufferCount = 1;\n> > >>> + size = data->sensor_->resolution();\n> > >>> + bufferCount = 1;\n> > >>> break;\n> > >>>\n> > >>> case StreamRole::VideoRecording:\n> > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>> - cfg.size = { 1920, 1080 };\n> > >>> - cfg.bufferCount = 4;\n> > >>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>> + size = { 1920, 1080 };\n> > >>> + bufferCount = 4;\n> > >>> break;\n> > >>>\n> > >>> case StreamRole::Viewfinder:\n> > >>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > >>> - cfg.size = { 800, 600 };\n> > >>> - cfg.bufferCount = 4;\n> > >>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > >>> + size = { 800, 600 };\n> > >>> + bufferCount = 4;\n> > >>> break;\n> > >>>\n> > >>> default:\n> > >>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > >>> break;\n> > >>> }\n> > >>>\n> > >>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> > >>> + std::map> deviceFormats;\n> > >>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> > >>> + [&](const decltype(fmts)::value_type &format) {\n> > >>> + return decltype(deviceFormats)::value_type{\n> > >>> + format.first.toPixelFormat(),\n> > >>> + format.second\n> > >>> + };\n> > >>> + });\n>\n> For the unicam device this looks fine to me, as the driver will\n> give us formats and sizes that match the capabilities of the sensor. For\n> the ISP, the list of supported pixel formats is queried from the\n> firmware, filtered against the list of pixel formats supported by the\n> driver, and all should be fine. However, for the frame sizes, the ISP\n> driver reports hardcoded minimum and maximum values of 64 and 16384\n> respectively. Shouldn't we instead, in the pipeline handler, take the\n> capabilities of both the sensor and the ISP into account to create a\n> range of sizes that would be closer to what can actually be achieved ?\n\nJust checking, so I should populate\nStreamFormat::std::vector with the device ranges? What\nabout the StreamConfiguration::size field that I currently populate\nwith a single value, should I leave that blank/empty?\n\nRegards,\nNaush\n\n\n>\n> For UVC the situation is different, as the kernel directly reports the\n> list of formats and sizes supported by the device.\n>\n> > >> Really took me a while to parse this, as I was not expecting to see\n> > >> std::inserter() but just deviceFormats.begin() there, but then I\n> > >> learned about inserter, and indeed if I remove std::inserter() I get\n> > >> an error due to the fact the copy operator of std::pair is deleted.\n> > >\n> > > I think that's the same as the conversion routine in the UVC pipeline\n> > > handler (which I think came from Laurent).\n> >\n> > Yes, I picked this from the uvc pipeline handler. Took me some goes\n> > to understand it as well :)\n> >\n> > > Not for this patch (I think this can go in as is) but we should really\n> > > make a helper for that conversion as it's likely to be used in multiple\n> > > places, and it is quite hard to parse on it's own ;-(.\n> >\n> > That would make sense. All pipeline handlers will require this\n> > conversion, I think!\n> >\n> > > However, I think that overlaps with changes that you (Jacopo) were\n> > > working on with ImageFormats anyway.\n>\n> Yes, we know this API needs to be reworked, so I wouldn't spend time on\n> creating a helper right now, but would rather land the ImageFormats\n> series first, and then discuss how we should rework stream configuration\n> handling. The fact that this code should consider both the sensor and\n> the ISP, as explained above, also makes it more difficult to create a\n> single helper function.\n>\n> > > Eitherway, this patch will help support more formats and enumeration on\n> > > the RPi pipeline handler:\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > >>> +\n> > >>> + /* Add the stream format based on the device node used for the use case. */\n> > >>> + StreamFormats formats(deviceFormats);\n> > >>> + StreamConfiguration cfg(formats);\n> > >>> + cfg.size = size;\n> > >>> + cfg.pixelFormat = pixelFormat;\n> > >>> + cfg.bufferCount = bufferCount;\n> > >>\n> > >> Patch looks good to me\n> > >>\n> > >> Reviewed-by: Jacopo Mondi \n> > >>\n> > >>> config->addConfiguration(cfg);\n> > >>> }\n> > >>>\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A91AE603BD\n\tfor ;\n\tMon, 22 Jun 2020 12:26:04 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id i3so18674077ljg.3\n\tfor ;\n\tMon, 22 Jun 2020 03:26:04 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"rFTeoNBA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=RBkkaw5wrzkIUK5SD8VVslVdvCZ628I/dJogh/xJLaQ=;\n\tb=rFTeoNBAK9yJ+yEDS7Sx5Ry4eTinHJ7M7zTbwh+KOW2FCE+aKhj1OXDfNAAqxfZ3c2\n\tNSwV8+1uz4f1Df6iARIbYuhjLZQ2yRPMQND7U3jdjo4UbFoFXPhkVPLoYqicSAeE+WCW\n\te0xLq1h5IuRCEDN1zYKY//UrSN8H3p50Smyg9/3MhPMaflzODlWGSG6t+8V2HlpzBAeM\n\tACM+tQ+oyp5gyrGoOraih+y8SCjdR4puFT++ycQV6FA7YB0W2YKPsG1JGo7lvSAV5XiX\n\tsz6Tu2ciHpS2K1NGpqqGU2L8Rvth343vzQlyxR8yehm4kVZzXHRHCSltcPooL3y/ZOLc\n\tHQOA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=RBkkaw5wrzkIUK5SD8VVslVdvCZ628I/dJogh/xJLaQ=;\n\tb=b5tJIQnCndM2JkvVFboEdxUPonjBpH0DFl+/AU4GWozmCcAcsTo+8T/v5HdZU+KKuS\n\t/va4ZqjfZ3UBekBfu3eOdPxsjs+XL7nMjiyc6W/KbjMTVJkl2Bek2KvA4tiLXSrF68WA\n\tQbN9CCh9fGwTTpYsOzqvUJM5fRT2NMa8ihlhaoq7SUVsw4NwSykbnzL7pS22XUV/XSMv\n\tqbZmrmx2OtsTFpeHXB1RI6iWGKLt/ETtNb/pnQnGohoMYe0NtB6B66BHuHAlybr1VvmQ\n\tnAs5zFFrH2TDaimtW35e99zsOVYan1RKOpGz9yrp0/9wIU+KjJFsYI5qhb6lwD2nJQWs\n\t6/Ig==","X-Gm-Message-State":"AOAM530FU+ijBvxPC9mGMkKd/GxOct7YqTI1KLRj5UTvk8aBIGA0EC2r\n\twfZNfm2bar6M2reH1HYPnzTMafXNEqlHILTaUdeAlA==","X-Google-Smtp-Source":"ABdhPJyI0Rhx0bB+yZUAD+ezskwzPBW7jtiiJqRO2fUOP9debp+gHs9nLsDWs6ThwhXg6sdIqUEgcxNiZZtiukjlhg0=","X-Received":"by 2002:a2e:b804:: with SMTP id u4mr8108563ljo.228.1592821563871;\n\tMon, 22 Jun 2020 03:26:03 -0700 (PDT)","MIME-Version":"1.0","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>","In-Reply-To":"<20200622045840.GN25355@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 22 Jun 2020 11:25:47 +0100","Message-ID":"","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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":"Mon, 22 Jun 2020 10:26:05 -0000"}},{"id":5338,"web_url":"https://patchwork.libcamera.org/comment/5338/","msgid":"<20200622234617.GN5852@pendragon.ideasonboard.com>","date":"2020-06-22T23:46:17","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Jun 22, 2020 at 11:25:47AM +0100, Naushir Patuck wrote:\n> On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart wrote:\n> > On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> >> On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> >>> On 18/06/2020 09:41, Jacopo Mondi wrote:\n> >>>> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> >>>>> In generateConfiguration(), add the device node specific formats to the\n> >>>>> StreamConfiguration for each StreamRole requested.\n> >>>>>\n> >>>>> Signed-off-by: Naushir Patuck \n> >>>>> ---\n> >>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> >>>>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> index e16a9c7f..03a1e641 100644\n> >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>>>> RPiCameraData *data = cameraData(camera);\n> >>>>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> >>>>> V4L2DeviceFormat sensorFormat;\n> >>>>> + unsigned int bufferCount;\n> >>>>> + PixelFormat pixelFormat;\n> >>>>> V4L2PixFmtMap fmts;\n> >>>>> + Size size;\n> >>>>>\n> >>>>> if (roles.empty())\n> >>>>> return config;\n> >>>>>\n> >>>>> for (const StreamRole role : roles) {\n> >>>>> - StreamConfiguration cfg{};\n> >>>>> -\n> >>>>> switch (role) {\n> >>>>> case StreamRole::StillCaptureRaw:\n> >>>>> - cfg.size = data->sensor_->resolution();\n> >>>>> + size = data->sensor_->resolution();\n> >>>>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> >>>>> - sensorFormat = findBestMode(fmts, cfg.size);\n> >>>>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >>>>> - ASSERT(cfg.pixelFormat.isValid());\n> >>>>> - cfg.bufferCount = 1;\n> >>>>> + sensorFormat = findBestMode(fmts, size);\n> >>>>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >>>>> + ASSERT(pixelFormat.isValid());\n> >>>>> + bufferCount = 1;\n> >>>>> break;\n> >>>>>\n> >>>>> case StreamRole::StillCapture:\n> >>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >\n> > This conflicts with recent rework of format handling, this line should\n> > now be\n> >\n> > + pixelFormat = formats::NV12;\n> >\n> > Same for the other formats below.\n> \n> Will update in the next patch.\n> \n> >>>>> /* Return the largest sensor resolution. */\n> >>>>> - cfg.size = data->sensor_->resolution();\n> >>>>> - cfg.bufferCount = 1;\n> >>>>> + size = data->sensor_->resolution();\n> >>>>> + bufferCount = 1;\n> >>>>> break;\n> >>>>>\n> >>>>> case StreamRole::VideoRecording:\n> >>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>> - cfg.size = { 1920, 1080 };\n> >>>>> - cfg.bufferCount = 4;\n> >>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>> + size = { 1920, 1080 };\n> >>>>> + bufferCount = 4;\n> >>>>> break;\n> >>>>>\n> >>>>> case StreamRole::Viewfinder:\n> >>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>>>> - cfg.size = { 800, 600 };\n> >>>>> - cfg.bufferCount = 4;\n> >>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>>>> + size = { 800, 600 };\n> >>>>> + bufferCount = 4;\n> >>>>> break;\n> >>>>>\n> >>>>> default:\n> >>>>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>>>> break;\n> >>>>> }\n> >>>>>\n> >>>>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> >>>>> + std::map> deviceFormats;\n> >>>>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> >>>>> + [&](const decltype(fmts)::value_type &format) {\n> >>>>> + return decltype(deviceFormats)::value_type{\n> >>>>> + format.first.toPixelFormat(),\n> >>>>> + format.second\n> >>>>> + };\n> >>>>> + });\n> >\n> > For the unicam device this looks fine to me, as the driver will\n> > give us formats and sizes that match the capabilities of the sensor. For\n> > the ISP, the list of supported pixel formats is queried from the\n> > firmware, filtered against the list of pixel formats supported by the\n> > driver, and all should be fine. However, for the frame sizes, the ISP\n> > driver reports hardcoded minimum and maximum values of 64 and 16384\n> > respectively. Shouldn't we instead, in the pipeline handler, take the\n> > capabilities of both the sensor and the ISP into account to create a\n> > range of sizes that would be closer to what can actually be achieved ?\n> \n> Just checking, so I should populate\n> StreamFormat::std::vector with the device ranges? What\n> about the StreamConfiguration::size field that I currently populate\n> with a single value, should I leave that blank/empty?\n\nStreamConfigure::size is the recommended size for the stream.\nStreamFormats is a map containing all the supported pixel formats, and\nfor each of them, the supported sizes. The sizes are expressed as a\nvector of SizeRange, which must contain at least one entry.\n\nYou populate StreamConfigure::size correctly as far as I can tell,\nthere's no reason to change. The StreamFormats, however, should be\npopulated with what the camera can produce, and that should be\nconditioned by the sizes supported by the sensor and the ISP\ncapabilities. You could, for instance, take all the sizes supported by\nthe sensor, and for each of them, create a SizeRange with the minimum\nset to the sensor size downscaled as much as possible by the ISP, and\nthe maximum set to the sensor size.\n\n> > For UVC the situation is different, as the kernel directly reports the\n> > list of formats and sizes supported by the device.\n> >\n> >>>> Really took me a while to parse this, as I was not expecting to see\n> >>>> std::inserter() but just deviceFormats.begin() there, but then I\n> >>>> learned about inserter, and indeed if I remove std::inserter() I get\n> >>>> an error due to the fact the copy operator of std::pair is deleted.\n> >>>\n> >>> I think that's the same as the conversion routine in the UVC pipeline\n> >>> handler (which I think came from Laurent).\n> >>\n> >> Yes, I picked this from the uvc pipeline handler. Took me some goes\n> >> to understand it as well :)\n> >>\n> >>> Not for this patch (I think this can go in as is) but we should really\n> >>> make a helper for that conversion as it's likely to be used in multiple\n> >>> places, and it is quite hard to parse on it's own ;-(.\n> >>\n> >> That would make sense. All pipeline handlers will require this\n> >> conversion, I think!\n> >>\n> >>> However, I think that overlaps with changes that you (Jacopo) were\n> >>> working on with ImageFormats anyway.\n> >\n> > Yes, we know this API needs to be reworked, so I wouldn't spend time on\n> > creating a helper right now, but would rather land the ImageFormats\n> > series first, and then discuss how we should rework stream configuration\n> > handling. The fact that this code should consider both the sensor and\n> > the ISP, as explained above, also makes it more difficult to create a\n> > single helper function.\n> >\n> >>> Eitherway, this patch will help support more formats and enumeration on\n> >>> the RPi pipeline handler:\n> >>>\n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>>>> +\n> >>>>> + /* Add the stream format based on the device node used for the use case. */\n> >>>>> + StreamFormats formats(deviceFormats);\n> >>>>> + StreamConfiguration cfg(formats);\n> >>>>> + cfg.size = size;\n> >>>>> + cfg.pixelFormat = pixelFormat;\n> >>>>> + cfg.bufferCount = bufferCount;\n> >>>>\n> >>>> Patch looks good to me\n> >>>>\n> >>>> Reviewed-by: Jacopo Mondi \n> >>>>\n> >>>>> config->addConfiguration(cfg);\n> >>>>> }\n> >>>>>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7643603BE\n\tfor ;\n\tTue, 23 Jun 2020 01:46:42 +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 30E7F327;\n\tTue, 23 Jun 2020 01:46:42 +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=\"RdzA9972\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592869602;\n\tbh=IuMEqQQsvPaM73Xcz6u6OKhU+QSYqi82Zxgwe/7KNuE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RdzA9972gH5Ti6/7GH0Nl8V2FPUdKNgJY8c5B2m7uVMqJ27fhLkXV5R8N+kDKNDvw\n\t/AoCfmcQfr8F7VyQw0yewlOca/vD5qx+kLhqrrhd3ayBiyJqrjOhnnNWZRYTy9p3Ph\n\tBWodpxHQdkIcY3LbEj+BHyFjAc7Kk75G270xk4OU=","Date":"Tue, 23 Jun 2020 02:46:17 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200622234617.GN5852@pendragon.ideasonboard.com>","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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":"Mon, 22 Jun 2020 23:46:42 -0000"}},{"id":5346,"web_url":"https://patchwork.libcamera.org/comment/5346/","msgid":"","date":"2020-06-23T07:59:49","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 23 Jun 2020 at 00:46, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Mon, Jun 22, 2020 at 11:25:47AM +0100, Naushir Patuck wrote:\n> > On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart wrote:\n> > > On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> > >> On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> > >>> On 18/06/2020 09:41, Jacopo Mondi wrote:\n> > >>>> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> > >>>>> In generateConfiguration(), add the device node specific formats to the\n> > >>>>> StreamConfiguration for each StreamRole requested.\n> > >>>>>\n> > >>>>> Signed-off-by: Naushir Patuck \n> > >>>>> ---\n> > >>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> > >>>>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> > >>>>>\n> > >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>>>> index e16a9c7f..03a1e641 100644\n> > >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>>>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > >>>>> RPiCameraData *data = cameraData(camera);\n> > >>>>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > >>>>> V4L2DeviceFormat sensorFormat;\n> > >>>>> + unsigned int bufferCount;\n> > >>>>> + PixelFormat pixelFormat;\n> > >>>>> V4L2PixFmtMap fmts;\n> > >>>>> + Size size;\n> > >>>>>\n> > >>>>> if (roles.empty())\n> > >>>>> return config;\n> > >>>>>\n> > >>>>> for (const StreamRole role : roles) {\n> > >>>>> - StreamConfiguration cfg{};\n> > >>>>> -\n> > >>>>> switch (role) {\n> > >>>>> case StreamRole::StillCaptureRaw:\n> > >>>>> - cfg.size = data->sensor_->resolution();\n> > >>>>> + size = data->sensor_->resolution();\n> > >>>>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > >>>>> - sensorFormat = findBestMode(fmts, cfg.size);\n> > >>>>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > >>>>> - ASSERT(cfg.pixelFormat.isValid());\n> > >>>>> - cfg.bufferCount = 1;\n> > >>>>> + sensorFormat = findBestMode(fmts, size);\n> > >>>>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > >>>>> + ASSERT(pixelFormat.isValid());\n> > >>>>> + bufferCount = 1;\n> > >>>>> break;\n> > >>>>>\n> > >>>>> case StreamRole::StillCapture:\n> > >>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >\n> > > This conflicts with recent rework of format handling, this line should\n> > > now be\n> > >\n> > > + pixelFormat = formats::NV12;\n> > >\n> > > Same for the other formats below.\n> >\n> > Will update in the next patch.\n> >\n> > >>>>> /* Return the largest sensor resolution. */\n> > >>>>> - cfg.size = data->sensor_->resolution();\n> > >>>>> - cfg.bufferCount = 1;\n> > >>>>> + size = data->sensor_->resolution();\n> > >>>>> + bufferCount = 1;\n> > >>>>> break;\n> > >>>>>\n> > >>>>> case StreamRole::VideoRecording:\n> > >>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>>>> - cfg.size = { 1920, 1080 };\n> > >>>>> - cfg.bufferCount = 4;\n> > >>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>>>> + size = { 1920, 1080 };\n> > >>>>> + bufferCount = 4;\n> > >>>>> break;\n> > >>>>>\n> > >>>>> case StreamRole::Viewfinder:\n> > >>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > >>>>> - cfg.size = { 800, 600 };\n> > >>>>> - cfg.bufferCount = 4;\n> > >>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>>>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > >>>>> + size = { 800, 600 };\n> > >>>>> + bufferCount = 4;\n> > >>>>> break;\n> > >>>>>\n> > >>>>> default:\n> > >>>>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > >>>>> break;\n> > >>>>> }\n> > >>>>>\n> > >>>>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> > >>>>> + std::map> deviceFormats;\n> > >>>>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> > >>>>> + [&](const decltype(fmts)::value_type &format) {\n> > >>>>> + return decltype(deviceFormats)::value_type{\n> > >>>>> + format.first.toPixelFormat(),\n> > >>>>> + format.second\n> > >>>>> + };\n> > >>>>> + });\n> > >\n> > > For the unicam device this looks fine to me, as the driver will\n> > > give us formats and sizes that match the capabilities of the sensor. For\n> > > the ISP, the list of supported pixel formats is queried from the\n> > > firmware, filtered against the list of pixel formats supported by the\n> > > driver, and all should be fine. However, for the frame sizes, the ISP\n> > > driver reports hardcoded minimum and maximum values of 64 and 16384\n> > > respectively. Shouldn't we instead, in the pipeline handler, take the\n> > > capabilities of both the sensor and the ISP into account to create a\n> > > range of sizes that would be closer to what can actually be achieved ?\n> >\n> > Just checking, so I should populate\n> > StreamFormat::std::vector with the device ranges? What\n> > about the StreamConfiguration::size field that I currently populate\n> > with a single value, should I leave that blank/empty?\n>\n> StreamConfigure::size is the recommended size for the stream.\n> StreamFormats is a map containing all the supported pixel formats, and\n> for each of them, the supported sizes. The sizes are expressed as a\n> vector of SizeRange, which must contain at least one entry.\n>\n> You populate StreamConfigure::size correctly as far as I can tell,\n> there's no reason to change. The StreamFormats, however, should be\n> populated with what the camera can produce, and that should be\n> conditioned by the sizes supported by the sensor and the ISP\n> capabilities. You could, for instance, take all the sizes supported by\n> the sensor, and for each of them, create a SizeRange with the minimum\n> set to the sensor size downscaled as much as possible by the ISP, and\n> the maximum set to the sensor size.\n>\n\nApologies if this is a silly question, but since the ISP hardware\ngenuinely does allow frame sizes in the range [64,16384] in both\ndimensions irrespective of the input size, is what I currently have\nfor populating SizeRange not correct?\n\nRegards,\nNaush\n\n\n> > > For UVC the situation is different, as the kernel directly reports the\n> > > list of formats and sizes supported by the device.\n> > >\n> > >>>> Really took me a while to parse this, as I was not expecting to see\n> > >>>> std::inserter() but just deviceFormats.begin() there, but then I\n> > >>>> learned about inserter, and indeed if I remove std::inserter() I get\n> > >>>> an error due to the fact the copy operator of std::pair is deleted.\n> > >>>\n> > >>> I think that's the same as the conversion routine in the UVC pipeline\n> > >>> handler (which I think came from Laurent).\n> > >>\n> > >> Yes, I picked this from the uvc pipeline handler. Took me some goes\n> > >> to understand it as well :)\n> > >>\n> > >>> Not for this patch (I think this can go in as is) but we should really\n> > >>> make a helper for that conversion as it's likely to be used in multiple\n> > >>> places, and it is quite hard to parse on it's own ;-(.\n> > >>\n> > >> That would make sense. All pipeline handlers will require this\n> > >> conversion, I think!\n> > >>\n> > >>> However, I think that overlaps with changes that you (Jacopo) were\n> > >>> working on with ImageFormats anyway.\n> > >\n> > > Yes, we know this API needs to be reworked, so I wouldn't spend time on\n> > > creating a helper right now, but would rather land the ImageFormats\n> > > series first, and then discuss how we should rework stream configuration\n> > > handling. The fact that this code should consider both the sensor and\n> > > the ISP, as explained above, also makes it more difficult to create a\n> > > single helper function.\n> > >\n> > >>> Eitherway, this patch will help support more formats and enumeration on\n> > >>> the RPi pipeline handler:\n> > >>>\n> > >>> Reviewed-by: Kieran Bingham \n> > >>>\n> > >>>>> +\n> > >>>>> + /* Add the stream format based on the device node used for the use case. */\n> > >>>>> + StreamFormats formats(deviceFormats);\n> > >>>>> + StreamConfiguration cfg(formats);\n> > >>>>> + cfg.size = size;\n> > >>>>> + cfg.pixelFormat = pixelFormat;\n> > >>>>> + cfg.bufferCount = bufferCount;\n> > >>>>\n> > >>>> Patch looks good to me\n> > >>>>\n> > >>>> Reviewed-by: Jacopo Mondi \n> > >>>>\n> > >>>>> config->addConfiguration(cfg);\n> > >>>>> }\n> > >>>>>\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 BD959603BA\n\tfor ;\n\tTue, 23 Jun 2020 10:00:07 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id g2so11173094lfb.0\n\tfor ;\n\tTue, 23 Jun 2020 01:00:07 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"OgSu0SZR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=lNichrdC9BX3rUBpYUuFjYJzs/iN0f900Cw1d354rAg=;\n\tb=OgSu0SZRUbWgjYA9mcsZtuBK+Yl8sGtfib1w3krs7BXpaieXLi8ud8Z3rV599DzO9d\n\t9tZ0tW/vzMclgmzG0DEfkxiKhnL5hW1FafJ8SQ8mnA9ZDLtBLS+g2EazLnZxrKAFCrMj\n\tsHAKlTQR8VP0DowsHrTsa+sww3+PRZ45dmdcWqja9BHefF6QZnA7cdtcInSfp6O0kk9o\n\t/oI/SZ/iYDVQxqlzBeIjarTnhEkAVmZnUZWEYcMJQJkhHL++5lV9F4x25FuDQHK/O3za\n\tYp42OfagIP1HP0u6p7RArzL6uNDssrtzapQDAgsMSYomc3pwXb9H/AozshBNh8GdsAOi\n\tV2TQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=lNichrdC9BX3rUBpYUuFjYJzs/iN0f900Cw1d354rAg=;\n\tb=erlkpyTRwbpv3M1m/Uhv0m1F+JofuQ8yULyvfvpc1j6tv4GX9b0EmyTumqTzrRmlDB\n\tt6PG59oXQQOQRf7kvrUtYCtUr09DAwUZpRQRu3hOPEIl5J3Au4u51dlAckLefiAzYCzV\n\t69//SzC8qg9NN8woGWHyxvjDxS1fu/HSJEZGEKkd5HgxMasBd9OWzc4Z7AfpxP/8aNY2\n\t6ZabWiWER/fLi7X98rT4ykb3/q/p8TS34KkQerb04AW8J7fh6mI4BPy1TfFHxIL/bdyU\n\trIF3MTvmbWkCJOaVDA+6wyETSyhq6+XAY8VQ6cKMPwtrsSVGsJMk8X8qrmFoLr0ibiiO\n\tOh7A==","X-Gm-Message-State":"AOAM531QHsJ8gN7E4zk/Z58MQGdvgxTM338PdBKzhEDkF5hAvwYDDJqF\n\tuMMoTGZCeyKSLw0sNjuaI3bhh0rZqVRMA8ILqZNrvRhl9NY=","X-Google-Smtp-Source":"ABdhPJytK7xFdG6kqKfvmqYzICbLoMCs0gjZc7GjMqfUCJgXlf8+qQF+sUYhwjRDNIajHflCHvz0BzZj/vyej9B/I4k=","X-Received":"by 2002:a19:e93:: with SMTP id\n\t141mr11978895lfo.107.1592899206662; \n\tTue, 23 Jun 2020 01:00:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>\n\t\n\t<20200622234617.GN5852@pendragon.ideasonboard.com>","In-Reply-To":"<20200622234617.GN5852@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 23 Jun 2020 08:59:49 +0100","Message-ID":"","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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":"Tue, 23 Jun 2020 08:00:08 -0000"}},{"id":5390,"web_url":"https://patchwork.libcamera.org/comment/5390/","msgid":"<20200625022126.GA5980@pendragon.ideasonboard.com>","date":"2020-06-25T02:21:26","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Jun 23, 2020 at 08:59:49AM +0100, Naushir Patuck wrote:\n> On Tue, 23 Jun 2020 at 00:46, Laurent Pinchart wrote:\n> > On Mon, Jun 22, 2020 at 11:25:47AM +0100, Naushir Patuck wrote:\n> >> On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart wrote:\n> >>> On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> >>>> On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> >>>>> On 18/06/2020 09:41, Jacopo Mondi wrote:\n> >>>>>> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> >>>>>>> In generateConfiguration(), add the device node specific formats to the\n> >>>>>>> StreamConfiguration for each StreamRole requested.\n> >>>>>>>\n> >>>>>>> Signed-off-by: Naushir Patuck \n> >>>>>>> ---\n> >>>>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> >>>>>>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> >>>>>>>\n> >>>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>>>> index e16a9c7f..03a1e641 100644\n> >>>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>>>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>>>>>> RPiCameraData *data = cameraData(camera);\n> >>>>>>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> >>>>>>> V4L2DeviceFormat sensorFormat;\n> >>>>>>> + unsigned int bufferCount;\n> >>>>>>> + PixelFormat pixelFormat;\n> >>>>>>> V4L2PixFmtMap fmts;\n> >>>>>>> + Size size;\n> >>>>>>>\n> >>>>>>> if (roles.empty())\n> >>>>>>> return config;\n> >>>>>>>\n> >>>>>>> for (const StreamRole role : roles) {\n> >>>>>>> - StreamConfiguration cfg{};\n> >>>>>>> -\n> >>>>>>> switch (role) {\n> >>>>>>> case StreamRole::StillCaptureRaw:\n> >>>>>>> - cfg.size = data->sensor_->resolution();\n> >>>>>>> + size = data->sensor_->resolution();\n> >>>>>>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> >>>>>>> - sensorFormat = findBestMode(fmts, cfg.size);\n> >>>>>>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >>>>>>> - ASSERT(cfg.pixelFormat.isValid());\n> >>>>>>> - cfg.bufferCount = 1;\n> >>>>>>> + sensorFormat = findBestMode(fmts, size);\n> >>>>>>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >>>>>>> + ASSERT(pixelFormat.isValid());\n> >>>>>>> + bufferCount = 1;\n> >>>>>>> break;\n> >>>>>>>\n> >>>>>>> case StreamRole::StillCapture:\n> >>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>\n> >>> This conflicts with recent rework of format handling, this line should\n> >>> now be\n> >>>\n> >>> + pixelFormat = formats::NV12;\n> >>>\n> >>> Same for the other formats below.\n> >>\n> >> Will update in the next patch.\n> >>\n> >>>>>>> /* Return the largest sensor resolution. */\n> >>>>>>> - cfg.size = data->sensor_->resolution();\n> >>>>>>> - cfg.bufferCount = 1;\n> >>>>>>> + size = data->sensor_->resolution();\n> >>>>>>> + bufferCount = 1;\n> >>>>>>> break;\n> >>>>>>>\n> >>>>>>> case StreamRole::VideoRecording:\n> >>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>>>> - cfg.size = { 1920, 1080 };\n> >>>>>>> - cfg.bufferCount = 4;\n> >>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>>>> + size = { 1920, 1080 };\n> >>>>>>> + bufferCount = 4;\n> >>>>>>> break;\n> >>>>>>>\n> >>>>>>> case StreamRole::Viewfinder:\n> >>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>>>>>> - cfg.size = { 800, 600 };\n> >>>>>>> - cfg.bufferCount = 4;\n> >>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>>>>>> + size = { 800, 600 };\n> >>>>>>> + bufferCount = 4;\n> >>>>>>> break;\n> >>>>>>>\n> >>>>>>> default:\n> >>>>>>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>>>>>> break;\n> >>>>>>> }\n> >>>>>>>\n> >>>>>>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> >>>>>>> + std::map> deviceFormats;\n> >>>>>>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> >>>>>>> + [&](const decltype(fmts)::value_type &format) {\n> >>>>>>> + return decltype(deviceFormats)::value_type{\n> >>>>>>> + format.first.toPixelFormat(),\n> >>>>>>> + format.second\n> >>>>>>> + };\n> >>>>>>> + });\n> >>>\n> >>> For the unicam device this looks fine to me, as the driver will\n> >>> give us formats and sizes that match the capabilities of the sensor. For\n> >>> the ISP, the list of supported pixel formats is queried from the\n> >>> firmware, filtered against the list of pixel formats supported by the\n> >>> driver, and all should be fine. However, for the frame sizes, the ISP\n> >>> driver reports hardcoded minimum and maximum values of 64 and 16384\n> >>> respectively. Shouldn't we instead, in the pipeline handler, take the\n> >>> capabilities of both the sensor and the ISP into account to create a\n> >>> range of sizes that would be closer to what can actually be achieved ?\n> >>\n> >> Just checking, so I should populate\n> >> StreamFormat::std::vector with the device ranges? What\n> >> about the StreamConfiguration::size field that I currently populate\n> >> with a single value, should I leave that blank/empty?\n> >\n> > StreamConfigure::size is the recommended size for the stream.\n> > StreamFormats is a map containing all the supported pixel formats, and\n> > for each of them, the supported sizes. The sizes are expressed as a\n> > vector of SizeRange, which must contain at least one entry.\n> >\n> > You populate StreamConfigure::size correctly as far as I can tell,\n> > there's no reason to change. The StreamFormats, however, should be\n> > populated with what the camera can produce, and that should be\n> > conditioned by the sizes supported by the sensor and the ISP\n> > capabilities. You could, for instance, take all the sizes supported by\n> > the sensor, and for each of them, create a SizeRange with the minimum\n> > set to the sensor size downscaled as much as possible by the ISP, and\n> > the maximum set to the sensor size.\n> \n> Apologies if this is a silly question, but since the ISP hardware\n> genuinely does allow frame sizes in the range [64,16384] in both\n> dimensions irrespective of the input size, is what I currently have\n> for populating SizeRange not correct?\n\nThen I have to apologize for considering the code wasn't correct :-) I\nassumed the scaling ratios were somehow limited.\n\n> >>> For UVC the situation is different, as the kernel directly reports the\n> >>> list of formats and sizes supported by the device.\n> >>>\n> >>>>>> Really took me a while to parse this, as I was not expecting to see\n> >>>>>> std::inserter() but just deviceFormats.begin() there, but then I\n> >>>>>> learned about inserter, and indeed if I remove std::inserter() I get\n> >>>>>> an error due to the fact the copy operator of std::pair is deleted.\n> >>>>>\n> >>>>> I think that's the same as the conversion routine in the UVC pipeline\n> >>>>> handler (which I think came from Laurent).\n> >>>>\n> >>>> Yes, I picked this from the uvc pipeline handler. Took me some goes\n> >>>> to understand it as well :)\n> >>>>\n> >>>>> Not for this patch (I think this can go in as is) but we should really\n> >>>>> make a helper for that conversion as it's likely to be used in multiple\n> >>>>> places, and it is quite hard to parse on it's own ;-(.\n> >>>>\n> >>>> That would make sense. All pipeline handlers will require this\n> >>>> conversion, I think!\n> >>>>\n> >>>>> However, I think that overlaps with changes that you (Jacopo) were\n> >>>>> working on with ImageFormats anyway.\n> >>>\n> >>> Yes, we know this API needs to be reworked, so I wouldn't spend time on\n> >>> creating a helper right now, but would rather land the ImageFormats\n> >>> series first, and then discuss how we should rework stream configuration\n> >>> handling. The fact that this code should consider both the sensor and\n> >>> the ISP, as explained above, also makes it more difficult to create a\n> >>> single helper function.\n> >>>\n> >>>>> Eitherway, this patch will help support more formats and enumeration on\n> >>>>> the RPi pipeline handler:\n> >>>>>\n> >>>>> Reviewed-by: Kieran Bingham \n> >>>>>\n> >>>>>>> +\n> >>>>>>> + /* Add the stream format based on the device node used for the use case. */\n> >>>>>>> + StreamFormats formats(deviceFormats);\n> >>>>>>> + StreamConfiguration cfg(formats);\n> >>>>>>> + cfg.size = size;\n> >>>>>>> + cfg.pixelFormat = pixelFormat;\n> >>>>>>> + cfg.bufferCount = bufferCount;\n> >>>>>>\n> >>>>>> Patch looks good to me\n> >>>>>>\n> >>>>>> Reviewed-by: Jacopo Mondi \n> >>>>>>\n> >>>>>>> config->addConfiguration(cfg);\n> >>>>>>> }\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 A3F04C0101\n\tfor ;\n\tThu, 25 Jun 2020 02:21:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D569609A5;\n\tThu, 25 Jun 2020 04:21:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0BF2603BB\n\tfor ;\n\tThu, 25 Jun 2020 04:21:27 +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 15F6172E;\n\tThu, 25 Jun 2020 04:21:27 +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=\"IudUCj5X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593051687;\n\tbh=Gmtr49VEW7JTHbBI79zpmhOxGvRoJal/bA7LvQiMEZE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IudUCj5XCe8GN8uaLgfApDUrtKX5HR6uOD4GbW7m/zIc3G8r/il0DfDFqpFTGAAnN\n\tlwW+R1NOYNHH6ecqNoiC9gwsIUj8jtrVwMN8j/onGD2Xm7iBlhDrCGuife70bs1qww\n\tiV+9OQeOoK3MdCjXDJlC49DfQGWmGbpMZOrgmHPM=","Date":"Thu, 25 Jun 2020 05:21:26 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200625022126.GA5980@pendragon.ideasonboard.com>","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>\n\t\n\t<20200622234617.GN5852@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":5391,"web_url":"https://patchwork.libcamera.org/comment/5391/","msgid":"<20200625023746.GA14860@pendragon.ideasonboard.com>","date":"2020-06-25T02:37:46","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Thu, Jun 25, 2020 at 05:21:26AM +0300, Laurent Pinchart wrote:\n> On Tue, Jun 23, 2020 at 08:59:49AM +0100, Naushir Patuck wrote:\n> > On Tue, 23 Jun 2020 at 00:46, Laurent Pinchart wrote:\n> >> On Mon, Jun 22, 2020 at 11:25:47AM +0100, Naushir Patuck wrote:\n> >>> On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart wrote:\n> >>>> On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> >>>>> On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> >>>>>> On 18/06/2020 09:41, Jacopo Mondi wrote:\n> >>>>>>> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> >>>>>>>> In generateConfiguration(), add the device node specific formats to the\n> >>>>>>>> StreamConfiguration for each StreamRole requested.\n> >>>>>>>>\n> >>>>>>>> Signed-off-by: Naushir Patuck \n> >>>>>>>> ---\n> >>>>>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> >>>>>>>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> >>>>>>>>\n> >>>>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>>>>> index e16a9c7f..03a1e641 100644\n> >>>>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>>>>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>>>>>>> RPiCameraData *data = cameraData(camera);\n> >>>>>>>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> >>>>>>>> V4L2DeviceFormat sensorFormat;\n> >>>>>>>> + unsigned int bufferCount;\n> >>>>>>>> + PixelFormat pixelFormat;\n> >>>>>>>> V4L2PixFmtMap fmts;\n> >>>>>>>> + Size size;\n> >>>>>>>>\n> >>>>>>>> if (roles.empty())\n> >>>>>>>> return config;\n> >>>>>>>>\n> >>>>>>>> for (const StreamRole role : roles) {\n> >>>>>>>> - StreamConfiguration cfg{};\n> >>>>>>>> -\n> >>>>>>>> switch (role) {\n> >>>>>>>> case StreamRole::StillCaptureRaw:\n> >>>>>>>> - cfg.size = data->sensor_->resolution();\n> >>>>>>>> + size = data->sensor_->resolution();\n> >>>>>>>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> >>>>>>>> - sensorFormat = findBestMode(fmts, cfg.size);\n> >>>>>>>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >>>>>>>> - ASSERT(cfg.pixelFormat.isValid());\n> >>>>>>>> - cfg.bufferCount = 1;\n> >>>>>>>> + sensorFormat = findBestMode(fmts, size);\n> >>>>>>>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> >>>>>>>> + ASSERT(pixelFormat.isValid());\n> >>>>>>>> + bufferCount = 1;\n> >>>>>>>> break;\n> >>>>>>>>\n> >>>>>>>> case StreamRole::StillCapture:\n> >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>\n> >>>> This conflicts with recent rework of format handling, this line should\n> >>>> now be\n> >>>>\n> >>>> + pixelFormat = formats::NV12;\n> >>>>\n> >>>> Same for the other formats below.\n> >>>\n> >>> Will update in the next patch.\n> >>>\n> >>>>>>>> /* Return the largest sensor resolution. */\n> >>>>>>>> - cfg.size = data->sensor_->resolution();\n> >>>>>>>> - cfg.bufferCount = 1;\n> >>>>>>>> + size = data->sensor_->resolution();\n> >>>>>>>> + bufferCount = 1;\n> >>>>>>>> break;\n> >>>>>>>>\n> >>>>>>>> case StreamRole::VideoRecording:\n> >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>>>>> - cfg.size = { 1920, 1080 };\n> >>>>>>>> - cfg.bufferCount = 4;\n> >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>>>>>>> + size = { 1920, 1080 };\n> >>>>>>>> + bufferCount = 4;\n> >>>>>>>> break;\n> >>>>>>>>\n> >>>>>>>> case StreamRole::Viewfinder:\n> >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>>>>>>> - cfg.size = { 800, 600 };\n> >>>>>>>> - cfg.bufferCount = 4;\n> >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> >>>>>>>> + size = { 800, 600 };\n> >>>>>>>> + bufferCount = 4;\n> >>>>>>>> break;\n> >>>>>>>>\n> >>>>>>>> default:\n> >>>>>>>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >>>>>>>> break;\n> >>>>>>>> }\n> >>>>>>>>\n> >>>>>>>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> >>>>>>>> + std::map> deviceFormats;\n> >>>>>>>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n\nOne additional comment, should this be deviceFormats.end(), to insert\nthe new entries at the end of the map ? It will make a difference in\nefficiency *if* new entries gets inserted right before the iterator.\nThere's no such guarantee, as fmts is sorted by V4L2PixelFormat, and we\nconvert that to a PixelFormat, but I think there's a bigger chance that\ninsertion would happen at the end than at the beginning. If this\nanalysis is correct, I'll fix the UVC pipeline handler accordingly.\n\nI can address both the formats:: conflict and this issue when applying,\nbut maybe it would be best if you could test a v2, in which case you\ncould as well send it out :-)\n\n> >>>>>>>> + [&](const decltype(fmts)::value_type &format) {\n> >>>>>>>> + return decltype(deviceFormats)::value_type{\n> >>>>>>>> + format.first.toPixelFormat(),\n> >>>>>>>> + format.second\n> >>>>>>>> + };\n> >>>>>>>> + });\n> >>>>\n> >>>> For the unicam device this looks fine to me, as the driver will\n> >>>> give us formats and sizes that match the capabilities of the sensor. For\n> >>>> the ISP, the list of supported pixel formats is queried from the\n> >>>> firmware, filtered against the list of pixel formats supported by the\n> >>>> driver, and all should be fine. However, for the frame sizes, the ISP\n> >>>> driver reports hardcoded minimum and maximum values of 64 and 16384\n> >>>> respectively. Shouldn't we instead, in the pipeline handler, take the\n> >>>> capabilities of both the sensor and the ISP into account to create a\n> >>>> range of sizes that would be closer to what can actually be achieved ?\n> >>>\n> >>> Just checking, so I should populate\n> >>> StreamFormat::std::vector with the device ranges? What\n> >>> about the StreamConfiguration::size field that I currently populate\n> >>> with a single value, should I leave that blank/empty?\n> >>\n> >> StreamConfigure::size is the recommended size for the stream.\n> >> StreamFormats is a map containing all the supported pixel formats, and\n> >> for each of them, the supported sizes. The sizes are expressed as a\n> >> vector of SizeRange, which must contain at least one entry.\n> >>\n> >> You populate StreamConfigure::size correctly as far as I can tell,\n> >> there's no reason to change. The StreamFormats, however, should be\n> >> populated with what the camera can produce, and that should be\n> >> conditioned by the sizes supported by the sensor and the ISP\n> >> capabilities. You could, for instance, take all the sizes supported by\n> >> the sensor, and for each of them, create a SizeRange with the minimum\n> >> set to the sensor size downscaled as much as possible by the ISP, and\n> >> the maximum set to the sensor size.\n> > \n> > Apologies if this is a silly question, but since the ISP hardware\n> > genuinely does allow frame sizes in the range [64,16384] in both\n> > dimensions irrespective of the input size, is what I currently have\n> > for populating SizeRange not correct?\n> \n> Then I have to apologize for considering the code wasn't correct :-) I\n> assumed the scaling ratios were somehow limited.\n> \n> >>>> For UVC the situation is different, as the kernel directly reports the\n> >>>> list of formats and sizes supported by the device.\n> >>>>\n> >>>>>>> Really took me a while to parse this, as I was not expecting to see\n> >>>>>>> std::inserter() but just deviceFormats.begin() there, but then I\n> >>>>>>> learned about inserter, and indeed if I remove std::inserter() I get\n> >>>>>>> an error due to the fact the copy operator of std::pair is deleted.\n> >>>>>>\n> >>>>>> I think that's the same as the conversion routine in the UVC pipeline\n> >>>>>> handler (which I think came from Laurent).\n> >>>>>\n> >>>>> Yes, I picked this from the uvc pipeline handler. Took me some goes\n> >>>>> to understand it as well :)\n> >>>>>\n> >>>>>> Not for this patch (I think this can go in as is) but we should really\n> >>>>>> make a helper for that conversion as it's likely to be used in multiple\n> >>>>>> places, and it is quite hard to parse on it's own ;-(.\n> >>>>>\n> >>>>> That would make sense. All pipeline handlers will require this\n> >>>>> conversion, I think!\n> >>>>>\n> >>>>>> However, I think that overlaps with changes that you (Jacopo) were\n> >>>>>> working on with ImageFormats anyway.\n> >>>>\n> >>>> Yes, we know this API needs to be reworked, so I wouldn't spend time on\n> >>>> creating a helper right now, but would rather land the ImageFormats\n> >>>> series first, and then discuss how we should rework stream configuration\n> >>>> handling. The fact that this code should consider both the sensor and\n> >>>> the ISP, as explained above, also makes it more difficult to create a\n> >>>> single helper function.\n> >>>>\n> >>>>>> Eitherway, this patch will help support more formats and enumeration on\n> >>>>>> the RPi pipeline handler:\n> >>>>>>\n> >>>>>> Reviewed-by: Kieran Bingham \n> >>>>>>\n> >>>>>>>> +\n> >>>>>>>> + /* Add the stream format based on the device node used for the use case. */\n> >>>>>>>> + StreamFormats formats(deviceFormats);\n> >>>>>>>> + StreamConfiguration cfg(formats);\n> >>>>>>>> + cfg.size = size;\n> >>>>>>>> + cfg.pixelFormat = pixelFormat;\n> >>>>>>>> + cfg.bufferCount = bufferCount;\n> >>>>>>>\n> >>>>>>> Patch looks good to me\n> >>>>>>>\n> >>>>>>> Reviewed-by: Jacopo Mondi \n> >>>>>>>\n> >>>>>>>> config->addConfiguration(cfg);\n> >>>>>>>> }\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 8967EC0100\n\tfor ;\n\tThu, 25 Jun 2020 02:37:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 00850609B3;\n\tThu, 25 Jun 2020 04:37:48 +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 9992F603BB\n\tfor ;\n\tThu, 25 Jun 2020 04:37:47 +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 059BA521;\n\tThu, 25 Jun 2020 04:37:46 +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=\"JS1p22H8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593052667;\n\tbh=ZB52w2mbhtA0lIsZmxpyxhlJSrZMFt2vvkykyMssT7Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JS1p22H8iaoeR3WoP5FQgDvt+viT6yMOKLfSMn7Fjlr+2HlnbsWCx1GxyOvEIeFjT\n\tTZhObVX9XaKal6KLcp/VGocyTeUrL9yThKBq2WpZvHZecc7hlkwr4Z5lG1DGCbxkux\n\tkcvyh8Ga0V9igqYQZtjuI0NXx8tATLsVfNiM59oc=","Date":"Thu, 25 Jun 2020 05:37:46 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200625023746.GA14860@pendragon.ideasonboard.com>","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>\n\t\n\t<20200622234617.GN5852@pendragon.ideasonboard.com>\n\t\n\t<20200625022126.GA5980@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200625022126.GA5980@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":5403,"web_url":"https://patchwork.libcamera.org/comment/5403/","msgid":"","date":"2020-06-25T07:30:55","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\n\nOn Thu, 25 Jun 2020 at 03:37, Laurent Pinchart\n wrote:\n>\n\n\n\n> > >>>>>>>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> > >>>>>>>> + std::map> deviceFormats;\n> > >>>>>>>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n>\n> One additional comment, should this be deviceFormats.end(), to insert\n> the new entries at the end of the map ? It will make a difference in\n> efficiency *if* new entries gets inserted right before the iterator.\n> There's no such guarantee, as fmts is sorted by V4L2PixelFormat, and we\n> convert that to a PixelFormat, but I think there's a bigger chance that\n> insertion would happen at the end than at the beginning. If this\n> analysis is correct, I'll fix the UVC pipeline handler accordingly.\n\nYes, that seems reasonable.\n\n>\n> I can address both the formats:: conflict and this issue when applying,\n> but maybe it would be best if you could test a v2, in which case you\n> could as well send it out :-)\n>\n\nI'll push version 2 of this patch shortly.\n\nRegards,\nNaush","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 1E8D4C0101\n\tfor ;\n\tThu, 25 Jun 2020 07:31:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93A17609C4;\n\tThu, 25 Jun 2020 09:31:14 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D60B609A3\n\tfor ;\n\tThu, 25 Jun 2020 09:31:12 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id q19so5380821lji.2\n\tfor ;\n\tThu, 25 Jun 2020 00:31:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"TuuaWDqX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=2nrNpXIPwDlMyFetWRnx2ibI+B8G/xwkXx3yNMrXI4Q=;\n\tb=TuuaWDqXTIOPZDQb6oTU5DzJEcrBTMZmYuWsj2RJxhsZ3zLQQ1LBw9vmyOALVmT40Q\n\t8jDpC1rB/kNTf4RCeSwt54Lt264wQ6CrjxUIfPhE7YvFj6PCmrYMlY9JWunvZ4J1IXYO\n\tkZfuJPwhJIdbV3Pxeg1GKuN/JdwnRO27klPXI8TtwGp830sV1OoQAXdXsecbejFWuzIU\n\t+8dR+6GtCSVZcvZkTdvYffF7d+gCGYw+SaOUPRi8g1BfmHYXkT0C4kKvPV2el1kQNQsw\n\t+hRTlIu5/8ifFEVkQY2bVO9Ha+61f4o9+pRNPl1GeGKBxVYkh6BaXK31u9ExlgLfqCSb\n\tsWog==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=2nrNpXIPwDlMyFetWRnx2ibI+B8G/xwkXx3yNMrXI4Q=;\n\tb=cTtYu/D7irNvrbUvaI4tGT2NHOsKYU9t2v93XHmOpONaeKhyMdvoj18pHLuhx3ZM4i\n\tnzL4enZJ2Em3FK+T1FA206gvke9dy7Y/M9SVFkmuDraNXaZjA/QnaHWwF0RfluDy73c/\n\tiKl63eKt4NXL+ITDg79ksXqs9cuPklzdQWfdpD0HknbKkBT4yP9td+dBhCfjf8+XI6AX\n\tRf40qLGlwoHyGafn6UfUnm1jqhi9T4Ye6Tyh6kVn0RWrNFlPsmxQpg47JcFOdqDAzPvs\n\tI986b/lJJgK0XMi1DhDLnx3+3k0T0Mblds5rphCKc2kI0A/ruOLU9Fv6G0G6qGlq1RKS\n\tciJQ==","X-Gm-Message-State":"AOAM532YRO/cVQBuzYi6toRCVYzMVS2XxPB7mnUxH4JlR07uhkoLYxPR\n\tFAPM+a+H8sBIFva9Hz0NVkSUYb+JcYsj+Ck+jVw3TNBaYYQ=","X-Google-Smtp-Source":"ABdhPJzg2B7cC/5J2GoYR9qBbUXB6qtUmNIbN1Pq+exUNwwdNmglMk4B0ZklzLdq9/qyXxQO8zN6uWaOF1X7D+8zXYo=","X-Received":"by 2002:a2e:3a04:: with SMTP id\n\th4mr16401959lja.103.1593070271409; \n\tThu, 25 Jun 2020 00:31:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>\n\t\n\t<20200622234617.GN5852@pendragon.ideasonboard.com>\n\t\n\t<20200625022126.GA5980@pendragon.ideasonboard.com>\n\t<20200625023746.GA14860@pendragon.ideasonboard.com>","In-Reply-To":"<20200625023746.GA14860@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Thu, 25 Jun 2020 08:30:55 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":5405,"web_url":"https://patchwork.libcamera.org/comment/5405/","msgid":"<20200625073900.s6usof7jvfmvflm5@uno.localdomain>","date":"2020-06-25T07:39:00","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Naush\n\nOn Thu, Jun 25, 2020 at 05:37:46AM +0300, Laurent Pinchart wrote:\n> Hi Naush,\n>\n> On Thu, Jun 25, 2020 at 05:21:26AM +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 23, 2020 at 08:59:49AM +0100, Naushir Patuck wrote:\n> > > On Tue, 23 Jun 2020 at 00:46, Laurent Pinchart wrote:\n> > >> On Mon, Jun 22, 2020 at 11:25:47AM +0100, Naushir Patuck wrote:\n> > >>> On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart wrote:\n> > >>>> On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> > >>>>> On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> > >>>>>> On 18/06/2020 09:41, Jacopo Mondi wrote:\n> > >>>>>>> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> > >>>>>>>> In generateConfiguration(), add the device node specific formats to the\n> > >>>>>>>> StreamConfiguration for each StreamRole requested.\n> > >>>>>>>>\n> > >>>>>>>> Signed-off-by: Naushir Patuck \n> > >>>>>>>> ---\n> > >>>>>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> > >>>>>>>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> > >>>>>>>>\n> > >>>>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>>>>>>> index e16a9c7f..03a1e641 100644\n> > >>>>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>>>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>>>>>>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > >>>>>>>> RPiCameraData *data = cameraData(camera);\n> > >>>>>>>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > >>>>>>>> V4L2DeviceFormat sensorFormat;\n> > >>>>>>>> + unsigned int bufferCount;\n> > >>>>>>>> + PixelFormat pixelFormat;\n> > >>>>>>>> V4L2PixFmtMap fmts;\n> > >>>>>>>> + Size size;\n> > >>>>>>>>\n> > >>>>>>>> if (roles.empty())\n> > >>>>>>>> return config;\n> > >>>>>>>>\n> > >>>>>>>> for (const StreamRole role : roles) {\n> > >>>>>>>> - StreamConfiguration cfg{};\n> > >>>>>>>> -\n> > >>>>>>>> switch (role) {\n> > >>>>>>>> case StreamRole::StillCaptureRaw:\n> > >>>>>>>> - cfg.size = data->sensor_->resolution();\n> > >>>>>>>> + size = data->sensor_->resolution();\n> > >>>>>>>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > >>>>>>>> - sensorFormat = findBestMode(fmts, cfg.size);\n> > >>>>>>>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > >>>>>>>> - ASSERT(cfg.pixelFormat.isValid());\n> > >>>>>>>> - cfg.bufferCount = 1;\n> > >>>>>>>> + sensorFormat = findBestMode(fmts, size);\n> > >>>>>>>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > >>>>>>>> + ASSERT(pixelFormat.isValid());\n> > >>>>>>>> + bufferCount = 1;\n> > >>>>>>>> break;\n> > >>>>>>>>\n> > >>>>>>>> case StreamRole::StillCapture:\n> > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>>>\n> > >>>> This conflicts with recent rework of format handling, this line should\n> > >>>> now be\n> > >>>>\n> > >>>> + pixelFormat = formats::NV12;\n> > >>>>\n> > >>>> Same for the other formats below.\n> > >>>\n> > >>> Will update in the next patch.\n> > >>>\n> > >>>>>>>> /* Return the largest sensor resolution. */\n> > >>>>>>>> - cfg.size = data->sensor_->resolution();\n> > >>>>>>>> - cfg.bufferCount = 1;\n> > >>>>>>>> + size = data->sensor_->resolution();\n> > >>>>>>>> + bufferCount = 1;\n> > >>>>>>>> break;\n> > >>>>>>>>\n> > >>>>>>>> case StreamRole::VideoRecording:\n> > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>>>>>>> - cfg.size = { 1920, 1080 };\n> > >>>>>>>> - cfg.bufferCount = 4;\n> > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > >>>>>>>> + size = { 1920, 1080 };\n> > >>>>>>>> + bufferCount = 4;\n> > >>>>>>>> break;\n> > >>>>>>>>\n> > >>>>>>>> case StreamRole::Viewfinder:\n> > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > >>>>>>>> - cfg.size = { 800, 600 };\n> > >>>>>>>> - cfg.bufferCount = 4;\n> > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > >>>>>>>> + size = { 800, 600 };\n> > >>>>>>>> + bufferCount = 4;\n> > >>>>>>>> break;\n> > >>>>>>>>\n> > >>>>>>>> default:\n> > >>>>>>>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > >>>>>>>> break;\n> > >>>>>>>> }\n> > >>>>>>>>\n> > >>>>>>>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> > >>>>>>>> + std::map> deviceFormats;\n> > >>>>>>>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n>\n> One additional comment, should this be deviceFormats.end(), to insert\n> the new entries at the end of the map ? It will make a difference in\n\nShouldn't this be handled by using back_inserter() if that's what we\nwant ?","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 36077C0101\n\tfor ;\n\tThu, 25 Jun 2020 07:35:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11718609B3;\n\tThu, 25 Jun 2020 09:35:34 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17076609A3\n\tfor ;\n\tThu, 25 Jun 2020 09:35:33 +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 relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 613C320018;\n\tThu, 25 Jun 2020 07:35:32 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 25 Jun 2020 09:39:00 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200625073900.s6usof7jvfmvflm5@uno.localdomain>","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>\n\t\n\t<20200622234617.GN5852@pendragon.ideasonboard.com>\n\t\n\t<20200625022126.GA5980@pendragon.ideasonboard.com>\n\t<20200625023746.GA14860@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200625023746.GA14860@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":5408,"web_url":"https://patchwork.libcamera.org/comment/5408/","msgid":"","date":"2020-06-25T07:47:36","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nOn Thu, 25 Jun 2020 at 08:35, Jacopo Mondi wrote:\n>\n> Hi Laurent, Naush\n>\n> On Thu, Jun 25, 2020 at 05:37:46AM +0300, Laurent Pinchart wrote:\n> > Hi Naush,\n> >\n> > On Thu, Jun 25, 2020 at 05:21:26AM +0300, Laurent Pinchart wrote:\n> > > On Tue, Jun 23, 2020 at 08:59:49AM +0100, Naushir Patuck wrote:\n> > > > On Tue, 23 Jun 2020 at 00:46, Laurent Pinchart wrote:\n> > > >> On Mon, Jun 22, 2020 at 11:25:47AM +0100, Naushir Patuck wrote:\n> > > >>> On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart wrote:\n> > > >>>> On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> > > >>>>> On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> > > >>>>>> On 18/06/2020 09:41, Jacopo Mondi wrote:\n> > > >>>>>>> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> > > >>>>>>>> In generateConfiguration(), add the device node specific formats to the\n> > > >>>>>>>> StreamConfiguration for each StreamRole requested.\n> > > >>>>>>>>\n> > > >>>>>>>> Signed-off-by: Naushir Patuck \n> > > >>>>>>>> ---\n> > > >>>>>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> > > >>>>>>>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> > > >>>>>>>>\n> > > >>>>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >>>>>>>> index e16a9c7f..03a1e641 100644\n> > > >>>>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >>>>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >>>>>>>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > >>>>>>>> RPiCameraData *data = cameraData(camera);\n> > > >>>>>>>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > > >>>>>>>> V4L2DeviceFormat sensorFormat;\n> > > >>>>>>>> + unsigned int bufferCount;\n> > > >>>>>>>> + PixelFormat pixelFormat;\n> > > >>>>>>>> V4L2PixFmtMap fmts;\n> > > >>>>>>>> + Size size;\n> > > >>>>>>>>\n> > > >>>>>>>> if (roles.empty())\n> > > >>>>>>>> return config;\n> > > >>>>>>>>\n> > > >>>>>>>> for (const StreamRole role : roles) {\n> > > >>>>>>>> - StreamConfiguration cfg{};\n> > > >>>>>>>> -\n> > > >>>>>>>> switch (role) {\n> > > >>>>>>>> case StreamRole::StillCaptureRaw:\n> > > >>>>>>>> - cfg.size = data->sensor_->resolution();\n> > > >>>>>>>> + size = data->sensor_->resolution();\n> > > >>>>>>>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > > >>>>>>>> - sensorFormat = findBestMode(fmts, cfg.size);\n> > > >>>>>>>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > > >>>>>>>> - ASSERT(cfg.pixelFormat.isValid());\n> > > >>>>>>>> - cfg.bufferCount = 1;\n> > > >>>>>>>> + sensorFormat = findBestMode(fmts, size);\n> > > >>>>>>>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > > >>>>>>>> + ASSERT(pixelFormat.isValid());\n> > > >>>>>>>> + bufferCount = 1;\n> > > >>>>>>>> break;\n> > > >>>>>>>>\n> > > >>>>>>>> case StreamRole::StillCapture:\n> > > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > >>>>\n> > > >>>> This conflicts with recent rework of format handling, this line should\n> > > >>>> now be\n> > > >>>>\n> > > >>>> + pixelFormat = formats::NV12;\n> > > >>>>\n> > > >>>> Same for the other formats below.\n> > > >>>\n> > > >>> Will update in the next patch.\n> > > >>>\n> > > >>>>>>>> /* Return the largest sensor resolution. */\n> > > >>>>>>>> - cfg.size = data->sensor_->resolution();\n> > > >>>>>>>> - cfg.bufferCount = 1;\n> > > >>>>>>>> + size = data->sensor_->resolution();\n> > > >>>>>>>> + bufferCount = 1;\n> > > >>>>>>>> break;\n> > > >>>>>>>>\n> > > >>>>>>>> case StreamRole::VideoRecording:\n> > > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > >>>>>>>> - cfg.size = { 1920, 1080 };\n> > > >>>>>>>> - cfg.bufferCount = 4;\n> > > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > >>>>>>>> + size = { 1920, 1080 };\n> > > >>>>>>>> + bufferCount = 4;\n> > > >>>>>>>> break;\n> > > >>>>>>>>\n> > > >>>>>>>> case StreamRole::Viewfinder:\n> > > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > > >>>>>>>> - cfg.size = { 800, 600 };\n> > > >>>>>>>> - cfg.bufferCount = 4;\n> > > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > > >>>>>>>> + size = { 800, 600 };\n> > > >>>>>>>> + bufferCount = 4;\n> > > >>>>>>>> break;\n> > > >>>>>>>>\n> > > >>>>>>>> default:\n> > > >>>>>>>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > >>>>>>>> break;\n> > > >>>>>>>> }\n> > > >>>>>>>>\n> > > >>>>>>>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> > > >>>>>>>> + std::map> deviceFormats;\n> > > >>>>>>>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> >\n> > One additional comment, should this be deviceFormats.end(), to insert\n> > the new entries at the end of the map ? It will make a difference in\n>\n> Shouldn't this be handled by using back_inserter() if that's what we\n> want ?\n\nFrom my understanding, yes, this is probably cleaner. However, they\nshould be both equivalent?\n\nRegards,\nNaush","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 57575C0101\n\tfor ;\n\tThu, 25 Jun 2020 07:47:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C86F5609C3;\n\tThu, 25 Jun 2020 09:47:55 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9F73603BE\n\tfor ;\n\tThu, 25 Jun 2020 09:47:53 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id e4so5433183ljn.4\n\tfor ;\n\tThu, 25 Jun 2020 00:47:53 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"jEs7X6cv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=UTaOhjKqc9DX4jaL+8jazHswriIuD7X1QSGmBfuENqU=;\n\tb=jEs7X6cvYlq/+UTRmT22oLeEK6LYCBFknIMg02+paSc8U5wYf2NiD46xHB0PDq4NJp\n\t8wK1g7uW2VSQPIp7Xc97+ifxWwR8fc55ezQIPulC2TdphnSiyThgxAB8//SU6hUH952g\n\tjtaqfIZAr+XmcrhZdus8FDwlUEOgkV5cJLrjEnweOn2QIG0xC7Inqr79mSGz1Qrr3rtV\n\t/JUmS/XgFuDLUmfHTjB9D466TZCPCRbZErOCYmLfN/svWgB5JSar+InVpB8Ptv+glPI6\n\tzJ2G0Tjt7Qpc02oFmyj4yhkN7lsnhfpneu0o0LpwAR9te0U8Qdkj9qHb8N3voPTbFWWH\n\td6nQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=UTaOhjKqc9DX4jaL+8jazHswriIuD7X1QSGmBfuENqU=;\n\tb=Ul254fr5v2Pr1oVxcTMruVwExEHJKtZxt60OugBD6EOlo7UGNQrquppZ2Y6iEc75tb\n\t1EHPWys7CU76YF+BfomSCcnZdcIMMClKClzAIYd0K3ftO3gRlBT3m4txGndlDXYe0Mlm\n\tQoUKUkD/cIS9XGSppJ80UIlolXPosbKnCNgD4jEsSsFkN39CJOi9ni6yCRQOZS26gPEI\n\t5o7ubo0BCAKQezjjFbGWBWXk8/o9VUgbibjTJnsESqJL15Nm0lVYYCGsdoAEnHIBzoaf\n\tQ2ZqoegpBPPeAJa/258R97t1z9ScqsuNLrKlZCcM/+o0xUWHQVJgeYEPVA262DEyJ1l+\n\tWthw==","X-Gm-Message-State":"AOAM531+xVFNm84WX8nU8UB9gUKtn4iNyzH8SNoRGWw7kdoRIQBFD4k3\n\tlwq7sFlb5ATgh5lvEP7TDqhhA4HN2Gek/EcIXukLXrbN","X-Google-Smtp-Source":"ABdhPJyfxUT4RdXjQStj8r++Fw3fCTolLTjzgMCeLgKZEpZcOdkrsMn+qRwmtX/0TUmt3F/vmYgpw9o6QsI8N2gFp80=","X-Received":"by 2002:a05:651c:508:: with SMTP id\n\to8mr15767214ljp.112.1593071273113; \n\tThu, 25 Jun 2020 00:47:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20200610142640.576529-1-naush@raspberrypi.com>\n\t<20200618084144.ibfuie3xfkmpk7b2@uno.localdomain>\n\t\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>\n\t\n\t<20200622234617.GN5852@pendragon.ideasonboard.com>\n\t\n\t<20200625022126.GA5980@pendragon.ideasonboard.com>\n\t<20200625023746.GA14860@pendragon.ideasonboard.com>\n\t<20200625073900.s6usof7jvfmvflm5@uno.localdomain>","In-Reply-To":"<20200625073900.s6usof7jvfmvflm5@uno.localdomain>","From":"Naushir Patuck ","Date":"Thu, 25 Jun 2020 08:47:36 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":5410,"web_url":"https://patchwork.libcamera.org/comment/5410/","msgid":"<20200625080334.bvulp3yf6tqmautm@uno.localdomain>","date":"2020-06-25T08:03:34","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nasuh,\n\nOn Thu, Jun 25, 2020 at 08:47:36AM +0100, Naushir Patuck wrote:\n> Hi all,\n>\n> On Thu, 25 Jun 2020 at 08:35, Jacopo Mondi wrote:\n> >\n> > Hi Laurent, Naush\n> >\n> > On Thu, Jun 25, 2020 at 05:37:46AM +0300, Laurent Pinchart wrote:\n> > > Hi Naush,\n> > >\n> > > On Thu, Jun 25, 2020 at 05:21:26AM +0300, Laurent Pinchart wrote:\n> > > > On Tue, Jun 23, 2020 at 08:59:49AM +0100, Naushir Patuck wrote:\n> > > > > On Tue, 23 Jun 2020 at 00:46, Laurent Pinchart wrote:\n> > > > >> On Mon, Jun 22, 2020 at 11:25:47AM +0100, Naushir Patuck wrote:\n> > > > >>> On Mon, 22 Jun 2020 at 05:59, Laurent Pinchart wrote:\n> > > > >>>> On Thu, Jun 18, 2020 at 10:18:38AM +0100, Naushir Patuck wrote:\n> > > > >>>>> On Thu, 18 Jun 2020 at 10:03, Kieran Bingham wrote:\n> > > > >>>>>> On 18/06/2020 09:41, Jacopo Mondi wrote:\n> > > > >>>>>>> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:\n> > > > >>>>>>>> In generateConfiguration(), add the device node specific formats to the\n> > > > >>>>>>>> StreamConfiguration for each StreamRole requested.\n> > > > >>>>>>>>\n> > > > >>>>>>>> Signed-off-by: Naushir Patuck \n> > > > >>>>>>>> ---\n> > > > >>>>>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 52 +++++++++++++------\n> > > > >>>>>>>> 1 file changed, 36 insertions(+), 16 deletions(-)\n> > > > >>>>>>>>\n> > > > >>>>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >>>>>>>> index e16a9c7f..03a1e641 100644\n> > > > >>>>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >>>>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >>>>>>>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > > >>>>>>>> RPiCameraData *data = cameraData(camera);\n> > > > >>>>>>>> CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > > > >>>>>>>> V4L2DeviceFormat sensorFormat;\n> > > > >>>>>>>> + unsigned int bufferCount;\n> > > > >>>>>>>> + PixelFormat pixelFormat;\n> > > > >>>>>>>> V4L2PixFmtMap fmts;\n> > > > >>>>>>>> + Size size;\n> > > > >>>>>>>>\n> > > > >>>>>>>> if (roles.empty())\n> > > > >>>>>>>> return config;\n> > > > >>>>>>>>\n> > > > >>>>>>>> for (const StreamRole role : roles) {\n> > > > >>>>>>>> - StreamConfiguration cfg{};\n> > > > >>>>>>>> -\n> > > > >>>>>>>> switch (role) {\n> > > > >>>>>>>> case StreamRole::StillCaptureRaw:\n> > > > >>>>>>>> - cfg.size = data->sensor_->resolution();\n> > > > >>>>>>>> + size = data->sensor_->resolution();\n> > > > >>>>>>>> fmts = data->unicam_[Unicam::Image].dev()->formats();\n> > > > >>>>>>>> - sensorFormat = findBestMode(fmts, cfg.size);\n> > > > >>>>>>>> - cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > > > >>>>>>>> - ASSERT(cfg.pixelFormat.isValid());\n> > > > >>>>>>>> - cfg.bufferCount = 1;\n> > > > >>>>>>>> + sensorFormat = findBestMode(fmts, size);\n> > > > >>>>>>>> + pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > > > >>>>>>>> + ASSERT(pixelFormat.isValid());\n> > > > >>>>>>>> + bufferCount = 1;\n> > > > >>>>>>>> break;\n> > > > >>>>>>>>\n> > > > >>>>>>>> case StreamRole::StillCapture:\n> > > > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > > > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > > >>>>\n> > > > >>>> This conflicts with recent rework of format handling, this line should\n> > > > >>>> now be\n> > > > >>>>\n> > > > >>>> + pixelFormat = formats::NV12;\n> > > > >>>>\n> > > > >>>> Same for the other formats below.\n> > > > >>>\n> > > > >>> Will update in the next patch.\n> > > > >>>\n> > > > >>>>>>>> /* Return the largest sensor resolution. */\n> > > > >>>>>>>> - cfg.size = data->sensor_->resolution();\n> > > > >>>>>>>> - cfg.bufferCount = 1;\n> > > > >>>>>>>> + size = data->sensor_->resolution();\n> > > > >>>>>>>> + bufferCount = 1;\n> > > > >>>>>>>> break;\n> > > > >>>>>>>>\n> > > > >>>>>>>> case StreamRole::VideoRecording:\n> > > > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > > >>>>>>>> - cfg.size = { 1920, 1080 };\n> > > > >>>>>>>> - cfg.bufferCount = 4;\n> > > > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > > > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > > > >>>>>>>> + size = { 1920, 1080 };\n> > > > >>>>>>>> + bufferCount = 4;\n> > > > >>>>>>>> break;\n> > > > >>>>>>>>\n> > > > >>>>>>>> case StreamRole::Viewfinder:\n> > > > >>>>>>>> - cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > > > >>>>>>>> - cfg.size = { 800, 600 };\n> > > > >>>>>>>> - cfg.bufferCount = 4;\n> > > > >>>>>>>> + fmts = data->isp_[Isp::Output0].dev()->formats();\n> > > > >>>>>>>> + pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);\n> > > > >>>>>>>> + size = { 800, 600 };\n> > > > >>>>>>>> + bufferCount = 4;\n> > > > >>>>>>>> break;\n> > > > >>>>>>>>\n> > > > >>>>>>>> default:\n> > > > >>>>>>>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > > >>>>>>>> break;\n> > > > >>>>>>>> }\n> > > > >>>>>>>>\n> > > > >>>>>>>> + /* Translate the V4L2PixelFormat to PixelFormat. */\n> > > > >>>>>>>> + std::map> deviceFormats;\n> > > > >>>>>>>> + std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),\n> > >\n> > > One additional comment, should this be deviceFormats.end(), to insert\n> > > the new entries at the end of the map ? It will make a difference in\n> >\n> > Shouldn't this be handled by using back_inserter() if that's what we\n> > want ?\n>\n> From my understanding, yes, this is probably cleaner. However, they\n> should be both equivalent?\n\nAh wait, possibly not, I overlooked the type of deviceFormats.\nback_inserter() creates an std::back_insert_iterator which calls the\ncontainer's push_back() operation which we don't have for maps. I\nthink std::inserter() with deviceFormats.end() is the way to go.\n\nSorry for the noise.\n\n>\n> Regards,\n> Naush","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 38590C0101\n\tfor ;\n\tThu, 25 Jun 2020 08:00:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D7F6609A9;\n\tThu, 25 Jun 2020 10:00:12 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E43E603BE\n\tfor ;\n\tThu, 25 Jun 2020 10:00:10 +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 relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 1F0A62001A;\n\tThu, 25 Jun 2020 08:00:06 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 25 Jun 2020 10:03:34 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20200625080334.bvulp3yf6tqmautm@uno.localdomain>","References":"\n\t\n\t<20200622045840.GN25355@pendragon.ideasonboard.com>\n\t\n\t<20200622234617.GN5852@pendragon.ideasonboard.com>\n\t\n\t<20200625022126.GA5980@pendragon.ideasonboard.com>\n\t<20200625023746.GA14860@pendragon.ideasonboard.com>\n\t<20200625073900.s6usof7jvfmvflm5@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add\n\tStreamFormats to StreamConfiguration","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]