[{"id":28996,"web_url":"https://patchwork.libcamera.org/comment/28996/","msgid":"<20240318124053.GB13682@pendragon.ideasonboard.com>","date":"2024-03-18T12:40:53","subject":"Re: [PATCH v3 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":"Hi Milan,\n\nThank you for the patch.\n\nOn Mon, Mar 18, 2024 at 11:43:57AM +0100, Milan Zamazal wrote:\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> Signed-off-by: Milan Zamazal \n> Reviewed-by: Kieran Bingham \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 | 20 +++++++++++++\n> 5 files changed, 88 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..ac53719d 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> \tif (!pattern_.empty())\n> \t\tfilename = pattern_;\n> \n> +\tbool pnm = filename.find(\".pnm\", filename.size() - 4) != std::string::npos;\n\nCould you move this after the HAVE_TIFF section, to sort the formats\nalphabetically ?\n\n> #ifdef HAVE_TIFF\n> \tbool 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> \tImage *image = mappedBuffers_[buffer].get();\n> \n> +\tif (pnm) {\n> +\t\tret = PNMWriter::write(filename.c_str(), stream->configuration(),\n> +\t\t\t\t image->data(0));\n> +\t\tif (ret < 0)\n> +\t\t\tstd::cerr << \"failed to write PNM file `\" << filename\n> +\t\t\t\t << \"'\" << std::endl;\n> +\n> +\t\treturn;\n> +\t}\n\nSame here.\n\n> #ifdef HAVE_TIFF\n> \tif (dng) {\n> \t\tret = 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> \t\t\t \"to write files, using the default file name. Otherwise it sets the\\n\"\n> \t\t\t \"full file path and name. The first '#' character in the file name\\n\"\n> \t\t\t \"is expanded to the camera index, stream name and frame sequence number.\\n\"\n> +\t\t\t \"If the file name ends with '.pnm', then the frame will be written to\\n\"\n> +\t\t\t \"the output file(s) in PNM format.\\n\"\n\nAnd here.\n\n> #ifdef HAVE_TIFF\n> \t\t\t \"If the file name ends with '.dng', then the frame will be written to\\n\"\n> \t\t\t \"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..9a63ec05\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> +\t\t const StreamConfiguration &config,\n> +\t\t const Span &data)\n> +{\n> +\tif (config.pixelFormat != formats::BGR888) {\n> +\t\tstd::cerr << \"Only BGR888 output pixel format is supported (\"\n> +\t\t\t << config.pixelFormat << \" requested)\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::ofstream output(filename, std::ios::binary);\n> +\tif (!output) {\n> +\t\tstd::cerr << \"Failed to open pnm file: \" << filename << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\toutput << \"P6\" << std::endl\n\nP6 refers to the Portable PixMap format, whose recommended extension is\n.ppm. Some tools have trouble opening PPM files when named with a .pnm\nsuffix. I would use a .ppm file extension.\n\nI'm also wondering if we shouldn't switch to the PAM format already (P7,\nsee https://en.wikipedia.org/wiki/Portable_anymap#PAM_graphics_format)\nas it also supports greyscale formats. It's not the universal raw file\nformat I'm looking for though, and maybe not worth it (I haven't checked\nhow well supported PAM is).\n\nFor the sake of completeness, I want to mention we have a DNG writer\nthat outputs DNG files, and DNG is a TIFF file under the hood. TIFF\nsupports a set of RGB and YCbCr formats too... Opinions ? :-)\n\n> +\t << config.size.width << \" \" << config.size.height << std::endl\n> +\t << \"255\" << std::endl;\n> +\tif (!output) {\n> +\t\tstd::cerr << \"Failed to write the file header\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tconst unsigned int rowLength = config.size.width * 3;\n> +\tconst unsigned int paddedRowLength = config.stride;\n> +\tconst char *row = reinterpret_cast(data.data());\n> +\tfor (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {\n\nYou could use config.stride directly here, and drop paddedRowLength.\n\n> +\t\toutput.write(row, rowLength);\n> +\t\tif (!output) {\n> +\t\t\tstd::cerr << \"Failed to write image data at row \" << y << std::endl;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n> +\n> +\treturn 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..6c1e7967\n> --- /dev/null\n> +++ b/src/apps/common/pnm_writer.h\n> @@ -0,0 +1,20 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat, Inc.\n> + *\n> + * pnm_writer.h - PNM writer\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +class PNMWriter\n> +{\n> +public:\n> +\tstatic int write(const char *filename,\n> +\t\t\t const libcamera::StreamConfiguration &config,\n> +\t\t\t const libcamera::Span &data);\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 9EA70C3272\n\tfor ;\n\tMon, 18 Mar 2024 12:40:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C29C162CA9;\n\tMon, 18 Mar 2024 13:40:57 +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 3E56061C6D\n\tfor ;\n\tMon, 18 Mar 2024 13:40:56 +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 E04A6B1;\n\tMon, 18 Mar 2024 13:40:29 +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=\"HcVBVe2G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710765630;\n\tbh=8XdSNT9vKXUky6+a60z0lhm8KUc4UGWIdKC77cod4SY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HcVBVe2G3qCyk8jkAD5DC2PvYkY+RoG5ybdb5bhsGaLDWBHAv5lbt53+k/I0AjVEz\n\tWI8/uE5/lIadEc4USQX/0DRMehcn8CHaxPwTw/E/xDxu/zqpfWgnb/mgK83BsAoDJQ\n\toMGSxmE5n0TJ2/iZft7vYVHlE8lk5kJtxd7EH3mk=","Date":"Mon, 18 Mar 2024 14:40:53 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v3 1/1] apps: cam: Add support for pnm output format","Message-ID":"<20240318124053.GB13682@pendragon.ideasonboard.com>","References":"<20240318104357.441724-1-mzamazal@redhat.com>\n\t<20240318104357.441724-2-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240318104357.441724-2-mzamazal@redhat.com>","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":29045,"web_url":"https://patchwork.libcamera.org/comment/29045/","msgid":"<8734sixcy2.fsf@redhat.com>","date":"2024-03-22T19:51:17","subject":"Re: [PATCH v3 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 Laurent,\n\nthank you for the review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Mon, Mar 18, 2024 at 11:43:57AM +0100, Milan Zamazal wrote:\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>> Signed-off-by: Milan Zamazal \n>> Reviewed-by: Kieran Bingham \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 | 20 +++++++++++++\n>> 5 files changed, 88 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..ac53719d 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>> \tif (!pattern_.empty())\n>> \t\tfilename = pattern_;\n>> \n>> +\tbool pnm = filename.find(\".pnm\", filename.size() - 4) != std::string::npos;\n>\n> Could you move this after the HAVE_TIFF section, to sort the formats\n> alphabetically ?\n\nDone.\n\n>> #ifdef HAVE_TIFF\n>> \tbool 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>> \tImage *image = mappedBuffers_[buffer].get();\n>> \n>> +\tif (pnm) {\n>> +\t\tret = PNMWriter::write(filename.c_str(), stream->configuration(),\n>> +\t\t\t\t image->data(0));\n>> +\t\tif (ret < 0)\n>> +\t\t\tstd::cerr << \"failed to write PNM file `\" << filename\n>> +\t\t\t\t << \"'\" << std::endl;\n>> +\n>> +\t\treturn;\n>> +\t}\n>\n> Same here.\n\nDone.\n\n>> #ifdef HAVE_TIFF\n>> \tif (dng) {\n>> \t\tret = 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>> \t\t\t \"to write files, using the default file name. Otherwise it sets the\\n\"\n>> \t\t\t \"full file path and name. The first '#' character in the file name\\n\"\n>> \t\t\t \"is expanded to the camera index, stream name and frame sequence number.\\n\"\n>> +\t\t\t \"If the file name ends with '.pnm', then the frame will be written to\\n\"\n>> +\t\t\t \"the output file(s) in PNM format.\\n\"\n>\n> And here.\n\nDone.\n\n>> #ifdef HAVE_TIFF\n>> \t\t\t \"If the file name ends with '.dng', then the frame will be written to\\n\"\n>> \t\t\t \"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..9a63ec05\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>> +\t\t const StreamConfiguration &config,\n>> +\t\t const Span &data)\n>> +{\n>> +\tif (config.pixelFormat != formats::BGR888) {\n>> +\t\tstd::cerr << \"Only BGR888 output pixel format is supported (\"\n>> +\t\t\t << config.pixelFormat << \" requested)\" << std::endl;\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tstd::ofstream output(filename, std::ios::binary);\n>> +\tif (!output) {\n>> +\t\tstd::cerr << \"Failed to open pnm file: \" << filename << std::endl;\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\toutput << \"P6\" << std::endl\n>\n> P6 refers to the Portable PixMap format, whose recommended extension is\n> .ppm. Some tools have trouble opening PPM files when named with a .pnm\n> suffix. I would use a .ppm file extension.\n\nOK, done. (My original motivation was to possibly cover other PNM formats but\nPPM is the proper extension and as discussed in related threads, we probably\ndon't want to add much in this area.)\n\n> I'm also wondering if we shouldn't switch to the PAM format already (P7,\n> see https://en.wikipedia.org/wiki/Portable_anymap#PAM_graphics_format)\n> as it also supports greyscale formats. It's not the universal raw file\n> format I'm looking for though, and maybe not worth it (I haven't checked\n> how well supported PAM is).\n\nIt's not supported everywhere, for example it's not recognized in Gqview or\nEmacs (*bummer*) on my computer. So better to stick with PPM.\n\n> For the sake of completeness, I want to mention we have a DNG writer\n> that outputs DNG files, and DNG is a TIFF file under the hood. TIFF\n> supports a set of RGB and YCbCr formats too... Opinions ? :-)\n\nI'm not sure whether DNG can support what we need. In any case, it's associated\nwith \"very\" raw camera images and not much supported outside raw converters and\nphoto editors.\n\nAs for TIFF, the advantage of using PPM is that writing it is really trivial and\nthere is no need to use (or replicate) libtiff. But yes, TIFF might be helpful\nat least with non-RGB outputs.\n\n>> +\t << config.size.width << \" \" << config.size.height << std::endl\n>> +\t << \"255\" << std::endl;\n>> +\tif (!output) {\n>> +\t\tstd::cerr << \"Failed to write the file header\" << std::endl;\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tconst unsigned int rowLength = config.size.width * 3;\n>> +\tconst unsigned int paddedRowLength = config.stride;\n>> +\tconst char *row = reinterpret_cast(data.data());\n>> +\tfor (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {\n>\n> You could use config.stride directly here, and drop paddedRowLength.\n\nDone.\n\n>> +\t\toutput.write(row, rowLength);\n>> +\t\tif (!output) {\n>> +\t\t\tstd::cerr << \"Failed to write image data at row \" << y << std::endl;\n>> +\t\t\treturn -EINVAL;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\treturn 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..6c1e7967\n>> --- /dev/null\n>> +++ b/src/apps/common/pnm_writer.h\n>> @@ -0,0 +1,20 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat, Inc.\n>> + *\n>> + * pnm_writer.h - PNM writer\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +class PNMWriter\n>> +{\n>> +public:\n>> +\tstatic int write(const char *filename,\n>> +\t\t\t const libcamera::StreamConfiguration &config,\n>> +\t\t\t const libcamera::Span &data);\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 4BB09BD160\n\tfor ;\n\tFri, 22 Mar 2024 19:51:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE1546307E;\n\tFri, 22 Mar 2024 20:51:25 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 648B163036\n\tfor ;\n\tFri, 22 Mar 2024 20:51:24 +0100 (CET)","from mail-wm1-f70.google.com (mail-wm1-f70.google.com\n\t[209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-636-azbPsSa-Po2K2Afso4nrTA-1; Fri, 22 Mar 2024 15:51:22 -0400","by mail-wm1-f70.google.com with SMTP id\n\t5b1f17b1804b1-4147faf154cso943165e9.2\n\tfor ;\n\tFri, 22 Mar 2024 12:51:21 -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\th19-20020a05600c315300b004146a1bf590sm396561wmo.32.2024.03.22.12.51.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 22 Mar 2024 12:51:18 -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=\"A6Ns1c+m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1711137083;\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=ippVQ0ug1y2H1OIrZpzeQJ8A9DjE89Q8n2oi3NtLp/Q=;\n\tb=A6Ns1c+m2k+L5qy7Z4+4Tv4IETcumWT3sldcLk8U1IpXhi1gNQmcWDJu7qf5OmQFHk0ZRM\n\tWJwFHCU0XgTqBkDHxgHPQyKOw/PBwEpH/T+Tz0gqeHXBbr3o4+lvq6Y3nF2ZrS0/wZuefh\n\taUqRupVItBKT7onjXPVH5S9X+BxtRP8=","X-MC-Unique":"azbPsSa-Po2K2Afso4nrTA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711137080; x=1711741880;\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=ippVQ0ug1y2H1OIrZpzeQJ8A9DjE89Q8n2oi3NtLp/Q=;\n\tb=m74CPbp17HlkyOLRJDXlMh3H9Pw1fnC1IZvBHBPEpEqGZ2PnYLh4jNQE7aYE1GQqTP\n\twObskv5HpR1km0V3HGcGocRo486ssRpLlC0K4P2wwavNcczxCEVJ32IT0ieller1lZuv\n\t+bgl9LYYQkzg7TskxCOmGnjsfVNJBYlLL56vJ3niDVxXHja/R+LBeI77r+bk3VjzvYdW\n\tA9m14brwzSKai38jEynXKU2Otd1IBVFyhQO7mDZMUU9/4jB2mgzJYmCpNuCokEsz+Xyz\n\te+vGtOvFWI7fxEGNhZQQu2ARIQnfBawOX2mqXnIWR2oB07dQ5AKtiOiCDH1DSdBdGRWs\n\tOtKA==","X-Gm-Message-State":"AOJu0Yy2U1GFhWuzJcoZbhBNNSFkML12pwG7Z6FOFTZQZsqXcn9Ao3zO\n\tBN35bNU4hLZ1ldgM6r7uCHbzB0HSiQFSxasOxh7SCM8Q2s1HZletpO1Sm21Yj/imZcBkvPFmLKG\n\tyPK63TyvLGcywARndyRhKLbfKhDDzW/xL6Y83bieIJHi47JHXk97BTjSG7aT4bGidvYkNJMXoHU\n\tLuONo=","X-Received":["by 2002:a05:600c:5248:b0:414:cbd:1ee2 with SMTP id\n\tfc8-20020a05600c524800b004140cbd1ee2mr310179wmb.35.1711137079884; \n\tFri, 22 Mar 2024 12:51:19 -0700 (PDT)","by 2002:a05:600c:5248:b0:414:cbd:1ee2 with SMTP id\n\tfc8-20020a05600c524800b004140cbd1ee2mr310164wmb.35.1711137079457; \n\tFri, 22 Mar 2024 12:51:19 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHAhQhhCRew/3046CylrZLxPrTrTfns2wlGFUdP7tOtW4Km51Xzkg++l9b6zHOUmlBgaWzyPQ==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t","Subject":"Re: [PATCH v3 1/1] apps: cam: Add support for pnm output format","In-Reply-To":"<20240318124053.GB13682@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 18 Mar 2024 14:40:53 +0200\")","References":"<20240318104357.441724-1-mzamazal@redhat.com>\n\t<20240318104357.441724-2-mzamazal@redhat.com>\n\t<20240318124053.GB13682@pendragon.ideasonboard.com>","Date":"Fri, 22 Mar 2024 20:51:17 +0100","Message-ID":"<8734sixcy2.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":29046,"web_url":"https://patchwork.libcamera.org/comment/29046/","msgid":"<20240322205309.GB11521@pendragon.ideasonboard.com>","date":"2024-03-22T20:53:09","subject":"Re: [PATCH v3 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":"Hi Milan,\n\nOn Fri, Mar 22, 2024 at 08:51:17PM +0100, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Mon, Mar 18, 2024 at 11:43:57AM +0100, Milan Zamazal wrote:\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> >> Signed-off-by: Milan Zamazal \n> >> Reviewed-by: Kieran Bingham \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 | 20 +++++++++++++\n> >> 5 files changed, 88 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..ac53719d 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> >> \tif (!pattern_.empty())\n> >> \t\tfilename = pattern_;\n> >> \n> >> +\tbool pnm = filename.find(\".pnm\", filename.size() - 4) != std::string::npos;\n> >\n> > Could you move this after the HAVE_TIFF section, to sort the formats\n> > alphabetically ?\n> \n> Done.\n> \n> >> #ifdef HAVE_TIFF\n> >> \tbool 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> >> \tImage *image = mappedBuffers_[buffer].get();\n> >> \n> >> +\tif (pnm) {\n> >> +\t\tret = PNMWriter::write(filename.c_str(), stream->configuration(),\n> >> +\t\t\t\t image->data(0));\n> >> +\t\tif (ret < 0)\n> >> +\t\t\tstd::cerr << \"failed to write PNM file `\" << filename\n> >> +\t\t\t\t << \"'\" << std::endl;\n> >> +\n> >> +\t\treturn;\n> >> +\t}\n> >\n> > Same here.\n> \n> Done.\n> \n> >> #ifdef HAVE_TIFF\n> >> \tif (dng) {\n> >> \t\tret = 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> >> \t\t\t \"to write files, using the default file name. Otherwise it sets the\\n\"\n> >> \t\t\t \"full file path and name. The first '#' character in the file name\\n\"\n> >> \t\t\t \"is expanded to the camera index, stream name and frame sequence number.\\n\"\n> >> +\t\t\t \"If the file name ends with '.pnm', then the frame will be written to\\n\"\n> >> +\t\t\t \"the output file(s) in PNM format.\\n\"\n> >\n> > And here.\n> \n> Done.\n> \n> >> #ifdef HAVE_TIFF\n> >> \t\t\t \"If the file name ends with '.dng', then the frame will be written to\\n\"\n> >> \t\t\t \"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..9a63ec05\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> >> +\t\t const StreamConfiguration &config,\n> >> +\t\t const Span &data)\n> >> +{\n> >> +\tif (config.pixelFormat != formats::BGR888) {\n> >> +\t\tstd::cerr << \"Only BGR888 output pixel format is supported (\"\n> >> +\t\t\t << config.pixelFormat << \" requested)\" << std::endl;\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\tstd::ofstream output(filename, std::ios::binary);\n> >> +\tif (!output) {\n> >> +\t\tstd::cerr << \"Failed to open pnm file: \" << filename << std::endl;\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\toutput << \"P6\" << std::endl\n> >\n> > P6 refers to the Portable PixMap format, whose recommended extension is\n> > .ppm. Some tools have trouble opening PPM files when named with a .pnm\n> > suffix. I would use a .ppm file extension.\n> \n> OK, done. (My original motivation was to possibly cover other PNM formats but\n> PPM is the proper extension and as discussed in related threads, we probably\n> don't want to add much in this area.)\n> \n> > I'm also wondering if we shouldn't switch to the PAM format already (P7,\n> > see https://en.wikipedia.org/wiki/Portable_anymap#PAM_graphics_format)\n> > as it also supports greyscale formats. It's not the universal raw file\n> > format I'm looking for though, and maybe not worth it (I haven't checked\n> > how well supported PAM is).\n> \n> It's not supported everywhere, for example it's not recognized in Gqview or\n> Emacs (*bummer*) on my computer. So better to stick with PPM.\n\nI was suspecting that. Fair enough.\n\n> > For the sake of completeness, I want to mention we have a DNG writer\n> > that outputs DNG files, and DNG is a TIFF file under the hood. TIFF\n> > supports a set of RGB and YCbCr formats too... Opinions ? :-)\n> \n> I'm not sure whether DNG can support what we need. In any case, it's associated\n> with \"very\" raw camera images and not much supported outside raw converters and\n> photo editors.\n> \n> As for TIFF, the advantage of using PPM is that writing it is really trivial and\n> there is no need to use (or replicate) libtiff. But yes, TIFF might be helpful\n> at least with non-RGB outputs.\n\nI'd really want a file format that can support all the image formats we\nneed. TIFF could in theory support quite a few (but not all), but not\nall of the features the file format supports are commonly used, so it\nmay end up not being very interoperable. Still, if all those features\nwere supported by a common conversion tool (e.g. imagemagick), it could\nbe a viable option.\n\nThe alternative is to create a universal format ourselves...\n\n> >> +\t << config.size.width << \" \" << config.size.height << std::endl\n> >> +\t << \"255\" << std::endl;\n> >> +\tif (!output) {\n> >> +\t\tstd::cerr << \"Failed to write the file header\" << std::endl;\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\tconst unsigned int rowLength = config.size.width * 3;\n> >> +\tconst unsigned int paddedRowLength = config.stride;\n> >> +\tconst char *row = reinterpret_cast(data.data());\n> >> +\tfor (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {\n> >\n> > You could use config.stride directly here, and drop paddedRowLength.\n> \n> Done.\n> \n> >> +\t\toutput.write(row, rowLength);\n> >> +\t\tif (!output) {\n> >> +\t\t\tstd::cerr << \"Failed to write image data at row \" << y << std::endl;\n> >> +\t\t\treturn -EINVAL;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\treturn 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..6c1e7967\n> >> --- /dev/null\n> >> +++ b/src/apps/common/pnm_writer.h\n> >> @@ -0,0 +1,20 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2024, Red Hat, Inc.\n> >> + *\n> >> + * pnm_writer.h - PNM writer\n> >> + */\n> >> +\n> >> +#pragma once\n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +class PNMWriter\n> >> +{\n> >> +public:\n> >> +\tstatic int write(const char *filename,\n> >> +\t\t\t const libcamera::StreamConfiguration &config,\n> >> +\t\t\t const libcamera::Span &data);\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 E4E42C3272\n\tfor ;\n\tFri, 22 Mar 2024 20:53:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E3D663055;\n\tFri, 22 Mar 2024 21:53:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F1B161C45\n\tfor ;\n\tFri, 22 Mar 2024 21:53:14 +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 B450982A;\n\tFri, 22 Mar 2024 21:52:44 +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=\"tERgGQLs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711140764;\n\tbh=O8pOHOC/F8i0Xm/F+rHF9hpRiH6PzXpVpL0Yv+Ionh4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tERgGQLsM64hxxZeSl9pAoGmcG6GLlCvrhMeqFPKv0NuraCVRQ6+8v57FRP162J1p\n\tu27hwitElkHZf7CzfmBbwQjVenUgCL0lSzLnPEFKUlzRueH0GYhGMi/nhqtNmDf15v\n\tKsjubDIiV2ABTpW24d0l/CQD0OI6CltifszpJlHs=","Date":"Fri, 22 Mar 2024 22:53:09 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v3 1/1] apps: cam: Add support for pnm output format","Message-ID":"<20240322205309.GB11521@pendragon.ideasonboard.com>","References":"<20240318104357.441724-1-mzamazal@redhat.com>\n\t<20240318104357.441724-2-mzamazal@redhat.com>\n\t<20240318124053.GB13682@pendragon.ideasonboard.com>\n\t<8734sixcy2.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<8734sixcy2.fsf@redhat.com>","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\" "}}]