[{"id":28935,"web_url":"https://patchwork.libcamera.org/comment/28935/","msgid":"<171026001996.3708697.170277967630913816@ping.linuxembedded.co.uk>","date":"2024-03-12T16:13:39","subject":"Re: [PATCH v2 1/1] apps: cam: Add support for pnm output format","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Milan,\n\nQuoting Milan Zamazal (2024-03-12 15:41:46)\n> When file output is requested from cam app, it simply dumps the processed data\n> and it must be converted to a readable image format manually. Let's add support\n> for pnm output file format to make files produced by cam directly readable by\n> image display and processing software.\n> \n> For now, only BGR888 output format, which is the simplest one to use, is\n> supported but nothing prevents adding support for other output formats if\n> needed.\n\nWhat other formats are possible to add directly with PNM? Do we have to\ndo format conversions or can we write them directly?\n\nhttps://www.fileformat.info/format/pbm/egff.htm looks like most other\nformats will need converting to more or less RGB888.\n\nI wish there was a way to 'standardise' outputting a header such as the\nPPM form, but with a FOURCC to describe the data so we can output our\nraw buffers but with a header ...\n\nI wonder if we could sabotage/extend PNM/PPM in the future as a way of\nstoring V4L2/DRM formats : \n\n (new format) (fourcc)\n P7 pRAA\n\nOr at that point maybe we call it our own custom fileformat anyway ;-)\n\n\n\n> Signed-off-by: Milan Zamazal \n> ---\n> src/apps/cam/file_sink.cpp | 11 +++++++\n> src/apps/cam/main.cpp | 2 ++\n> src/apps/common/meson.build | 1 +\n> src/apps/common/pnm_writer.cpp | 54 ++++++++++++++++++++++++++++++++++\n> src/apps/common/pnm_writer.h | 18 ++++++++++++\n> 5 files changed, 86 insertions(+)\n> create mode 100644 src/apps/common/pnm_writer.cpp\n> create mode 100644 src/apps/common/pnm_writer.h\n> \n> diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp\n> index dca350c4..3db59925 100644\n> --- a/src/apps/cam/file_sink.cpp\n> +++ b/src/apps/cam/file_sink.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> \n> #include \"../common/dng_writer.h\"\n> +#include \"../common/pnm_writer.h\"\n> #include \"../common/image.h\"\n> \n> #include \"file_sink.h\"\n> @@ -73,6 +74,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,\n> if (!pattern_.empty())\n> filename = pattern_;\n> \n> + bool pnm = filename.find(\".pnm\", filename.size() - 4) != std::string::npos;\n> #ifdef HAVE_TIFF\n> bool dng = filename.find(\".dng\", filename.size() - 4) != std::string::npos;\n> #endif /* HAVE_TIFF */\n> @@ -90,6 +92,15 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,\n> \n> Image *image = mappedBuffers_[buffer].get();\n> \n> + if (pnm) {\n> + ret = PNMWriter::write(filename.c_str(), stream->configuration(),\n> + image->data(0).data());\n> + if (ret < 0)\n> + std::cerr << \"failed to write PNM file `\" << filename\n> + << \"'\" << std::endl;\n> +\n> + return;\n> + }\n> #ifdef HAVE_TIFF\n> if (dng) {\n> ret = DNGWriter::write(filename.c_str(), camera_,\n> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> index 179cc376..cdaa0baa 100644\n> --- a/src/apps/cam/main.cpp\n> +++ b/src/apps/cam/main.cpp\n> @@ -150,6 +150,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \"to write files, using the default file name. Otherwise it sets the\\n\"\n> \"full file path and name. The first '#' character in the file name\\n\"\n> \"is expanded to the camera index, stream name and frame sequence number.\\n\"\n> + \"If the file name ends with '.pnm', then the frame will be written to\\n\"\n> + \"the output file(s) in PNM format.\\n\"\n> #ifdef HAVE_TIFF\n> \"If the file name ends with '.dng', then the frame will be written to\\n\"\n> \"the output file(s) in DNG format.\\n\"\n> diff --git a/src/apps/common/meson.build b/src/apps/common/meson.build\n> index 479326cd..47b7e41a 100644\n> --- a/src/apps/common/meson.build\n> +++ b/src/apps/common/meson.build\n> @@ -3,6 +3,7 @@\n> apps_sources = files([\n> 'image.cpp',\n> 'options.cpp',\n> + 'pnm_writer.cpp',\n> 'stream_options.cpp',\n> ])\n> \n> diff --git a/src/apps/common/pnm_writer.cpp b/src/apps/common/pnm_writer.cpp\n> new file mode 100644\n> index 00000000..b505f97e\n> --- /dev/null\n> +++ b/src/apps/common/pnm_writer.cpp\n> @@ -0,0 +1,54 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024 Red Hat, Inc.\n> + *\n> + * pnm_writer.cpp - PNM writer\n> + */\n> +\n> +#include \"pnm_writer.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +int PNMWriter::write(const char *filename,\n> + const StreamConfiguration &config,\n> + const void *data)\n\nShouldn't/couldn't the void *data be a Span? Or even an Image?\n\n> +{\n> + if (config.pixelFormat != formats::BGR888) {\n\nI sort of wonder if the outputs (FileSink etc) should expose their\nhandling as part of the pipeline configuration in cam, so that the\nvalidation happens before the frames are captured. But honestly I don't\nthink it matters at this stage.\n\nBut I'd expect cam to be able to determine what capabilities are exposed\nby the outputs and use that to filter the stream configuration to a\nmatching mode if one is not specified.\n\nStill this will work for now as long as the full configuration is\naccepted on both the camera and the PNMWriter...\n\n\n\n> + std::cerr << \"Only BGR888 output pixel format is supported (\"\n> + << config.pixelFormat << \" requested)\" << std::endl;\n> + return -EINVAL;\n> + }\n> +\n> + std::ofstream output(filename, std::ios::binary);\n> + if (!output) {\n> + std::cerr << \"Failed to open pnm file: \" << filename << std::endl;\n> + return -EINVAL;\n> + }\n> +\n> + output << \"P6\" << std::endl\n> + << config.size.width << \" \" << config.size.height << std::endl\n> + << \"255\" << std::endl;\n> + if (!output) {\n> + std::cerr << \"Failed to write the file header\" << std::endl;\n> + return -EINVAL;\n> + }\n> +\n> + const unsigned int rowLength = config.size.width * 3;\n> + const unsigned int paddedRowLength = config.stride;\n> + const char *row = reinterpret_cast(data);\n> + for (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {\n> + output.write(row, rowLength);\n> + if (!output) {\n> + std::cerr << \"Failed to write image data at row \" << y << std::endl;\n> + return -EINVAL;\n> + }\n> + }\n> +\n> + return 0;\n> +}\n> diff --git a/src/apps/common/pnm_writer.h b/src/apps/common/pnm_writer.h\n> new file mode 100644\n> index 00000000..a0ae4587\n> --- /dev/null\n> +++ b/src/apps/common/pnm_writer.h\n> @@ -0,0 +1,18 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi Ltd\n> + *\n\nIt looks like this should be fixed.\n\n\n> + * pnm_writer.h - PNM writer\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +class PNMWriter\n> +{\n> +public:\n> + static int write(const char *filename,\n> + const libcamera::StreamConfiguration &config,\n> + const void *data);\n\nWith the copyright fixed, and preferably a more scoped data variable\nhere:\n\n\nReviewed-by: Kieran Bingham \n\n> +};\n> -- \n> 2.42.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B4405BD1F1\n\tfor ;\n\tTue, 12 Mar 2024 16:13:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01EFC62C80;\n\tTue, 12 Mar 2024 17:13:44 +0100 (CET)","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 8E7C86286C\n\tfor ;\n\tTue, 12 Mar 2024 17:13:43 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95ECD593;\n\tTue, 12 Mar 2024 17:13:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"g2Bn0msM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710260001;\n\tbh=uSWcUQQmlT5vEr9wTCulAxg0HGFZCXIpT4I+pxrhFo4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=g2Bn0msMF6aDNTtw0LQfFCOZC5z6wNRN3D9NsNwHW9dmyyzB7m4+53APuMtykm1q6\n\tQJqMkKcZA0peDv5LtiNQ8MHqWYBGQH+UBkYJcDScOX21kLet7Wf/mH4FJBV+zXUM/S\n\tkD/9Vo6zTfmxMYLaqLU/CtiFwIUTOuwRcSeoSnGk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240312154146.1135416-2-mzamazal@redhat.com>","References":"<20240312154146.1135416-1-mzamazal@redhat.com>\n\t<20240312154146.1135416-2-mzamazal@redhat.com>","Subject":"Re: [PATCH v2 1/1] apps: cam: Add support for pnm output format","From":"Kieran Bingham ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Tue, 12 Mar 2024 16:13:39 +0000","Message-ID":"<171026001996.3708697.170277967630913816@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28991,"web_url":"https://patchwork.libcamera.org/comment/28991/","msgid":"<87frwnkedw.fsf@redhat.com>","date":"2024-03-18T10:44:59","subject":"Re: [PATCH v2 1/1] apps: cam: Add support for pnm output format","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Kieran,\n\nthank you for the review.\n\nKieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-03-12 15:41:46)\n>> When file output is requested from cam app, it simply dumps the processed data\n>> and it must be converted to a readable image format manually. Let's add support\n>> for pnm output file format to make files produced by cam directly readable by\n>> image display and processing software.\n>> \n>> For now, only BGR888 output format, which is the simplest one to use, is\n>> supported but nothing prevents adding support for other output formats if\n>> needed.\n>\n> What other formats are possible to add directly with PNM? Do we have to\n> do format conversions or can we write them directly?\n>\n> https://www.fileformat.info/format/pbm/egff.htm looks like most other\n> formats will need converting to more or less RGB888.\n\nYes, the same as with other popular formats.\n\n> I wish there was a way to 'standardise' outputting a header such as the\n> PPM form, but with a FOURCC to describe the data so we can output our\n> raw buffers but with a header ...\n>\n> I wonder if we could sabotage/extend PNM/PPM in the future as a way of\n> storing V4L2/DRM formats : \n>\n> (new format) (fourcc)\n> P7 pRAA\n>\n> Or at that point maybe we call it our own custom fileformat anyway ;-)\n\nThe idea of this patch is to be able to read or process the resulting image\ndirectly with common image viewers and processing tools. What would be the use\nof a custom format? We would need something to convert it to a common format\nanyway and then we can omit the custom format as an intermediary step, unless\nit's needed for speed or something, but would there be a real need for something\nlike that?\n\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/apps/cam/file_sink.cpp | 11 +++++++\n>> src/apps/cam/main.cpp | 2 ++\n>> src/apps/common/meson.build | 1 +\n>> src/apps/common/pnm_writer.cpp | 54 ++++++++++++++++++++++++++++++++++\n>> src/apps/common/pnm_writer.h | 18 ++++++++++++\n>> 5 files changed, 86 insertions(+)\n>> create mode 100644 src/apps/common/pnm_writer.cpp\n>> create mode 100644 src/apps/common/pnm_writer.h\n>> \n>> diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp\n>> index dca350c4..3db59925 100644\n>> --- a/src/apps/cam/file_sink.cpp\n>> +++ b/src/apps/cam/file_sink.cpp\n>> @@ -16,6 +16,7 @@\n>> #include \n>> \n>> #include \"../common/dng_writer.h\"\n>> +#include \"../common/pnm_writer.h\"\n>> #include \"../common/image.h\"\n>> \n>> #include \"file_sink.h\"\n>> @@ -73,6 +74,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,\n>> if (!pattern_.empty())\n>> filename = pattern_;\n>> \n>> + bool pnm = filename.find(\".pnm\", filename.size() - 4) != std::string::npos;\n>> #ifdef HAVE_TIFF\n>> bool dng = filename.find(\".dng\", filename.size() - 4) != std::string::npos;\n>> #endif /* HAVE_TIFF */\n>> @@ -90,6 +92,15 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,\n>> \n>> Image *image = mappedBuffers_[buffer].get();\n>> \n>> + if (pnm) {\n>> + ret = PNMWriter::write(filename.c_str(), stream->configuration(),\n>> + image->data(0).data());\n>> + if (ret < 0)\n>> + std::cerr << \"failed to write PNM file `\" << filename\n>> + << \"'\" << std::endl;\n>> +\n>> + return;\n>> + }\n>> #ifdef HAVE_TIFF\n>> if (dng) {\n>> ret = DNGWriter::write(filename.c_str(), camera_,\n>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n>> index 179cc376..cdaa0baa 100644\n>> --- a/src/apps/cam/main.cpp\n>> +++ b/src/apps/cam/main.cpp\n>> @@ -150,6 +150,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n>> \"to write files, using the default file name. Otherwise it sets the\\n\"\n>> \"full file path and name. The first '#' character in the file name\\n\"\n>> \"is expanded to the camera index, stream name and frame sequence number.\\n\"\n>> + \"If the file name ends with '.pnm', then the frame will be written to\\n\"\n>> + \"the output file(s) in PNM format.\\n\"\n>> #ifdef HAVE_TIFF\n>> \"If the file name ends with '.dng', then the frame will be written to\\n\"\n>> \"the output file(s) in DNG format.\\n\"\n>> diff --git a/src/apps/common/meson.build b/src/apps/common/meson.build\n>> index 479326cd..47b7e41a 100644\n>> --- a/src/apps/common/meson.build\n>> +++ b/src/apps/common/meson.build\n>> @@ -3,6 +3,7 @@\n>> apps_sources = files([\n>> 'image.cpp',\n>> 'options.cpp',\n>> + 'pnm_writer.cpp',\n>> 'stream_options.cpp',\n>> ])\n>> \n>> diff --git a/src/apps/common/pnm_writer.cpp b/src/apps/common/pnm_writer.cpp\n>> new file mode 100644\n>> index 00000000..b505f97e\n>> --- /dev/null\n>> +++ b/src/apps/common/pnm_writer.cpp\n>> @@ -0,0 +1,54 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024 Red Hat, Inc.\n>> + *\n>> + * pnm_writer.cpp - PNM writer\n>> + */\n>> +\n>> +#include \"pnm_writer.h\"\n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +using namespace libcamera;\n>> +\n>> +int PNMWriter::write(const char *filename,\n>> + const StreamConfiguration &config,\n>> + const void *data)\n>\n> Shouldn't/couldn't the void *data be a Span? Or even an Image?\n\nYes, I think using Span here is better, done in v3.\n\n>> +{\n>> + if (config.pixelFormat != formats::BGR888) {\n>\n> I sort of wonder if the outputs (FileSink etc) should expose their\n> handling as part of the pipeline configuration in cam, so that the\n> validation happens before the frames are captured. But honestly I don't\n> think it matters at this stage.\n>\n> But I'd expect cam to be able to determine what capabilities are exposed\n> by the outputs and use that to filter the stream configuration to a\n> matching mode if one is not specified.\n\nThis would be definitely good. Especially because when taking multiple frames,\nthe current version captures them all and fails for all of them.\n\n> Still this will work for now as long as the full configuration is\n> accepted on both the camera and the PNMWriter...\n\nYes. If the choice is between having this now or maybe a better version in the\nfuture, I'd vote for having this now and maybe a better version in the future. :-)\n\n>> + std::cerr << \"Only BGR888 output pixel format is supported (\"\n>> + << config.pixelFormat << \" requested)\" << std::endl;\n>> + return -EINVAL;\n>> + }\n>> +\n>> + std::ofstream output(filename, std::ios::binary);\n>> + if (!output) {\n>> + std::cerr << \"Failed to open pnm file: \" << filename << std::endl;\n>> + return -EINVAL;\n>> + }\n>> +\n>> + output << \"P6\" << std::endl\n>> + << config.size.width << \" \" << config.size.height << std::endl\n>> + << \"255\" << std::endl;\n>> + if (!output) {\n>> + std::cerr << \"Failed to write the file header\" << std::endl;\n>> + return -EINVAL;\n>> + }\n>> +\n>> + const unsigned int rowLength = config.size.width * 3;\n>> + const unsigned int paddedRowLength = config.stride;\n>> + const char *row = reinterpret_cast(data);\n>> + for (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {\n>> + output.write(row, rowLength);\n>> + if (!output) {\n>> + std::cerr << \"Failed to write image data at row \" << y << std::endl;\n>> + return -EINVAL;\n>> + }\n>> + }\n>> +\n>> + return 0;\n>> +}\n>> diff --git a/src/apps/common/pnm_writer.h b/src/apps/common/pnm_writer.h\n>> new file mode 100644\n>> index 00000000..a0ae4587\n>> --- /dev/null\n>> +++ b/src/apps/common/pnm_writer.h\n>> @@ -0,0 +1,18 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Raspberry Pi Ltd\n>> + *\n>\n> It looks like this should be fixed.\n\nRight, done in v3.\n\n>> + * pnm_writer.h - PNM writer\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +class PNMWriter\n>> +{\n>> +public:\n>> + static int write(const char *filename,\n>> + const libcamera::StreamConfiguration &config,\n>> + const void *data);\n>\n> With the copyright fixed, and preferably a more scoped data variable\n> here:\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n>> +};\n>> -- \n>> 2.42.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D4B54C3272\n\tfor ;\n\tMon, 18 Mar 2024 10:45:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 716EE62CAC;\n\tMon, 18 Mar 2024 11:45:07 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8377962C92\n\tfor ;\n\tMon, 18 Mar 2024 11:45:05 +0100 (CET)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-615-P0qZzqsePpWvYbdxYq1XXQ-1; Mon, 18 Mar 2024 06:45:03 -0400","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-3416632aeffso675380f8f.1\n\tfor ;\n\tMon, 18 Mar 2024 03:45:03 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tn16-20020adffe10000000b0033de2f2a88dsm9457251wrr.103.2024.03.18.03.44.59\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 18 Mar 2024 03:44:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"MVHiJ/4z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1710758704;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=f3Rzo8Sr4djIjnVZuPNl+rL7T377pw3Wyw2KnNz/AJ0=;\n\tb=MVHiJ/4zGlylQrgVevD3cm2S5gLxzSzl0obMNQt/KZFZ0Ye9/FKeVskMU++Tryyv7NN/NW\n\tXARaLRC6oT8nXG8HGobt+W7ETjEoQVC8e5v9MdDhzBbhiiajUVw6AYCyEiN0iUIOpwAz6y\n\tJJzQR37AZ3NqKEoEtTiGfUsI44j5xIM=","X-MC-Unique":"P0qZzqsePpWvYbdxYq1XXQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1710758701; x=1711363501;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=f3Rzo8Sr4djIjnVZuPNl+rL7T377pw3Wyw2KnNz/AJ0=;\n\tb=kHVGYydQobbJ9uUgovmyDDGNqRSr11Ii26ZfibQC83W9EFRjri0MWOJifkSbudhN8Y\n\t3lZmWWxCyLl6FMpbJjHpJejgd722gNE0puYLIBz66o8JXHxm3+211btaNaH70clTPKu7\n\tDXVSIy3yazZ44XyJPflg0Im3fKOfI7jxOZ0Q5ffk4WjAqZndY4c0nJwg28SMkhhseGA2\n\thq6MOCRIGVr3QLMJ208F8gC8b9CLk0WzEO1+2YjwmoNgIMUWYou5lebb+vnIZm9lRVoo\n\t+bNf2DuTPlftvdUlzE/Hc59H2cO850WOFEMGRf5K+sw1ZNDciA/E1ngAJYalvHL/LgHZ\n\t91Mg==","X-Gm-Message-State":"AOJu0Yz60I08Xu1U2VgvCy2Q9ac+vuetNe5ukyX2PqHhsyQf2S71uJJd\n\to/V9LHp+J0dd4sjsBLO+achEXzGyCpJaI4bLUDaLeP81zNhjrHK1LtRZlU1epFGbBG/fzzl7h3q\n\tZu4CinzdzYaXHQGdhC2YDN7aSZf9vRen9kAKL62fw6aKpA4C2krOY2Kk/BaPCOn2X2dZKKrpChW\n\tMzPlOghRrfZ6XZ5cUmkVRdEfSR3PphQ/MiRgbdBt/xotNVXztNU2WRa7I=","X-Received":["by 2002:adf:f7c9:0:b0:33e:7719:325d with SMTP id\n\ta9-20020adff7c9000000b0033e7719325dmr9784628wrq.2.1710758700950; \n\tMon, 18 Mar 2024 03:45:00 -0700 (PDT)","by 2002:adf:f7c9:0:b0:33e:7719:325d with SMTP id\n\ta9-20020adff7c9000000b0033e7719325dmr9784610wrq.2.1710758700437; \n\tMon, 18 Mar 2024 03:45:00 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGstPHmZabLNUWfAfb+B3FhsTKpnetk9rm1fuwJvCSleiacwAkDtrzdZrVS++OK+cd6z2vY1w==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 1/1] apps: cam: Add support for pnm output format","In-Reply-To":"<171026001996.3708697.170277967630913816@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Tue, 12 Mar 2024 16:13:39 +0000\")","References":"<20240312154146.1135416-1-mzamazal@redhat.com>\n\t<20240312154146.1135416-2-mzamazal@redhat.com>\n\t<171026001996.3708697.170277967630913816@ping.linuxembedded.co.uk>","Date":"Mon, 18 Mar 2024 11:44:59 +0100","Message-ID":"<87frwnkedw.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28993,"web_url":"https://patchwork.libcamera.org/comment/28993/","msgid":"<171076115528.1676185.2116979934389952002@ping.linuxembedded.co.uk>","date":"2024-03-18T11:25:55","subject":"Re: [PATCH v2 1/1] apps: cam: Add support for pnm output format","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-03-18 10:44:59)\n> Hi Kieran,\n> \n> thank you for the review.\n> \n> Kieran Bingham writes:\n> \n> > Quoting Milan Zamazal (2024-03-12 15:41:46)\n> >> When file output is requested from cam app, it simply dumps the processed data\n> >> and it must be converted to a readable image format manually. Let's add support\n> >> for pnm output file format to make files produced by cam directly readable by\n> >> image display and processing software.\n> >> \n> >> For now, only BGR888 output format, which is the simplest one to use, is\n> >> supported but nothing prevents adding support for other output formats if\n> >> needed.\n> >\n> > What other formats are possible to add directly with PNM? Do we have to\n> > do format conversions or can we write them directly?\n> >\n> > https://www.fileformat.info/format/pbm/egff.htm looks like most other\n> > formats will need converting to more or less RGB888.\n> \n> Yes, the same as with other popular formats.\n> \n> > I wish there was a way to 'standardise' outputting a header such as the\n> > PPM form, but with a FOURCC to describe the data so we can output our\n> > raw buffers but with a header ...\n> >\n> > I wonder if we could sabotage/extend PNM/PPM in the future as a way of\n> > storing V4L2/DRM formats : \n> >\n> > (new format) (fourcc)\n> > P7 pRAA\n> >\n> > Or at that point maybe we call it our own custom fileformat anyway ;-)\n> \n> The idea of this patch is to be able to read or process the resulting image\n> directly with common image viewers and processing tools. What would be the use\n> of a custom format? We would need something to convert it to a common format\n> anyway and then we can omit the custom format as an intermediary step, unless\n> it's needed for speed or something, but would there be a real need for something\n> like that?\n\nOh - absolutely, I agree the aim here is to be able to read the data\nwith standard tools. My comments are more about the future options.\nCurrently when we write out 'raw' binary frames - they are headerless.\nSo it can be difficult/awkward to determine what is in those raw frames.\nThe PPM header looks very easy to manage expressing what a raw buffer\ncontains, so I could imagine that a custom 'raw' frame writer could add\na header much alike this. Of course it wouldn't be readable by standard\ntools ... but maybe we could propose something to extend 'standard'\ntools to support the extensions in the future.\n\nThings like https://git.retiisi.eu/?p=~sailus/raw2rgbpnm.git let us\nconvert raw captured frames, but we have to tell that tool exactly what\nformat was stored, and the correct size. A header would make that far\neasier, as I frequently find it cumbersome to handle and essentially\nhave to have custom scripts for everything, which could otherwise be\nsolved with a binary raw format header.\n\n--\nKieran\n\n\n> \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> src/apps/cam/file_sink.cpp | 11 +++++++\n> >> src/apps/cam/main.cpp | 2 ++\n> >> src/apps/common/meson.build | 1 +\n> >> src/apps/common/pnm_writer.cpp | 54 ++++++++++++++++++++++++++++++++++\n> >> src/apps/common/pnm_writer.h | 18 ++++++++++++\n> >> 5 files changed, 86 insertions(+)\n> >> create mode 100644 src/apps/common/pnm_writer.cpp\n> >> create mode 100644 src/apps/common/pnm_writer.h\n> >> \n> >> diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp\n> >> index dca350c4..3db59925 100644\n> >> --- a/src/apps/cam/file_sink.cpp\n> >> +++ b/src/apps/cam/file_sink.cpp\n> >> @@ -16,6 +16,7 @@\n> >> #include \n> >> \n> >> #include \"../common/dng_writer.h\"\n> >> +#include \"../common/pnm_writer.h\"\n> >> #include \"../common/image.h\"\n> >> \n> >> #include \"file_sink.h\"\n> >> @@ -73,6 +74,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,\n> >> if (!pattern_.empty())\n> >> filename = pattern_;\n> >> \n> >> + bool pnm = filename.find(\".pnm\", filename.size() - 4) != std::string::npos;\n> >> #ifdef HAVE_TIFF\n> >> bool dng = filename.find(\".dng\", filename.size() - 4) != std::string::npos;\n> >> #endif /* HAVE_TIFF */\n> >> @@ -90,6 +92,15 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,\n> >> \n> >> Image *image = mappedBuffers_[buffer].get();\n> >> \n> >> + if (pnm) {\n> >> + ret = PNMWriter::write(filename.c_str(), stream->configuration(),\n> >> + image->data(0).data());\n> >> + if (ret < 0)\n> >> + std::cerr << \"failed to write PNM file `\" << filename\n> >> + << \"'\" << std::endl;\n> >> +\n> >> + return;\n> >> + }\n> >> #ifdef HAVE_TIFF\n> >> if (dng) {\n> >> ret = DNGWriter::write(filename.c_str(), camera_,\n> >> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> >> index 179cc376..cdaa0baa 100644\n> >> --- a/src/apps/cam/main.cpp\n> >> +++ b/src/apps/cam/main.cpp\n> >> @@ -150,6 +150,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n> >> \"to write files, using the default file name. Otherwise it sets the\\n\"\n> >> \"full file path and name. The first '#' character in the file name\\n\"\n> >> \"is expanded to the camera index, stream name and frame sequence number.\\n\"\n> >> + \"If the file name ends with '.pnm', then the frame will be written to\\n\"\n> >> + \"the output file(s) in PNM format.\\n\"\n> >> #ifdef HAVE_TIFF\n> >> \"If the file name ends with '.dng', then the frame will be written to\\n\"\n> >> \"the output file(s) in DNG format.\\n\"\n> >> diff --git a/src/apps/common/meson.build b/src/apps/common/meson.build\n> >> index 479326cd..47b7e41a 100644\n> >> --- a/src/apps/common/meson.build\n> >> +++ b/src/apps/common/meson.build\n> >> @@ -3,6 +3,7 @@\n> >> apps_sources = files([\n> >> 'image.cpp',\n> >> 'options.cpp',\n> >> + 'pnm_writer.cpp',\n> >> 'stream_options.cpp',\n> >> ])\n> >> \n> >> diff --git a/src/apps/common/pnm_writer.cpp b/src/apps/common/pnm_writer.cpp\n> >> new file mode 100644\n> >> index 00000000..b505f97e\n> >> --- /dev/null\n> >> +++ b/src/apps/common/pnm_writer.cpp\n> >> @@ -0,0 +1,54 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024 Red Hat, Inc.\n> >> + *\n> >> + * pnm_writer.cpp - PNM writer\n> >> + */\n> >> +\n> >> +#include \"pnm_writer.h\"\n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +using namespace libcamera;\n> >> +\n> >> +int PNMWriter::write(const char *filename,\n> >> + const StreamConfiguration &config,\n> >> + const void *data)\n> >\n> > Shouldn't/couldn't the void *data be a Span? Or even an Image?\n> \n> Yes, I think using Span here is better, done in v3.\n> \n> >> +{\n> >> + if (config.pixelFormat != formats::BGR888) {\n> >\n> > I sort of wonder if the outputs (FileSink etc) should expose their\n> > handling as part of the pipeline configuration in cam, so that the\n> > validation happens before the frames are captured. But honestly I don't\n> > think it matters at this stage.\n> >\n> > But I'd expect cam to be able to determine what capabilities are exposed\n> > by the outputs and use that to filter the stream configuration to a\n> > matching mode if one is not specified.\n> \n> This would be definitely good. Especially because when taking multiple frames,\n> the current version captures them all and fails for all of them.\n> \n> > Still this will work for now as long as the full configuration is\n> > accepted on both the camera and the PNMWriter...\n> \n> Yes. If the choice is between having this now or maybe a better version in the\n> future, I'd vote for having this now and maybe a better version in the future. :-)\n> \n> >> + std::cerr << \"Only BGR888 output pixel format is supported (\"\n> >> + << config.pixelFormat << \" requested)\" << std::endl;\n> >> + return -EINVAL;\n> >> + }\n> >> +\n> >> + std::ofstream output(filename, std::ios::binary);\n> >> + if (!output) {\n> >> + std::cerr << \"Failed to open pnm file: \" << filename << std::endl;\n> >> + return -EINVAL;\n> >> + }\n> >> +\n> >> + output << \"P6\" << std::endl\n> >> + << config.size.width << \" \" << config.size.height << std::endl\n> >> + << \"255\" << std::endl;\n> >> + if (!output) {\n> >> + std::cerr << \"Failed to write the file header\" << std::endl;\n> >> + return -EINVAL;\n> >> + }\n> >> +\n> >> + const unsigned int rowLength = config.size.width * 3;\n> >> + const unsigned int paddedRowLength = config.stride;\n> >> + const char *row = reinterpret_cast(data);\n> >> + for (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {\n> >> + output.write(row, rowLength);\n> >> + if (!output) {\n> >> + std::cerr << \"Failed to write image data at row \" << y << std::endl;\n> >> + return -EINVAL;\n> >> + }\n> >> + }\n> >> +\n> >> + return 0;\n> >> +}\n> >> diff --git a/src/apps/common/pnm_writer.h b/src/apps/common/pnm_writer.h\n> >> new file mode 100644\n> >> index 00000000..a0ae4587\n> >> --- /dev/null\n> >> +++ b/src/apps/common/pnm_writer.h\n> >> @@ -0,0 +1,18 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2020, Raspberry Pi Ltd\n> >> + *\n> >\n> > It looks like this should be fixed.\n> \n> Right, done in v3.\n> \n> >> + * pnm_writer.h - PNM writer\n> >> + */\n> >> +\n> >> +#pragma once\n> >> +\n> >> +#include \n> >> +\n> >> +class PNMWriter\n> >> +{\n> >> +public:\n> >> + static int write(const char *filename,\n> >> + const libcamera::StreamConfiguration &config,\n> >> + const void *data);\n> >\n> > With the copyright fixed, and preferably a more scoped data variable\n> > here:\n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >> +};\n> >> -- \n> >> 2.42.0\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 7D614BD160\n\tfor ;\n\tMon, 18 Mar 2024 11:26:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6189962CA9;\n\tMon, 18 Mar 2024 12:25:59 +0100 (CET)","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 47E9161C6D\n\tfor ;\n\tMon, 18 Mar 2024 12:25:58 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 339507E9;\n\tMon, 18 Mar 2024 12:25:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uY8+1TgT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710761132;\n\tbh=ys9vvqZx1o/W13Ny76CyY+LYjQGsi1FtfyvOID69IHw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=uY8+1TgTVJFUDGHT37etYNpjTqJiwTU8DVoFk2thinMqFLMMB6e5idw9C1pFUX0f9\n\tHA6AknPjaKXpB1BINEz+DBxL+Mp3iy3gPBtkGVrF1Z1mj+3khjml6ipYwQReddZafd\n\t/7QfhgLsIyGaygXuORpaaankoSt2irvWX5pw/qXk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<87frwnkedw.fsf@redhat.com>","References":"<20240312154146.1135416-1-mzamazal@redhat.com>\n\t<20240312154146.1135416-2-mzamazal@redhat.com>\n\t<171026001996.3708697.170277967630913816@ping.linuxembedded.co.uk>\n\t<87frwnkedw.fsf@redhat.com>","Subject":"Re: [PATCH v2 1/1] apps: cam: Add support for pnm output format","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Milan Zamazal ","Date":"Mon, 18 Mar 2024 11:25:55 +0000","Message-ID":"<171076115528.1676185.2116979934389952002@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28994,"web_url":"https://patchwork.libcamera.org/comment/28994/","msgid":"<20240318121527.GA13682@pendragon.ideasonboard.com>","date":"2024-03-18T12:15:27","subject":"Re: [PATCH v2 1/1] apps: cam: Add support for pnm output format","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Mar 18, 2024 at 11:25:55AM +0000, Kieran Bingham wrote:\n> Quoting Milan Zamazal (2024-03-18 10:44:59)\n> > Kieran Bingham writes:\n> > > Quoting Milan Zamazal (2024-03-12 15:41:46)\n> > >> When file output is requested from cam app, it simply dumps the processed data\n> > >> and it must be converted to a readable image format manually. Let's add support\n> > >> for pnm output file format to make files produced by cam directly readable by\n> > >> image display and processing software.\n> > >> \n> > >> For now, only BGR888 output format, which is the simplest one to use, is\n> > >> supported but nothing prevents adding support for other output formats if\n> > >> needed.\n> > >\n> > > What other formats are possible to add directly with PNM? Do we have to\n> > > do format conversions or can we write them directly?\n> > >\n> > > https://www.fileformat.info/format/pbm/egff.htm looks like most other\n> > > formats will need converting to more or less RGB888.\n> > \n> > Yes, the same as with other popular formats.\n\nWhat we need here, I think, is a standardized file format that can store\na raw image with enough metadata to allow interoperability between\napplications. I don't think conversion and encoding to other formats is\na good idea, as it will quickly get out of control, and isn't the\npurpose of cam. I'm fine adding PPM support when the data is already in\nRGB888 format as a quick workaround though, until we have something\nbetter, but I don't want to expand it with format conversion.\n\nI haven't researched this problem area in great details. One possibly\ninteresting option starting point for the metadata related to the format\ndescription is the Khronos Data Format Specification ([1]). It's missing\na colour model for raw Bayer data, but we could work with Khronos to add\nit.\n\nRegarding file formats, there's the Khronos Texture specification ([2]),\nbut that uses Vulkan formats and is likely too specific to the needs of\ntextures (although we should still check it, just in case).\n\nIs anyone aware of a standard format we could use ? \n\n[1] https://www.khronos.org/dataformat\n[2] https://www.khronos.org/ktx/\n\n> > > I wish there was a way to 'standardise' outputting a header such as the\n> > > PPM form, but with a FOURCC to describe the data so we can output our\n> > > raw buffers but with a header ...\n> > >\n> > > I wonder if we could sabotage/extend PNM/PPM in the future as a way of\n> > > storing V4L2/DRM formats : \n> > >\n> > > (new format) (fourcc)\n> > > P7 pRAA\n> > >\n> > > Or at that point maybe we call it our own custom fileformat anyway ;-)\n> > \n> > The idea of this patch is to be able to read or process the resulting image\n> > directly with common image viewers and processing tools. What would be the use\n> > of a custom format? We would need something to convert it to a common format\n> > anyway and then we can omit the custom format as an intermediary step, unless\n> > it's needed for speed or something, but would there be a real need for something\n> > like that?\n> \n> Oh - absolutely, I agree the aim here is to be able to read the data\n> with standard tools. My comments are more about the future options.\n> Currently when we write out 'raw' binary frames - they are headerless.\n> So it can be difficult/awkward to determine what is in those raw frames.\n> The PPM header looks very easy to manage expressing what a raw buffer\n> contains, so I could imagine that a custom 'raw' frame writer could add\n> a header much alike this. Of course it wouldn't be readable by standard\n> tools ... but maybe we could propose something to extend 'standard'\n> tools to support the extensions in the future.\n> \n> Things like https://git.retiisi.eu/?p=~sailus/raw2rgbpnm.git let us\n> convert raw captured frames, but we have to tell that tool exactly what\n> format was stored, and the correct size. A header would make that far\n> easier, as I frequently find it cumbersome to handle and essentially\n> have to have custom scripts for everything, which could otherwise be\n> solved with a binary raw format header.\n\nI agree, and that's the direction I'd like to take. If we can find an\nexisting standard format suitable to store raw frames, and readable by\nexisting tools, that's great. If we can't, we can create one, and work\non getting conversion tools such as raw2rgbpnm to understand it.\n\n> > >> Signed-off-by: Milan Zamazal \n> > >> ---\n> > >> src/apps/cam/file_sink.cpp | 11 +++++++\n> > >> src/apps/cam/main.cpp | 2 ++\n> > >> src/apps/common/meson.build | 1 +\n> > >> src/apps/common/pnm_writer.cpp | 54 ++++++++++++++++++++++++++++++++++\n> > >> src/apps/common/pnm_writer.h | 18 ++++++++++++\n> > >> 5 files changed, 86 insertions(+)\n> > >> create mode 100644 src/apps/common/pnm_writer.cpp\n> > >> create mode 100644 src/apps/common/pnm_writer.h\n> > >> \n> > >> diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp\n> > >> index dca350c4..3db59925 100644\n> > >> --- a/src/apps/cam/file_sink.cpp\n> > >> +++ b/src/apps/cam/file_sink.cpp\n> > >> @@ -16,6 +16,7 @@\n> > >> #include \n> > >> \n> > >> #include \"../common/dng_writer.h\"\n> > >> +#include \"../common/pnm_writer.h\"\n> > >> #include \"../common/image.h\"\n> > >> \n> > >> #include \"file_sink.h\"\n> > >> @@ -73,6 +74,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,\n> > >> if (!pattern_.empty())\n> > >> filename = pattern_;\n> > >> \n> > >> + bool pnm = filename.find(\".pnm\", filename.size() - 4) != std::string::npos;\n> > >> #ifdef HAVE_TIFF\n> > >> bool dng = filename.find(\".dng\", filename.size() - 4) != std::string::npos;\n> > >> #endif /* HAVE_TIFF */\n> > >> @@ -90,6 +92,15 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,\n> > >> \n> > >> Image *image = mappedBuffers_[buffer].get();\n> > >> \n> > >> + if (pnm) {\n> > >> + ret = PNMWriter::write(filename.c_str(), stream->configuration(),\n> > >> + image->data(0).data());\n> > >> + if (ret < 0)\n> > >> + std::cerr << \"failed to write PNM file `\" << filename\n> > >> + << \"'\" << std::endl;\n> > >> +\n> > >> + return;\n> > >> + }\n> > >> #ifdef HAVE_TIFF\n> > >> if (dng) {\n> > >> ret = DNGWriter::write(filename.c_str(), camera_,\n> > >> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> > >> index 179cc376..cdaa0baa 100644\n> > >> --- a/src/apps/cam/main.cpp\n> > >> +++ b/src/apps/cam/main.cpp\n> > >> @@ -150,6 +150,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > >> \"to write files, using the default file name. Otherwise it sets the\\n\"\n> > >> \"full file path and name. The first '#' character in the file name\\n\"\n> > >> \"is expanded to the camera index, stream name and frame sequence number.\\n\"\n> > >> + \"If the file name ends with '.pnm', then the frame will be written to\\n\"\n> > >> + \"the output file(s) in PNM format.\\n\"\n> > >> #ifdef HAVE_TIFF\n> > >> \"If the file name ends with '.dng', then the frame will be written to\\n\"\n> > >> \"the output file(s) in DNG format.\\n\"\n> > >> diff --git a/src/apps/common/meson.build b/src/apps/common/meson.build\n> > >> index 479326cd..47b7e41a 100644\n> > >> --- a/src/apps/common/meson.build\n> > >> +++ b/src/apps/common/meson.build\n> > >> @@ -3,6 +3,7 @@\n> > >> apps_sources = files([\n> > >> 'image.cpp',\n> > >> 'options.cpp',\n> > >> + 'pnm_writer.cpp',\n> > >> 'stream_options.cpp',\n> > >> ])\n> > >> \n> > >> diff --git a/src/apps/common/pnm_writer.cpp b/src/apps/common/pnm_writer.cpp\n> > >> new file mode 100644\n> > >> index 00000000..b505f97e\n> > >> --- /dev/null\n> > >> +++ b/src/apps/common/pnm_writer.cpp\n> > >> @@ -0,0 +1,54 @@\n> > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > >> +/*\n> > >> + * Copyright (C) 2024 Red Hat, Inc.\n> > >> + *\n> > >> + * pnm_writer.cpp - PNM writer\n> > >> + */\n> > >> +\n> > >> +#include \"pnm_writer.h\"\n> > >> +\n> > >> +#include \n> > >> +#include \n> > >> +\n> > >> +#include \n> > >> +#include \n> > >> +\n> > >> +using namespace libcamera;\n> > >> +\n> > >> +int PNMWriter::write(const char *filename,\n> > >> + const StreamConfiguration &config,\n> > >> + const void *data)\n> > >\n> > > Shouldn't/couldn't the void *data be a Span? Or even an Image?\n> > \n> > Yes, I think using Span here is better, done in v3.\n> > \n> > >> +{\n> > >> + if (config.pixelFormat != formats::BGR888) {\n> > >\n> > > I sort of wonder if the outputs (FileSink etc) should expose their\n> > > handling as part of the pipeline configuration in cam, so that the\n> > > validation happens before the frames are captured. But honestly I don't\n> > > think it matters at this stage.\n\nIt would be useful for the display sink too, so that cam would configure\nthe stream with a supported format automatically (unless overridden by\nthe -s argument).\n\n> > >\n> > > But I'd expect cam to be able to determine what capabilities are exposed\n> > > by the outputs and use that to filter the stream configuration to a\n> > > matching mode if one is not specified.\n> > \n> > This would be definitely good. Especially because when taking multiple frames,\n> > the current version captures them all and fails for all of them.\n> > \n> > > Still this will work for now as long as the full configuration is\n> > > accepted on both the camera and the PNMWriter...\n> > \n> > Yes. If the choice is between having this now or maybe a better version in the\n> > future, I'd vote for having this now and maybe a better version in the future. :-)\n> > \n> > >> + std::cerr << \"Only BGR888 output pixel format is supported (\"\n> > >> + << config.pixelFormat << \" requested)\" << std::endl;\n> > >> + return -EINVAL;\n> > >> + }\n> > >> +\n> > >> + std::ofstream output(filename, std::ios::binary);\n> > >> + if (!output) {\n> > >> + std::cerr << \"Failed to open pnm file: \" << filename << std::endl;\n> > >> + return -EINVAL;\n> > >> + }\n> > >> +\n> > >> + output << \"P6\" << std::endl\n> > >> + << config.size.width << \" \" << config.size.height << std::endl\n> > >> + << \"255\" << std::endl;\n> > >> + if (!output) {\n> > >> + std::cerr << \"Failed to write the file header\" << std::endl;\n> > >> + return -EINVAL;\n> > >> + }\n> > >> +\n> > >> + const unsigned int rowLength = config.size.width * 3;\n> > >> + const unsigned int paddedRowLength = config.stride;\n> > >> + const char *row = reinterpret_cast(data);\n> > >> + for (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {\n> > >> + output.write(row, rowLength);\n> > >> + if (!output) {\n> > >> + std::cerr << \"Failed to write image data at row \" << y << std::endl;\n> > >> + return -EINVAL;\n> > >> + }\n> > >> + }\n> > >> +\n> > >> + return 0;\n> > >> +}\n> > >> diff --git a/src/apps/common/pnm_writer.h b/src/apps/common/pnm_writer.h\n> > >> new file mode 100644\n> > >> index 00000000..a0ae4587\n> > >> --- /dev/null\n> > >> +++ b/src/apps/common/pnm_writer.h\n> > >> @@ -0,0 +1,18 @@\n> > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > >> +/*\n> > >> + * Copyright (C) 2020, Raspberry Pi Ltd\n> > >> + *\n> > >\n> > > It looks like this should be fixed.\n> > \n> > Right, done in v3.\n> > \n> > >> + * pnm_writer.h - PNM writer\n> > >> + */\n> > >> +\n> > >> +#pragma once\n> > >> +\n> > >> +#include \n> > >> +\n> > >> +class PNMWriter\n> > >> +{\n> > >> +public:\n> > >> + static int write(const char *filename,\n> > >> + const libcamera::StreamConfiguration &config,\n> > >> + const void *data);\n> > >\n> > > With the copyright fixed, and preferably a more scoped data variable\n> > > here:\n> > >\n> > >\n> > > Reviewed-by: Kieran Bingham \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 8D060C3272\n\tfor ;\n\tMon, 18 Mar 2024 12:15:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B273262CA9;\n\tMon, 18 Mar 2024 13:15:31 +0100 (CET)","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 B64DE61C6D\n\tfor ;\n\tMon, 18 Mar 2024 13:15:30 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5EC781BB1;\n\tMon, 18 Mar 2024 13:15:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uUwhPGv7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710764104;\n\tbh=tUGSLPb0lbQsT2Fpv+e/fM0kXnh5MjMxtsIrA1N83Zs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uUwhPGv7fRQbEU7er23+n8IwUIHYOeMeoiaosQ6kqH0wy3l/H2pStmgy+I3JzPls8\n\tvyavv/9j9vbVQ4dW3PihM56WmiIfWj3+GW9ZBpObANULFBOd5Z+5ILrLkITOMfAlqG\n\tCCFRBT5mJqvb3JgzOZsf0ZvBqBUxJZS5DSkSuHus=","Date":"Mon, 18 Mar 2024 14:15:27 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 1/1] apps: cam: Add support for pnm output format","Message-ID":"<20240318121527.GA13682@pendragon.ideasonboard.com>","References":"<20240312154146.1135416-1-mzamazal@redhat.com>\n\t<20240312154146.1135416-2-mzamazal@redhat.com>\n\t<171026001996.3708697.170277967630913816@ping.linuxembedded.co.uk>\n\t<87frwnkedw.fsf@redhat.com>\n\t<171076115528.1676185.2116979934389952002@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171076115528.1676185.2116979934389952002@ping.linuxembedded.co.uk>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]