[{"id":4710,"web_url":"https://patchwork.libcamera.org/comment/4710/","msgid":"<20200501172330.GM5951@pendragon.ideasonboard.com>","date":"2020-05-01T17:23:30","subject":"Re: [libcamera-devel] [PATCH v2 2/3] qcam: Add DNGWriter","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, May 01, 2020 at 05:27:44PM +0200, Niklas Söderlund wrote:\n> Add an initial DNG file writer. The writer can only deal with a small\n> set of pixelformats. The generated file consumable by standard tools.\n\ns/consumable/is consumable/ ?\n\n> The writer needs to be extended to write more metadata to the generated\n> file.\n> \n> This change also makes libtiff-4 mandatory for qcam. In the future it\n> could be made an optional dependency and only enable DNG support if it's\n> available.\n\nI think it would be really nice to keep this optional. It shouldn't be\ndifficult, and I can give it a go if you need help.\n\nEdit: See below :-)\n\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/qcam/dng_writer.cpp | 152 ++++++++++++++++++++++++++++++++++++++++\n>  src/qcam/dng_writer.h   |  32 +++++++++\n>  src/qcam/meson.build    |   9 ++-\n>  3 files changed, 190 insertions(+), 3 deletions(-)\n>  create mode 100644 src/qcam/dng_writer.cpp\n>  create mode 100644 src/qcam/dng_writer.h\n> \n> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> new file mode 100644\n> index 0000000000000000..b58a6b9938992ed0\n> --- /dev/null\n> +++ b/src/qcam/dng_writer.cpp\n> @@ -0,0 +1,152 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * dng_writer.cpp - DNG writer\n> + */\n> +\n> +#include \"dng_writer.h\"\n> +\n> +#include <iomanip>\n> +#include <iostream>\n> +#include <map>\n> +#include <sstream>\n> +\n> +#include <tiffio.h>\n> +\n> +using namespace libcamera;\n> +\n> +struct FormatInfo {\n> +\tuint8_t bitsPerSample;\n> +\tuint8_t pattern[4]; /* R = 0, G = 1, B = 2 */\n> +\tvoid (*packScanline)(void *, void *, unsigned int);\n> +};\n> +\n> +void packScanlineSBGGR10P(void *output, void *input, unsigned int width)\n\ninput should be a const pointer.\n\n> +{\n> +\tuint8_t *in = static_cast<uint8_t *>(input);\n> +\tuint8_t *out = static_cast<uint8_t *>(output);\n\nHow about passing uint8_t * to the function ? Up to you.\n\n> +\n> +\t/* \\todo: Can this be made more efficient? */\n\ns/todo:/todo/\n\nAnd I'm sure it can, using SIMD for instance. Clearly something for\nlater.\n\n> +\tfor (unsigned int i = 0; i < width; i += 4) {\n\ns/i/x/ ?\n\n> +\t\t*out++ = in[0];\n> +\t\t*out++ = (in[4] & 0x03) << 6 | in[1] >> 2;\n> +\t\t*out++ = (in[1] & 0x03) << 6 | (in[4] & 0x0c) << 2 | in[2] >> 4;\n> +\t\t*out++ = (in[2] & 0x0f) << 4 | (in[4] & 0x30) >> 2 | in[3] >> 6;\n> +\t\t*out++ = (in[3] & 0x3f) << 2 | (in[4] & 0xc0) >> 6;\n> +\t\tin += 5;\n> +\t}\n> +}\n> +\n> +static const std::map<PixelFormat, FormatInfo> formatInfo = {\n> +\t{ PixelFormat(DRM_FORMAT_SBGGR10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 2, 1, 1, 0 }, packScanlineSBGGR10P } },\n> +\t{ PixelFormat(DRM_FORMAT_SGBRG10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 1, 2, 0, 1 }, packScanlineSBGGR10P } },\n> +\t{ PixelFormat(DRM_FORMAT_SGRBG10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 1, 0, 2, 1 }, packScanlineSBGGR10P } },\n> +\t{ PixelFormat(DRM_FORMAT_SRGGB10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 0, 1, 1, 2 }, packScanlineSBGGR10P } },\n> +};\n> +\n> +const FormatInfo *getFormatInfo(const PixelFormat &format)\n> +{\n> +\tconst auto it = formatInfo.find(format);\n> +\tif (it == formatInfo.cend())\n> +\t\treturn nullptr;\n> +\n> +\treturn &it->second;\n> +\n> +\treturn nullptr;\n\nThe last two lines can be removed.\n\n> +}\n> +\n> +DNGWriter::DNGWriter(const std::string &pattern)\n> +\t: pattern_(pattern)\n> +{\n> +}\n> +\n> +int DNGWriter::write(const Camera *camera, const Stream *stream,\n> +\t\t     FrameBuffer *buffer, void *data)\n> +{\n> +\tconst StreamConfiguration &config = stream->configuration();\n\nI wonder if the function shouldn't take the stream configuration instead\nof the stream.\n\n> +\n> +\tconst FormatInfo *info = getFormatInfo(config.pixelFormat);\n\nAs you use this function in a single place and the implementation is\nvery small, I would just inline it here.\n\n> +\tif (!info) {\n> +\t\tstd::cerr << \"Unsupported pixel format\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tuint8_t *scanline = new uint8_t[config.size.width * info->bitsPerSample];\n> +\tif (!scanline) {\n> +\t\tstd::cerr << \"Can't allocate memory for scanline\" << std::endl;\n> +\t\treturn -ENOMEM;\n> +\t}\n\nDoes this really need to be allocated on the heap, can you allocate it\non the stack ?\n\n\tuint8_t scanline[config.size.width * info->bitsPerSample];\n\n> +\n> +\tstd::string filename = genFilename(buffer->metadata());\n> +\n> +\tTIFF *tif = TIFFOpen(filename.c_str(), \"w\");\n> +\tif (!tif) {\n> +\t\tstd::cerr << \"Failed to open tiff file\" << std::endl;\n> +\t\tdelete[] scanline;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tconst uint8_t version[] = \"\\01\\02\\00\\00\";\n\nWould this read better as\n\n\tconst uint8_t version[] = { 1, 2, 0, 0 };\n\n? Or does TIFFSetField expect the version to be a null-terminated \"binary\nstring\" (\"\\01\\02\\00\\00\" is really stored as { 1, 2, 0, 0, 0 } in memory) ?\n\n> +\tconst uint16_t cfa_repeatpatterndim[] = { 2, 2 };\n> +\tTIFFSetField(tif, TIFFTAG_DNGVERSION, version);\n> +\tTIFFSetField(tif, TIFFTAG_DNGBACKWARDVERSION, version);\n> +\tTIFFSetField(tif, TIFFTAG_SUBFILETYPE, 0);\n> +\tTIFFSetField(tif, TIFFTAG_IMAGEWIDTH, config.size.width);\n> +\tTIFFSetField(tif, TIFFTAG_IMAGELENGTH, config.size.height);\n> +\tTIFFSetField(tif, TIFFTAG_BITSPERSAMPLE, info->bitsPerSample);\n> +\tTIFFSetField(tif, TIFFTAG_COMPRESSION, COMPRESSION_NONE);\n> +\tTIFFSetField(tif, TIFFTAG_PHOTOMETRIC, PHOTOMETRIC_CFA);\n> +\tTIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB);\n> +\tTIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> +\tTIFFSetField(tif, TIFFTAG_MODEL, camera->name().c_str());\n> +\tTIFFSetField(tif, TIFFTAG_UNIQUECAMERAMODEL, camera->name().c_str());\n> +\tTIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT);\n> +\tTIFFSetField(tif, TIFFTAG_SAMPLESPERPIXEL, 1);\n> +\tTIFFSetField(tif, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG);\n> +\tTIFFSetField(tif, TIFFTAG_SOFTWARE, \"qcam\");\n> +\tTIFFSetField(tif, TIFFTAG_SAMPLEFORMAT, SAMPLEFORMAT_UINT);\n> +\tTIFFSetField(tif, TIFFTAG_CFAREPEATPATTERNDIM, cfa_repeatpatterndim);\n> +\tTIFFSetField(tif, TIFFTAG_CFAPATTERN, info->pattern);\n> +\tTIFFSetField(tif, TIFFTAG_CFAPLANECOLOR, 3, \"\\00\\01\\02\");\n> +\tTIFFSetField(tif, TIFFTAG_CFALAYOUT, 1);\n> +\n> +\t/* \\todo: Add more EXIF fields to output. */\n> +\n> +\t/* Write RAW content */\n\nWould this be more efficient ?\n\n\tuint8_t *row = static_cast<uint8_t *>(data);\n\n> +\tfor (unsigned int row = 0; row < config.size.height; row++) {\n\nWith s/row/y/ here.\n\n> +\t\tuint8_t *start =\n> +\t\t\tstatic_cast<uint8_t *>(data) + config.stride * row;\n\nDelete this line.\n\n> +\n> +\t\tinfo->packScanline(scanline, start, config.size.width);\n> +\n> +\t\tif (TIFFWriteScanline(tif, scanline, row, 0) != 1) {\n> +\t\t\tstd::cerr << \"Failed to write scanline\" << std::endl;\n> +\t\t\tTIFFClose(tif);\n> +\t\t\tdelete[] scanline;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n\n\t\trow += config.stride;\n\n> +\t}\n> +\n> +\tTIFFWriteDirectory(tif);\n> +\n> +\tTIFFClose(tif);\n> +\n> +\tdelete[] scanline;\n> +\n> +\treturn 0;\n> +}\n> +\n> +std::string DNGWriter::genFilename(const FrameMetadata &metadata)\n> +{\n> +\tstd::string filename = pattern_;\n> +\n> +\tsize_t pos = filename.find_first_of('#');\n> +\tif (pos != std::string::npos) {\n> +\t\tstd::stringstream ss;\n> +\t\tss << std::setw(6) << std::setfill('0') << metadata.sequence;\n> +\t\tfilename.replace(pos, 1, ss.str());\n> +\t}\n> +\n> +\treturn filename;\n> +}\n> diff --git a/src/qcam/dng_writer.h b/src/qcam/dng_writer.h\n> new file mode 100644\n> index 0000000000000000..c7ccbffd5db69dbe\n> --- /dev/null\n> +++ b/src/qcam/dng_writer.h\n> @@ -0,0 +1,32 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * dng_writer.h - DNG writer\n> + */\n> +#ifndef __LIBCAMERA_DNG_WRITER_H__\n> +#define __LIBCAMERA_DNG_WRITER_H__\n> +\n> +#include <string>\n> +\n> +#include <libcamera/buffer.h>\n> +#include <libcamera/camera.h>\n> +#include <libcamera/stream.h>\n> +\n> +using namespace libcamera;\n> +\n> +class DNGWriter\n> +{\n> +public:\n> +\tDNGWriter(const std::string &pattern = \"frame-#.dng\");\n> +\n> +\tint write(const Camera *camera, const Stream *stream,\n> +\t\t  FrameBuffer *buffer, void *data);\n> +\n> +private:\n> +\tstd::string genFilename(const FrameMetadata &metadata);\n> +\n> +\tstd::string pattern_;\n\nI'd prefer keeping file name handling in the caller, as it's not\nspecific to DNG, and the need is shared with JPG capture. I know there's\na writer class in cam that handles file names, but I think it covers a\ndifferent case, as it saves all frames. Furthermore, if we want to allow\nselection of a file name by the user (and I think it's useful, as I\nexpect we'll want to save files with distinct names that will describe\nthe capture environment for tuning purpose for instance), we have to\npass the file name to the write() function.\n\nThe write() function could then become static, and you won't have to\ninstantiate this class, you could just call DNGWrite::write(...).\n\n> +};\n> +\n> +#endif /* __LIBCAMERA_DNG_WRITER_H__ */\n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index 895264be4a3388f4..8b9a35904facbce0 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -1,9 +1,10 @@\n>  qcam_sources = files([\n> +    '../cam/options.cpp',\n> +    '../cam/stream_options.cpp',\n> +    'dng_writer.cpp',\n>      'format_converter.cpp',\n>      'main.cpp',\n>      'main_window.cpp',\n> -    '../cam/options.cpp',\n> -    '../cam/stream_options.cpp',\n>      'viewfinder.cpp',\n>  ])\n>  \n> @@ -23,6 +24,8 @@ qt5_dep = dependency('qt5',\n>                       required : false)\n>  \n>  if qt5_dep.found()\n> +    tiff_dep = dependency('libtiff-4', required : true)\n> +\n\n    qcam_deps = [\n        libcamera_dep,\n        qt5_dep,\n    ]\n\n    tiff_dep = dependency('libtiff-4', required : false)\n    if tiff_dep.found()\n        qt5_cpp_args += [ '-DHAVE_TIFF' ]\n        qcam_deps += [ tiff_dep ]\n        qcam_sources += files([\n            'dng_writer.cpp',\n        ])\n    endif\n\n>      qt5_cpp_args = [ '-DQT_NO_KEYWORDS' ]\n>  \n>      # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until\n> @@ -40,6 +43,6 @@ if qt5_dep.found()\n>  \n>      qcam  = executable('qcam', qcam_sources, resources,\n>                         install : true,\n> -                       dependencies : [libcamera_dep, qt5_dep],\n> +                       dependencies : [libcamera_dep, qt5_dep, tiff_dep],\n\n                       dependencies : qcam_deps,\n\n>                         cpp_args : qt5_cpp_args)\n>  endif","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D944603F3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  1 May 2020 19:23:34 +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 98CD872C;\n\tFri,  1 May 2020 19:23:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jRimyw+i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588353813;\n\tbh=CnoTWa/35OCV4uXJDqMQyLJFrpKXUBf82njE5icY6po=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jRimyw+ioTET3DfswB4eui+oxMIDI3Up+/wKloxIXJhm3+V2aUrilmyYr2f3L+IJd\n\t5sdnJHWqKisi5OLFmDXAXRu0/S/TBj0ODkAg+ux6EDzmtgzT5id7bd16jSx+ratQya\n\t1x+zFUZG8+hjfcpWtCi+ABw2Yit4SfPHDqTEB/H4=","Date":"Fri, 1 May 2020 20:23:30 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200501172330.GM5951@pendragon.ideasonboard.com>","References":"<20200501152745.437777-1-niklas.soderlund@ragnatech.se>\n\t<20200501152745.437777-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200501152745.437777-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] qcam: Add DNGWriter","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 01 May 2020 17:23:34 -0000"}},{"id":4714,"web_url":"https://patchwork.libcamera.org/comment/4714/","msgid":"<20200501230948.GD2569889@oden.dyn.berto.se>","date":"2020-05-01T23:09:48","subject":"Re: [libcamera-devel] [PATCH v2 2/3] qcam: Add DNGWriter","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback. I have taken all comments except the one I \ndisagree with and comment on bellow :-)\n\nOn 2020-05-01 20:23:30 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Fri, May 01, 2020 at 05:27:44PM +0200, Niklas Söderlund wrote:\n> > Add an initial DNG file writer. The writer can only deal with a small\n> > set of pixelformats. The generated file consumable by standard tools.\n> \n> s/consumable/is consumable/ ?\n> \n> > The writer needs to be extended to write more metadata to the generated\n> > file.\n> > \n> > This change also makes libtiff-4 mandatory for qcam. In the future it\n> > could be made an optional dependency and only enable DNG support if it's\n> > available.\n> \n> I think it would be really nice to keep this optional. It shouldn't be\n> difficult, and I can give it a go if you need help.\n> \n> Edit: See below :-)\n> \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  src/qcam/dng_writer.cpp | 152 ++++++++++++++++++++++++++++++++++++++++\n> >  src/qcam/dng_writer.h   |  32 +++++++++\n> >  src/qcam/meson.build    |   9 ++-\n> >  3 files changed, 190 insertions(+), 3 deletions(-)\n> >  create mode 100644 src/qcam/dng_writer.cpp\n> >  create mode 100644 src/qcam/dng_writer.h\n> > \n> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> > new file mode 100644\n> > index 0000000000000000..b58a6b9938992ed0\n> > --- /dev/null\n> > +++ b/src/qcam/dng_writer.cpp\n> > @@ -0,0 +1,152 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * dng_writer.cpp - DNG writer\n> > + */\n> > +\n> > +#include \"dng_writer.h\"\n> > +\n> > +#include <iomanip>\n> > +#include <iostream>\n> > +#include <map>\n> > +#include <sstream>\n> > +\n> > +#include <tiffio.h>\n> > +\n> > +using namespace libcamera;\n> > +\n> > +struct FormatInfo {\n> > +\tuint8_t bitsPerSample;\n> > +\tuint8_t pattern[4]; /* R = 0, G = 1, B = 2 */\n> > +\tvoid (*packScanline)(void *, void *, unsigned int);\n> > +};\n> > +\n> > +void packScanlineSBGGR10P(void *output, void *input, unsigned int width)\n> \n> input should be a const pointer.\n> \n> > +{\n> > +\tuint8_t *in = static_cast<uint8_t *>(input);\n> > +\tuint8_t *out = static_cast<uint8_t *>(output);\n> \n> How about passing uint8_t * to the function ? Up to you.\n\nI like to keep pass void * as I hope we will have more pack functions \nadded in the future and would like to not impose an interpretation of \nthe data from the arguments.\n\n> \n> > +\n> > +\t/* \\todo: Can this be made more efficient? */\n> \n> s/todo:/todo/\n> \n> And I'm sure it can, using SIMD for instance. Clearly something for\n> later.\n> \n> > +\tfor (unsigned int i = 0; i < width; i += 4) {\n> \n> s/i/x/ ?\n> \n> > +\t\t*out++ = in[0];\n> > +\t\t*out++ = (in[4] & 0x03) << 6 | in[1] >> 2;\n> > +\t\t*out++ = (in[1] & 0x03) << 6 | (in[4] & 0x0c) << 2 | in[2] >> 4;\n> > +\t\t*out++ = (in[2] & 0x0f) << 4 | (in[4] & 0x30) >> 2 | in[3] >> 6;\n> > +\t\t*out++ = (in[3] & 0x3f) << 2 | (in[4] & 0xc0) >> 6;\n> > +\t\tin += 5;\n> > +\t}\n> > +}\n> > +\n> > +static const std::map<PixelFormat, FormatInfo> formatInfo = {\n> > +\t{ PixelFormat(DRM_FORMAT_SBGGR10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 2, 1, 1, 0 }, packScanlineSBGGR10P } },\n> > +\t{ PixelFormat(DRM_FORMAT_SGBRG10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 1, 2, 0, 1 }, packScanlineSBGGR10P } },\n> > +\t{ PixelFormat(DRM_FORMAT_SGRBG10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 1, 0, 2, 1 }, packScanlineSBGGR10P } },\n> > +\t{ PixelFormat(DRM_FORMAT_SRGGB10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 0, 1, 1, 2 }, packScanlineSBGGR10P } },\n> > +};\n> > +\n> > +const FormatInfo *getFormatInfo(const PixelFormat &format)\n> > +{\n> > +\tconst auto it = formatInfo.find(format);\n> > +\tif (it == formatInfo.cend())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn &it->second;\n> > +\n> > +\treturn nullptr;\n> \n> The last two lines can be removed.\n> \n> > +}\n> > +\n> > +DNGWriter::DNGWriter(const std::string &pattern)\n> > +\t: pattern_(pattern)\n> > +{\n> > +}\n> > +\n> > +int DNGWriter::write(const Camera *camera, const Stream *stream,\n> > +\t\t     FrameBuffer *buffer, void *data)\n> > +{\n> > +\tconst StreamConfiguration &config = stream->configuration();\n> \n> I wonder if the function shouldn't take the stream configuration instead\n> of the stream.\n> \n> > +\n> > +\tconst FormatInfo *info = getFormatInfo(config.pixelFormat);\n> \n> As you use this function in a single place and the implementation is\n> very small, I would just inline it here.\n> \n> > +\tif (!info) {\n> > +\t\tstd::cerr << \"Unsupported pixel format\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tuint8_t *scanline = new uint8_t[config.size.width * info->bitsPerSample];\n> > +\tif (!scanline) {\n> > +\t\tstd::cerr << \"Can't allocate memory for scanline\" << std::endl;\n> > +\t\treturn -ENOMEM;\n> > +\t}\n> \n> Does this really need to be allocated on the heap, can you allocate it\n> on the stack ?\n> \n> \tuint8_t scanline[config.size.width * info->bitsPerSample];\n> \n> > +\n> > +\tstd::string filename = genFilename(buffer->metadata());\n> > +\n> > +\tTIFF *tif = TIFFOpen(filename.c_str(), \"w\");\n> > +\tif (!tif) {\n> > +\t\tstd::cerr << \"Failed to open tiff file\" << std::endl;\n> > +\t\tdelete[] scanline;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tconst uint8_t version[] = \"\\01\\02\\00\\00\";\n> \n> Would this read better as\n> \n> \tconst uint8_t version[] = { 1, 2, 0, 0 };\n> \n> ? Or does TIFFSetField expect the version to be a null-terminated \"binary\n> string\" (\"\\01\\02\\00\\00\" is really stored as { 1, 2, 0, 0, 0 } in memory) ?\n> \n> > +\tconst uint16_t cfa_repeatpatterndim[] = { 2, 2 };\n> > +\tTIFFSetField(tif, TIFFTAG_DNGVERSION, version);\n> > +\tTIFFSetField(tif, TIFFTAG_DNGBACKWARDVERSION, version);\n> > +\tTIFFSetField(tif, TIFFTAG_SUBFILETYPE, 0);\n> > +\tTIFFSetField(tif, TIFFTAG_IMAGEWIDTH, config.size.width);\n> > +\tTIFFSetField(tif, TIFFTAG_IMAGELENGTH, config.size.height);\n> > +\tTIFFSetField(tif, TIFFTAG_BITSPERSAMPLE, info->bitsPerSample);\n> > +\tTIFFSetField(tif, TIFFTAG_COMPRESSION, COMPRESSION_NONE);\n> > +\tTIFFSetField(tif, TIFFTAG_PHOTOMETRIC, PHOTOMETRIC_CFA);\n> > +\tTIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB);\n> > +\tTIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> > +\tTIFFSetField(tif, TIFFTAG_MODEL, camera->name().c_str());\n> > +\tTIFFSetField(tif, TIFFTAG_UNIQUECAMERAMODEL, camera->name().c_str());\n> > +\tTIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT);\n> > +\tTIFFSetField(tif, TIFFTAG_SAMPLESPERPIXEL, 1);\n> > +\tTIFFSetField(tif, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG);\n> > +\tTIFFSetField(tif, TIFFTAG_SOFTWARE, \"qcam\");\n> > +\tTIFFSetField(tif, TIFFTAG_SAMPLEFORMAT, SAMPLEFORMAT_UINT);\n> > +\tTIFFSetField(tif, TIFFTAG_CFAREPEATPATTERNDIM, cfa_repeatpatterndim);\n> > +\tTIFFSetField(tif, TIFFTAG_CFAPATTERN, info->pattern);\n> > +\tTIFFSetField(tif, TIFFTAG_CFAPLANECOLOR, 3, \"\\00\\01\\02\");\n> > +\tTIFFSetField(tif, TIFFTAG_CFALAYOUT, 1);\n> > +\n> > +\t/* \\todo: Add more EXIF fields to output. */\n> > +\n> > +\t/* Write RAW content */\n> \n> Would this be more efficient ?\n> \n> \tuint8_t *row = static_cast<uint8_t *>(data);\n> \n> > +\tfor (unsigned int row = 0; row < config.size.height; row++) {\n> \n> With s/row/y/ here.\n> \n> > +\t\tuint8_t *start =\n> > +\t\t\tstatic_cast<uint8_t *>(data) + config.stride * row;\n> \n> Delete this line.\n> \n> > +\n> > +\t\tinfo->packScanline(scanline, start, config.size.width);\n> > +\n> > +\t\tif (TIFFWriteScanline(tif, scanline, row, 0) != 1) {\n> > +\t\t\tstd::cerr << \"Failed to write scanline\" << std::endl;\n> > +\t\t\tTIFFClose(tif);\n> > +\t\t\tdelete[] scanline;\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> \n> \t\trow += config.stride;\n> \n> > +\t}\n> > +\n> > +\tTIFFWriteDirectory(tif);\n> > +\n> > +\tTIFFClose(tif);\n> > +\n> > +\tdelete[] scanline;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +std::string DNGWriter::genFilename(const FrameMetadata &metadata)\n> > +{\n> > +\tstd::string filename = pattern_;\n> > +\n> > +\tsize_t pos = filename.find_first_of('#');\n> > +\tif (pos != std::string::npos) {\n> > +\t\tstd::stringstream ss;\n> > +\t\tss << std::setw(6) << std::setfill('0') << metadata.sequence;\n> > +\t\tfilename.replace(pos, 1, ss.str());\n> > +\t}\n> > +\n> > +\treturn filename;\n> > +}\n> > diff --git a/src/qcam/dng_writer.h b/src/qcam/dng_writer.h\n> > new file mode 100644\n> > index 0000000000000000..c7ccbffd5db69dbe\n> > --- /dev/null\n> > +++ b/src/qcam/dng_writer.h\n> > @@ -0,0 +1,32 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * dng_writer.h - DNG writer\n> > + */\n> > +#ifndef __LIBCAMERA_DNG_WRITER_H__\n> > +#define __LIBCAMERA_DNG_WRITER_H__\n> > +\n> > +#include <string>\n> > +\n> > +#include <libcamera/buffer.h>\n> > +#include <libcamera/camera.h>\n> > +#include <libcamera/stream.h>\n> > +\n> > +using namespace libcamera;\n> > +\n> > +class DNGWriter\n> > +{\n> > +public:\n> > +\tDNGWriter(const std::string &pattern = \"frame-#.dng\");\n> > +\n> > +\tint write(const Camera *camera, const Stream *stream,\n> > +\t\t  FrameBuffer *buffer, void *data);\n> > +\n> > +private:\n> > +\tstd::string genFilename(const FrameMetadata &metadata);\n> > +\n> > +\tstd::string pattern_;\n> \n> I'd prefer keeping file name handling in the caller, as it's not\n> specific to DNG, and the need is shared with JPG capture. I know there's\n> a writer class in cam that handles file names, but I think it covers a\n> different case, as it saves all frames. Furthermore, if we want to allow\n> selection of a file name by the user (and I think it's useful, as I\n> expect we'll want to save files with distinct names that will describe\n> the capture environment for tuning purpose for instance), we have to\n> pass the file name to the write() function.\n> \n> The write() function could then become static, and you won't have to\n> instantiate this class, you could just call DNGWrite::write(...).\n> \n> > +};\n> > +\n> > +#endif /* __LIBCAMERA_DNG_WRITER_H__ */\n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index 895264be4a3388f4..8b9a35904facbce0 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -1,9 +1,10 @@\n> >  qcam_sources = files([\n> > +    '../cam/options.cpp',\n> > +    '../cam/stream_options.cpp',\n> > +    'dng_writer.cpp',\n> >      'format_converter.cpp',\n> >      'main.cpp',\n> >      'main_window.cpp',\n> > -    '../cam/options.cpp',\n> > -    '../cam/stream_options.cpp',\n> >      'viewfinder.cpp',\n> >  ])\n> >  \n> > @@ -23,6 +24,8 @@ qt5_dep = dependency('qt5',\n> >                       required : false)\n> >  \n> >  if qt5_dep.found()\n> > +    tiff_dep = dependency('libtiff-4', required : true)\n> > +\n> \n>     qcam_deps = [\n>         libcamera_dep,\n>         qt5_dep,\n>     ]\n> \n>     tiff_dep = dependency('libtiff-4', required : false)\n>     if tiff_dep.found()\n>         qt5_cpp_args += [ '-DHAVE_TIFF' ]\n>         qcam_deps += [ tiff_dep ]\n>         qcam_sources += files([\n>             'dng_writer.cpp',\n>         ])\n>     endif\n> \n> >      qt5_cpp_args = [ '-DQT_NO_KEYWORDS' ]\n> >  \n> >      # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until\n> > @@ -40,6 +43,6 @@ if qt5_dep.found()\n> >  \n> >      qcam  = executable('qcam', qcam_sources, resources,\n> >                         install : true,\n> > -                       dependencies : [libcamera_dep, qt5_dep],\n> > +                       dependencies : [libcamera_dep, qt5_dep, tiff_dep],\n> \n>                        dependencies : qcam_deps,\n> \n> >                         cpp_args : qt5_cpp_args)\n> >  endif\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E70BC601B8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  2 May 2020 01:09:51 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id l19so3992226lje.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 01 May 2020 16:09:51 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tl7sm3144879lfg.79.2020.05.01.16.09.49\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 01 May 2020 16:09:49 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"Zh+eJayU\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=1VlwKwV4LqOZ4QqOZ6Xuz6E/JltcH0RTtHwBtlHW0J8=;\n\tb=Zh+eJayUlsVTyAjYK63FTIoFfxqCISCsJw547mJ+YGY1UJx6Qof2nZqYVyAKAcU1o9\n\tzxUPVP8NwU+OV/eupf+u2/KPg846KuqfL0r4vZn9WIAEKzK9sHtxh4cGZOllMQI5DnL8\n\tjda56+S7JXo4jIaecyvBSlWHkSyyXo6FZzoKxXqjGVC5WCxlmCrM2pokfb5gmZuGTvdn\n\tItECBa6oWI5kV7Rn3X04MbW/0H7G/CV0t32Ynvv8SNoXBd2YO568JB20D+++73X5jFES\n\tPEjBfNOj3rfyvvkugYhHhRFWCpJuAqSXo4h+stSB6JCDaKMolEmlhhtWUNadf454W1HT\n\tz/zw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=1VlwKwV4LqOZ4QqOZ6Xuz6E/JltcH0RTtHwBtlHW0J8=;\n\tb=MLeosnmjytaZoYSRanYQ5BoZKClLSgiAqPpKI+IMyPUtbKV2eoXdWJvZbB8m5eOUc/\n\tBmFOLnZI3gJtpWJm2PLZMmuxREBGim9FICAV3wEqBoyfXiXMcJy5r92WMAeJiQumXifJ\n\t0xbR3awXweTuZIGSmi9QMBnIe1UGCG6IpuW4K0Euq6jXEtP7DPOizP0VeJwpd+FkLD7u\n\tO3sqmNENgX96NCAuOVK+kXAuh/ANbrhgwdgECiAaoEsJRH7gDINYJwHHF4FUa9/08FFv\n\t8YXyTTzM7dNSIisFVlk79SzsaG07ObUMZnXxeTn9l9k1LTFhHekgWNe+jvTWH9wUtIrl\n\tqkTA==","X-Gm-Message-State":"AGi0Pua0xTO4sOk0aeUrHLVHtr9BJKQ18fQ3PMaAm88cTV6U0iEAln08\n\t6RS7aZQie0iC2CidIvIKn4JH9UEBrcw=","X-Google-Smtp-Source":"APiQypLeoHgVBMpdI/GHV4drJMG2zumzMRnG+QG1FlSTZdAwe5i91c6H9QxAJlannHxgljByUu7VwA==","X-Received":"by 2002:a2e:8108:: with SMTP id d8mr3533346ljg.184.1588374590769;\n\tFri, 01 May 2020 16:09:50 -0700 (PDT)","Date":"Sat, 2 May 2020 01:09:48 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200501230948.GD2569889@oden.dyn.berto.se>","References":"<20200501152745.437777-1-niklas.soderlund@ragnatech.se>\n\t<20200501152745.437777-3-niklas.soderlund@ragnatech.se>\n\t<20200501172330.GM5951@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200501172330.GM5951@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] qcam: Add DNGWriter","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 01 May 2020 23:09:52 -0000"}}]